emacs-evil / evil-magit

Black magic or evil keys for magit
https://github.com/justbur/evil-magit
GNU General Public License v3.0
272 stars 16 forks source link

Support for forge #54

Closed edkolev closed 5 years ago

edkolev commented 5 years ago

Now that forge's been released, it might be a good idea to consider what binding would be appropriate. I'm not sure if evil-magit is the best place for such a binding to be defined though, evil-collection also seems like an option.

Looking at here, ' is the default binding, which is now used for Submodules by evil-magit.

Linuus commented 5 years ago

Yes! This would be awesome!

yuhan0 commented 5 years ago

As a quick reference, I went over all the current bindings in magit-status-mode, and found the following keys still up for grabs:

None of these options seem particularly mnemonic, but for what it's worth I went with the backtick ` in my own config, mostly because of visual similarity to magit's ' default binding.

justbur commented 5 years ago

I haven't tried forge yet. I'm open to suggestions for the key binding. Q and J are interesting. I was also thinking about @, but I don't have a strong opinion.

edkolev commented 5 years ago

How about using the default ' and binding submodules to something else, would that be an option?

justbur commented 5 years ago

Definitely an option but that seems more disruptive

Linuus commented 5 years ago

I think it’s nice to keep as many default bindings as possible. I do understand if you are hesitant to changing the binding for submodules though.

yuhan0 commented 5 years ago

The issue is submodule and subtree popup bindings are a lower/uppercase pair ( ' / " , originally o / O). Binding submodules alone to something else wouldn't be optimal, and the only other easily accesible pair available is ` / ~ .

@ might be a good option too - it's what Spacemacs binds the magithub popup to, and vaguely meaningful (your git repos being hosted "at" a forge)

But it is rather more inconvenient to type, and I imagine most users would be using forge commands more frequently than subtree/submodule ones?

Linuus commented 5 years ago

I’m fine with @

justbur commented 5 years ago

How about gh?

Linuus commented 5 years ago

I’d prefer one key to be consistent with the other commands.

yuhan0 commented 5 years ago

Same here. As a side note, it would also clash with the spacemacs convention of "gh" navigating to a parent heading.

Linuus commented 5 years ago

Hm, perhaps @ is not a good idea since it would override the evil macro execution command.

justbur commented 5 years ago

I like the following although this is more drastic

I'm not sure if putting push so close to pull is a good idea, but it's easier to remember. Of course, it's going to throw people off for a bit

Linuus commented 5 years ago

I like it 👍

justbur commented 5 years ago

See 49978d0. Note that the changes only take affect after forge is loaded. The default bindings will be in place before that.

Linuus commented 5 years ago

Great! Thanks @justbur

Linuus commented 5 years ago

@justbur Small nitpick... Would it be possible to move the P in the magit-dispatch-popup to be after p? This is how it looks now: screen shot 2019-01-17 at 08 44 26

All the lowercase and uppercase letters are next to each other but p and P.

justbur commented 5 years ago

Yes, try 49978d0

Linuus commented 5 years ago

@justbur I think that is what I am running? Otherwise I wouldn't have forge on F etc?

justbur commented 5 years ago

@Linuus Sorry, I thought I had pushed the change. It's c3eae3b2347c4b1798d6d05de18ad073dbbb27d3 which is available now

Linuus commented 5 years ago

@justbur Cool, looks perfect! 👍

justbur commented 5 years ago

Any thoughts from people in this thread on the new bindings? It was inevitable, but the people who don't like them showed up in #60. I can address some of those problems, but I'm wondering if anyone wants me to revert them entirely here.

yuhan0 commented 5 years ago

I thought it would be a good idea at first, but soon realised that it was hard to keep the p/P distinction straight without stopping to think - once I accidentally typed "p -f u" by muscle memory and overwrote my local changes with the remote instead of force pushing. After that incident I decided to revert to the original bindings with ` bound to Forge.

yuhan0 commented 5 years ago

Having a common binding suddenly change meaning upon installation/loading of a package also seems like a bad idea in terms of user experience. At the least it should be consistent - p pulls even without Forge enabled, and F displays a warning message like

Pulling popup is bound to p, please install Forge to use the F popup

justbur commented 5 years ago

ok, I'll revert it for now.

Linuus commented 5 years ago

For me, the bindings have worked out well. No wrong pushing :) I’m, of course, fine with moving to a better key binding.

Perhaps # is fine?

justbur commented 5 years ago

Thanks @Linuus. I think they're sensible, but do take some getting used to. I think there are legitimate uses for #. I thought of ~ today, which I think is interesting for forge. There's also @, which seems reasonable too.

Miciah commented 5 years ago

What do you think of C (upper-case letter c)? It does not conflict with any key binding from Magit, its Evil binding (evil-change-line) is not useful in the Magit buffer, its position tends to be a little more stable than punctuation marks' across different keyboard layouts, and after all, Forge is for Collaboration.

Linuus commented 5 years ago

As I mentioned above I don’t think @ is good since it would break evil macros. Even though I don’t know if I’ve ever used them in Magit I can imagine they are useful.

syl20bnr commented 5 years ago

Maybe we could use the binding that was used for magit-gh-pulls which becomes obsolete now we have forge. The binding was #.

The issue with ~ is that it can be a dead key with international layouts (which I use 👼 ).

syl20bnr commented 5 years ago

@ is already used by magithub if I'm not mistaken. Maybe magithub will become obsolete because of forge as well.

justbur commented 5 years ago

I'm going with @.

@Linuus I'm not sure macros are more useful than the other options, but you can certainly move the binding if it doesn't work for you.

@syl20bnr I think # is useful, and according to magithub's site most of the functionality is available in forge.

@Miciah C is taken in some of the section maps

syl20bnr commented 5 years ago

Fine with me.

Linuus commented 5 years ago

@justbur No worries :) I’ll try it out. And I probably have never actually used @ in Magit yet so it’s probably fine for me as well :)

Good job 👏🏼