bug#51281: 28.0.60; repeat-mode issues

bug#51281: 28.0.60; repeat-mode issues

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

Tags: patch

Currently there is a bug where the prefix arg changed for the repeatable
commands, is applied to the next non-repeatable command.  For example:

  C-- C-x o o C-n
or
  C-x o C-- o C-n

`C-n' moves up, not down.  This patch fixes this bug:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: repeat-mode-fixes.patch --]
[-- Type: text/x-diff, Size: 3944 bytes --]

diff --git a/lisp/repeat.el b/lisp/repeat.el
index ee9e14b515..f4526c20f4 100644
--- a/lisp/repeat.el
+++ b/lisp/repeat.el
@@ -390,7 +390,10 @@ repeat-mode
 See `describe-repeat-maps' for a list of all repeatable command."
   :global t :group 'convenience
   (if (not repeat-mode)
-      (remove-hook 'post-command-hook 'repeat-post-hook)
+      (progn
+        (remove-hook 'pre-command-hook 'repeat-pre-hook)
+        (remove-hook 'post-command-hook 'repeat-post-hook))
+    (add-hook 'pre-command-hook 'repeat-pre-hook)
     (add-hook 'post-command-hook 'repeat-post-hook)
     (let* ((keymaps nil)
            (commands (all-completions
@@ -402,27 +405,60 @@ repeat-mode
                (length commands)
                (length (delete-dups keymaps))))))
 
-(defun repeat-post-hook ()
-  "Function run after commands to set transient keymap for repeatable keys."
-  (let ((was-in-progress repeat-in-progress))
-    (setq repeat-in-progress nil)
+(defun repeat-map ()
+  "Return a map for keys repeatable after the current command."
   (when repeat-mode
     (let ((rep-map (or repeat-map
                        (and (symbolp real-this-command)
                             (get real-this-command 'repeat-map)))))
       (when rep-map
-          (when (boundp rep-map)
+        (when (and (symbolp rep-map) (boundp rep-map))
           (setq rep-map (symbol-value rep-map)))
-          (let ((map (copy-keymap rep-map)))
 
+        (if repeat-exit-key
+            ;; `repeat-exit-key' modifies the map by adding keys
+            (copy-keymap rep-map)
+          rep-map)))))
+
+(defun repeat-map-valid (map)
+  "Check if MAP can be used for the next command.
+Can contain more conditions."
+  (and map
+       ;; Avoid using repeatable keys when minibuffer prompt pops up
+       (zerop (minibuffer-depth))
        ;; Exit when the last char is not among repeatable keys,
        ;; so e.g. `C-x u u' repeats undo, whereas `C-/ u' doesn't.
-            (when (and (zerop (minibuffer-depth)) ; avoid remapping in prompts
-                       (or (lookup-key map (this-command-keys-vector))
-                           prefix-arg))
+       (or (lookup-key map (vector last-nonmenu-event))
+           ;; `prefix-arg' can affect next repeatable commands
+           ;; (and repeat-keep-prefix prefix-arg)
+           )))
+
+(defun repeat-pre-hook ()
+  "Function run before commands to handle repeatable keys."
+  ;; Reset prefix-arg before the next non-repeatable command,
+  ;; e.g. `C-- C-x o o C-n' or `C-x o C-- o C-n', so `C-n'
+  ;; should not use `prefix-arg' to go in opposite direction.
+  (when (and repeat-keep-prefix prefix-arg repeat-in-progress)
+    (let ((map (repeat-map)))
+      (if map
+          ;; Optimize to use less logic in `repeat-map'
+          ;; when called again from `repeat-post-hook'
+          (setq repeat-map map)
+        ;; When `repeat-post-hook' will exit the repeatable sequence,
+        ;; this means the current command is not repeatable,
+        ;; so reset `prefix-arg' enabled for repeatable commands only.
+        (setq prefix-arg nil)))))
+
+(defun repeat-post-hook ()
+  "Function run after commands to set transient keymap for repeatable keys."
+  (let ((was-in-progress repeat-in-progress))
+    (setq repeat-in-progress nil)
+
+    (let ((map (repeat-map)))
+      (when (repeat-map-valid map)
 
         ;; Messaging
-              (unless prefix-arg
+        (unless prefix-arg ;; Don't overwrite prefix arg echo
           (funcall repeat-echo-function map))
 
         ;; Adding an exit key
@@ -446,7 +482,7 @@ repeat-post-hook
                    (lambda ()
                      (setq repeat-in-progress nil)
                      (funcall exitfun)
-                           (funcall repeat-echo-function nil)))))))))))
+                     (funcall repeat-echo-function nil))))))))
 
     (setq repeat-map nil)
     (when (and was-in-progress (not repeat-in-progress))

bug#51281: 28.0.60; repeat-mode issues

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

> Currently there is a bug where the prefix arg changed for the repeatable
> commands, is applied to the next non-repeatable command.  For example:
>
>   C-- C-x o o C-n
> or
>   C-x o C-- o C-n

Actually, this too confusing feature that allows changing prefix args
during the repeating sequence increases code complexity enormously.
For posterity I'll leave here the patch that completely supports it.
But this feature will be removed (unless someone will ask to leave it):

  C-x } }  C-1 C-2 } }  C-3 C-4 } }

Only this will remain:

  C-x } }  C-1 C-2 C-x } }  C-3 C-4 C-x } }


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: repeat-prefix-arg.patch --]
[-- Type: text/x-diff, Size: 7134 bytes --]

diff --git a/lisp/repeat.el b/lisp/repeat.el
index ee9e14b515..47b080bc6c 100644
--- a/lisp/repeat.el
+++ b/lisp/repeat.el
@@ -390,7 +390,10 @@ repeat-mode
 See `describe-repeat-maps' for a list of all repeatable command."
   :global t :group 'convenience
   (if (not repeat-mode)
-      (remove-hook 'post-command-hook 'repeat-post-hook)
+      (progn
+        (remove-hook 'pre-command-hook 'repeat-pre-hook)
+        (remove-hook 'post-command-hook 'repeat-post-hook))
+    (add-hook 'pre-command-hook 'repeat-pre-hook)
     (add-hook 'post-command-hook 'repeat-post-hook)
     (let* ((keymaps nil)
            (commands (all-completions
@@ -402,51 +405,108 @@ repeat-mode
                (length commands)
                (length (delete-dups keymaps))))))
 
+(defun repeat-map ()
+  "Return a transient map for keys repeatable after the current command."
+  (let ((rep-map (or repeat-map
+                     (and (symbolp real-this-command)
+                          (get real-this-command 'repeat-map)))))
+    (when rep-map
+      (when (and (symbolp rep-map) (boundp rep-map))
+        (setq rep-map (symbol-value rep-map)))
+      rep-map)))
+
+(defvar repeat-last-prefix-command nil)
+
+(defun repeat-check-map (map)
+  "Decides whether MAP can be used for the next command.
+Can contain more conditions."
+  (and map
+       ;; Avoid using repeatable keys when a minibuffer prompt pops up.
+       ;; FIXME: Instead of disallowing repeatable keys in the minibuffer,
+       ;; it would be better to detect when `minibuffer-depth' changes
+       ;; during a repeatable sequence, but this is impossible to do
+       ;; when a repeatable command that activates own minibuffer
+       ;; was called from the minibuffer, e.g. `M-x repeatable-command RET'
+       ;; where in `exit-minibuffer' (bound to RET) minibuffer-depth is 1,
+       ;; and if repeatable-command uses the minibuffer, it's also 1.
+       (zerop (minibuffer-depth))
+       (or
+        ;; Allow prefix commands change `prefix-arg' for next repeatable
+        ;; commands, i.e. don't disable transient map on such sequence
+        ;; `C-x } C-1 C-2 }' that changes window enlargement step to 12.
+        (and repeat-keep-prefix
+             (or (memq this-command
+                       '(universal-argument universal-argument-more
+                         digit-argument negative-argument))
+                 prefix-arg))
+        ;; Exit when the last char is not among repeatable keys,
+        ;; so e.g. `C-x u u' repeats undo, whereas `C-/ u' doesn't.
+        (lookup-key map (vector last-nonmenu-event)))))
+
+(defun repeat-pre-hook ()
+  "Function run before commands to handle repeatable keys."
+  ;; Reset prefix-arg before the next non-repeatable command,
+  ;; e.g. `C-- C-x } } C-n' or `C-x } C-- } C-n', so `C-n'
+  ;; should not use `prefix-arg' to go in opposite direction.
+  (when (and repeat-mode repeat-keep-prefix prefix-arg repeat-in-progress)
+    (if (not (memq this-command
+                   '(universal-argument universal-argument-more
+                     digit-argument negative-argument)))
+        (let ((map (repeat-map)))
+          (if (repeat-check-map map)
+              ;; Optimize to use less logic in the function `repeat-map'.
+              ;; When called again from `repeat-post-hook' it will use
+              ;; the variable `repeat-map'.
+              (setq repeat-map map)
+            ;; When `repeat-post-hook' will exit the repeatable sequence,
+            ;; this means the current command is not repeatable,
+            ;; so reset `prefix-arg' enabled for repeatable commands only.
+            (setq prefix-arg nil)))
+      (unless (memq repeat-last-prefix-command
+                    '(universal-argument universal-argument-more
+                      digit-argument negative-argument))
+        (setq prefix-arg nil)))
+    (setq repeat-last-prefix-command this-command)))
+
 (defun repeat-post-hook ()
   "Function run after commands to set transient keymap for repeatable keys."
   (let ((was-in-progress repeat-in-progress))
     (setq repeat-in-progress nil)
-    (when repeat-mode
-      (let ((rep-map (or repeat-map
-                         (and (symbolp real-this-command)
-                              (get real-this-command 'repeat-map)))))
-        (when rep-map
-          (when (boundp rep-map)
-            (setq rep-map (symbol-value rep-map)))
-          (let ((map (copy-keymap rep-map)))
 
-            ;; Exit when the last char is not among repeatable keys,
-            ;; so e.g. `C-x u u' repeats undo, whereas `C-/ u' doesn't.
-            (when (and (zerop (minibuffer-depth)) ; avoid remapping in prompts
-                       (or (lookup-key map (this-command-keys-vector))
-                           prefix-arg))
+    (let ((map (when repeat-mode (repeat-map))))
+      (when (repeat-check-map map)
 
-              ;; Messaging
-              (unless prefix-arg
-                (funcall repeat-echo-function map))
+        ;; Messaging
+        (unless prefix-arg ;; Don't overwrite prefix arg echo
+          (funcall repeat-echo-function map))
 
-              ;; Adding an exit key
-              (when repeat-exit-key
-                (define-key map repeat-exit-key 'ignore))
+        ;; Adding an exit key
+        (when repeat-exit-key
+          (setq map (copy-keymap map))
+          (define-key map repeat-exit-key 'ignore))
 
-              (when (and repeat-keep-prefix (not prefix-arg))
-                (setq prefix-arg current-prefix-arg))
+        ;; When the current command is not one of commands
+        ;; that set `prefix-arg' then keep the current prefix arg
+        ;; for the next command via `prefix-arg'.
+        (when (and repeat-keep-prefix
+                   (not prefix-arg))
+          (setq prefix-arg current-prefix-arg))
 
-              (setq repeat-in-progress t)
-              (let ((exitfun (set-transient-map map)))
+        (setq repeat-in-progress t)
+        (let ((exitfun (set-transient-map map)))
 
-                (when repeat-exit-timer
-                  (cancel-timer repeat-exit-timer)
-                  (setq repeat-exit-timer nil))
+          (when repeat-exit-timer
+            (cancel-timer repeat-exit-timer)
+            (setq repeat-exit-timer nil))
 
-                (when repeat-exit-timeout
-                  (setq repeat-exit-timer
-                        (run-with-idle-timer
-                         repeat-exit-timeout nil
-                         (lambda ()
-                           (setq repeat-in-progress nil)
-                           (funcall exitfun)
-                           (funcall repeat-echo-function nil)))))))))))
+          (when repeat-exit-timeout
+            (setq repeat-exit-timer
+                  (run-with-idle-timer
+                   repeat-exit-timeout nil
+                   (lambda ()
+                     (setq repeat-in-progress nil)
+                     (funcall exitfun)
+                     (funcall repeat-echo-function nil))))))))
 
     (setq repeat-map nil)
     (when (and was-in-progress (not repeat-in-progress))

bug#51281: [External] : bug#51281: 28.0.60; repeat-mode issues

[Drew Adams]

> > Currently there is a bug where the prefix arg changed for the
> > repeatable commands, is applied to the next non-repeatable
> > command.  For example: C-- C-x o o C-n  or  C-x o C-- o C-n
> 
> Actually, this too confusing feature

It's not confusing at all.

> that allows changing prefix args during the repeating
> sequence increases code complexity enormously.

Too bad.  In that case, feel free to revert the changes
you've made that impose such complexity.  The code just
worked - has worked for a long time.

> this feature will be removed (unless someone will ask
> to leave it)

"unless someone..."

Yes, I ask that this important feature be kept,
not lost.  Users should continue to be able to
add/change a prefix arg (of any kind) at any
time during repetition.

Starting the command over (with a different
prefix arg) is not _at all_ the same thing as
continuing the command invocation.

(Think command state, and Emacs state in general,
in addition to user interaction.)

bug#51281: [External] : bug#51281: 28.0.60; repeat-mode issues

[Juri Linkov]

> Yes, I ask that this important feature be kept,
> not lost.  Users should continue to be able to
> add/change a prefix arg (of any kind) at any
> time during repetition.

Ok, then I'll continue fixing this feature.

bug#51281: [External] : bug#51281: 28.0.60; repeat-mode issues

[Drew Adams]

> > Yes, I ask that this important feature be kept,
> > not lost.  Users should continue to be able to
> > add/change a prefix arg (of any kind) at any
> > time during repetition.
> 
> Ok, then I'll continue fixing this feature.

Thank you.

bug#51281: 28.0.60; repeat-mode issues

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>> Currently there is a bug where the prefix arg changed for the repeatable
>> commands, is applied to the next non-repeatable command.  For example:
>>
>>   C-- C-x o o C-n
>> or
>>   C-x o C-- o C-n
>
> Actually, this too confusing feature that allows changing prefix args
> during the repeating sequence increases code complexity enormously.
> For posterity I'll leave here the patch that completely supports it.
> But this feature will be removed (unless someone will ask to leave it):
>
>   C-x } }  C-1 C-2 } }  C-3 C-4 } }
>
> Only this will remain:
>
>   C-x } }  C-1 C-2 C-x } }  C-3 C-4 C-x } }

Well, it may be slightly on the confusing side, but it does feel
natural, I think?  So I think fixing it is better than removing it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51281: 28.0.60; repeat-mode issues

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

>> Actually, this too confusing feature that allows changing prefix args
>> during the repeating sequence increases code complexity enormously.
>> For posterity I'll leave here the patch that completely supports it.
>> But this feature will be removed (unless someone will ask to leave it):
>>
>>   C-x } }  C-1 C-2 } }  C-3 C-4 } }
>>
>> Only this will remain:
>>
>>   C-x } }  C-1 C-2 C-x } }  C-3 C-4 C-x } }
>
> Well, it may be slightly on the confusing side, but it does feel
> natural, I think?  So I think fixing it is better than removing it.

I hesitate to make such massive change in repeat.el on the release branch.

I have more patches for repeat.el that I'm unsure about to push.
One of them is a patch that allows kbd syntax for 'repeat-exit-key':


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: repeat-exit-key-kbd.patch --]
[-- Type: text/x-diff, Size: 1568 bytes --]

diff --git a/lisp/repeat.el b/lisp/repeat.el
index 7bbb398873..8199bd3287 100644
--- a/lisp/repeat.el
+++ b/lisp/repeat.el
@@ -338,6 +338,7 @@ repeat-exit-key
   "Key that stops the modal repeating of keys in sequence.
 For example, you can set it to <return> like `isearch-exit'."
   :type '(choice (const :tag "No special key to exit repeating sequence" nil)
+                 (string :tag "Kbd string that exits repeating sequence")
                  (key-sequence :tag "Key that exits repeating sequence"))
   :group 'convenience
   :version "28.1")
@@ -437,7 +455,10 @@ repeat-post-hook
 
               ;; Adding an exit key
               (when repeat-exit-key
-                (define-key map repeat-exit-key 'ignore))
+                (define-key map (if (stringp repeat-exit-key)
+                                    (kbd repeat-exit-key)
+                                  repeat-exit-key)
+                            'ignore))
 
               (when (and repeat-keep-prefix (not prefix-arg))
                 (setq prefix-arg current-prefix-arg))
@@ -476,7 +497,9 @@ repeat-echo-message-string
                                keys ", ")
                     (if repeat-exit-key
                         (format ", or exit with %s"
-                                (key-description repeat-exit-key))
+                                (if (stringp repeat-exit-key)
+                                    repeat-exit-key
+                                  (key-description repeat-exit-key)))
                       ""))))
 
 (defun repeat-echo-message (keymap)

bug#51281: 28.0.60; repeat-mode issues

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 01 Dec 2021 19:58:49 +0200
> Cc: 51281@debbugs.gnu.org
> 
> I hesitate to make such massive change in repeat.el on the release branch.

Yes, please install on master.  The problem sounds sufficiently
obscure so as not to be too urgent to fix.

Thanks.

bug#55986: 28.1; (setq repeat-keep-prefix t) breaks repeat-mode

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> I hesitate to make such massive change in repeat.el on the release branch.
>
> I have more patches for repeat.el that I'm unsure about to push.
> One of them is a patch that allows kbd syntax for 'repeat-exit-key':

(I only skimmed these two bug reports lightly.)  This was more than half
a year ago, but at least some of this hasn't been pushed yet?  Are these
changes still on the planning change?

bug#55986: 28.1; (setq repeat-keep-prefix t) breaks repeat-mode

[Juri Linkov]

>> I hesitate to make such massive change in repeat.el on the release branch.
>>
>> I have more patches for repeat.el that I'm unsure about to push.
>> One of them is a patch that allows kbd syntax for 'repeat-exit-key':
>
> (I only skimmed these two bug reports lightly.)  This was more than half
> a year ago, but at least some of this hasn't been pushed yet?  Are these
> changes still on the planning change?

One of the patches is pushed in b374952b51, but another change is more massive.

bug#55986: 28.1; (setq repeat-keep-prefix t) breaks repeat-mode

[John Bernier]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

When using repeat-mode, (setq repeat-keep-prefix t) should allow a
user to keep the previously used prefix when repeating a command.
Instead, setting it to t will somehow break the repeat functionality.
'emacs -Q', reloading the init file or toggling repeat-mode off/on does
not fix it.

[-- Attachment #2: Type: text/html, Size: 410 bytes --]

bug#55986: 28.1; (setq repeat-keep-prefix t) breaks repeat-mode

[Juri Linkov]

> When using repeat-mode, (setq repeat-keep-prefix t) should allow a
> user to keep the previously used prefix when repeating a command.
> Instead, setting it to t will somehow break the repeat functionality.
> 'emacs -Q', reloading the init file or toggling repeat-mode off/on does
> not fix it.

This should be implemented in the next release.

bug#55986: 28.1; (setq repeat-keep-prefix t) breaks repeat-mode

[Stefan Kangas]

Juri Linkov <juri@linkov.net> writes:

>> When using repeat-mode, (setq repeat-keep-prefix t) should allow a
>> user to keep the previously used prefix when repeating a command.
>> Instead, setting it to t will somehow break the repeat functionality.
>> 'emacs -Q', reloading the init file or toggling repeat-mode off/on does
>> not fix it.
>
> This should be implemented in the next release.

Do you mean that this is already fixed on master?

bug#55986: 28.1; (setq repeat-keep-prefix t) breaks repeat-mode

[Juri Linkov]

forcemerge 51281 55986
thanks

>>> When using repeat-mode, (setq repeat-keep-prefix t) should allow a
>>> user to keep the previously used prefix when repeating a command.
>>> Instead, setting it to t will somehow break the repeat functionality.
>>> 'emacs -Q', reloading the init file or toggling repeat-mode off/on does
>>> not fix it.
>>
>> This should be implemented in the next release.
>
> Do you mean that this is already fixed on master?

Please see the commit ef4954b69c17831f4c8360c436352170305666ea that says:

    * lisp/repeat.el (repeat-keep-prefix): Change default to nil.
    'repeat-keep-prefix' doesn't yet have sufficient support
    that covers all cases in bug#51281, so it's disabled now.

There is a patch in bug#51281 that was too late to install
on the release branch, and moreover it needs more work
that should be finished before the next release.

bug#55986: 28.1; (setq repeat-keep-prefix t) breaks repeat-mode

[Juri Linkov]

close 55986 29.0.60
thanks

> When using repeat-mode, (setq repeat-keep-prefix t) should allow a
> user to keep the previously used prefix when repeating a command.
> Instead, setting it to t will somehow break the repeat functionality.
> 'emacs -Q', reloading the init file or toggling repeat-mode off/on does
> not fix it.

Thanks for the bug report.  The bug is fixed now, and closed.