`evil-define-key' for minor mode does not take effect until a state transition

[TheBB]

Originally reported by: Anonymous

---

Unit test. (you can change flyspell to any toggleble minor mode)

(require 'flyspell)
(evil-define-key 'normal flyspell-mode-map [f5]
(lambda () (interactive) (message "Hey")))

In scratch buffer

M-x: flyspell-mode

press F5, key is not bound.
press i then esc, key is now bound

This is a problem because evil is enabled from find-file-hook. before major mode specific hooks are called. If major mode specific hooks enable any minor modes, evil is already initialized, and it does not see those minor modes until a state transition is made, so any bindings for them are in-active

Tested with

GNU Emacs 24.3.1 (x86_64-suse-linux-gnu, X toolkit, Xaw scroll bars) of 2013-05-26 on momoland

Evil from git 8f07f5b0b1fc888428b913e14944e89a0341e7b5

---

• Bitbucket: https://bitbucket.org/lyro/evil/issue/301

[TheBB]

Original comment by Frank Fischer (Bitbucket: lyro, GitHub: lyro):

---

I've been aware of this problem for some time now. I just do not know a solution. Evil uses special keymaps that hold the bindings, but in order to figure out which of these special keymaps to install Evil checks which other keymaps (flyspell-mode-map here) are active.

This check is done in evil-normalize-keymaps and this function is run on each state transition. The problem is that I do not know of a reliable way to detect whether a certain command installed a new keymap (or if it enabled or disabled some minor-mode). The only way would be to call evil-normalize-keymaps from a post-command-hook, but normalizing keymaps might be quite expensive.

I would be happy if some could tell me a good way to check fast when a certain minor-mode has been toggled.

[TheBB]

Original comment by Nick DeCoursin (Bitbucket: CapinCape, GitHub: Unknown):

---

Here's a couple more issues that reference the same bug:
[issue 497](https://bitbucket.org/lyro/evil/issues/497/regarding-initial-states* 1. )
and issue 130

[TheBB]

Original comment by Justin Burkett (Bitbucket: justbur, GitHub: justbur):

---

What if you checked minor-mode-list for changes in the values of the symbols before running evil-normalize-maps?

[TheBB]

Original comment by Justin Burkett (Bitbucket: justbur, GitHub: justbur):

---

Or maybe minor-mode-map-alist

[TheBB]

Original comment by Frank Fischer (Bitbucket: lyro, GitHub: lyro):

---

The problem is not what evil-normalize-maps does, that works perfectly well, but to decide when this function is called. Currently it is called whenever the state in some buffer changes. However, changing the set of active keymaps is not something that can easily be observed (as far as I know). For instance, activating some minor-mode is just setting some variable from nil to t, and this activates the associated keymap.

Note that I'm absolutely no fan of this whole evil-define-key stuff, i.e., the concept of associating some Evil keys depending on some other keymap. This causes a lot of trouble, this issue being only one (another one, at least in the current implementation, is the duplication of some menu entries). The reason for this opinion is that keymaps are simply not the right concept for activating or deactivating some feature Emacs. That's what minor-mode are for. So the correct way of activating some keybindings depending on some other package/mode/whatever is to use appropriate minor-mode hooks and the something like the buffer-local keymaps evil-local-normal-state-map or so. The downside is that this only works with proper minor-modes that follow usual conventions (like running hooks). And other types of keymaps like major-mode maps (ok, that's not a problem because they run hooks, too) or keymaps installed in some overlays won't work (although the latter probably do not work reliably now anyway, so that might not be an issue).

But I think that a lot of people rely on and make heavy use of evil-define-key right now. That's why I have not removed that (in my opinion flawed) feature, yet. No idea when I find the time to replace it with something better ;)

[TheBB]

Original comment by Justin Burkett (Bitbucket: justbur, GitHub: justbur):

---

Here's an idea that does things a little differently than evil-normalize-keymaps. If there's interest I can make a formal pr. It's a little rough, but I think the basic idea comes across

#!lisp

(defvar evil-aliased-modes '())
(defvar evil-states '(normal motion visual emacs insert operator))

(defmacro evil-define-key (state mode key def)
  (let* ((mode-alias (intern (format "evil-%s" mode)))
         (map (intern (format "evil-%s-%s-map" mode state))))
    `(progn
       (if (assq ',mode-alias evil-aliased-modes)
           (define-key ,map ,key ,def)
         (defvar ,map (make-sparse-keymap))
         (define-key ,map ,key ,def)
         (defvaralias ',mode-alias ',mode)
         (add-to-list 'evil-aliased-modes ',mode)
         (add-to-list 'minor-mode-map-alist '(,mode-alias . ,(make-sparse-keymap))
                      nil (lambda (a b) (eq (car a) (car b))))))))

(defun evil-update-state-keymaps-hook ()
  (dolist (mode evil-aliased-modes)
    (let* ((mode-alias (intern (format "evil-%s" mode)))
           (state-map (intern (format "evil-%s-%s-map" mode evil-state))))
      (cond
       ((and (assq mode-alias minor-mode-map-alist)
             state-map
             (keymapp (symbol-value state-map)))
        (setcdr (assq mode-alias minor-mode-map-alist)
                (symbol-value (intern (format "evil-%s-%s-map" mode evil-state)))))
       ((assq mode-alias minor-mode-map-alist)
        (setcdr (assq mode-alias minor-mode-map-alist) (make-sparse-keymap)))))))
(add-hook 'evil-insert-state-entry-hook 'evil-update-state-keymaps-hook)
(add-hook 'evil-normal-state-entry-hook 'evil-update-state-keymaps-hook)

(evil-define-key-2 insert smartparens-mode "\C-a" 'forward-char)
(evil-define-key-2 normal smartparens-mode "\C-a" 'forward-line)

[TheBB]

Original comment by Justin Burkett (Bitbucket: justbur, GitHub: justbur):

---

I can explain the idea in the previous post in case it's not clear. The issue as I understand it is that evil-normalize-keymaps is not always run when necessary, mainly because it doesn't "know" about minor modes turning on and off. Of course it can be added to minor mode hooks, but that seems messy.

The proposed solution is to use the minor-mode-map-alist functionality built into emacs directly to control activating the correct evil maps. I alias the corresponding mode variable with evil-define-key and place that aliased variable and the corresponding map into the alist. Now emacs controls activating these maps, and evil just needs to make sure that the right ones are present in that alist, which it does by swapping out the maps on state changes.

[TheBB]

Original comment by Frank Fischer (Bitbucket: lyro, GitHub: lyro):

---

I see. I like the idea, but in the end it ties the bindings to modes, not to keymaps (as it has been done before). Although this should be sufficient in most cases, there are some modes where this does not work (IIRC undo-tree being one of them, although that might have changed in the meantime).

Besides, I still find the the code hard to follow and very hard to check if it's correct. In particular, why do different buffers not influence each other? If one buffer has Evil and smartparens-mode enabled, and another one has only smartparens-mode enabled, then changing the state in the Evil-active buffer will influence the active keymaps in the other buffer, too. That's because minor-mode-map-alist is a global variable and the activity of the newly generated keymaps depend only on the specified minor-mode (here smartparens-mode) through the aliased variable and not on whether Evil is active in that buffer or not.

Don't get me wrong, I really like the approach and appreciate your efforts to improve that really ugly part of Evil. However, it's a core feature and modifying this stuff will definitely break something for someone (that's for sure in my experience ;)). That's why I have to insist on an implementation and a documentation that I can follow and that (at least) gives me the feeling that everything works well :)

[TheBB]

Original comment by Justin Burkett (Bitbucket: justbur, GitHub: justbur):

---

Good point on it being a global variable, and I agree with and understand the sentiment. Although I think from a user's perspective tying a key binding to a mode is probably the more natural way to think about things. Of course that means less freedom (since some modes have multiple keymaps), but I think a little more transparency.

FWIW I put the code there to suggest a different conceptual approach to swapping maps in response to state and mode changes. I thought the code would communicate the idea better than I could in words. I never meant it to be a final product.

If you would entertain changes to evil-normalize-maps, I would be willing to spend time on coming up with a more "automated" solution like the one above that is well documented and cleanly implemented. If not, no worries.

[TheBB]

Original comment by Justin Burkett (Bitbucket: justbur, GitHub: justbur):

---

Okay, here's another approach I thought of this morning that seems simpler and ties bindings to maps instead of modes. The idea is to insert state-specific maps as parents of the mode-maps (so we don't modify the mode-map, and also get the benefit of emacs taking care of when the maps are active). There could be a list keeping track of all the mode-maps that have a evil key defined for them, so you don't need to search all of the mode-maps for evil bindings.

This is a clean way to do this imo and again only requires that we swap out maps on state changes. The drawback of this method is that since it uses parents, the evil bindings have lower priority than the mode map ones (you could automatically unbind those for people who wanted that though). I don't see this as a major issue, but it is a change in behavior.

Here's a working example to illustrate:

(defun init-example ()
  (setq mode-map (make-sparse-keymap)
        mode-map-parent (make-sparse-keymap)
        state1-map (make-sparse-keymap)
        state2-map (make-sparse-keymap))
  (set-keymap-parent mode-map mode-map-parent)
  (define-key mode-map "a" "A")
  (define-key mode-map-parent "p" "P")
  (define-key state1-map [evil-state] 'evil-state)
  (define-key state1-map "b" "state1 b")
  (define-key state2-map [evil-state] 'evil-state)
  (define-key state2-map "b" "state2 b"))

(defun swap-state-map (mode-map new-state-map)
  (let ((parent (keymap-parent mode-map)))
    (cond
     ;; parent is a state-map => Replace parent with new-state-map
     ((and parent
           (eq (lookup-key parent [evil-state]) 'evil-state))
      (when (keymap-parent parent)
        (set-keymap-parent new-state-map (keymap-parent parent)))
      (set-keymap-parent mode-map new-state-map))
     ;; parent is not a state-map => Insert new parent
     (parent
      (set-keymap-parent new-state-map parent)
      (set-keymap-parent mode-map new-state-map))
     ;; no parent => create one
     (t
      (set-keymap-parent mode-map new-state-map))))
  mode-map)

(init-example)
(lookup-key mode-map "a")
(lookup-key mode-map "b")
(lookup-key mode-map "p")

(swap-state-map mode-map state1-map)
(lookup-key mode-map "a")
(lookup-key mode-map "b")
(lookup-key mode-map "p")

(swap-state-map mode-map state2-map)
(lookup-key mode-map "a")
(lookup-key mode-map "b")
(lookup-key mode-map "p")

(swap-state-map mode-map state1-map)
(lookup-key mode-map "a")
(lookup-key mode-map "b")
(lookup-key mode-map "p")

[TheBB]

Original comment by Frank Fischer (Bitbucket: lyro, GitHub: lyro):

---

Well, I think it is a major issue. In fact, a lot of the machinery around keymaps is exactly for Evil keybindings taking priority over regular bindings. And the problem with different buffers using the same mode (and mode-map) and therefore sharing a common global variable remains.

[TheBB]

Original comment by Justin Burkett (Bitbucket: justbur, GitHub: justbur):

---

Ok, one more, and then I will give up (I promise :-)). Here's some sample code I came up with today

(defvar evil-minor-mode-maps
  '((insert)
    (normal)
    ;; remaining states
    ))

(defun evil-define-key-for-mode (state mode key def)
  (let ((state-map-alist (assq state evil-minor-mode-maps))
        map)
    (cond ((and state-map-alist
                (assq mode (cdr state-map-alist)))
           (setq map (cdr (assq mode (cdr state-map-alist)))))
          (state-map-alist
           (setq map (make-sparse-keymap))
           (setcdr state-map-alist
                   (append (list (cons mode map)) (cdr state-map-alist))))
          (t
           (setq map (make-sparse-keymap))
           (push (cons state (list (cons mode map))) evil-minor-mode-maps)))
    (define-key map key def)))

(evil-define-key-for-mode 'insert 'emacs-lisp-mode "\C-a" "a")
(evil-define-key-for-mode 'insert 'lisp-mode "\C-a" "a")

The idea again is to use the mode instead of the mode-map, which at the very least can complement the current evil-define-key with another method of defining keys for modes. I still think it's more common to think in terms of the mode you want to associate a binding with instead of the map, but let me just explain the idea here.

The idea is to create an evil minor-mode-map-alist associated with each state for just the modes that someone defines a key in. evil-minor-mode-maps associates states with different minor-mode-map-alists. Given the mode and state a user wants to bind in we simply adjust the corresponding component of evil-minor-mode-maps. Then in evil-normalize-maps on each state change you extract the cdr of the corresponding state and insert it into evil-mode-map-alist.

This seems simple to me and avoids some of the issues above. You could even use this in a way that keeps the evil-define-key functionality around, maybe after deciding on the priority between these maps and the ones added using evil-define-key.