Fix race condition with reselecting the focused branch and rendering

[stefanhaller]

In ff4ae4a we changed the order of the calls to render before selecting the branch. This was done only to save an extra call to ReApplyFilter, which is done by refreshView; I claimed that the order of refreshView vs. SetSelectedLineIdx doesn't matter here. I guess I was wrong about that, it makes the integration test custom_commands/suggestions_preset.go flaky. To fix this, put the refreshView call back to where it was (after the SetSelectedLineIdx call), and instead insert an extra call to ReApplyFilter where necessary to fix the bug that ff4ae4a was trying to fix.

I still don't think there was any user facing problem caused by this (@ChrisMcD1 correct me if I'm wrong), so I don't think we need to cut a 0.46.1 release with the fix (otherwise it would have been a regression in 0.46), and I only label it as maintenance because it only fixes CI.

Fixes #4267.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for daf1fd31	✅ 100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (daf1fd3)	Report Missing	Report Missing	Report Missing
Head commit (3e11e34)	53095	45995	86.63%

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 (#4268)	4	4	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

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

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. ↩

[jesseduffield]

LGTM

[ChrisMcD1]

@stefanhaller There could be a user facing problem caused by this, but I imagine it would be small. I normally only switch branches by hovering over them and hitting <space>, so I never need the focusing behavior following a branch re-ordering to return me to a non-zero index in the branches. I imagine others are similar.

I just tried setting up my local build with the equivalent config to the flaky integration test:

customCommands:
  - key:     "a"
    context: "localBranches"
    command: "git checkout {{.Form.Branch}}"
    prompts:
      - key:   "Branch"
        type:  "input"
        title: "Enter a branch name"
        suggestions: 
          preset: "branches"

and was unable to reproduce it right now on the lazygit repository, or a small test repository. But I'm also not sitting here and re-launching lazygit 100x like the integration tests did. The bug was inconsistent enough that I had to bump the attempts from 25x after my first git bisect run landed me on the wrong commit.

If we do know a non-contrived flow that relies on this focusing behavior, it might be worth giving it a few reps to see if it gets a more unlucky roll of the dice for some reason.

[stefanhaller]

I suppose you are right. And the focussing behavior that we are talking about is not only needed when running a custom command that checks out a branch (I guess you could also checkout the branch on the command line in another terminal and then come back to lazygit), but also when the sort order of the branches list changes for some other reason. The commit message of 2c9b477 describes one such scenario that I actually find pretty important.

I still don't understand what exactly went wrong, and I don't have time right now to dig deeper (will try tomorrow), but if the issue is that sometimes the call to SetSelectedLineIdx happened before the call to FocusLine, then that's only a visual problem, not a behavior problem. We are selecting the right branch after all, we might just not render it as selected. But invoking any command on the selected branch (including arrow up/down to select the prev/next) will work correctly because these are using the model selection.

The reason why the test failed is because integration tests don't know anything about the model selection, they can only look at the view highlight.

Ok, I did some testing with the following custom command:

customCommands:
  - key: a
    command: git checkout -
    context: localBranches

Then, check out some branch, check out some other branch, and then keep pressing a to switch between the two. It should always keep the currently selected one selected.

I observed that sometimes it keeps the same index selected, but only for a brief moment (like half a second or so), then the highlight jumps to the right branch. And I can see this both with this branch and with master, which confuses me quite a bit.

I have to stop now, will continue tomorrow.

[stefanhaller]

I didn't get around to do any deeper investigations, but I guess the fix is good anyway, so I'm just going to merge it now.