Retain index position of allBranchesLogCmd when toggling status panel #4556
Conversation
This made some amount of sense in the previous world. If users were using `statusPanelView: dashboard`, then it would make sense for them to go to their first log when they switch over from the dashboard to the log commands by pressing `a`. This doesn't end up working out super well though for users of `statusPanelView: allBranchesLog`, where upon reaching the view with `1`, then would press `a` and see nothing. Upon returning to the view with a `2` and then `1`, they would then see the new value! Overall, it seems better that the command that you are viewing the output of is still the index of the command stored in lazygit's state. If we want to add more code to make it so that lazygit knows it is _not yet_ showing a branch command, and is instead on the dashboard, and make it so that it doesn't rotate at all in that case, we could do that. But that seems like too much work that no user would ever appreciate. I am half assuming here that users who love allBranchesLogCmds, and use multiple of them, are also running with `statusPanelView: allBranchesLog`, so they will not be negatively impacted by this.
| } | ||
|
|
||
| for _, context := range []types.Context{ | ||
| gui.State.Contexts.Status, |
There was a problem hiding this comment.
I am in no way positive this is the correct way to register a context as focusable, when it has no particular behaviors that it needs besides search-ability.
I just saw this added in https://github.com/jesseduffield/lazygit/pull/4429/files#diff-451b1ef5904657beb9097bcb881b06951fa00d4339d317f67030f556f456012eR269-R282, and some basic manual testing makes it seem like I now have the 0 keybinding I would want and searching works.
There was a problem hiding this comment.
Yes, this is all we need. What happens here is that it attaches the SwitchToFocusedMainViewController to the context, which takes care of handling the 0 press; once the main view is focused, the MainViewController takes over and handles all the keybindings for scrolling, begin/end etc.
I don't agree with the commit message when it says "It only needs to be focused in order to search through it." Focusing is also useful for scrolling it, which of course isn't needed for the dashboard, but could be useful after pressing a (also without the rest of this branch).
It only needs to be focused in order to search through it. Searching through it is particularly useful when you are using the `allBranchesLogCmd` view and you want to search through your log for something useful.
3b6c43f to
43d2ea2
Compare
|
@ChrisMcD1 I hope to get around to reviewing this soon. In the meantime, it seems that the third commit is unrelated to the other changes, and should be a PR of its own. It's useful even without the other changes, and deserves its own release notes entry. Also, it will fix this crash. |
stefanhaller
left a comment
There was a problem hiding this comment.
Looks ok to me, but see comments below.
I think the whole behavior of showing the all branches log in the status panel is questionable in several ways, but I don't find it important enough to redesign this now. Your changes definitely look like an improvement over the current behavior.
|
@stefanhaller I've pushed new changes to my
I removed this commit from the branch. I'll wait to PR it later so I can test it out some more, since I just learned more about the main view when checking out that other PR you put up. What should I do since you closed this PR already? (And what should I take away from that fact? I'm used to a world where a PR that should have something split off would get a "Request Changes" until that is done, and closing a PR is normally only used when the entire idea is being scrapped in favor of something else) |
If I closed this, it was totally unintentionally. Sorry for that! Must have clicked the wrong button accidentally. Sometimes github closes PRs automatically, e.g. if they are based on a different branch than main, and that other branch is merged; but I don't think this happened here. It doesn't look like it's possible to reopen the PR, so I'm afraid you'll have to open a new one. Really sorry for the trouble. |
…md (#4574) - **PR Description** This is a recreation of #4556. Compared to that PR description, this has actually solved the problem that repeated attempts to `1` `a` when `gui.statusPanelView: dashboard`. Now we only rotate logs on the 2nd `a` press. While this does differ from current behavior, I believe it is better, and I have more details in my commit description. Fixes #4469
Currently the behaviors of pressing
1to jump to the status panel (withgui.StatusPanelView: allBranchesLogCmd) and pressingaonce in the status panel, are identical. This results in the bug described in #4469, where the simple config:rotates between the two commands as you press
1,2,1,2.This PR splits the behavior of
1andasuch that1just shows the current index without mutating anything, andarotates the index, and then displays is. This latter behavior is not without controversy, as it changes existing behavior. See my long commit body for more details. The only way to "fix" this is to add additional understanding into the command as to what view is currently being rendered in the main view, which seemed to be non-trivial with how this view is implemented. If we wanted to fix this behavior, it might make sense to make more drastic changes and add an actual Tab for this allBranchesLog view.Fixes #4469
go generate ./...)