bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

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

For a command and its key in a repeat map:
- "activate" means the command makes the repeat map active
- "continue" means pressing the key keeps the repeat map active

With this patch, the available directives are:
:continue (default) - activates and continues
:exit - neither activates nor continues
:continue-only (new) - continues, but does not activate

A similar feature has been available for many years in the package
https://tildegit.org/acdw/define-repeat-map.el.  The inclusion here
was motivated by the discussions at
https://github.com/jwiegley/use-package/issues/964 and
https://github.com/jwiegley/use-package/pull/974.

A basic example indicating why this might be useful:

(bind-keys
 :repeat-map paragraph-repeat-map
 :continue
 ("]" . forward-paragraph)
 ("}" . forward-paragraph)
 ("[" . backward-paragraph)
 ("{" . backward-paragraph)
 ("k" . kill-paragraph)
 :continue-only
 ;; These commands will be available during paragraph manipulation
 ;; but won't activate paragraph-repeat-map themselves
 ("y" . yank)
 ("C-/" . undo))

Or equivalently, with use-package:

(use-package emacs
  :ensure nil
  :bind
  (:repeat-map
   paragraph-repeat-map
   ("]" . forward-paragraph)
   ("}" . forward-paragraph)
   ("[" . backward-paragraph)
   ("{" . backward-paragraph)
   ("k" . kill-paragraph)
   :continue-only
   ;; These commands will be available during paragraph manipulation
   ;; but won't activate paragraph-repeat-map themselves
   ("y" . yank)
   ("C-/" . undo)))

[-- Attachment #2: 0001-Add-continue-only-directive-to-bind-keys-and-use-pac.patch --]
[-- Type: application/x-patch, Size: 5059 bytes --]

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

> For a command and its key in a repeat map:
> - "activate" means the command makes the repeat map active
> - "continue" means pressing the key keeps the repeat map active
>
> With this patch, the available directives are:
> :continue (default) - activates and continues
> :exit - neither activates nor continues
> :continue-only (new) - continues, but does not activate

How does this map to the properties ':enter' and ':exit' of 'defvar-keymap'?

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

On Fri, Nov 1, 2024 at 9:15 AM Juri Linkov <juri@linkov.net> wrote:
>
> > For a command and its key in a repeat map:
> > - "activate" means the command makes the repeat map active
> > - "continue" means pressing the key keeps the repeat map active
> >
> > With this patch, the available directives are:
> > :continue (default) - activates and continues
> > :exit - neither activates nor continues
> > :continue-only (new) - continues, but does not activate
>
> How does this map to the properties ':enter' and ':exit' of 'defvar-keymap'?

:exit has the same meaning in both.

In defvar-keymap, :enter means "activate, but do not continue".

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

On Fri, Nov 1, 2024 at 9:29 AM Paul Nelson <ultrono@gmail.com> wrote:
>
> On Fri, Nov 1, 2024 at 9:15 AM Juri Linkov <juri@linkov.net> wrote:
> >
> > > For a command and its key in a repeat map:
> > > - "activate" means the command makes the repeat map active
> > > - "continue" means pressing the key keeps the repeat map active
> > >
> > > With this patch, the available directives are:
> > > :continue (default) - activates and continues
> > > :exit - neither activates nor continues
> > > :continue-only (new) - continues, but does not activate
> >
> > How does this map to the properties ':enter' and ':exit' of 'defvar-keymap'?
>
> :exit has the same meaning in both.
>
> In defvar-keymap, :enter means "activate, but do not continue".

Another difference is that :enter from defvar-keymap does not actually
bind a key in the keymap, thus:

                  activate    continue     bind
:continue         yes         yes          yes
:continue-only    no          yes          yes
:exit             no          no           yes
:enter            yes         no           no

The ergonomics between the two macros are also quite different.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

>> > > With this patch, the available directives are:
>> > > :continue (default) - activates and continues
>> > > :exit - neither activates nor continues
>> > > :continue-only (new) - continues, but does not activate
>> >
>> > How does this map to the properties ':enter' and ':exit' of 'defvar-keymap'?
>>
>> :exit has the same meaning in both.
>>
>> In defvar-keymap, :enter means "activate, but do not continue".
>
> Another difference is that :enter from defvar-keymap does not actually
> bind a key in the keymap, thus:
>
>                   activate    continue     bind
> :continue         yes         yes          yes
> :continue-only    no          yes          yes
> :exit             no          no           yes
> :enter            yes         no           no

Thanks, this table provides a clear overview.

So bind-keys doesn't need :enter?  Ok.

Since defvar-keymap uses :continue by default,
what is missing in defvar-keymap is :continue-only.
For adding :continue-only to defvar-keymap
I looked how you implemented it for bind-keys,
and it seems making an alias doesn't look
like a clean solution.

I know that 'define-repeat-map' makes an alias as well.
So some time ago I had one idea how such aliases
could be avoided.  The solution would be to put
a new property like this:

  (put 'yank 'repeat-continue-keys '("y"))
  (put 'undo 'repeat-continue-keys '("C-/"))

Its semantics is that when such a property exists
then repeat-mode will check if the last typed keys
don't exist in this list, only then the repeat map
should be activated.

Then bind-keys could put such new properties
from the definition like in your example:

  :continue-only
  ;; These commands will be available during paragraph manipulation
  ;; but won't activate paragraph-repeat-map themselves
  ("y" . yank)
  ("C-/" . undo)

Do you think such a symbol property would cover all possible uses?

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

> >                   activate    continue     bind
> > :continue         yes         yes          yes
> > :continue-only    no          yes          yes
> > :exit             no          no           yes
> > :enter            yes         no           no
>
> Thanks, this table provides a clear overview.
>
> So bind-keys doesn't need :enter?  Ok.

I understand the purpose of bind-keys to be "bind keys to commands in
a keymap".  Since :enter doesn't bind a key, it doesn't fit in
bind-keys.

There is a missing fourth case ("yes, no, yes") describing a key bound
to a command in a repeat map which the command activates but the key
exits.  I haven't been imaginative enough to think of a good use case.
"Why does this command start a repeat sequence, but then terminate it
when I try to use it in that sequence?"

>
> Since defvar-keymap uses :continue by default,
> what is missing in defvar-keymap is :continue-only.
> For adding :continue-only to defvar-keymap
> I looked how you implemented it for bind-keys,
> and it seems making an alias doesn't look
> like a clean solution.
>
> I know that 'define-repeat-map' makes an alias as well.
> So some time ago I had one idea how such aliases
> could be avoided.  The solution would be to put
> a new property like this:
>
>   (put 'yank 'repeat-continue-keys '("y"))
>   (put 'undo 'repeat-continue-keys '("C-/"))
>
> Its semantics is that when such a property exists
> then repeat-mode will check if the last typed keys
> don't exist in this list, only then the repeat map
> should be activated.
>

I didn't quite follow.  If the repeat map is active and the user
presses "C-/", then repeat-mode will see that the key in question
exists in the repeat-continue-keys list, and so will not activate the
repeat map.  This is the opposite of the intended behavior.  Perhaps
I've misunderstood?


I think that in an ideal world, "repeat-map" would be a property of a
keybinding inside a keymap, rather than a command.  The alias-based
approach gives one approximation to that.

I guess one alias-free approach would be to introduce a
repeat-continue property describing keymaps that a command should
perpetuate, e.g.,

  (put 'yank 'repeat-continue '(paragraph-repeat-map))
  (put 'undo 'repeat-continue '(paragraph-repeat-map))

Anyway, is the idea that you'd like to see equivalent functionality in
defvar-keymap and/or to have both implementations avoid aliases?

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

>> >                   activate    continue     bind
>> > :continue         yes         yes          yes
>> > :continue-only    no          yes          yes
>> > :exit             no          no           yes
>> > :enter            yes         no           no
>>
>> Thanks, this table provides a clear overview.
>>
>> So bind-keys doesn't need :enter?  Ok.
>
> I understand the purpose of bind-keys to be "bind keys to commands in
> a keymap".  Since :enter doesn't bind a key, it doesn't fit in
> bind-keys.

In defvar-keymap, :enter defines for a list of commands
that activate a repeat-map with their global keybindings.
Such as e.g. 'C-x u' for 'undo' when there are no keys
with the 'undo' keybinding in the repeat-map.

> There is a missing fourth case ("yes, no, yes") describing a key bound
> to a command in a repeat map which the command activates but the key
> exits.  I haven't been imaginative enough to think of a good use case.
> "Why does this command start a repeat sequence, but then terminate it
> when I try to use it in that sequence?"

I guess the case "yes, no, yes" is not needed because
no one might want to define a global keybinding via
bind-keys or defvar-keymap.

>> Since defvar-keymap uses :continue by default,
>> what is missing in defvar-keymap is :continue-only.
>> For adding :continue-only to defvar-keymap
>> I looked how you implemented it for bind-keys,
>> and it seems making an alias doesn't look
>> like a clean solution.
>>
>> I know that 'define-repeat-map' makes an alias as well.
>> So some time ago I had one idea how such aliases
>> could be avoided.  The solution would be to put
>> a new property like this:
>>
>>   (put 'yank 'repeat-continue-keys '("y"))
>>   (put 'undo 'repeat-continue-keys '("C-/"))
>>
>> Its semantics is that when such a property exists
>> then repeat-mode will check if the last typed keys
>> don't exist in this list, only then the repeat map
>> should be activated.
>
> I didn't quite follow.  If the repeat map is active and the user
> presses "C-/", then repeat-mode will see that the key in question
> exists in the repeat-continue-keys list, and so will not activate the
> repeat map.  This is the opposite of the intended behavior.  Perhaps
> I've misunderstood?

The property 'repeat-continue-keys' is intended to be used only
to decide whether a global key sequence should activate the repeat-map
or not.

So maybe a better property name would be 'repeat-no-enter-keys'.
Then your new bind-keys directive :continue-only could be named
:no-entry (but :continue-only is fine if it's more clear).

In any case such property could check not a command name,
but a global key sequence, then decide whether it should activate
the repeat-map.

> I think that in an ideal world, "repeat-map" would be a property of a
> keybinding inside a keymap, rather than a command.  The alias-based
> approach gives one approximation to that.

Extending the standard keymap format would complicate it.
So a less radical change would be to add extra information
in the symbol property list.  I see now that the previous idea
was insufficient since it lacks a map.  So a better way is to define
a combination of a repeat-map and a key:

  (put 'undo 'repeat-continue-keys '((paragraph-repeat-map . "C-/")))

> I guess one alias-free approach would be to introduce a
> repeat-continue property describing keymaps that a command should
> perpetuate, e.g.,
>
>   (put 'yank 'repeat-continue '(paragraph-repeat-map))
>   (put 'undo 'repeat-continue '(paragraph-repeat-map))

Keys are required as well to be able to decide whether
to activate the repeat-map.

There are two possible ways to do this: opt-in and out-out.

1. opt-in defines all global keys that activate a repeat-map. e.g.:

  (put 'undo 'repeat-entry-keys '((paragraph-repeat-map . "C-x u")))

2. out-out defines only keys from the repeat-map that should not activate it
   by a global key such as the global "u" ('self-insert-command')
   should not activate paragraph-repeat-map:

  (put 'undo 'repeat-no-entry-keys '((paragraph-repeat-map . "u")))

> Anyway, is the idea that you'd like to see equivalent functionality in
> defvar-keymap and/or to have both implementations avoid aliases?

Yes, we need to keep feature parity between bind-keys and defvar-keymap,
and I'd like to avoid aliases in defvar-keymap.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

> I see now that the previous idea
> was insufficient since it lacks a map.  So a better way is to define
> a combination of a repeat-map and a key:
>
>   (put 'undo 'repeat-continue-keys '((paragraph-repeat-map . "C-/")))
>
> > I guess one alias-free approach would be to introduce a
> > repeat-continue property describing keymaps that a command should
> > perpetuate, e.g.,
> >
> >   (put 'yank 'repeat-continue '(paragraph-repeat-map))
> >   (put 'undo 'repeat-continue '(paragraph-repeat-map))
>
> Keys are required as well to be able to decide whether
> to activate the repeat-map.
>

I still don't follow.  Couldn't we somehow keep track of the active
repeat-map (or really, its underlying symbol) and check whether that
lies in the provided list?  Why should it be necessary to keep track
of the key that activated it?  In any event, we don't know in advance
whether the specific key is the same or different across the keymaps
that we do or do not want the command to participate in, so I don't
see how knowing the key could be a useful differentiating factor.

> There are two possible ways to do this: opt-in and out-out.
>
> 1. opt-in defines all global keys that activate a repeat-map. e.g.:
>
>   (put 'undo 'repeat-entry-keys '((paragraph-repeat-map . "C-x u")))
>
> 2. out-out defines only keys from the repeat-map that should not activate it
>    by a global key such as the global "u" ('self-insert-command')
>    should not activate paragraph-repeat-map:
>
>   (put 'undo 'repeat-no-entry-keys '((paragraph-repeat-map . "u")))

What do you have in mind if the user updates the binds in the keymaps?

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

>> Keys are required as well to be able to decide whether
>> to activate the repeat-map.
>
> I still don't follow.  Couldn't we somehow keep track of the active
> repeat-map (or really, its underlying symbol) and check whether that
> lies in the provided list?  Why should it be necessary to keep track
> of the key that activated it?  In any event, we don't know in advance
> whether the specific key is the same or different across the keymaps
> that we do or do not want the command to participate in, so I don't
> see how knowing the key could be a useful differentiating factor.

Keys will help to avoid aliases.  In your example:

 :continue-only
 ;; These commands will be available during paragraph manipulation
 ;; but won't activate paragraph-repeat-map themselves
 ("y" . yank)
 ("C-/" . undo))

to be able to not activate repeat-map on executing the command 'yank'
with the global keybinding such as 'C-y', we can check
that last-command-event is 'y'.

Or do you think it would help to take into account the fact
that the current repeat-map is already activated?  But still
in this case I don't see how it's possible to avoid checking the key.

>> There are two possible ways to do this: opt-in and out-out.
>>
>> 1. opt-in defines all global keys that activate a repeat-map. e.g.:
>>
>>   (put 'undo 'repeat-entry-keys '((paragraph-repeat-map . "C-x u")))
>>
>> 2. out-out defines only keys from the repeat-map that should not activate it
>>    by a global key such as the global "u" ('self-insert-command')
>>    should not activate paragraph-repeat-map:
>>
>>   (put 'undo 'repeat-no-entry-keys '((paragraph-repeat-map . "u")))
>
> What do you have in mind if the user updates the binds in the keymaps?

After any updates the user needs to update symbol properties as well.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

>> I still don't follow.  Couldn't we somehow keep track of the active
>> repeat-map (or really, its underlying symbol) and check whether that
>> lies in the provided list?  Why should it be necessary to keep track
>> of the key that activated it?  In any event, we don't know in advance
>> whether the specific key is the same or different across the keymaps
>> that we do or do not want the command to participate in, so I don't
>> see how knowing the key could be a useful differentiating factor.
>
> Keys will help to avoid aliases.  In your example:
>
>  :continue-only
>  ;; These commands will be available during paragraph manipulation
>  ;; but won't activate paragraph-repeat-map themselves
>  ("y" . yank)
>  ("C-/" . undo))
>
> to be able to not activate repeat-map on executing the command 'yank'
> with the global keybinding such as 'C-y', we can check
> that last-command-event is 'y'.
>

My point was that the "C-/" here is the same in the repeat map and the
global map, so we can't use it to distinguish the two.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

>> Keys will help to avoid aliases.  In your example:
>>
>>  :continue-only
>>  ;; These commands will be available during paragraph manipulation
>>  ;; but won't activate paragraph-repeat-map themselves
>>  ("y" . yank)
>>  ("C-/" . undo))
>>
>> to be able to not activate repeat-map on executing the command 'yank'
>> with the global keybinding such as 'C-y', we can check
>> that last-command-event is 'y'.
>
> My point was that the "C-/" here is the same in the repeat map and the
> global map, so we can't use it to distinguish the two.

Thanks for pointing out the case when the command is bound to the same key
globally and in the repeat map.  So checking for a key can't help here.

Therefore, I implemented another solution in repeat.el.  There is now
a new symbol property 'repeat-continue-only'.  And a command with this
symbol property will not activate the repeat map, but will only continue
the already activated repeating sequence.   This is implemented by
a simple change:

      (when (and (repeat-check-map map)
                 (or (null (repeat--command-property 'repeat-continue-only))
                     was-in-progress))

Also I improved the existing tests to use more mnemonic keys:

  a - Activate (enter),
  c - Continue,
  o - continue-Only (not activate),
  q - Quit (exit)

Currently the test uses (put 'repeat-tests-call-o 'repeat-continue-only t)
but a new keyword ':continue-only' will be added to 'defvar-keymap'.

The same way you could try to use the new symbol property for 'bind-keys'.

I wrote a new test for 'bind-keys' in test/lisp/repeat-tests.el.
But the part for :continue-only is currently commented out.

BTW, while writing the 'bind-keys' test, I noticed there is no way
to specify a command that only activates, but not continues
(the same as :entry in 'defvar-keymap').  Is it true that 'bind-keys'
has no such keyword, so there is a need to do this explicitly with:

  (put 'repeat-tests-bind-call-a 'repeat-map 'repeat-tests-bind-keys-repeat-map)

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

> Thanks for pointing out the case when the command is bound to the same key
> globally and in the repeat map.  So checking for a key can't help here.
>
> Therefore, I implemented another solution in repeat.el.  There is now
> a new symbol property 'repeat-continue-only'.  And a command with this
> symbol property will not activate the repeat map, but will only continue
> the already activated repeating sequence.   This is implemented by
> a simple change:
>
>       (when (and (repeat-check-map map)
>                  (or (null (repeat--command-property 'repeat-continue-only))
>                      was-in-progress))
>

Thanks, I took a look.  One disadvantage of this approach is that it
does not allow the same command to enter one repeat map and continue
another.  I'll confess that I'm not aware of any such examples in my
config, but it still seems like an undesirable "non-local" effect.

This issue motivated my suggestion that the symbol property should be a
list of repeat maps that the command continues, although I haven't
considered the details (e.g., concerning the map vs. the symbol that
points to it) - maybe you have a clearer picture of those.

> BTW, while writing the 'bind-keys' test, I noticed there is no way
> to specify a command that only activates, but not continues
> (the same as :entry in 'defvar-keymap').  Is it true that 'bind-keys'
> has no such keyword, so there is a need to do this explicitly with:
>
>   (put 'repeat-tests-bind-call-a 'repeat-map 'repeat-tests-bind-keys-repeat-map)

Yes, that's also my understanding (but it's not clear to me that it
requires such a keyword if its purpose is to bind keys in a map).

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

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

>> Thanks for pointing out the case when the command is bound to the same key
>> globally and in the repeat map.  So checking for a key can't help here.
>>
>> Therefore, I implemented another solution in repeat.el.  There is now
>> a new symbol property 'repeat-continue-only'.  And a command with this
>> symbol property will not activate the repeat map, but will only continue
>> the already activated repeating sequence.   This is implemented by
>> a simple change:
>>
>>       (when (and (repeat-check-map map)
>>                  (or (null (repeat--command-property 'repeat-continue-only))
>>                      was-in-progress))
>>
>
> Thanks, I took a look.  One disadvantage of this approach is that it
> does not allow the same command to enter one repeat map and continue
> another.  I'll confess that I'm not aware of any such examples in my
> config, but it still seems like an undesirable "non-local" effect.

Other existing properties such as 'repeat-exit-timeout' or 'repeat-check-key'
don't accept a list of repeat maps, but only a non-nil value (including the
special symbol :no).  However, this can be changed gradually by adding
support for the list to them.

So below is a patch that adds support for a list of maps.
And users don't have to populate the list manually because
'defvar-keymap' does this automatically with :continue-only
in the same patch.

> This issue motivated my suggestion that the symbol property should be a
> list of repeat maps that the command continues, although I haven't
> considered the details (e.g., concerning the map vs. the symbol that
> points to it) - maybe you have a clearer picture of those.

Using a variable value for the map property is supported but not encouraged.
There are too many problems when not using a symbol.  So let's support
only symbols in the list of repeat maps for :continue-only.

>> BTW, while writing the 'bind-keys' test, I noticed there is no way
>> to specify a command that only activates, but not continues
>> (the same as :entry in 'defvar-keymap').  Is it true that 'bind-keys'
>> has no such keyword, so there is a need to do this explicitly with:
>>
>>   (put 'repeat-tests-bind-call-a 'repeat-map 'repeat-tests-bind-keys-repeat-map)
>
> Yes, that's also my understanding (but it's not clear to me that it
> requires such a keyword if its purpose is to bind keys in a map).

My goal was to write the same test for 'bind-keys'
as for 'defvar-keymap' in test/lisp/repeat-tests.el:

  (bind-keys
   :map repeat-tests-bind-keys-map
   ("C-M-a" . repeat-tests-bind-call-a)
   ("C-M-o" . repeat-tests-bind-call-o)
   :repeat-map repeat-tests-bind-keys-repeat-map
   :continue
   ("c"     . repeat-tests-bind-call-c)
   :continue-only
   ("C-M-o" . repeat-tests-bind-call-o)
   :exit
   ("q"     . repeat-tests-bind-call-q))

But I can't find a way to do the same what the :enter key
does in 'defvar-keymap', so needed to set the symbol explicitly:

  (put 'repeat-tests-bind-call-a 'repeat-map 'repeat-tests-bind-keys-repeat-map)


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

diff --git a/lisp/keymap.el b/lisp/keymap.el
index 9b133e1ca82..43c8d918ba7 100644
--- a/lisp/keymap.el
+++ b/lisp/keymap.el
@@ -687,6 +687,7 @@ defvar-keymap
 `:exit' and `:hints', for example:
 
      :repeat (:enter (commands ...) :exit (commands ...)
+              :continue-only (commands ...)
               :hints ((command . \"hint\") ...))
 
 `:enter' specifies the list of additional commands that only
@@ -702,6 +703,10 @@ defvar-keymap
 in this specific map, but should not have the `repeat-map' symbol
 property.
 
+`:continue-only' specifies the list of commands that should not
+enter `repeat-mode'.  These command should only continue the
+already activated repeating sequence.
+
 `:hints' is a list of cons pairs where car is a command and
 cdr is a string that is displayed alongside of the repeatable key
 in the echo area.
@@ -740,6 +745,10 @@ defvar-keymap
             def)
         (dolist (def (plist-get repeat :enter))
           (push `(put ',def 'repeat-map ',variable-name) props))
+        (dolist (def (plist-get repeat :continue-only))
+          (push `(put ',def 'repeat-continue-only
+                      (cons ',variable-name (get ',def 'repeat-continue-only)))
+                props))
         (while defs
           (pop defs)
           (setq def (pop defs))
diff --git a/lisp/repeat.el b/lisp/repeat.el
index 11d26a477b6..45888d9db08 100644
--- a/lisp/repeat.el
+++ b/lisp/repeat.el
@@ -505,8 +505,12 @@ repeat-post-hook
     (setq repeat-in-progress nil)
     (let ((map (repeat-get-map)))
       (when (and (repeat-check-map map)
-                 (or (null (repeat--command-property 'repeat-continue-only))
-                     was-in-progress))
+                 (let ((continue-only (repeat--command-property 'repeat-continue-only)))
+                   (or (null continue-only)
+                       (and (or (not (consp continue-only))
+                                (memq (repeat--command-property 'repeat-map)
+                                      continue-only))
+                            was-in-progress))))
         ;; Messaging
         (funcall repeat-echo-function map)
 
diff --git a/test/lisp/repeat-tests.el b/test/lisp/repeat-tests.el
index c560a283039..527963e3ef8 100644
--- a/test/lisp/repeat-tests.el
+++ b/test/lisp/repeat-tests.el
@@ -63,17 +63,15 @@ repeat-tests-map
 
 (defvar-keymap repeat-tests-repeat-map
   :doc "Keymap for repeating sequences."
-  :repeat ( :enter (repeat-tests-call-a)
-            :exit  (repeat-tests-call-q))
+  :repeat ( :enter         (repeat-tests-call-a)
+            :continue-only (repeat-tests-call-o)
+            :exit          (repeat-tests-call-q))
   "a"     'ignore ;; for non-nil repeat-check-key only
   "c"     'repeat-tests-call-c
   "d"     'repeat-tests-call-d
   "C-M-o" 'repeat-tests-call-o
   "q"     'repeat-tests-call-q)
 
-;; TODO: add new keyword ':continue-only (repeat-tests-call-o)'
-(put 'repeat-tests-call-o 'repeat-continue-only t)
-
 ;; Test using a variable instead of the symbol:
 (put 'repeat-tests-call-b 'repeat-map repeat-tests-repeat-map)
 

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

> So below is a patch that adds support for a list of maps.
> And users don't have to populate the list manually because
> 'defvar-keymap' does this automatically with :continue-only
> in the same patch.

Now this patch is pushed, so you can use the new property
to implement the :continue-only directive in bind-keys.

BTW, bind-keys needs more repeat-related fixes
in the new bug report bug#74660.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

Thanks, I'll try it out soon and amend my patch accordingly.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Eli Zaretskii]

> Cc: 74140@debbugs.gnu.org
> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 03 Dec 2024 20:12:36 +0200
> 
> > So below is a patch that adds support for a list of maps.
> > And users don't have to populate the list manually because
> > 'defvar-keymap' does this automatically with :continue-only
> > in the same patch.
> 
> Now this patch is pushed, so you can use the new property
> to implement the :continue-only directive in bind-keys.

Thanks, but I think the NEWS entry needs a small clarification:

   ** New symbol property 'repeat-continue-only' for 'repeat-mode'.
  -A command with this symbol property whose value is non-nil will not
  -activate the repeat map in 'repeat-mode', it will only continue the
  -already activated repeating sequence.
  +A command with this symbol property whose value is a list of repeat
  +maps will not activate the repeat map in 'repeat-mode'.  It will only
  +continue the already activated repeating sequence.  Also 'defvar-keymap'
  +supports a new keyword ':continue-only' with a list of commands that
  +only continue the active repeating sequence.

This doesn't say what is the significance of the repeat maps included
in the value of the property (as opposed to those not included).  I'm
guessing they have some influence on the result, so NEWS should at
least hint on that.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

>    ** New symbol property 'repeat-continue-only' for 'repeat-mode'.
>   -A command with this symbol property whose value is non-nil will not
>   -activate the repeat map in 'repeat-mode', it will only continue the
>   -already activated repeating sequence.
>   +A command with this symbol property whose value is a list of repeat
>   +maps will not activate the repeat map in 'repeat-mode'.  It will only
>   +continue the already activated repeating sequence.  Also 'defvar-keymap'
>   +supports a new keyword ':continue-only' with a list of commands that
>   +only continue the active repeating sequence.
>
> This doesn't say what is the significance of the repeat maps included
> in the value of the property (as opposed to those not included).  I'm
> guessing they have some influence on the result, so NEWS should at
> least hint on that.

When I tried to explain better, I realized there is no need to have
a list of repeat maps, because a command can't have more than 1 map
in its symbol property 'repeat-map'.

Let's wait Paul to confirm whether it will be sufficient for
bind-keys to just set 'repeat-continue-only' to t, not a list.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

>
> Let's wait Paul to confirm whether it will be sufficient for
> bind-keys to just set 'repeat-continue-only' to t, not a list.

Thanks, I'll take a careful look this coming weekend and let you know.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

Thanks Juri, I took a look at your implementation.  It has at least the
following issue: the same command cannot be used to continue several
repeat maps.  Consider for instance the following example:

(defvar-keymap repeat-list-map
  :doc "Keymap for repeating sequences."
  :repeat ( :continue-only (yank undo))
  "n" 'forward-list
  "p" 'backward-list
  "C-/" 'undo
  "y" 'yank)

(defvar-keymap repeat-paragraph-map
  :doc "Keymap for repeating sequences."
  :repeat ( :continue-only (yank undo))
  "]" 'forward-paragraph
  "}" 'forward-paragraph
  "[" 'backward-paragraph
  "{" 'backward-paragraph
  "C-/" 'undo
  "y" 'yank)

This is why I think the repeat-continue-only property should be a list
specifying which repeat maps the command should continue.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

> Thanks Juri, I took a look at your implementation.  It has at least the
> following issue: the same command cannot be used to continue several
> repeat maps.  Consider for instance the following example:
>
> (defvar-keymap repeat-list-map
>   :doc "Keymap for repeating sequences."
>   :repeat ( :continue-only (yank undo))
>   "n" 'forward-list
>   "p" 'backward-list
>   "C-/" 'undo
>   "y" 'yank)
>
> (defvar-keymap repeat-paragraph-map
>   :doc "Keymap for repeating sequences."
>   :repeat ( :continue-only (yank undo))
>   "]" 'forward-paragraph
>   "}" 'forward-paragraph
>   "[" 'backward-paragraph
>   "{" 'backward-paragraph
>   "C-/" 'undo
>   "y" 'yank)
>
> This is why I think the repeat-continue-only property should be a list
> specifying which repeat maps the command should continue.

When I tried to evaluate your repeat-maps, I see that
the 'repeat-continue-only' property is already a list:

  (symbol-plist 'undo)
  => (repeat-map repeat-paragraph-map
      repeat-continue-only (repeat-paragraph-map repeat-list-map))

Maybe the problem is that currently the 'repeat-map' property
is not a list?

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

Juri Linkov <juri@linkov.net> writes:

>
> When I tried to evaluate your repeat-maps, I see that
> the 'repeat-continue-only' property is already a list:
>
>   (symbol-plist 'undo)
>   => (repeat-map repeat-paragraph-map
>       repeat-continue-only (repeat-paragraph-map repeat-list-map))
>
> Maybe the problem is that currently the 'repeat-map' property
> is not a list?

I think the repeat-map property is fine as is, but the logic should be a
bit different:

- If a command is called while repeat map MAP is active, then:

  - If the command's repeat-continue-only property contains MAP, then
    MAP remains active (i.e., that map "continues").  [Maybe a better
    name for the property would be simply "repeat-continue".]

  - Otherwise, if the command has a repeat-map property, then that
    becomes the new repeat map.

  - Otherwise, the repeat map deactivates.

- If a command is called while no repeat map is active, then we proceed
  as before.

Do you agree that this is the desired behavior?  The difference is that
with the current implementation, only the repeat-map property is
relevant for determining which repeat map a command activates or
continues.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Juri Linkov]

> I think the repeat-map property is fine as is, but the logic should be a
> bit different:
>
> - If a command is called while repeat map MAP is active, then:
>
>   - If the command's repeat-continue-only property contains MAP, then
>     MAP remains active (i.e., that map "continues").  [Maybe a better
>     name for the property would be simply "repeat-continue".]

So the property is renamed now to 'repeat-continue'.

>   - Otherwise, if the command has a repeat-map property, then that
>     becomes the new repeat map.
>
>   - Otherwise, the repeat map deactivates.
>
> - If a command is called while no repeat map is active, then we proceed
>   as before.
>
> Do you agree that this is the desired behavior?  The difference is that
> with the current implementation, only the repeat-map property is
> relevant for determining which repeat map a command activates or
> continues.

Thanks for the clear explanation.  Now this is completely implemented
with tests.

bug#74140: [PATCH] Add :continue-only directive for repeat maps in bind-keys, use-package

[Paul Nelson]

Juri Linkov <juri@linkov.net> writes:

>> I think the repeat-map property is fine as is, but the logic should be a
>> bit different:
>>
>> - If a command is called while repeat map MAP is active, then:
>>
>>   - If the command's repeat-continue-only property contains MAP, then
>>     MAP remains active (i.e., that map "continues").  [Maybe a better
>>     name for the property would be simply "repeat-continue".]
>
> So the property is renamed now to 'repeat-continue'.
>
>>   - Otherwise, if the command has a repeat-map property, then that
>>     becomes the new repeat map.
>>
>>   - Otherwise, the repeat map deactivates.
>>
>> - If a command is called while no repeat map is active, then we proceed
>>   as before.
>>
>> Do you agree that this is the desired behavior?  The difference is that
>> with the current implementation, only the repeat-map property is
>> relevant for determining which repeat map a command activates or
>> continues.
>
> Thanks for the clear explanation.  Now this is completely implemented
> with tests.

Thanks Juri, I tried it out and have noticed an issue.  Consider as
before the example:

(defvar-keymap repeat-list-map
  :doc "Keymap for repeating sequences."
  :repeat ( :continue (yank undo))
  "n" 'forward-list
  "p" 'backward-list
  "C-/" 'undo
  "y" 'yank)

(defvar-keymap repeat-paragraph-map
  :doc "Keymap for repeating sequences."
  :repeat ( :continue (yank undo))
  "]" 'forward-paragraph
  "}" 'forward-paragraph
  "[" 'backward-paragraph
  "{" 'backward-paragraph
  "C-/" 'undo
  "y" 'yank)

If we now do forward-list followed by two repeat-map yanks (i.e., C-M-n
y y), then we wind up in repeat-paragraph-map (but we should have never
left repeat-list-map).

To diagnose, let's look at (symbol-plist #'yank):

(... repeat-continue (repeat-paragraph-map repeat-list-map)
     repeat-map repeat-paragraph-map)

Here the precise value of the repeat-map property of yank should not
affect behavior (and so that property should not exist), but it does,
due to the way repeat-post-hook is currently coded.  What happens is
that when we are continuing a repeat map via repeat-continue, the value
of map-sym is discarded in the evaluation of

  (repeat-get-map map-sym)

but gets used later in

  (setq repeat-in-progress map-sym)

In such cases, we should instead (somehow) first modify map-sym so that
it is the symbol describing the "continue" map whose value is ultimately
returned via (repeat-get-map map-sym).  I'm not sure how best to do so.

Hope that makes sense; happy to clarify or think further if it'd help.

Paul