Which key indicator does not update for nested submenus

[minad]

I observed this when looking at the consult submenu, see https://github.com/oantolin/embark/issues/122#issuecomment-770213594.

[oantolin]

Oh, no. This sounds like it needs some restructuring. Currently the indicator function gets called once with the original keymap, and then an entire key sequence in read. I'll think about it. Probably proper support for prefix keys is something worth having, but we could also just try avoiding them in the bindings that come with embark and embark-consult (although that sounds tough, it is getting a little crowded).

[minad]

You could go back to the transient keymap :smiling_imp:

[oantolin]

You could go back to the transient keymap 😈

I was thinking that but not saying it...

[oantolin]

Maybe I will go back to transient maps. Embark used to be pretty buggy when I used transient maps, but I think I figured out how to fix almost all the bugs it used to have. The one thing I don't know how to implement nicely is embark-post-action-hook. Right now, it's trivial: since I run the action myself, I just run the hook afterwards.

In the transient map strategy, I don't run the action, the command loop does. I have access to the action right before it runs and, if it does prompt the user, I also have access during the first minibuffer prompt. I could add self-destructing :after advice to the action command to run the post action hook; and just apologize to @astoff for embark-post-action-hook not supporting anonymous commands.

[minad]

Why not use the post-command-hook for this? Is this not possible? Is it not possible to add advices to lambdas? Alternatively dynamically generate a wrapper command and set this-command to the wrapper (I think I proposed that before). There seem to be many alternatives?

[oantolin]

The problem with the post-command-hook is that it runs after every command, so it runs a little too soon. Say the action is rename-file, a post-command-hook would run after you type the first character of the new name, since you just ran self-insert-command.

One thing I could do is check in the post-command-hook if I'm in a minibuffer for the action, and if so, "postpone it until after the minibuffer exits", that is, remove the post-command-hook and schedule the post-command-hook to be re-added in the minibuffer-exit-hook.

I don't know if it is possible to add advice to lambdas, but I think it isn't, the argument of advice-add is called SYMBOL.

Alternatively dynamically generate a wrapper command and set this-command to the wrapper (I think I proposed that before). There seem to be many alternatives?

I had forgotten about this option, thanks for reminding me!

[astoff]

One thing I could do is check in the post-command-hook if I'm in a minibuffer for the action, and if so, "postpone it until after the minibuffer exits"

Sounds so scary!

[oantolin]

Alternatively dynamically generate a wrapper command and set this-command to the wrapper

After a bunch of experiments it seems this is the winning strategy. I'll try to make the change in the next few days.

[minad]

After a bunch of experiments it seems this is the winning strategy. I'll try to make the change in the next few days.

My suggestion would be to not use a lambda but generate a temporary symbol and fset it to a lambda. This is probably the more robust strategy. But this whole idea could also turn into a memory leak, if the this-commands are somehow logged (the same issue with lambdas).

Maybe define a global variable embark--this-command, set this-command to embark--run-this-command. embark--run-this-command should then contain a check that the embark--this-command variable is non-nil and should properly reset the variable afterwards.

[oantolin]

Maybe instead of a brand new symbol each time, I should base the symbol name on the action that will run. Have all the delete-file actions reuse a name like embark-action--delete-file. That way you only keep around one lambda per distinct action, even if someone is logging this-command. And the hypothetical log would be readable with those names.

[minad]

Yes, that would work too.

[minad]

I believe this is actually a which-key bug, see https://github.com/justbur/emacs-which-key/issues/284. But I wonder why you set the overriding keymap around read-key-sequence. This seems unnecessary. Is this only in order to enable which-key, which unfortunately does not work even with which-key-show-transient-maps=t.

[oantolin]

What is it that seems unnecessary? If I am to use read-key-sequence, the overriding keymap is necessary so that read-key-sequence knows when a full key sequence has been read. Using read-key-sequence is unnecessary in the sense that the whole strategy could be changed to use inversion of control instead: instead of reading the key sequence and running the action, I could use set-transient-map to arrange for Emacs' event loop to run the action.

[minad]

Makes sense!

[xendk]

Any news on this? Reading through https://github.com/raxod502/selectrum/issues/504 (great tips there) I see that you guys are more into the completing-read prompter, but I couldn't quite get the hang of it. Of course this is a personal thing, but I find that which-key helps me learning the key better than the completing-read prompt does (which on the other hand makes it quicker to actually run the command as I don't have to find it. It's a trade-off). @oantolin's suggestion of sticking with Emacs default C-h in order to learn the keys properly does make some sense, but I do like the eager helpfulness of which-key.

I recently switched ivy/counsel for selectrum/consult/maginalia/embark and absolutely love it. Embark have even replaced the hydras I used to have, for a more tight Emacs setup.

[minad]

This issue is fixed by #287. The wiki/README still needs an update. Closing.

[oantolin]

The wiki has been updated.

[oantolin]

I never liked the which-key indicator before, but it seems kind of cool now. I think it's because I know what it took to get here. 😆

[minad]

Oh no, your Emacs will be full-featured soon with which-key. Don't abandon the minimal indicator!

[xendk]

Doesn't quite work for me, nothing happens when i press C and I get Error running timer: (void-variable keymap) in messages...

But I'm totally digging the vindicator, so I'm going with that for the time being.

[minad]

lexical scoping?

[xendk]

Possibly? I've just copied the code from the wiki to use-packages :config section. I must admit that my elisp-fu is too weak for me to figure out how to get the keymap into the lambda elegantly.

[oantolin]

@xendk Did you figure out that problem with the void-variable keymap error? It does sounds like you need to turn on lexical binding as @minad suggested. Just add this to the top of the file where you have the code:

;; -*- lexical-binding: t; -*-

[xendk]

@oantolin No, I didn't, thanks for asking. I kinda fear how many fixes I'll have to apply to my init.el if I do that, but I guess it would be healthy for it.

But maybe I'll just stick with the verbose indicator, it's quite excellent.

[oantolin]

I'm glad the verbose indicator is working out for you. However, if you decide you do want to use the which-key indicator, you don't need to enable lexical-binding for your init.el, you can put the which-key in it's own file with lexical binding and then just load it from your init.el!