clear-transient-map bug

Update. There is no bug, but a mistake in Xah Fly Keys.

following is the pull request with explanation. From Grigory Starinkin https://github.com/velimir

https://github.com/xahlee/xah-fly-keys/pull/146

xah-fly-command-mode-init uses a feature of the set-transient-map that explicitly disables a clean up on a pre-command-hook invocation (keep-pred parameter):

(setq xah-fly--deactivate-command-mode-func
        (set-transient-map xah-fly-command-map (lambda () t)))

In our case it is (lambda () t).

From the code of the set-transient-map:

However, if the optional argument KEEP-PRED is t, MAP stays active if a key from MAP is used. KEEP-PRED can also be a function of no arguments: it is called from `pre-command-hook' and if it returns non-nil, then MAP stays active.

Although xah-fly-command-map map stays active until insert mode is started (as it should be since we don't want quite a command mode prematurely) and XFK takes responsibility of invoking the cleanup function (xah-fly--deactivate-command-mode-func) when insert mode is activated, xah-fly-command-mode-init could be called multiple times.

Subsequent activation of the command mode overwrites xah-fly--deactivate-command-mode-func, meaning that we loose a reference to a clean up func that is supposed to remove the clear-transient-map items from the pre-command-hook.

This commit ensures that a subsequent command mode activation clears up a transient map set by already active command mode.

Closes #145

old bug since at least 2014.

From:	Thierry Volpiatto
Subject:	bug#17642: pre-command-hook has thousands of `clear-transient-map' in 24.3.91.1
Date:	Fri, 30 May 2014 23:52:39 +0200
User-agent:	Gnus/5.13 (Gnus v5.13) Emacs/24.3.91 (gnu/linux)
Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I'm not sure if it's related to what I'm seeing.  If I remove all
>> `clear-transient-map'  from my pre-command-hook, they come back in
>> quick order.
>
> That's weird.  Do you see any strange message in *Messages*?

Helm is using `set-transient-map' a lot and it seems `set-transient-map'
doesn't remove properly its `clearfun' from `pre-command-hook' or more
exactly the `clearfun' doesn't remove itself from `pre-command-hook'.
So the many entries found in `pre-command-hook' may come from this.

BTW `set-transient-map' description in info is not accorded to its
docstring.

--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997

https://lists.gnu.org/archive/html/bug-gnu-emacs/2014-05/msg01290.html

debug session

pre-command-hook
;; Its value is
;; (clear-transient-map clear-transient-map clear-transient-map delete-selection-pre-hook tooltip-hide)

;; does not work
(remove-hook 'pre-command-hook 'clear-transient-map)

;; does not work
(delete 'clear-transient-map pre-command-hook)
;; does not work
(delq 'clear-transient-map pre-command-hook)

;; because
(eq 'clear-transient-map (pop pre-command-hook) )
;; nil

;; delq doesn't work because clear-transient-map is not in the main symbol table obarray
(delq (make-symbol "xx1") '(xx1 aa bb cc))
;; (xx1 aa bb cc)

(eq (make-symbol "qqq") (make-symbol "qqq") )
;; nil

;; it's a symbol though
(symbolp (pop pre-command-hook))
;; t

;; symbol clear-transient-map is not defined
(fboundp 'clear-transient-map) ; nil
(boundp 'clear-transient-map) ; nil

;; however, the clear-transient-map in pre-command-hook is defined
(fboundp (pop pre-command-hook)) ; t
(boundp (pop pre-command-hook)) ; nil

;; this means, symbol clear-transient-map is diff than the one in pre-command-hook
;; this is probably because, partly, the set-transient-map function created it by make-symbol, not intern, so the symbol is not in obarray.

;; get symbol's function cell value
(symbol-function (pop pre-command-hook)) ; #[0 "\3051A�\301\204�\306\2029�\300A@=\204 �\300AA>\203 �\307\2029�\301\307=\2037�\310\300\311 \"\211\2052�=\262\2029�\301 ?\205?�\302 0\207\312\313\"\210\306\207" [(keymap (127 . xah-fly-leader-key-map) (47 . hippie-expand) (116 . next-line) (98 . isearch-forward) (44 . xah-shrink-whitespaces) (46 . backward-kill-word) (102 . undo) (107 . xah-paste-or-paste-previous) (59 . xah-comment-dwim) (111 . open-line) (120 . xah-toggle-letter-case) ...) (closure (t) nil t) #[0 "\303\300\304\"\210\305\306\302\"\210\301\205�\301 \207" [... nil clear-transient-map internal-pop-keymap overriding-terminal-local-map remove-hook pre-command-hook] 3] overriding-terminal-local-map this-command (debug error) nil t lookup-key this-command-keys-vector message "set-transient-map PCH: %S"] 4]

;; get symbol's property list cell value
(symbol-plist (pop pre-command-hook)) ; nil

• gah. this bug is nasty.
• have to do with deep elisp symbols and obarray
• Image
• and once a symbol is used, it is placed into obarray.
• this make it hard to debug.
• currently, i no unstand why set-transient-map has to create a (make-symbol "clear-transient-map")
• and not in standard obarray, and why we need clear-transient-map at all.
• but also why in xah fly keys
• Image
• there's xah-fly--deactivate-command-mode-func
• that's the code of written by will and friends, which did the new engine using transient-map
• so currently there's 2 approach to fix for real.
• one is, dive down to elisp symbols and obarray, and understand set-transient-map in depth, and why it creates clear-transient-map, and why not using obarray
• the other is, maybe just fix something in xah fly keys, the xah-fly-command-mode-init function. possibly this can fix, possibly not.

After running a profiler it turned out that it's set-transient-map that takes majority of the time. add-hook in particular that is called to add clear-transient-map to the pre-command-hook:

https://github.com/emacs-mirror/emacs/blob/5a223c7f2ef4c31abbd46367b6ea83cd19d30aa7/lisp/subr.el#L5799

After a day that accumulated 1.5K items in the hook 😐

(length (default-value 'pre-command-hook)) ; 1562

Vast majority of the items there is this clear-transient-map.

Would you happen to have any thoughts on the above? Trying to my options to identify what and where things went wrong.

pre-command-hook is a variable defined in 'C source code'.

Its value is

(clear-transient-map clear-transient-map clear-transient-map clear-transient-map
delete-selection-pre-hook tooltip-hide)

;; 2022-06-14
;; GNU Emacs 28.1 (build 52, x86_64-w64-mingw32) of 2022-04-04

;; clear-transient-map is not defined
(fboundp 'clear-transient-map) ; nil
(boundp 'clear-transient-map) ; nil

;; value after 2 hour of emacs
;; (clear-transient-map clear-transient-map clear-transient-map clear-transient-map
;; clear-transient-map clear-transient-map clear-transient-map clear-transient-map
;; clear-transient-map clear-transient-map clear-transient-map clear-transient-map
;; clear-transient-map clear-transient-map clear-transient-map clear-transient-map
;; clear-transient-map clear-transient-map clear-transient-map clear-transient-map
;; clear-transient-map clear-transient-map delete-selection-pre-hook tooltip-hide)

;; no effect
(remove-hook 'pre-command-hook 'clear-transient-map)

;; works
(setq pre-command-hook nil)

(intern-soft 'clear-transient-map)

(defun set-transient-map (map &optional keep-pred on-exit)
  "Set MAP as a temporary keymap taking precedence over other keymaps.
Normally, MAP is used only once, to look up the very next key.
However, if the optional argument KEEP-PRED is t, MAP stays
active if a key from MAP is used.  KEEP-PRED can also be a
function of no arguments: it is called from `pre-command-hook' and
if it returns non-nil, then MAP stays active.

Optional arg ON-EXIT, if non-nil, specifies a function that is
called, with no arguments, after MAP is deactivated.

This uses `overriding-terminal-local-map', which takes precedence over all
other keymaps.  As usual, if no match for a key is found in MAP, the normal
key lookup sequence then continues.

This returns an \"exit function\", which can be called with no argument
to deactivate this transient map, regardless of KEEP-PRED."
  (let* ((clearfun (make-symbol "clear-transient-map"))
         (exitfun
          (lambda ()
            (internal-pop-keymap map 'overriding-terminal-local-map)
            (remove-hook 'pre-command-hook clearfun)
            (when on-exit (funcall on-exit)))))
    ;; Don't use letrec, because equal (in add/remove-hook) could get trapped
    ;; in a cycle. (bug#46326)
    (fset clearfun
          (lambda ()
            (with-demoted-errors "set-transient-map PCH: %S"
              (unless (cond
                       ((null keep-pred) nil)
                       ((and (not (eq map (cadr overriding-terminal-local-map)))
                             (memq map (cddr overriding-terminal-local-map)))
                        ;; There's presumably some other transient-map in
                        ;; effect.  Wait for that one to terminate before we
                        ;; remove ourselves.
                        ;; For example, if isearch and C-u both use transient
                        ;; maps, then the lifetime of the C-u should be nested
                        ;; within isearch's, so the pre-command-hook of
                        ;; isearch should be suspended during the C-u one so
                        ;; we don't exit isearch just because we hit 1 after
                        ;; C-u and that 1 exits isearch whereas it doesn't
                        ;; exit C-u.
                        t)
                       ((eq t keep-pred)
                        (let ((mc (lookup-key map (this-command-keys-vector))))
                          ;; If the key is unbound `this-command` is
                          ;; nil and so is `mc`.
                          (and mc (eq this-command mc))))
                       (t (funcall keep-pred)))
                (funcall exitfun)))))
    (add-hook 'pre-command-hook clearfun)
    (internal-push-keymap map 'overriding-terminal-local-map)
    exitfun))