Skip to content
This repository has been archived by the owner on Oct 25, 2022. It is now read-only.

Support for forge #54

Closed
edkolev opened this issue Dec 23, 2018 · 34 comments
Closed

Support for forge #54

edkolev opened this issue Dec 23, 2018 · 34 comments

Comments

@edkolev
Copy link
Member

edkolev commented Dec 23, 2018

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
Copy link

Linuus commented Dec 26, 2018

Yes! This would be awesome!

@yuhan0
Copy link

yuhan0 commented Dec 27, 2018

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

  • 5-9 are digit-argument, not very useful with 1-4 being bound to magit-show-level-* commands
  • H (also bound by magithub and the horizontal-movement setting)
  • J
  • Q
  • i/I both bound to ignore-popup, maybe one could be rebound?
  • p/P both push
  • v/V both activate evil-visual-line
  • ;, `, ~, *, #, %, etc. symbols

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
Copy link
Member

justbur commented Dec 28, 2018

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
Copy link
Member Author

edkolev commented Dec 28, 2018

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

@justbur
Copy link
Member

justbur commented Dec 28, 2018

Definitely an option but that seems more disruptive

@Linuus
Copy link

Linuus commented Dec 28, 2018

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.

Miciah added a commit to Miciah/spacemacs that referenced this issue Dec 28, 2018
Note that Forge binds the ' key to the Forge dispatch popup, but evil-magit
binds the ' key to the submodule dispatch popup (see
emacs-evil/evil-magit#54), and evil-magit's key
binding takes precedence with the vim editing style.  Because Forge's key
binding does not always work, it is omitted from the README.

Note also that Magit binds the % key to its worktree dispatch popup, but
Spacemacs binds the % key to the magit-gitflow popup, and Spacemacs's key
binding takes precedence.  Because there is no available key binding for
the worktree dispatch popup, Forge's key binding under that dispatch popup
is omitted from the README.

* layers/+source-control/github/packages.el (github-packages): Add forge.
* layers/+source-control/github/packages.el (github/init-forge): Load forge
after magit.  Configure forge to use spacemacs-cache-directory.
* CHANGELOG.develop:
* layers/+source-control/github/README.org: Update.
@yuhan0
Copy link

yuhan0 commented Dec 29, 2018

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
Copy link

Linuus commented Dec 29, 2018

I’m fine with @

@justbur
Copy link
Member

justbur commented Dec 29, 2018

How about gh?

@Linuus
Copy link

Linuus commented Dec 30, 2018

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

Miciah added a commit to Miciah/spacemacs that referenced this issue Dec 30, 2018
Note that Forge binds the ' key to the Forge dispatch popup, but evil-magit
binds the ' key to the submodule dispatch popup (see
emacs-evil/evil-magit#54), and evil-magit's key
binding takes precedence with the vim editing style.  Because Forge's key
binding does not always work, it is omitted from the README.

Note also that Magit binds the % key to its worktree dispatch popup, but
Spacemacs binds the % key to the magit-gitflow popup, and Spacemacs's key
binding takes precedence.  Because there is no available key binding for
the worktree dispatch popup, Forge's key binding under that dispatch popup
is omitted from the README.

* CHANGELOG.develop:
* layers/+source-control/github/README.org: Update.
* layers/+source-control/github/packages.el (github-packages): Add forge.
(github/init-forge): Load forge after magit. Configure forge to use
spacemacs-cache-directory.
(github/init-magithub): Disabling injecting issues and pull-requests
sections if forge is installed.
Miciah added a commit to Miciah/spacemacs that referenced this issue Dec 30, 2018
Note that Forge binds the ' key to the Forge dispatch popup, but evil-magit
binds the ' key to the submodule dispatch popup (see
emacs-evil/evil-magit#54), and evil-magit's key
binding takes precedence with the vim editing style.  Because Forge's key
binding does not always work, it is omitted from the README.

Note also that Magit binds the % key to its worktree dispatch popup, but
Spacemacs binds the % key to the magit-gitflow popup, and Spacemacs's key
binding takes precedence.  Because there is no available key binding for
the worktree dispatch popup, Forge's key binding under that dispatch popup
is omitted from the README.

* CHANGELOG.develop:
* layers/+source-control/github/README.org: Update.
* layers/+source-control/github/packages.el (github-packages): Add forge.
(github/init-forge): Load forge after magit. Configure forge to use
spacemacs-cache-directory.
(github/init-magithub): Disabling injecting issues and pull-requests
sections if forge is installed.
@yuhan0
Copy link

yuhan0 commented Jan 1, 2019

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

@Linuus
Copy link

Linuus commented Jan 1, 2019

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

@justbur
Copy link
Member

justbur commented Jan 11, 2019

I like the following although this is more drastic

  • Make p pull
  • Make F forge

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
Copy link

Linuus commented Jan 11, 2019

I like it 👍

sdwolfz pushed a commit to syl20bnr/spacemacs that referenced this issue Jan 14, 2019
Note that Forge binds the ' key to the Forge dispatch popup, but evil-magit
binds the ' key to the submodule dispatch popup (see
emacs-evil/evil-magit#54), and evil-magit's key
binding takes precedence with the vim editing style.  Because Forge's key
binding does not always work, it is omitted from the README.

Note also that Magit binds the % key to its worktree dispatch popup, but
Spacemacs binds the % key to the magit-gitflow popup, and Spacemacs's key
binding takes precedence.  Because there is no available key binding for
the worktree dispatch popup, Forge's key binding under that dispatch popup
is omitted from the README.

* CHANGELOG.develop:
* layers/+source-control/github/README.org: Update.
* layers/+source-control/github/packages.el (github-packages): Add forge.
(github/init-forge): Load forge after magit. Configure forge to use
spacemacs-cache-directory.
(github/init-magithub): Disabling injecting issues and pull-requests
sections if forge is installed.
vv111y pushed a commit to vv111y/spacemacs that referenced this issue Jan 15, 2019
Note that Forge binds the ' key to the Forge dispatch popup, but evil-magit
binds the ' key to the submodule dispatch popup (see
emacs-evil/evil-magit#54), and evil-magit's key
binding takes precedence with the vim editing style.  Because Forge's key
binding does not always work, it is omitted from the README.

Note also that Magit binds the % key to its worktree dispatch popup, but
Spacemacs binds the % key to the magit-gitflow popup, and Spacemacs's key
binding takes precedence.  Because there is no available key binding for
the worktree dispatch popup, Forge's key binding under that dispatch popup
is omitted from the README.

* CHANGELOG.develop:
* layers/+source-control/github/README.org: Update.
* layers/+source-control/github/packages.el (github-packages): Add forge.
(github/init-forge): Load forge after magit. Configure forge to use
spacemacs-cache-directory.
(github/init-magithub): Disabling injecting issues and pull-requests
sections if forge is installed.
@justbur
Copy link
Member

justbur commented Jan 16, 2019

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

@Linuus
Copy link

Linuus commented Jan 16, 2019

Great! Thanks @justbur

@Linuus
Copy link

Linuus commented Jan 17, 2019

@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
Copy link
Member

justbur commented Jan 17, 2019

Yes, try 49978d0

@Linuus
Copy link

Linuus commented Jan 17, 2019

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

@justbur
Copy link
Member

justbur commented Jan 17, 2019

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

@Linuus
Copy link

Linuus commented Jan 18, 2019

@justbur Cool, looks perfect! 👍

@justbur
Copy link
Member

justbur commented Feb 3, 2019

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
Copy link

yuhan0 commented Feb 3, 2019

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
Copy link

yuhan0 commented Feb 3, 2019

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
Copy link
Member

justbur commented Feb 3, 2019

ok, I'll revert it for now.

@Linuus
Copy link

Linuus commented Feb 3, 2019

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
Copy link
Member

justbur commented Feb 5, 2019

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
Copy link

Miciah commented Feb 5, 2019

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
Copy link

Linuus commented Feb 5, 2019

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
Copy link
Member

syl20bnr commented Feb 9, 2019

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
Copy link
Member

syl20bnr commented Feb 9, 2019

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

@justbur
Copy link
Member

justbur commented Feb 16, 2019

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
Copy link
Member

Fine with me.

@Linuus
Copy link

Linuus commented Feb 16, 2019

@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 👏🏼

billy1kaplan pushed a commit to billy1kaplan/spacemacs that referenced this issue Apr 19, 2019
Note that Forge binds the ' key to the Forge dispatch popup, but evil-magit
binds the ' key to the submodule dispatch popup (see
emacs-evil/evil-magit#54), and evil-magit's key
binding takes precedence with the vim editing style.  Because Forge's key
binding does not always work, it is omitted from the README.

Note also that Magit binds the % key to its worktree dispatch popup, but
Spacemacs binds the % key to the magit-gitflow popup, and Spacemacs's key
binding takes precedence.  Because there is no available key binding for
the worktree dispatch popup, Forge's key binding under that dispatch popup
is omitted from the README.

* CHANGELOG.develop:
* layers/+source-control/github/README.org: Update.
* layers/+source-control/github/packages.el (github-packages): Add forge.
(github/init-forge): Load forge after magit. Configure forge to use
spacemacs-cache-directory.
(github/init-magithub): Disabling injecting issues and pull-requests
sections if forge is installed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants