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.
| @@ -266,6 +266,7 @@ func (gui *Gui) resetHelpersAndControllers() { | |||
| } | |||
|
|
|||
| 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.
ChrisMcD1
force-pushed
the
4469-cycle-panels
branch
from
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. |
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 ./...)