add which-key integration

[blaenk]

Continuing our discussion from #22.

I just started creating spacemacs-like bindings where I have a prefix like SPC and you can drill down from there, paired with which-key, and it's pretty cool but it kind of becomes a pain to have to call a different function to label the prefix keys that arise from this setup. It'd be great if we could have a way to label them via general.el, perhaps something like was discussed in #22, a more flexible form to which the current form is expanded:

(general-define-key :blah 'okay
  "C-x k" '(:prefix t :which-key "some prefix")
  "C-x k f" 'some-cmd
  "C-x k b" '(:command other-cmd :which-key "kill buffer"))

So the first is a prefix, which I guess it can either detect from the :prefix t (perhaps you can devise another way if you prefer). :which-key can be applied to any binding though.

Note that the second style, the current style, would be rewritten to the third style, i.e. '(:command some-cmd), so that you can then handle them all uniformly. Or perhaps that's not necessary, just a suggestion.

This kind of provides some form of type hinting too considering the issue you were running into earlier, for example we can introduce :keymap helm-command-map to bind a key to a keymap and so it knows that it has to wait for that keymap to load before actually creating the bind, or perhaps generating an autoload for it or whatever, and we can also use :package so we can specify to which package that keymap belongs.

This would be more flexible and would allow for future functionality, all while not bothering current users or the general simple case.

This is slightly unrelated but I was wondering also that it feels like binding to nil to unbind doesn't seem very intuitive. bind-key has unbind-key for example. Rather than introducing yet another function though perhaps it would be cool if we could "bind" to :unbind or :unset or something, although that's such a minor cosmetic thing that's probably not worth the effort, but just something I was thinking about.

[noctuid]

I added initial support for these "extended definitions" (no documentation in the readme yet); they are user extensible. Your example could be written like this:

(general-define-key :prefix "C-x k"
  "" '(:ignore t :which-key "some prefix")
  "f" 'some-cmd
  "b" '(other-cmd :which-key "kill buffer"))

:ignore will prevent a binding from being created (here it is specified just to create the which-key description). Additionally, you can specify :mode to use which-key-add-major-mode-key-based-replacements instead.

"Autoloading" keymaps like in bind-key is also implemented. You can specify :package globally or locally in the definition.

For other bindings, :command is optional, and you can instead just put the command (or nil or whatever) as the first element.

Let me know if you have any problems. Feel free to make another issue if you have any suggestions for other keywords.

Rather than introducing yet another function though perhaps it would be cool if we could "bind" to :unbind or :unset

I could just have :unbind translated to nil, but I think I'd rather just keep unbinding the same as it normally is.

[blaenk]

Amazing work! I'll give it a shot.

I could just have :unbind translated to nil, but I think I'd rather just keep unbinding the same as it normally is.

Yeah I'm ok with nil.

[blaenk]

I'm gonna give this a shot now. By the way, I imagine and hope that this will also be awesome for handling the case of multiple prefixes, similar to what we're talking about in #28. That is, I previously had this very annoying configuration:

  (which-key-add-key-based-replacements
    "SPC b" "buffer"
    "SPC b m" "move"

    "SPC c" "flycheck"
    "SPC e" "emacs"
    "SPC f" "frame"

    "SPC g" "git"
    "SPC g g" "gist"

    "SPC k" "kill"
    "SPC k f" "frame"
    "SPC k w" "window"

    "SPC h" "helm"

    "SPC o" "open"
    "SPC o a" "all"

    "SPC p" "projectile"

    "SPC t" "toggle"
    "SPC t r" "rainbow"

    "SPC w" "window"
    "SPC w s" "split"
    )

but naturally it only applied to my SPC prefix. I imagine one added benefit of your integration with which-key is that it'll work with all of them: global, non-normal, and regular prefix.

[blaenk]

Ah I think I misunderstood. So it seems that :which-key is tied to the definer's :prefix right? I was doing something like this:

(my-map
  ;; kill things
  "k" '(:ignore t :which-key "kill")
  "k e" 'save-buffers-kill-terminal
  "k b" 'my-kill-this-buffer
  "k f" 'delete-frame
  "k o f" 'delete-other-frames
  "k o w" 'delete-other-windows

  ;; windows
  "w" '(:ignore t :which-key "window")
  "w f" 'my-pop-to-frame
  "w s v" 'evil-window-vsplit
  "w s h" 'evil-window-split
  "w s p" 'my-split-with-previous-buffer

  ;; frames
  "w" '(:ignore t :which-key "frame")
  "f o" 'other-frame
  "f f" 'toggle-frame-fullscreen

  ;; buffers
  "w" '(:ignore t :which-key "buffer")
  "b b" 'bury-buffer
  "b o" 'my-switch-to-previous-buffer
  "b s" 'my-force-save)

I'll split it out into the separate ones. Besides it's what I'd do once we get ad-hoc prefixes with :infix. Still I think it would've been cool to be able to specify arbitrary which-key descriptions like I have in my example. Is that possible?

[blaenk]

The backtrace of the error I get when I try that is:

As an image because I haven't figured out how to "sanitize" the buffer for copying and pasting, since it includes the binary byte compiled data or something.

[blaenk]

Yeah I guess I'll try this once we get ad-hoc prefixes cause otherwise I'll only be able to apply it to one prefix like SPC.

But also I'd be interested in the ability to specify arbitrary which-keys like I showed in my most recent example. Because for example consider this part:

  "w" '(:ignore t :which-key "window")
  "w f" 'my-pop-to-frame

  "w s" '(:ignore t :which-key "split")
  "w s v" 'evil-window-vsplit
  "w s h" 'evil-window-split
  "w s p" 'my-split-with-previous-buffer

See how I was able to label the new level of binds (for w s) within the same call. If that's not possible it means I have to create a separate call which can get tedious. However, if it's too much trouble, I can live with it. I would need it to obey :infix though so I can then do something like (my-map :infix "w s" ...).

[noctuid]

I imagine one added benefit of your integration with which-key is that it'll work with all of them: global, non-normal, and regular prefix.

Yes, they should be added for every prefix.

Ah I think I misunderstood. So it seems that :which-key is tied to the definer's :prefix right? I was doing something like this

Your definition looks right to me. Previously I was giving the keys to the which-key functions as they would be given to define-key. This works for some keys (it worked for a prefix of C-c), but it won't work for SPC. It should be fixed now. As for the error with void-variable mode, I'm not sure why that's happening. Maybe it had to do with the placement of eval-after-load. Let me know if it is fixed now.

[blaenk]

I imagine void-variable mode was happening because it was trying to use which-key-add-major-mode-key-based-replacements even though I didn't supply a :mode. It looks like you now conditionally call it based on whether a :mode was found, so hopefully that helps. I'll report back.

[blaenk]

Nevermind looks like you had that before. Not sure why that was happening.

[blaenk]

Yeah I think it was because of the variable being declared before and outside of eval-after-load, something about lexical binding.

[blaenk]

I ran into it myself a while back.

[noctuid]

Yeah it looks like now the function arguments aren't available with the new placement. Thanks, I'll look into/fix it. It shouldn't be a problem if which-key is already loaded though.

[noctuid]

Sorry about that. I don't know why I was thinking that eval-after-load was a macro (especially since you generally quote the form).

[blaenk]

No worries! 👍 💃

[blaenk]

Personally I always use with-eval-after-load now since it always works for my use cases and I'd rather not worry about quoting or macro expansion for simple use cases. Maybe that + lexical-binding: t in your file can simplify things? I have no idea.

[blaenk]

I can confirm this all seems to be working perfectly! Check it out, it's pretty awesome!! 🎉

I'm gonna clean this up but I put it this way to show the various things now possible.

(my-map
  ;; kill things
  "k" '(:ignore t :which-key "kill")
  "k e" 'save-buffers-kill-terminal
  "k b" 'my-kill-this-buffer
  "k f" 'delete-frame
  "k o f" 'delete-other-frames
  "k o w" 'delete-other-windows

  ;; frames
  "f" '(:ignore t :which-key "frame")
  "f o" 'other-frame
  "f f" 'toggle-frame-fullscreen

  ;; buffers
  "b" '(:ignore t :which-key "buffer")
  "b b" 'bury-buffer
  "b o" 'my-switch-to-previous-buffer
  "b s" 'my-force-save)

(my-map :infix "w"
  ;; ;; windows
  "" '(:ignore t :which-key "window")
  "f" 'my-pop-to-frame

  "s" '(:ignore t :which-key "split")
  "s v" 'evil-window-vsplit
  "s h" 'evil-window-split
  "s p" 'my-split-with-previous-buffer)

[blaenk]

Also this works as expected:

  "b b" '(:command bury-buffer :which-key "bury")

[blaenk]

Also what you said, putting the command as the first element:

  "b b" '(bury-buffer :which-key "bury")

[blaenk]

I think I found a problem. When I use '(:keymap some-map) with my own wrapper in :general, I get wrong type argument: symbolp, (:keymap projectile-command-map). However if I eval-last-sexp the wrapper sexp, I get no errors.

(use-package projectile
  :demand t

  :general
  ("C-M-/" 'helm-projectile-ag)

  (my-map
    "p" '(:keymap projectile-command-map)
    "e e" 'my-edit-inits)
)

So that gives that error, but if I eval the (my-map ...) sexp I get no such error and the bind works.

[blaenk]

It expands to this:

(progn
  (progn
    (require 'package)
    (use-package-ensure-elpa 'projectile))
  (unless
      (fboundp 'helm-projectile-ag)
    (autoload #'helm-projectile-ag "projectile" nil t))
  (unless
      (fboundp
       '(:keymap projectile-command-map))
    (autoload
      #'(:keymap projectile-command-map)
      "projectile" nil t))
  (unless
      (fboundp 'my-edit-inits)
    (autoload #'my-edit-inits "projectile" nil t))
  (if
      (not
       (require 'projectile nil 'noerror))
      (ignore
       (message
        (format "Cannot load %s" 'projectile))))
  (ignore
   (general-define-key "C-M-/" 'helm-projectile-ag)
   (my-map "p"
     '(:keymap projectile-command-map)
     "e e" 'my-edit-inits)))

[blaenk]

Seems like a problem with the autoloads.

[blaenk]

I remember you had said that in :general the :package would be inferred, but I'm not sure if you did implement that or not, so I added it in and it still expands to the same (with :package projectile included now):

(progn
  (progn
    (require 'package)
    (use-package-ensure-elpa 'projectile))
  (unless
      (fboundp 'helm-projectile-ag)
    (autoload #'helm-projectile-ag "projectile" nil t))
  (unless
      (fboundp
       '(:keymap projectile-command-map :package projectile))
    (autoload
      #'(:keymap projectile-command-map :package projectile)
      "projectile" nil t))
  (unless
      (fboundp 'my-edit-inits)
    (autoload #'my-edit-inits "projectile" nil t))
  (if
      (not
       (require 'projectile nil 'noerror))
      (ignore
       (message
        (format "Cannot load %s" 'projectile))))
  (ignore
   (general-define-key "C-M-/" 'helm-projectile-ag)
   (my-map "p"
     '(:keymap projectile-command-map :package projectile)
     "e e" 'my-edit-inits)))

[blaenk]

Ohhh okay it seems like it's simply the case that you haven't implemented :general integration yet. I guess you wanted to see if this worked out before moving on to that, makes sense! Yeah everything works great outside of :general (i.e. as a regular function call).

[noctuid]

Yeah I just hadn't changed the handler yet. Try it now.

Personally I always use with-eval-after-load now since it always works for my use cases and I'd rather not worry about quoting or macro expansion for simple use cases. Maybe that + lexical-binding: t in your file can simplify things? I have no idea.

The quoting isn't too hard to read in this situation, but maybe I'll switch to using closures in the future. I probably should be using lexical binding anyway since it's faster (I don't know if it would matter at all for this package though).

[blaenk]

Seems to work perfectly well! Feel free to close this unless you have more prepared!

[blaenk]

Hey I think I might have found one last thing. It seems like when I use :keymap it doesn't use my wrapper even though it's within a wrapper. Instead it uses general-define-key. Check this out:

Basically I had a bind like this:

(use-package projectile
  :general
  ("C-M-/" 'helm-projectile-ag)

  (my-map
    "p" '(:keymap projectile-command-map
          :package projectile
          :which-key "projectile")))

So I would go to press SPC p (one of the prefixes I have) and the keys echoed at the bottom woudl disappear, then it'd reappear as if I had just pressed SPC, so it seemed like it was going SPC p SPC p etc. I'm not entirely sure.

So I decided to C-h k SPC p to see what intermediary function it was bound to ad it seems to me like it indeed it's not even using my wrapper? I'm not sure.

(lambda nil
  (interactive)

  (unless
      (or (featurep (quote projectile))
          (require (quote projectile) nil t))
    (error (concat "Failed to load package: " (symbol-name (quote projectile)))))

  (unless (and (boundp (quote projectile-command-map))
               (keymapp (symbol-value (quote projectile-command-map))))
    (error (format "A keymap called %s is not defined in the %s package"
                   (quote projectile-command-map)
                   (quote projectile))))

  (let ((keys (this-command-keys)))
    (general-define-key
      :states (quote normal)
      :keymaps (quote global)
      keys projectile-command-map)
    (setq unread-command-events (listify-key-sequence keys)))

[blaenk]

I should've done this earlier but I ruled out helm-projectile by commenting it out of my configuration and the problem persists.

[noctuid]

This is another key formatting bug (kbd being used when the keys were already correct). I've updated the code/tests; it should be fixed now. I'll also mention that there is another minor issue I haven't gotten around to changing yet where kbd won't be applied if you happen to bind a key to an extended definition where the :command portion is a string.

[blaenk]

kbd won't be applied if you happen to bind a key to an extended definition where the :command portion is a string.

So when :command "blah" for example? Ah okay, I didn't even know that was possible. Or actually I think I've seen that where it ends up emulating those keys no? In any case that shouldn't affect me.

Thanks for the prompt fix, I'll report back!

[noctuid]

I accidentally committed/pushed some unfinished changes and just force pushed again (just a notice).

that where it ends up emulating those keys no?

Yes, it's comparable to binding a key to a keyboard macro.

[blaenk]

Ah okay, no problem! Thanks again!!

[noctuid]

I actually still may have broke something. You may want to put off testing until the next commit.

[blaenk]

No worries!

[noctuid]

Okay, things should now be working. I also added documentation. I'll close this after fixing the other problem I mentioned unless you have any other issues.

[noctuid]

Thanks a lot for all the suggestions and bug reports. Hopefully everything is working as expected now. One noteworthy change is that :major-mode is now used instead of :mode for the which-key functionality. I've changes this to limit confusion in the case that I add something like :minor-modes (which would end up using evil-define-minor-mode-key).

[blaenk]

Agreed on the name change for :mode, I was going to bring it up but I couldn't think of what else you'd use :mode for.

[blaenk]

So if I have something like this:

(use-package helm-projectile
  :diminish projectile-mode

  :general
  ;; FIXME
  ;; https://github.com/noctuid/general.el/issues/26
  (my-map
    "p" '(:keymap projectile-command-map
          :package helm-projectile
          :which-key "projectile")))

When I press SPC p (given SPC is my prefix), it'll load helm-projectile, then it should re-simulate that sequence SPC p right? Because instead what it seems to be re-simulating is SPC by itself, because it shows at the bottom SPC -.

What does seem to be fixed is that it no longer cycles infinitely, i.e. the binding is correctly established and I can enter it, it's just that when I first trigger it it doesn't re-simulate the complete sequence just the prefix. Also it's not clobbering my p bind in global normal keymap, so that's fixed as well.

[blaenk]

I was going to try doing EDebug on the intermediary function to see what's happening but it's a lambda and I'm not sure how or if I can EDebug a lambda. Perhaps I can EDebug a key trigger or something? Not sure.

[blaenk]

Actually I guess I could put that in scratch, give it a name, re-attach it to the bind, then debug it, duh.

[blaenk]

Heh, yeah I'm not sure how to go about debugging this correctly. I was doing it but then (this-command-keys) ends up returning the value of C-c C-s which is the EDebug bind to step. Perhaps I can put a breakpoint.

[blaenk]

A breakpoint didn't work because it keeps stopping at the first line which ends up replacing/clobbering the keys that it'll read when I press the key to continue until the breakpoint.

I ended up just rigging up messages:

(defun my-intermediary-func ()
  (interactive)
  (unless (or (featurep (quote helm-projectile))
              (require (quote helm-projectile) nil t))
    (error (concat "Failed to load package: " (symbol-name (quote helm-projectile)))))
  (unless (and (boundp (quote projectile-command-map))
               (keymapp (symbol-value (quote projectile-command-map))))
    (error (format "A keymap called %s is not defined in the %s package"
                   (quote projectile-command-map)
                   (quote helm-projectile))))
  (let ((keys (this-command-keys))
        (general-implicit-kbd nil))
    (message "keys: %s" keys)
    (general-define-key
      :states (quote normal)
      :keymaps (quote global)
      keys projectile-command-map)
    (setq unread-command-events (listify-key-sequence keys))
    (message "seq: %s" unread-command-events)))

Then I replace the bind (my-map "p" 'my-intermediary-func). This is the output I get:

keys:  p
seq: (32 112)

So it seems correct to me, 32 is space and 112 is p, and yet right after this I have SPC - show up at the bottom, not SPC p -. Any ideas?

[blaenk]

I'm still a bit unsure as to why the bind is created with general-define-key on :states 'normal :keymaps 'global even though I'm using my-map.

[noctuid]

I have no idea why this happens. It looks like this bug also happens with use-package's :bind-keymap. Maybe (set-transient-map ',keymap) will work instead.

I'm still a bit unsure as to why the bind is created with general-define-key on :states 'normal :keymaps 'global even though I'm using my-map.

general--maybe-autoload-keymap is not supposed to use any wrapper. If you look at the intermediate function for SPC p in motion state, you'll see that will use general-define-key with :states 'motion. Each key only rebinds itself in the state/keymap it was called/originally bound in.

[blaenk]

Woah I just noticed something, I hope it helps.

So I launch emacs, then I press SPC p, so then it shows SPC - at the bottom right, but it seems as if the p DID register. So since it shows SPC - at the bottom I figured it didn't replay the p, so I press p myself, but the effect is actually p of helm-command-map, i.e. projectile-switch-project. Here's C-h l (I think called lossage?):

SPC p [anonymous-command]
SPC p p [projectile-switch-project]

So it seems as if somehow it did re-apply SPC p, but emacs only shows SPC - at the bottom.

[blaenk]

I GOT IT!

[noctuid]

Yes, it will still work. Another more annoying problems is that which-key will show the bindings under SPC as well. Did you figure out how to prevent this?

[blaenk]

Check it. I figured I'd see how which-key replays key sequences, and the way they do it works perfectly!

(defun my-intermediary-func ()
  (interactive)
  (unless (or (featurep (quote helm-projectile))
              (require (quote helm-projectile) nil t))
    (error (concat "Failed to load package: " (symbol-name (quote helm-projectile)))))
  (unless (and (boundp (quote projectile-command-map))
               (keymapp (symbol-value (quote projectile-command-map))))
    (error (format "A keymap called %s is not defined in the %s package"
                   (quote projectile-command-map)
                   (quote helm-projectile))))
  (let ((keys (this-command-keys))
        (general-implicit-kbd nil))
    (message "keys: %s" keys)
    (general-define-key
      :states (quote normal)
      :keymaps (quote global)
      keys projectile-command-map)
    (message "events: %s" unread-command-events)
    (which-key-reload-key-sequence (listify-key-sequence keys))
    (message "seq: %s" unread-command-events)))

Output:

keys:  p
events: nil
seq: ((t . 32) (t . 112))

I did notice the (t . event) thing in the help page but I wasn't sure how to use it, I was doing (t . sequence) but it's (t . event). It works!!!

[blaenk]

Another more annoying problems is that which-key will show the binding under SPC as well. Did you figure out how to prevent this?

Can you rephrase or elaborate?

Nevermind I saw #29.