Band-aid fix for rare crashes when refreshing files

[stefanhaller]

Users have filed issues with crash reports that seem to indicate that the FileTreeViewModel gets swapped out (by a refresh) while a call to itemsSelected is in progress, iterating over the previous items. Guard against this by locking the mutex that we already have for this for the duration of the call.

I don't have a good way of testing whether the fix helps, because the crashes only occurred very infrequently. Let's just see if the crash reports stop coming in after we ship this.

Note also that this is only the minimal fix for the crashes that were reported. Theoretically, the same problem could happen for a key handler itself, but we never saw reports about that, so we don't bother doing anything about that yet.

Note also that long-term I envision a different solution to this class of problems (discussed in #2974), that's why I want to avoid locking mutexes more than necessary now.

Fixes #3646
Fixes #4154
Fixes #4301
Fixes #5070

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for d1d2bb21	✅ 100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (d1d2bb2)	Report Missing	Report Missing	Report Missing
Head commit (d274474)	59216	51506	86.98%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5074)	11	11	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[ChrisMcD1]

Band aid looks good to me!

I never got my fix to a happy place, it was going to be a big lift!

The big idea of my large change was instead of having a:

type IFileTreeViewModel interface {
	IFileTree
	types.IListCursor
}

that other people can mess around with and screw up our connection between our IFileTree and IListCursor, we would have a version that has locks around each path, so that it could only be viewed in a consistent state.

type IFileTreeViewModel interface {
	GetSelected() *FileNode
	GetSelectedItemId() string
	GetSelectedItems() ([]*FileNode, int, int)
	GetSelectedItemIds() ([]string, int, int)
	GetSelectedFile() *models.File
	GetSelectedPath() string
	RefreshTree()
	SetStatusFilter(filter FileTreeDisplayFilter)
	ToggleShowTree()
	CollapseAll()
	ExpandAll()
}

The LOC change was going to be tremendous I imagine, and I would want to do a similar thing to other combinations of List and View, which did not seem tenable

[stefanhaller]

The big idea of my large change was instead of having a:

As I tried to explain in #4301, this is not a direction I want to take. This only addresses the specific problem of the file tree, but we have the same (or similar) concurrency issues all over the place with other data (like other model stuff, and even views), and I'd rather have a more generic solution that addresses all of that at a more fundamental level.

[ChrisMcD1]

As I tried to explain in #4301, this is not a direction I want to take.

Wow, I had forgotten that we actually had this discussion 🫣 In my memory, it was all an internal debate in my head with myself

I also briefly explored a total inversion of our whole MVC stack. That might be similar the more generic thing you are envisioning? The git updates would update the model, which then wholesale updated the view all at once. I remember it having a decent amount of duplication, since the model and the view needed their own copy of the data to work with, that the model couldn't be secretly mutating under the hood

[stefanhaller]

Hum, no, that doesn't sound like what I have in mind. I explained my proposal in #4301 (comment) and #2974 (comment).

I don't have time to go into more detail about this right now. It's going to be a pretty major undertaking, I'm planning to go about it at a point where I can devote a bit of focused time to it (which is not very soon 😄).