1
0
mirror of https://github.com/jesseduffield/lazygit.git synced 2025-04-23 12:18:51 +02:00
lazygit/pkg/gui/filetree/file_tree_view_model.go
Jesse Duffield bd7fabef1f Reduce the chance of race condition with list cursor
Before this commit, we had pkg/integration/tests/submodule/add.go
failing with a panic. I'm pretty sure the issue is this: we're now
calling quite a few GetDisabledReason calls on each layout() call,
and if a background thread happens to update a model slice while
we're doing this, we can end up with a selection index that's now
out of bounds because it hasn't been clamped to match the new list
length.

Specifically, here we had the selected index being -1 (the list starts
empty and somehow the value is -1 in this case) and then the list
gets a new submodule so the length is now 1, but the list cursor
doesn't know about this so remains on the old value. Then we confirm
the length is greater than zero and try to get the selected submodule
and get an out of bounds error.

This commit fixes the issue by clamping the selected index whenever
we get the length of the list so that it stays in-sync. This is not
a perfect solution because the length can change at any time, but
it seems to reliably fix the test, and using mutexes didn't seem to
make a difference.

Note that we're swapping the order of IFileTree and IListCursor in
the file tree view model to ensure that the list cursor's Len()
method is called (which performs the clamping).

Also, comment from the PR:
This 'trait' pattern we're using is convenient but can lead to awkward
situations. In this case we have both the list view model and the
(embedded) list cursor with a Len() method. The list cursor Len()
method just calls the list view model Len() method. But I wanted
to make it that the list view model now calls ClampSelection() on the
list cursor whenever it obtains the length. This will cause an
infinite loop because ClampSelection() internally calls Len()
(which calls the list view model's Len() method which in turn
calls ClampSelection() again, etc).

The only reason we were passing the list view model into the list
cursor was to supply the length method, so now we're just doing
that directly, and letting the list view model delegate the Len()
call to the list cursor, which now itself calls ClampSelection.
2024-01-28 09:20:52 +11:00

193 lines
5.1 KiB
Go

package filetree
import (
"sync"
"github.com/jesseduffield/lazygit/pkg/commands/models"
"github.com/jesseduffield/lazygit/pkg/gui/context/traits"
"github.com/jesseduffield/lazygit/pkg/gui/types"
"github.com/jesseduffield/lazygit/pkg/utils"
"github.com/samber/lo"
"github.com/sirupsen/logrus"
)
type IFileTreeViewModel interface {
IFileTree
types.IListCursor
}
// This combines our FileTree struct with a cursor that retains information about
// which item is selected. It also contains logic for repositioning that cursor
// after the files are refreshed
type FileTreeViewModel struct {
sync.RWMutex
types.IListCursor
IFileTree
}
var _ IFileTreeViewModel = &FileTreeViewModel{}
func NewFileTreeViewModel(getFiles func() []*models.File, log *logrus.Entry, showTree bool) *FileTreeViewModel {
fileTree := NewFileTree(getFiles, log, showTree)
listCursor := traits.NewListCursor(fileTree.Len)
return &FileTreeViewModel{
IFileTree: fileTree,
IListCursor: listCursor,
}
}
func (self *FileTreeViewModel) GetSelected() *FileNode {
if self.Len() == 0 {
return nil
}
return self.Get(self.GetSelectedLineIdx())
}
func (self *FileTreeViewModel) GetSelectedItemId() string {
item := self.GetSelected()
if item == nil {
return ""
}
return item.ID()
}
func (self *FileTreeViewModel) GetSelectedItems() ([]*FileNode, int, int) {
if self.Len() == 0 {
return nil, 0, 0
}
startIdx, endIdx := self.GetSelectionRange()
nodes := []*FileNode{}
for i := startIdx; i <= endIdx; i++ {
nodes = append(nodes, self.Get(i))
}
return nodes, startIdx, endIdx
}
func (self *FileTreeViewModel) GetSelectedItemIds() ([]string, int, int) {
selectedItems, startIdx, endIdx := self.GetSelectedItems()
ids := lo.Map(selectedItems, func(item *FileNode, _ int) string {
return item.ID()
})
return ids, startIdx, endIdx
}
func (self *FileTreeViewModel) GetSelectedFile() *models.File {
node := self.GetSelected()
if node == nil {
return nil
}
return node.File
}
func (self *FileTreeViewModel) GetSelectedPath() string {
node := self.GetSelected()
if node == nil {
return ""
}
return node.GetPath()
}
func (self *FileTreeViewModel) SetTree() {
newFiles := self.GetAllFiles()
selectedNode := self.GetSelected()
// for when you stage the old file of a rename and the new file is in a collapsed dir
for _, file := range newFiles {
if selectedNode != nil && selectedNode.Path != "" && file.PreviousName == selectedNode.Path {
self.ExpandToPath(file.Name)
}
}
prevNodes := self.GetAllItems()
prevSelectedLineIdx := self.GetSelectedLineIdx()
self.IFileTree.SetTree()
if selectedNode != nil {
newNodes := self.GetAllItems()
newIdx := self.findNewSelectedIdx(prevNodes[prevSelectedLineIdx:], newNodes)
if newIdx != -1 && newIdx != prevSelectedLineIdx {
self.SetSelection(newIdx)
}
}
self.ClampSelection()
}
// Let's try to find our file again and move the cursor to that.
// If we can't find our file, it was probably just removed by the user. In that
// case, we go looking for where the next file has been moved to. Given that the
// user could have removed a whole directory, we continue iterating through the old
// nodes until we find one that exists in the new set of nodes, then move the cursor
// to that.
// prevNodes starts from our previously selected node because we don't need to consider anything above that
func (self *FileTreeViewModel) findNewSelectedIdx(prevNodes []*FileNode, currNodes []*FileNode) int {
getPaths := func(node *FileNode) []string {
if node == nil {
return nil
}
if node.File != nil && node.File.IsRename() {
return node.File.Names()
} else {
return []string{node.Path}
}
}
for _, prevNode := range prevNodes {
selectedPaths := getPaths(prevNode)
for idx, node := range currNodes {
paths := getPaths(node)
// If you started off with a rename selected, and now it's broken in two, we want you to jump to the new file, not the old file.
// This is because the new should be in the same position as the rename was meaning less cursor jumping
foundOldFileInRename := prevNode.File != nil && prevNode.File.IsRename() && node.Path == prevNode.File.PreviousName
foundNode := utils.StringArraysOverlap(paths, selectedPaths) && !foundOldFileInRename
if foundNode {
return idx
}
}
}
return -1
}
func (self *FileTreeViewModel) SetStatusFilter(filter FileTreeDisplayFilter) {
self.IFileTree.SetStatusFilter(filter)
self.IListCursor.SetSelection(0)
}
// If we're going from flat to tree we want to select the same file.
// If we're going from tree to flat and we have a file selected we want to select that.
// If instead we've selected a directory we need to select the first file in that directory.
func (self *FileTreeViewModel) ToggleShowTree() {
selectedNode := self.GetSelected()
self.IFileTree.ToggleShowTree()
if selectedNode == nil {
return
}
path := selectedNode.Path
if self.InTreeMode() {
self.ExpandToPath(path)
} else if len(selectedNode.Children) > 0 {
path = selectedNode.GetLeaves()[0].Path
}
index, found := self.GetIndexForPath(path)
if found {
self.SetSelectedLineIdx(index)
}
}