jesseduffield
add custom destination option to reset menu
Previously when you pressed 'g' you would be presented a list of reset "strengths". There is now a 4th option ('c') for setting a custom destination via a prompt, it then returns you to the "strength" menu but now with the new custom destination instead of whatever the default value was.
First-time contributor, so any feedback is more than welcome!
Related issue: https://github.com/jesseduffield/lazygit/issues/1985
Nice work @ryand67 . Before I look into the code more closely, I'm wondering what the typical use case would be here. E.g. resetting to origin/master perhaps. If we know how people are using it that could help us to decide whether to include a suggestions section while the user types, and if so, what to include in that section.
Mark had even mentioned doing the suggestions and it completely slipped my mind. Long day at work I suppose, I’ll get that added first thing tomorrow.
And use cases would be resetting to origin/main like you mentioned, or if you branch off of a branch and need to reset to the first one. I’ve been there more than a couple times personally. Maybe Mark had more in mind!
I'm wondering what the typical use case would be here. E.g. resetting to origin/master perhaps.
Since I'm mostly working on other people's stuff, origin isn't always what I want to reset to, depending on the repo it could be upstream, fork, pr2047fork etc. There's no way for me to currently indicate that other than switching the upstream of the current branch, going to Files tab, and then resetting with g, and setting the upstream back to what it was in case I want to push something to my fork.
Another pain point actually is that resetting to master and origin/master are under two different panes, namely Local Branches and Files, both under g. Could we actually bundle the two changes into this PR?
The g under Files is rather in an unexpected place, and I propose the reset --[soft,mixed,hard] @{upstream} be moved to Local Branches, with the keybindings S, M, and H, alongside c for custom?
Mark had even mentioned doing the suggestions and it completely slipped my mind.
That's what PR's are for! :)
Also I'd have the confirmation of the fuzzy search pane validate the said branch and not change the destination it if it doesn't exist, but return the user to the prompt.
There's likely some overlap with my above review and what @mark2185 has already stated: I had that review in a pending state but hadn't gotten around to submitting it.
I agree with @mark2185 that it would be good to better handle upstreams, given how often we need to use them. I'm not sure though whether it's better to have the upstreams included in the one menu or if there should be a two-step process where first you select the branch you're resetting to, and then a menu pops up for you to decide whether it's gonna be soft/mixed/hard.
I.e. you could have the initial menu for branch blah contain:
- reset to blah
- reset to origin/blah
- enter ref to reset to
And then after you've picked the ref you're resetting to, we show the menu with the soft/mixed/hard options. It means an extra keypress for those who have the muscle memory already, but I think that approach is actually cleaner and more easily extensible.
I'm not sure though whether it's better to have the upstreams included in the one menu or if there should be a two-step process where first you select the branch you're resetting to, and then a menu pops up for you to decide whether it's gonna be soft/mixed/hard.
I think having:
s,m,hfor@{branch}S,M,Hfor@{upstream-branch}note: not necessarilyorigin/branchcfor thedestinationand then other menu for the type of reset
... covers 99% of my needs (the remainder is a buffer in case I remember some other use case).
@mark2185 you've convinced me. Just to be clear on the pros/cons in my head:
benefits of the two-phased approach:
- fewer items in any given menu, so easier to parse
- the final menu that we get is the same no matter whether we're resetting from within the branches panel or commits panel, or entering a custom ref to reset to
- more extensible
benefits of the one-phased approach:
- one fewer keypress in the typical case compared to the two-phased approach
That one fewer keypress does seem to be the deciding factor to me. Worst case scenario, we find some new use case we hadn't thought of before and need to switch to a two-phased approach, but that will be no more disruptive than changing it now.
That one fewer keypress does seem to be the deciding factor to me.
That, and my hot path is hard resetting to an upstream. I've never needed a soft or a mixed reset to a branch (resetting to a commit, on the other hand, is a different story). The sample size is not very scientific, but hey.
Worst case scenario, we find some new use case we hadn't thought of before and need to switch to a two-phased approach, but that will be no more disruptive than changing it now.
We'll cross that bridge when (and if) we get to it! :)
Hey all, sorry for being mia. caught a nasty bug and was out for a few days. Just pushed up changes including the branches suggestions before I read the discussion about redoing a chunk of the menu lol. As far as UX, I can see pros and cons of both. I personally am usually resetting to upstream more than anything, so if I had to go through an extra menu to grab it I think that would get old for me potentially. But maybe I'm wildly underutilizing git reset's and should defer to the seniors ;) I can get to work on either direction as soon as you give the word though!
@ryand67 hope you're feeling better now :)
Let's go ahead with the one-phase approach described in https://github.com/jesseduffield/lazygit/pull/2047#issuecomment-1193788532
Perfect! And just to make sure I'm understanding correctly, the new ux workflow should be
in branches (3) panel g menu appears with the 7 options listed above with @{branch} being current branch and @{branch-upstream} being current upstream if c is hit it goes to prompt and then a menu with strengths for the custom branch but this should only be the workflow if it's the branches panel
sorry, still have the sick brain fog and want to clarify that last point especially haha. will be working on this tomorrow :)
@ryand67 yep that's all good. I'd also add that if we're opening the reset menu from anywhere else (e.g. commits panel) we should have the option there for typing in a custom ref as well.
Ok, just pushed up some changes. Hope I understood the ask correctly. Very appreciative of you all's patience, and had a lot of fun looking through the project so will hopefully be around more often :)
Also I think we could get the files panel to call the function passing 'HEAD' instead of '@{upstream}' so that we end up with options for HEAD and HEAD@{upstream} i.e. upstream of the current branch. That will probably trip people up who have the muscle memory for resetting to upstream from the files panel but we can just make that change clear in the release notes
Yeah those suggestions make much more sense. Maybe should’ve given the Covid fog another day to clear 😅 Again, appreciate your patience. Will have these up early tomorrow. Thanks!
resetting to upstream from the files panel
Will that be removed, or...? It makes more sense for it to be moved to the Branches panel, not duplicated.
Will that be removed, or...? It makes more sense for it to be moved to the Branches panel, not duplicated.
Yeah we should probably just scrap it. it's trivially easy to navigate to the branches panel to press 'g' on the current branch. I was mostly worried about people who were used to pressing 'g' on the files panel but it wasn't very intuitive in the first place (and also we can already reset to HEAD with shift+D in the files panel)
pushed some changes up, re: the S/M/H difficulty, for the time being I set it to u because I didn't seem to be having any issues with it and IMO it makes sense. But if there any strong feelings about it I can obviously change that around. Hope it's looking better! Feels a lot better now, thanks for the feedback and input! Let me know how it's looking :)
Nice work @ryand67
I like how it works by having 'u' take you to the menu for the upstream. I believe you took issue with that though @mark2185 , if so could you elaborate?
Some things I would change:
- if a menu item will open another menu upon being pressed, we should set OpensMenu to true on the menu item. This gives it some styling that indicates to the user that they can freely press the item without an actual action being performed. So we should set OpensMenu to true for the upstream option and the set custom destination option. BTW you'll want to rebase on master beforehand because I've just merged a PR to make this work because it was actually broken before haha (https://github.com/jesseduffield/lazygit/pull/2071)
- As per the convo above between me and @mark2185 , let's scrap the
createResetToUpstreamMenuhandler in the files controller entirely. We'll see if anybody complains about it being removed.
I believe you took issue with that though @mark2185 , if so could you elaborate?
Not that there's a problem with u, I was just expressing my dissatisfaction about pussyfooting around H not being available in menus :)
I know why it's there, it just makes me frown every time I encounter that problem >:)
Uhhh, I get a crash by pressing g on a branch.
$> gor main.go
2022/07/30 22:52:19 An error occurred! Please create an issue at: https://github.com/jesseduffield/lazygit/issues
*errors.errorString Message for the developer of this app: you've set opensMenu with displaystrings on the menu panel. Bad developer!. Apologies, user
/home/mark/Downloads/gits/lazygit/pkg/app/app.go:52 (0xa3cfd6)
Run: newErr := errors.Wrap(err, 0)
/home/mark/Downloads/gits/lazygit/main.go:154 (0xa4aa30)
main: app.Run(appConfig, common, types.NewStartArgs(filterPath, parsedGitArg))
/usr/lib/go/src/runtime/proc.go:250 (0x438012)
main: fn()
/usr/lib/go/src/runtime/asm_amd64.s:1571 (0x465941)
goexit: BYTE $0x90 // NOP
exit status 1
@ryand67 one more thing: could we use OpensMenu: true on the 'set custom destination' menu item? I've just realised that OpensMenu is actually an inappropriate name for that field: it should be 'opens another popup' or something like that. At any rate we can fix the naming of that field later, but for now let's set that field to true for the custom destination option