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).