`define-key' in `defadvice' takes effect only after second invocation

`define-key' in `defadvice' takes effect only after second invocation

[Alexander Shukaev]

Hello,

As a part of my custom Vi layer written on top of Evil, I'm in the
process of adding the following (simple package):

(require 'devil-common)
(require 'devil-core)
(require 'devil-states)
;;
(defgroup devil-repeat-motion
  nil
  "Devil repeat motion."
  :group 'devil
  :prefix 'devil-repeat-motion)
;;
(defcustom devil-repeat-motion-key
  (kbd "SPC")
  "Key used to repeat (last) motion."
  :group 'devil-repeat-motion
  :type 'key-sequence)
;;
(defcustom devil-repeat-motion-key-reverse
  (kbd "S-SPC")
  "Key used to repeat (last) motion in reverse direction."
  :group 'devil-repeat-motion
  :type 'key-sequence)
;;
;;;###autoload
(defun devil-repeat-motion
    (key-or-command
     key-or-command-next
     key-or-command-previous
     &optional keymap)
  "\
Make KEY-OR-COMMAND repeatable."
  (setq keymap (or keymap evil-motion-state-map))
  (let ((command          (devil-key-command key-or-command          keymap))
        (command-next     (devil-key-command key-or-command-next     keymap))
        (command-previous (devil-key-command key-or-command-previous keymap)))
    (eval
     `(defadvice ,command
          (before
           ,(intern (format "devil-repeat-motion--%s" (symbol-name command)))
           activate)
        ,(format "\
Make `%s' repeatable.
Repeatable with `%s'.
Repeatable with `%s' in reverse direction."
                 command
                 command-next
                 command-previous)
        (unless (eq last-command #',command)
          (evil-define-key 'motion devil-repeat-motion-mode-map
            ,devil-repeat-motion-key         #',command-next
            ,devil-repeat-motion-key-reverse #',command-previous))))
    (list command
          command-next
          command-previous)))
;;
;;;###autoload
(defmacro devil-repeat-motions
    (&rest ...)
  "Apply `devil-repeat-motion' to each three consecutive arguments.
Return list of results where each element is the return value of the
corresponding `devil-repeat-motion' application."
  (declare (debug  t)
           (indent defun))
  (let (body)
    (while ...
      (push `(devil-repeat-motion ,(pop ...) ,(pop ...) ,(pop ...)) body))
    (setq body (nreverse body))
    `(list ,@body)))
;;
;;;###autoload
(define-minor-mode devil-repeat-motion-mode
  "\
Devil repeat motion mode."
  :global t
  :group 'devil-repeat-motion
  :keymap (make-sparse-keymap)
  :lighter " DRM")
;;
(provide 'devil-repeat-motion)

I hope this is not too overwhelming to read, but I really need help
with the problem.  First of all, the code above works as intended.
Secondly, let me briefly introduce the idea behind it.

Assume one has defined keys for some (Evil) motions in the
`evil-motion-state-map' like so:

(devil-define-key evil-motion-state-map
  (kbd "] c") #'evil-find-char
  (kbd "[ c") #'evil-find-char-backward
  ;;
  (kbd "] t") #'evil-find-char-to
  (kbd "[ t") #'evil-find-char-to-backward
  ;;
  (kbd "f") #'evil-repeat-find-char
  (kbd "F") #'evil-repeat-find-char-reverse)

And one wants to make the first four ones repeatable forward and
backward with the SPC and S-SPC respectively.  So with the above
package, one can write

(devil-repeat-motions
  (kbd "] c") (kbd "f") (kbd "F")
  (kbd "[ c") (kbd "f") (kbd "F")
  ;;
  (kbd "] t") (kbd "f") (kbd "F")
  (kbd "[ t") (kbd "f") (kbd "F")))

Note, that it is also possible to write it like this:

(devil-repeat-motions
  #'evil-find-char #'evil-repeat-find-char #'evil-repeat-find-char-reverse
  #'evil-find-char-backward #'evil-repeat-find-char
#'evil-repeat-find-char-reverse
  ;;
  #'evil-find-char-to #'evil-repeat-find-char #'evil-repeat-find-char-reverse
  #'evil-find-char-to-backward #'evil-repeat-find-char
#'evil-repeat-find-char-reverse))

The first variant is just for convenience (works only when keys for
commands are already define), while the second one is more general
(works anyways).

Now to the problem.  I start Emacs.  I enter
`devil-repeat-motion-mode' with, say, M-x devil-repeat-motion-mode
RET.  I do ] c t (to find the next 't' char forward).  This entails
advice which binds SPC and S-SPC appropriately (I can see that with
both C-h v devil-repeat-motion-mode-map RET and C-h b).  But, wait,
tapping SPC or S-SPC gives me "SPC is undefined".  How come?  Now the
most interesting.  I do ] c t one more time, and now it works!  Could
anybody explain this behavior to me?  I mean, maybe I missed
something, and adding binding like this "on fly" requires some
additional command to sort of "commit" them to be available
immediately or whatever.  Thanks a lot, and sorry for lengthy post.

Regards,
Alexander

Re: `define-key' in `defadvice' takes effect only after second invocation

[Alexander Shukaev]

> (require 'devil-common)
> (require 'devil-core)
> (require 'devil-states)
> ;;
> (defgroup devil-repeat-motion
>   nil
>   "Devil repeat motion."
>   :group 'devil
>   :prefix 'devil-repeat-motion)
> ;;
> (defcustom devil-repeat-motion-key
>   (kbd "SPC")
>   "Key used to repeat (last) motion."
>   :group 'devil-repeat-motion
>   :type 'key-sequence)
> ;;
> (defcustom devil-repeat-motion-key-reverse
>   (kbd "S-SPC")
>   "Key used to repeat (last) motion in reverse direction."
>   :group 'devil-repeat-motion
>   :type 'key-sequence)
> ;;
> ;;;###autoload
> (defun devil-repeat-motion
>     (key-or-command
>      key-or-command-next
>      key-or-command-previous
>      &optional keymap)
>   "\
> Make KEY-OR-COMMAND repeatable."
>   (setq keymap (or keymap evil-motion-state-map))
>   (let ((command          (devil-key-command key-or-command          keymap))
>         (command-next     (devil-key-command key-or-command-next     keymap))
>         (command-previous (devil-key-command key-or-command-previous keymap)))
>     (eval
>      `(defadvice ,command
>           (before
>            ,(intern (format "devil-repeat-motion--%s" (symbol-name command)))
>            activate)
>         ,(format "\
> Make `%s' repeatable.
> Repeatable with `%s'.
> Repeatable with `%s' in reverse direction."
>                  command
>                  command-next
>                  command-previous)
>         (unless (eq last-command #',command)
>           (evil-define-key 'motion devil-repeat-motion-mode-map
>             ,devil-repeat-motion-key         #',command-next
>             ,devil-repeat-motion-key-reverse #',command-previous))))
>     (list command
>           command-next
>           command-previous)))
> ;;
> ;;;###autoload
> (defmacro devil-repeat-motions
>     (&rest ...)
>   "Apply `devil-repeat-motion' to each three consecutive arguments.
> Return list of results where each element is the return value of the
> corresponding `devil-repeat-motion' application."
>   (declare (debug  t)
>            (indent defun))
>   (let (body)
>     (while ...
>       (push `(devil-repeat-motion ,(pop ...) ,(pop ...) ,(pop ...)) body))
>     (setq body (nreverse body))
>     `(list ,@body)))
> ;;
> ;;;###autoload
> (define-minor-mode devil-repeat-motion-mode
>   "\
> Devil repeat motion mode."
>   :global t
>   :group 'devil-repeat-motion
>   :keymap (make-sparse-keymap)
>   :lighter " DRM")
> ;;
> (provide 'devil-repeat-motion)

Right, I found the problem.  Indeed, Evil needs the commit of key
bindings with `evil-normalize-keymaps'.  Hence, had to add the
following:

(define-minor-mode devil-repeat-motion-mode
  "\
Devil repeat motion mode."
  :global t
  :group 'devil-repeat-motion
  :keymap (make-sparse-keymap)
  :lighter " DRM"
  (evil-normalize-keymaps)) ;; <<<

and

        (unless (eq last-command #',command)
          (evil-define-key 'motion devil-repeat-motion-mode-map
            ,devil-repeat-motion-key         #',command-next
            ,devil-repeat-motion-key-reverse #',command-previous)
          (evil-normalize-keymaps)))) ;; <<<

Works as expected now.  Sorry, for the noise.