Discussion: keybindings to support more functions?

[carlfriedrich]

Check list

• [x] I have read through the README
• [x] I have the latest version of forgit
• [x] I have searched through the existing issues

Environment info

• OS
  • [x] Linux
  • [x] Mac OS X
  • [ ] Windows
  • [ ] Others:
• Shell
  • [x] bash
  • [x] zsh
  • [x] fish

Problem / Steps to reproduce

This came up during a discussion in #251.

fzf supports a list of possible hotkeys that can be bound to custom commands and then be called on list items while browsing them. I personally make use of this in my gss selector by defining these options:

# Use forgit diff on stash enter
export FORGIT_STASH_ENTER_COMMAND='echo {} | cut -d: -f1 | xargs -I % git forgit diff %^1 %'

# Pop stash on alt-enter
export FORGIT_STASH_POP_COMMAND='echo {} | cut -d: -f1 | xargs -I % git stash pop %'

# Drop stash on alt-backspace
export FORGIT_STASH_DROP_COMMAND='echo {} | cut -d: -f1 | xargs -I % git stash drop %'

export FORGIT_STASH_FZF_OPTS="
	--preview-window=top:50%
	--bind='enter:execute($FORGIT_STASH_ENTER_COMMAND)'
	--bind='alt-enter:execute($FORGIT_STASH_POP_COMMAND)+accept'
	--bind='alt-bspace:execute($FORGIT_STASH_DROP_COMMAND)+reload(git stash list)'
	--prompt='[ENTER] show   [ALT+ENTER] pop   [ALT+BACKSPACE] drop > '
"

This makes the stash selector look as follows:

I can now view, apply or drop stashes all from within the same stash list.

I kind of "hijacked" the fzf prompt to display the available keybindings. Something similar could be achieved passing --header "[ENTER] show [ALT+ENTER] pop [ALT+BACKSPACE] drop" --header-first to fzf, which might be a bit cleaner. I just found putting it into the prompt visually more appealing.

@cjappl @sandr01d @wfxr How do you like this? Can you imagine including this kind of functionality into forgit? We could make use of this in other forgit dialogs as well, e.g. keybindings in glo for reverting the selected commit or starting a rebase on it.

Let me know what you think.

[sandr01d]

I do like this functionality and think it would be a great addition. That being said, I think we should stick to the obvious git commands for each scenario and allow adding esoteric ones via variables as you did. E.g. I would consider apply, pop and drop obvious choices for gss, but checkout esoteric, as most people probably don't often use checkout on stashes. There should probably also be a way to override the commands and key bindings and an option to disable them completely. I think if we implement these similar to the pagers this shouldn't clutter the code too much.

[cjappl]

Just spitballing here, instead of having gss which is just a glorified viewer, why don't we do

gss - interactive git stash save (similar to the functionality created by @sandr01d recently) gsd - interactive git stash drop gsa - interactive git stash apply

That may fit more into the current design of forgit, and not add a possibly hard to remember shortcut for our users (that may be mapped to other things.

the current gss is kind of an oddity in forgit in that it doesn't really have any interaction you can have with the output. It's probably my least used one, along with gd, but I could see myself using the 3 above fairly frequently.

An added bonus is that gsd and gsa would be pretty easy to create and could share 99% of their code in some helper function.

[carlfriedrich]

Hey @cjappl, thanks for your opinion here. A few points from my side on that:

1. I definitely need the existing gss to show stashes. I use it multiple times a day. Yes, it's a viewer. So is glo, and I assume we all strongly need that as well (don't you?). Hence I don't see gss as an oddity, it's just glo for stashes. However, even if I personally wouldn't use it, I still find that we shouldn't remove existing functionality, because it will always break someone's workflow.

2. I see that your proposed functions definitely fit more into the existing forgit design. That's why I opened this discussion, it's a proposal to extend forgit's workflow capabilities for the sake of efficiency. When I find myself browsing through stashes using gss and want to drop a stash, it would annoy me having to quit the current viewer and enter a new command to show me the exact same list I saw just before to execute a different function.

3. With my proposal from the screenshot and the code above, users wouldn't have to remember the shortcuts from their head, because they are displayed on the screen when you need them.

4. I agree there's a risk that the keybindings collide with user's existing shortcuts. As @sandr01d already suggested, we could work against this by making the shortcuts configurable. fzf supports a huge list of possible shortcuts that are configured in a human-readable way, so that we could easily pass these via configurable env variables and even use them for displaying them in the help text line (which is currently hard-coded in my code snippet above).

5. I don't see the "bonus" of your proposed functions being easy to create. Yes, they would share lots of code with other forgit functions, but this is because they add redundancy. Just like I wrote above: we're using three different commands to display the exact same list, just to do different things with our enter key afterwards.

That being said, no matter if we integrate the keybindings into the upstream forgit code or not, I will definitely keep my setup like described above, because that's what I find most efficient. forgit improved my workflow a lot, so I just like to keep improving forgit to make other people's workflow even more efficient.

[cjappl]

Yeah I see your points for sure @carlfriedrich. I think the "maintain backwards compatibility" argument is the strongest, we shouldn't break bwc lightly.

1. I would say from my side, I use the viewers (gd, glo, and gss) the least of any of the commands, and I use the actions extremely frequently. That's probably why my initial gut was to go to the new action focused commands.

2. Yeah, I definitely see that.

3. / 4. That screenshot is pretty nice. Definitely support adding some configurable shortcuts if we go this way! :)

Let me give your config a try for a bit and see how it feels in practice.

forgit improved my workflow a lot, so I just like to keep improving forgit to make other people's workflow even more efficient

100% agreed on that one. That's why we're all here working on it together 🎉

Thanks for the thoughtful responses. Interested to hear what @wfxr thinks, and I'll report back when I have some soak time with your configuration.

[cjappl]

Wow. Just used this config for the first time and I have to say it's extremely nice. Worked exactly as intended, easy to customize to new keybinds.

I guess my only hangup for mainlining this to forgit is "this is very different than anything that forgit does so far, is that ok??"

Some ways around that question would be:

• Put this as part of the readme, so people can implement if they desire
• add it as an option extension

Or we could just say "hey this is nice, let's just do it. Again defer to @wfxr when he is available.

Overall very very useful. Will be using it locally for the foreseeable future @

[carlfriedrich]

@cjappl Great to hear that you like it. :-)

Yes, you are right, it will be a new kind of feature for forgit. Again some thoughts on that from my side:

1. Adding it to the README is something I would consider as well. But since this change only adds something and doesn't change any existing behavior, do you think that there are people who will actually be disturbed by it? Otherwise, from a user's perspective, I don't see why we shouldn't directly add it to the code instead of only to the README (current issue #265 shows that people obviously don't even read a single line of the README).

2. If we're adding this as a feature, we should apply it to other forgit functions as well to make the feature set consistent. The following forgit functions all present the same list:

Commit list

• forgit::log
• forgit::rebase
• forgit::checkout_commit
• forgit::revert_commit
• forgit::fixup

So we could add the lower four as keybindings in forgit::log (that's actually what I wanted to configure in my personal setup next anyway).

Changed files list

• forgit::diff
• forgit::add
• forgit::checkout::file

Here we could add the lower two as keybindings in forgit::diff.

Branch list

• forgit::checkout::branch
• forgit::cherry::pick::from::branch
• forgit::branch::delete

For this list we don't have a "viewer" function, so we could add a forgit::branch function (gb?) which adds the above three functions as keybindings.

[cjappl]

(Haven’t forgotten about this, need to look with fresh eyes after the holiday. Happy holidays and happy new year everyone!)

On Wed, Dec 21, 2022 at 12:34 AM, Tim @.***> wrote:

@.***(https://github.com/cjappl) Great to hear that you like it. :-)

Yes, you are right, it will be a new kind of feature for forgit. Again some thoughts on that from my side:

Adding it to the README is something I would consider as well. But since this change only adds something and doesn't change any existing behavior, do you think that there are people who will actually be disturbed by it? Otherwise, from a user's perspective, I don't see why we shouldn't directly add it to the code instead of only to the README (current issue #265 shows that people obviously don't even read a single line of the README).

If we're adding this as a feature, we should apply it to other forgit functions as well to make the feature set consistent. The following forgit functions all present the same list:

Commit list

• forgit::log
• forgit::rebase
• forgit::checkout_commit
• forgit::revert_commit
• forgit::fixup

So we could add the lower four as keybindings in forgit::log (that's actually what I wanted to configure in my personal setup next anyway).

Changed files list

• forgit::diff
• forgit::add
• forgit::checkout::file

Here we could add the lower two as keybindings in forgit::diff.

Branch list

• forgit::checkout::branch
• forgit::cherry::pick::from::branch
• forgit::branch::delete

For this list we don't have a "viewer" function, so we could add a forgit::branch function (gb?) which adds the above three functions as keybindings.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[cjappl]

Alright I'm back.

I definitely see all your points, and like the idea of making things consistent (some command for doing things with stashes, one for commits, one for branches). Makes sense to me.

Definitely want to hear @wfxr opinion on it.

I also have something in the back of my head that is on the fence about it. I personally like the flow of "look at something with 'glo' then act on something with a "verb" like 'grc'. The paradigm that we have been using so far.

I'm wondering if the gain in functionality of this change is worth the extra maintenance burden etc, as well as shifting the paradigm. This is basically adding a second way to do a lot of things - is that a DRY violation?

I worry about 'forgit' trying to become a swiss army knife, and having too much functionality not used by everyone, everyone with different configs running different versions of the same commands that we have to support

Maybe this worry is overblown, but I can't help but to think about it. That is absolutely not to say this isn't nice to have, I just worry about implementing a "second interface" for a lot of core commands.

What do you think @carlfriedrich ?

[sandr01d]

@cjappl Great to hear that you like it. :-)

Yes, you are right, it will be a new kind of feature for forgit. Again some thoughts on that from my side:

1. Adding it to the README is something I would consider as well. But since this change only _adds_ something and doesn't change any existing behavior, do you think that there are people who will actually be disturbed by it? Otherwise, from a user's perspective, I don't see why we shouldn't directly add it to the code instead of only to the README (current issue [zplug: forgit is not available #265](https://github.com/wfxr/forgit/issues/265) shows that people obviously don't even read a single line of the README).

2. If we're adding this as a feature, we should apply it to other forgit functions as well to make the feature set consistent. The following forgit functions all present the same list:

Commit list

* `forgit::log`

* `forgit::rebase`

* `forgit::checkout_commit`

* `forgit::revert_commit`

* `forgit::fixup`

So we could add the lower four as keybindings in forgit::log (that's actually what I wanted to configure in my personal setup next anyway).

Changed files list

* `forgit::diff`

* `forgit::add`

* `forgit::checkout::file`

Here we could add the lower two as keybindings in forgit::diff.

Branch list

* `forgit::checkout::branch`

* `forgit::cherry::pick::from::branch`

* `forgit::branch::delete`

For this list we don't have a "viewer" function, so we could add a forgit::branch function (gb?) which adds the above three functions as keybindings.

Why not have all the keybindings available for all the commands in their respective group? I think this would make it even more consistent and also more flexible in certain situations. E.g. When I stage files, I often find some files that I've made changes on which are no longer necessary and I want to checkout those files instead. Having the ability to checkout files in ga would be very nice in this kind of situation. Sure I could use gd instead (when this is the command the keybindings are available with), but this is a bit counter intuitive and assumes that I have the files I want to checkout in mind before looking at my changes while I stage.

[carlfriedrich]

I'm wondering if the gain in functionality of this change is worth the extra maintenance burden etc, as well as shifting the paradigm. This is basically adding a second way to do a lot of things - is that a DRY violation?

@cjappl I don't think that the maintenance burden is as huge as you might think. What would we have to do?

1. Define variables and default values for each keybinding
2. Define functions for the keybdindings' actions
3. Add these variables and functions as arguments to each fzf call

The functionality is already implemented, as we already have everything in the forgit functions. We can share the code for each keybinding with the according forgit function. This requires some refactoring, but it would keep the code DRY.

Why not have all the keybindings available for all the commands in their respective group? I think this would make it even more consistent and also more flexible in certain situations. E.g. When I stage files, I often find some files that I've made changes on which are no longer necessary and I want to checkout those files instead. Having the ability to checkout files in ga would be very nice in this kind of situation. Sure I could use gd instead (when this is the command the keybindings are available with), but this is a bit counter intuitive and assumes that I have the files I want to checkout in mind before looking at my changes while I stage.

@sandr01d Yes, we can definitely do that.

[carlfriedrich]

@wfxr Very curious about your opinion on this. Would appreciate if you find a few minutes to comment here.

[wfxr]

@carlfriedrich I'm very sorry, it's been hard for me to maintain the project recently. I think your idea is great. I only suggest not to list the shortcuts on the prompt. Maybe we can use readme and/or release notes to introduce these features. Another option is to allow the user to configure whether to display these shortcuts in prompt line. @carlfriedrich @cjappl What do you think?

[carlfriedrich]

@wfxr No worries, thanks for your input! Do you want the shortcuts not in the prompt or not visible at all? An alternative would be to use fzf's --header argument, e.g. like this:

I think that would be cleaner. Would you prefer that?

We can still make the display configurable, though. And of course we will add the shortcuts to the README.

[cjappl]

Cool! I'm all for it if you all are down 😀

I have found the original git stash adjustment quite useful.

I have no strong opinions on the prompts being on the top or bottom. I do like having them displayed though, to remind myself which is which. I suppose in a later commit we could have a variable that turns them on or off (but not necessary for v1 IMO)

Let me know if you need help implanting @carlfriedrich ! Otherwise happy to review and test

[wfxr]

I think that would be cleaner. Would you prefer that?

We can still make the display configurable, though. And of course we will add the shortcuts to the README.

@carlfriedrich --header way looks better! And it will be even better if we make the display configurable!

[carlfriedrich]

Great, thanks for your feedback everyone! I will prepare a PR as soon as I find time for it. Might take a while, though, since I am quite busy atm.

[carlfriedrich]

Just a quick heads-up on this: unfortunately implementing these shortcuts are harder than I thought, mostly because we eventually have to reload the input list for fzf after pressing a shortcut, while the command for generating the input list is non-trivial. Maybe things will get easier with #324, so I will wait for that.