ediff-mode-map is invalid

[CeleritasCelery]

ediff-mode-map is nil outside of an ediff session (even after the feature is loaded). This means that when running the hooks in evil-collection-setup-hook the keymap name is passed (ediff-mode-map), but it does not point to a valid keymap (i.e. does not return t for keymapp). This causes failures in startup if a function is defined for evil-collection-setup-hook. I am not sure whether the best solution is to not pass the invalid keymap name, or somehow run the hook while an ediff session is active.

[Ambrevar]

Some other modes suffer from the same issue, see EMMS for an example workaround.

Can you give a recipe to reproduce the issue?

[CeleritasCelery]

Here is my example

(add-hook 'evil-collection-setup-hook
          (defun unmap-leader (_mode keymaps)
              (when keymaps
                 (general-define-key
                  :states 'normal
                  :keymaps keymaps
                  "SPC" nil))))

(evil-collection-init)
(ediff-files "file1" "file2")

[jojojames]

@CeleritasCelery Could you clarify what's wrong with the above snippet? (I haven't tried it out. :))

As I understand it, keymaps should be a quoted list of keymaps given the current mode, so:

(defconst evil-collection-ediff-maps '(ediff-mode-map))

Given the definition of general-define-key->keymaps keywords:

Unlike with normal key definitions functions, the keymaps in KEYMAPS should be
quoted (this allows using the keymap name for other purposes, e.g. deferring
keybindings if the keymap symbol is not bound, optionally inferring the
corresponding major mode for a symbol by removing "-map" for :which-key,
easily storing the keymap name for use with ‘general-describe-keybindings’,
etc.). Note that general.el provides other key definer macros that do not
require quoting keymaps.

I would expect that to work? @noctuid should have a better idea here.

As for the general problem of the keymap not being available after the feature is loaded, I'm open to other ideas, including an extra hook run when the mode runs so that users can choose to do some additional setup at that time.

[CeleritasCelery]

Yes I would expect that to work to, and it works for every other feature I have tested besides ediff. The problem is not with general, but with the ediff-mode-map. Most features define a keymap that is defined when the feature is loaded. And evil-collection-setup-hook is called with-eval-after-load so it is expecting the keymap to exist by the time the feature is done loading. But that is not true for ediff, the ediff-mode-map only exists after ediff-mode has been called. This means that the hook fails because even after the feature is loaded there is no keymap called ediff-mode-map. Let me know if that doesn't make sense.

I think your suggestion of running evil-collection-setup-hook at a different time, or having a different hook, is going in the right direction. But I don't know what should be changed because the current implementation works for all other features AFAIK.

[jojojames]

it is expecting the keymap to exist by the time the feature is done loading.

What is expecting the keymap to exist? The setup-hook just passes back a quoted list of keymaps.

For example, I expect this should work:

(add-hook 'evil-collection-setup-hook (lambda (_ _))))

This means that the hook fails because even after the feature is loaded there is no keymap called ediff-mode-map.

If this is true, the above snippet should fail right?

Let me know if that doesn't make sense.

Yes, please! We're probably talking about different things so lets drill down exactly what is failing.

Again, I can see the general case of the keymap not being available and someone wanting to do something with the value of the keymap which is why for ediff, it doesn't work but I don't understand how your recipe fails given general is supposed to work with a quoted list of keymaps (I'd expect it to be able to handle deferring keymaps).

[CeleritasCelery]

What is expecting the keymap to exist? The setup-hook just passes back a quoted list of keymaps.

general-define-key

If this is true, the above snippet should fail right?

No, the above snippet is fine because it is not checking for existence of the keymaps. It is not the "hook" that fails per se, but the functions in the hook that try and act on the keymaps.

I'd expect it to be able to handle deferring keymaps

Hmm, looks like you might be on to something here. In the desciption for general it says

Allows deferring keybindings until the specified keymap exists (no need to use (with-eval-after-load) (like evil-define-key)

which makes it seems like it should handle this case fine. I will have to look into that.

[CeleritasCelery]

noctuid/general.el#59

This looks like an issue with ediff-mode itself. I am not sure what can be done from evil-collection (or even if anything should be done). but it seems problematic to pass in a keymap that is not defined. Maybe the best course of action would be to update the documentation to let users know that they need to check for the keymap before trying to use it and leave everything else as is.

[noctuid]

The way ediff (and eshell) handle their keymaps is really broken. Both packages have FIXMEs, so it's a known issue. Ideally someone would submit a patch to fix this or maybe bring it up on the mailing list. I don't use either package, and since there is a workaround, it's a not a huge priority for me to fix.

[Ambrevar]

I suppose that deferring the desired change to ediff-mode-hook would work. Something like (untested):

(add-hook 'evil-collection-setup-hook
          (lambda (_mode keymaps)
              (add-hook 'ediff-mode-hook
                (lambda ()
                 (... keymaps ...)))))

And yes, we should report upstream.

[mohkale]

tbh. I think ediff mode should just be setup like a regular mode and offer a hydra to describe available bindings. The current solution hardcodes a tonne of stuff including keybindings into help messages. I can't see anyway of changing it meaningfully without breaking backwards compatibility... evil-ediff seems to just use regexp to change the help messages at runtime to keep them in sync. so maybe we should just offer another package which does what ediff does in a modern sense... I'd be willing to take on the task... but I don't even understand half the features ediff offers, so I doubt I'll be of much use here 😞.

[jojojames]

hydra to describe available bindings.

Dunno how I feel about this. I don't use hydra myself. Maybe if we do a check around hydra existing as a feature (e.g no hard dependency on hydra). I'm assuming you mean "describe" in the sense that you can also "call" the action afterwards.

I can't see anyway of changing it meaningfully without breaking backwards compatibility...

Which ones? I'm fine with breaking backwards compatibility as long as the new change is better.

[mohkale]

Maybe if we do a check around hydra existing as a feature (e.g no hard dependency on hydra). I'm assuming you mean "describe" in the sense that you can also "call" the action afterwards.

as it stands ediff mode shows a little help buffer which is collapsed by default but with ? it expands to show a list of bindings you can invoke. like so.

it behaves very much like how a hydra does, except where hydras are programmable both in interface and mappings, ediff seems to have hardcoded quite a lot of it behind the scenes into strings and a function which generates local keymaps for each ediff session. This makes customisation kind of a pain, because its hard to know where, when and how something is bound and you need to make sure the help messages are kept in sync with them. A hydra behind the scenes creates custom keymaps for each hydra (making adding new bindings easy), and lets you include elisp in help messages evaluated every time they're displayed so keeping things organised would be easier. I've got a similair sort of hydra (half stolen from spacemacs) for perspectives which does something quite like this.

I agree that installing hydra as a prerequesite for ediff is somewhat harsh.

Which ones? I'm fine with breaking backwards compatibility as long as the new change is better.

Not really compatibility so to speak, though tbh I'd want to get rid of the help buffer (if you've got so many bindings you can't remember them, thats what hydras are great for). It's just making everything more complicated to program IMO (doing so I believe will break evil-ediff-mode because it alters the regexp of the string in the help message). Then I'd stop defining ediff-mode-map as a map local to each ediff session, but I'm sure there's a good reason it's like that (something to do with all the other features ediff has (which I haven't explored)), which would lead to deprecating the ediff-keymap-setup-hook honestly I say good riddance. I'm not familliar enough with the code base to say what else should be changed... but those are my imprompt suggestions for anyone brave enough to tackle this. 😄

[jojojames]

Documented.