Select next stageable line correctly after staging a range of lines #2444
Conversation
While we try to keep the view's cursor position in sync with the context state's selectedLineIdx (at least when pressing up or down), there are enough situations where the two run out of sync; for example when initially opening the view, or after staging a hunk, or when scrolling the view using the wheel. While it would be possible to fix these situations to keep them always in sync, it doesn't seem worth it, because the view's cursor position isn't really used for anything else. So we rather special-case the SelectedLine/SelectedLineIdx functions of ViewDriver to query the context state's selectedLineIdx directly if it is a patch explorer context.
The selected line is not in the right position after staging a range of lines; see next commit.
We already have this very convenient behavior of jumping to the next stageable line after staging something. However, while this worked well for staging single lines or hunks, it didn't work correctly when staging a range of lines; in this case we want to start searching from the first line of the range.
There are two reasons for doing this: 1. The view cursor position is often out of sync with the selected line; see first commit of this branch. 2. The highlighting is already turned off when the view loses focus, and never turned back on thereafter. So just turn it off from the start then.
Now that the cursor highlight is never shown (see previous commit), there's no reason to update the cursor position any more.
Uffizzi Preview Environment☁️ https://app.uffizzi.com/github.com/jesseduffield/lazygit/pull/2444 ⚙️ Updating now by workflow run 4208409512. What is Uffizzi? Learn more |
| } | ||
| return 0, false | ||
| } | ||
|
|
There was a problem hiding this comment.
As I'm still struggling with getting a handle on lazygit's architecture, it's entire possible that there's a nicer/cleaner way of doing this. Suggestions welcome.
I did consider the alternative of keeping the view's cursor position in sync with the state's selectedLineIdx, but I abandoned the idea because it seems to be a lot of work, it's hard to verify that it's correct at all times, and it just doesn't seem worth it.
| ) | ||
|
|
||
| // TODO: find out why we can't use .SelectedLine() on the staging/stagingSecondary views. | ||
|
|
There was a problem hiding this comment.
@jesseduffield I wasn't totally sure what you wanted to use this for in this test; below are some suggestions, but if you had something else in mind, feel free to push fixups.
There was a problem hiding this comment.
I'm happy with the existing changes
|
@stefanhaller I've played around with this locally and I've pushed a change that should keep the view in-sync with the state. Let me know your thoughts |
There's still at least one situation where they run out of sync: when you scroll the view with the wheel. I don't know if there are others. I pushed a "DROPME" commit that makes it easier to test this. Also, I'd be interested to know why this approach is preferable. As far as I can tell, the view cursor is only used for the integration tests, not for anything else. And it seems to be hard and error-prone to get this completely in sync in all cases, and it could easily break again in the future when code is refactored or new features are added. Nobody will notice, since the highlight is not displayed. So isn't it preferable to use the real source of truth in tests? |
|
@stefanhaller I actually spent some time working to get highlighting on the view enabled again because I think it's a visual improvement, but parked it as it looked like it required a couple more steps (probably the same steps it would take to fix the scrolling issue you mention). If we could get that working, would you agree that it's a preferable method? |
|
Oh, so you actually want to see the cursor line in bold; I didn't expect that. I always thought it's a visual bug that it shows at all (initially), but maybe that was just because it was out of sync so often. In that case, sure, that would be preferable then. Do you think you can get to that soon? I'm afraid I won't be able to help with this much, I don't understand the architecture well enough yet. The thing is that the actual bug fix in this branch (selecting the right line after staging a range) is important enough for me that I'd like to have it soon. Do you think it would be acceptable to merge just the fix itself (i.e. this little hunk) for now, and add a test later when the highligthing is fixed? |
jesseduffield
force-pushed
the
fix-selecting-next-stageable-line
branch
from
4bc1971 to
01bf7f2
Compare
|
I've pushed a change just now that should do the trick wrt highlighting. If all tests pass I'll merge |
|
That's great, thank you so much for solving this so quickly. ❤️ Only thing is: I would have loved to clean up the branch a little before merging, there's now some unpleasant back-and-forth with early commits doing one thing and then later commits undoing some of it. Lazygit makes it so easy to clean up stuff like that to make it much easier to understand. I created #2447 to cleanup up some code that we didn't need after all. |
|
Ah yes, my bad. In my haste to get the PR merged I neglected to think about squashing things up. I'll be more judicious about that in future |
2 participants
After staging a range of lines, the next stageable line wasn't selected correctly; this PR fixes that.
As part of this, it fixes the problem that SelectedLine/SelectedLineIdx didn't work correctly for the staging/stagingSecondary views in integration tests.
go run scripts/cheatsheet/main.go generate)docs/Config.md) have been updated if necessary