Remap of magit-submodule suffix in magit-dispatch transient prefix causes forge to print an error

[sin-ack]

Hi. This line:

evil-collection/modes/magit/evil-collection-magit.el

Line 574 in 6709c1e

(magit-dispatch "o" "'" magit-submodule)

remaps magit-submodule's key in the magit-dispatch transient prefix from o to '. Since magit/forge@9fbd00e, forge will now print an error when a Magit buffer is first visited:

Cannot insert ("N" "Forge" forge-dispatch) into magit-dispatch; o not found

as it cannot find the o key to insert its own key before. Not quite sure how to approach fixing this, because Forge can technically do that just fine, but remapping the keys should be fine too.

[jojojames]

Hmnn.. this one is tricky since it seems like those changes are at the top level in forge.el. Any suggestions @sin-ack ?

[tshu-w]

This only happens if we run (evil-collection-magit-setup) before (require 'forge), we can (require 'forge) before (evil-collection-magit-init) to avoid this annoying error:

  (defun evil-collection-magit-setup@override ()
    "Set up `evil' bindings for `magit'."

    (evil-collection-define-key 'normal 'magit-blame-mode-map
      "q" 'magit-blame-quit)
    (evil-collection-define-key 'normal 'magit-blame-read-only-mode-map
      "q" 'magit-blame-quit)

    (require 'forge)
    (evil-collection-magit-init))

or seperate forge setup from magit in evil-collections.

[tshu-w]

and in latest forge forge-dispatch is now bound to N instead of ' https://emacsair.me/2021/10/14/forge-0.3/

[jojojames]

We don't want to forcer equire forge as not everyone uses it (me included).

[tshu-w]

Sorry, I wasn't clear, require forge is a temporary solution to an annoying error if the user is using forge (otherwise they wouldn't encounter this error).

[jojojames]

@tarsius Would you be able to help here?

How evil-collection is doing it right now is using with-eval-after-load on packages to then rebind/bind new keys/etc.

So it goes:

magit loads -> evil-collection-magit loads (had forge related tweaks too) loads (changes a key popup key) -> forge loads -> expected key is gone -> errors out since key is gone

We could move forge integrations out of evil-collection-magit but I think the problem will still occur as the popup key being changed is in 'magit layer'.

Any suggestions welcome for how to work around this and/or make this more bulletproof in the future will be super helpful.

[tarsius]

I would be willing to add a forge-add-default-bindings variable that you could set to nil before loading forge. You would then have to bind yourself all the commands that forge binds in magit's maps and transients by default, but in turn you wouldn't have to undo anything.

magit loads -> evil-collection-magit loads (had forge related tweaks too) loads (changes a key popup key) -> forge loads -> expected key is gone -> errors out since key is gone

I assume you do that to avoid loading forge for users who don't use it. That works in so far as you don't have to load it when magit loads, but when a transient prefix is invoked, then it makes sure all suffix commands are actually defined. If a command is autoloaded, then it is loaded at that time. (If a command is neither loaded nor autoloaded, then that is an error.)

So once the user invokes magit-dispatch then forge is loaded anyway, and you might as well load it right after loading magit (but before changing any magit and forge bindings). Since that worked until the recent breakage, I assume that enabling the magit layer of evil-collection always installs forge, even for people who are not using it.

This is how it works for non-evil users too, they are advised to load forge like so:

(with-eval-after-load 'magit
  (require 'forge))

Of course, apparently unlike evil-collection+magit users, they have a choice in whether they install forge and add these lines to their init file. It seems to me that it would make sense to add a separate forge layer to avoid installing, and as we have no learned also loading, forge for magit users that don't want forge.

Here too improvements could be made on my side, this time in transient. I could allow this:

(transient-insert-suffix 'magit-dispatch "A"
  '("!" "Ceci n'est pas une commande" no-command
    :if (lambda () (require 'no-library nil t))))

But note how this would load no-library even if the user never types !.

Currently this does not work because :if is not evaluated until after we check if the command is loaded/loadable and error out if it is not.

[jojojames]

I would be willing to add a forge-add-default-bindings variable that you could set to nil before loading forge. You would then have to bind yourself all the commands that forge binds in magit's maps and transients by default, but in turn you wouldn't have to undo anything.

Sounds good to me.

I assume you do that to avoid loading forge for users who don't use it.

The goal was to avoid loading/requiring the setup file/setup block up code if the feature is never loaded.

magit loads, but when a transient prefix is invoked, then it makes sure all suffix commands are actually defined.

I reported something similar (maybe same?) here at some point when Transient was new. magit/transient#61

I ended up just hacking it up here: https://github.com/jojojames/matcha/blob/237a52964613c1be39565896545bb09a40c58d11/matcha-me.el#L281 by wrapping the autoloaded function with another function that calls it to avoid the auto-loading. Not perfect... but good enough for me.

[tarsius]

I added the forge-add-default-{sections,bindings} options on the no-defaults branch, but because of the involved autoload I am not sure this would actually work. Could you please try it out?

[jojojames]

Thanks @tarsius ! I made some initial changes here (https://github.com/emacs-evil/evil-collection/pull/549/files) and it looks okay to me but I don't really use forge so not sure if anything is missing. @sin-ack Can you pull @tarsius 's changes for forge on his no-defaults branch and evil-collection's forge branch and see if it works for you?

If not we can probably go with the current set of changes on both branches.

[tshu-w]

Can you pull @tarsius 's changes for forge on his no-defaults branch and evil-collection's forge branch and see if it works for you?

This works for me.