Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

[Stefan Monnier]

Hi Jury,

> +(defvar next-error-repeat-map
> +  (let ((map (make-sparse-keymap)))
> +    (define-key map    "n" 'next-error)
> +    (define-key map "\M-n" 'next-error)
> +    (define-key map    "p" 'previous-error)
> +    (define-key map "\M-p" 'previous-error)
> +    map)
> +  "Keymap to repeat next-error key sequences.  Used in `repeat-mode'.")
> +(put 'next-error 'repeat-map 'next-error-repeat-map)
> +(put 'previous-error 'repeat-map 'next-error-repeat-map)

Could we avoid this duplication between the map and the
`repeat-map` property?

> +            (set-transient-map map)))))))

For example passing a second argument t to `set-transient-map` makes
`set-transient-map` look to see if the key just typed was found in the
map and if so keep the transient alive, which seems to [after an
admittedly cursory check of the code] play the same role as what your
`repeat-map` property does, but without the duplication.


        Stefan

Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

[Juri Linkov]

>> +(defvar next-error-repeat-map
>> +  (let ((map (make-sparse-keymap)))
>> +    (define-key map    "n" 'next-error)
>> +    (define-key map "\M-n" 'next-error)
>> +    (define-key map    "p" 'previous-error)
>> +    (define-key map "\M-p" 'previous-error)
>> +    map)
>> +  "Keymap to repeat next-error key sequences.  Used in `repeat-mode'.")
>> +(put 'next-error 'repeat-map 'next-error-repeat-map)
>> +(put 'previous-error 'repeat-map 'next-error-repeat-map)
>
> Could we avoid this duplication between the map and the
> `repeat-map` property?
>
>> +            (set-transient-map map)))))))
>
> For example passing a second argument t to `set-transient-map` makes
> `set-transient-map` look to see if the key just typed was found in the
> map and if so keep the transient alive, which seems to [after an
> admittedly cursory check of the code] play the same role as what your
> `repeat-map` property does, but without the duplication.

The current design relies on post-command-hook that runs after every command.
Using the second argument KEEP-PRED of set-transient-map will add duplication:
the same map will be set by both post-command-hook and KEEP-PRED.

So currently there is no duplication.  But is it possible to avoid duplication
when using KEEP-PRED too?  Could post-command-hook detect that KEEP-PRED
is in use for the next command, and not to call set-transient-map again?

Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

[Stefan Monnier]

>>> +(defvar next-error-repeat-map
>>> +  (let ((map (make-sparse-keymap)))
>>> +    (define-key map    "n" 'next-error)
>>> +    (define-key map "\M-n" 'next-error)
>>> +    (define-key map    "p" 'previous-error)
>>> +    (define-key map "\M-p" 'previous-error)
>>> +    map)
>>> +  "Keymap to repeat next-error key sequences.  Used in `repeat-mode'.")
>>> +(put 'next-error 'repeat-map 'next-error-repeat-map)
>>> +(put 'previous-error 'repeat-map 'next-error-repeat-map)
>>
>> Could we avoid this duplication between the map and the
>> `repeat-map` property?
>>
>>> +            (set-transient-map map)))))))
>>
>> For example passing a second argument t to `set-transient-map` makes
>> `set-transient-map` look to see if the key just typed was found in the
>> map and if so keep the transient alive, which seems to [after an
>> admittedly cursory check of the code] play the same role as what your
>> `repeat-map` property does, but without the duplication.
>
> The current design relies on post-command-hook that runs after every command.
> Using the second argument KEEP-PRED of set-transient-map will add duplication:
> the same map will be set by both post-command-hook and KEEP-PRED.

My point was not to say "use `keep-pred`", but to point out that
`keep-pred` found a way to solve the same problem without the duplication.

[ I'd be nice to be able to use `keep-pred` directly since it would
  likely make the code simpler, but it may well be that this is
  incompatible with the current code's design or with its intended
  behavior, or that it would require further changes that would make the
  overall code more complex.  ]

> So currently there is no duplication.

There is in the above code snippet: the `put` statements are redundant
since the same info is fundamentally already available from
`next-error-repeat-map`.


        Stefan

Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

[Juri Linkov]

>>>> +(defvar next-error-repeat-map
>>>> +  (let ((map (make-sparse-keymap)))
>>>> +    (define-key map    "n" 'next-error)
>>>> +    (define-key map "\M-n" 'next-error)
>>>> +    (define-key map    "p" 'previous-error)
>>>> +    (define-key map "\M-p" 'previous-error)
>>>> +    map)
>>>> +  "Keymap to repeat next-error key sequences.  Used in `repeat-mode'.")
>>>> +(put 'next-error 'repeat-map 'next-error-repeat-map)
>>>> +(put 'previous-error 'repeat-map 'next-error-repeat-map)
>>>
>>> Could we avoid this duplication between the map and the
>>> `repeat-map` property?
>
> There is in the above code snippet: the `put` statements are redundant
> since the same info is fundamentally already available from
> `next-error-repeat-map`.

Now I see where is duplication.  But currently I have no idea
in what keymaps post-command-hook should look for keybindings.

When a command is associated with a keymap `next-error-repeat-map`
via `put` property, then post-command-hook knows where to look
for keybindings for the current command (using `this-command`).

Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

[Stefan Monnier]

>>>>> +(defvar next-error-repeat-map
>>>>> +  (let ((map (make-sparse-keymap)))
>>>>> +    (define-key map    "n" 'next-error)
>>>>> +    (define-key map "\M-n" 'next-error)
>>>>> +    (define-key map    "p" 'previous-error)
>>>>> +    (define-key map "\M-p" 'previous-error)
>>>>> +    map)
>>>>> +  "Keymap to repeat next-error key sequences.  Used in `repeat-mode'.")
>>>>> +(put 'next-error 'repeat-map 'next-error-repeat-map)
>>>>> +(put 'previous-error 'repeat-map 'next-error-repeat-map)
>>>>
>>>> Could we avoid this duplication between the map and the
>>>> `repeat-map` property?
>>
>> There is in the above code snippet: the `put` statements are redundant
>> since the same info is fundamentally already available from
>> `next-error-repeat-map`.
>
> Now I see where is duplication.  But currently I have no idea
> in what keymaps post-command-hook should look for keybindings.

I don't see the problem: since your code installs the keymap, it can
also arrange to stash the info of which keymap to look up.


        Stefan

Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

[Juri Linkov]

>>>>>> +(defvar next-error-repeat-map
>>>>>> +  (let ((map (make-sparse-keymap)))
>>>>>> +    (define-key map    "n" 'next-error)
>>>>>> +    (define-key map "\M-n" 'next-error)
>>>>>> +    (define-key map    "p" 'previous-error)
>>>>>> +    (define-key map "\M-p" 'previous-error)
>>>>>> +    map)
>>>>>> +  "Keymap to repeat next-error key sequences.  Used in `repeat-mode'.")
>>>>>> +(put 'next-error 'repeat-map 'next-error-repeat-map)
>>>>>> +(put 'previous-error 'repeat-map 'next-error-repeat-map)
>>>>>
>>>>> Could we avoid this duplication between the map and the
>>>>> `repeat-map` property?
>>>
>>> There is in the above code snippet: the `put` statements are redundant
>>> since the same info is fundamentally already available from
>>> `next-error-repeat-map`.
>>
>> Now I see where is duplication.  But currently I have no idea
>> in what keymaps post-command-hook should look for keybindings.
>
> I don't see the problem: since your code installs the keymap, it can
> also arrange to stash the info of which keymap to look up.

In

  (put 'next-error 'repeat-map 'next-error-repeat-map)
  (put 'previous-error 'repeat-map 'next-error-repeat-map)

both next-error and previous-error are entry points to
a repeatable key sequence, i.e. both can start a key sequence:

  M-g n n p ...
  M-g p p n ...

Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

[Stefan Monnier]

>> I don't see the problem: since your code installs the keymap, it can
>> also arrange to stash the info of which keymap to look up.
>
> In
>
>   (put 'next-error 'repeat-map 'next-error-repeat-map)
>   (put 'previous-error 'repeat-map 'next-error-repeat-map)
>
> both next-error and previous-error are entry points to
> a repeatable key sequence, i.e. both can start a key sequence:
>
>   M-g n n p ...
>   M-g p p n ...

I don't understand why you think what I said is related to what you say.
Some code somewhere installs a transient keymap.  *That* code can do
some extra work to stash that keymap somewhere else as well so you can
look it up from there instead of having to check the `repeat-map` property.


        Stefan

Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

[Juri Linkov]

>>> I don't see the problem: since your code installs the keymap, it can
>>> also arrange to stash the info of which keymap to look up.
>>
>> In
>>
>>   (put 'next-error 'repeat-map 'next-error-repeat-map)
>>   (put 'previous-error 'repeat-map 'next-error-repeat-map)
>>
>> both next-error and previous-error are entry points to
>> a repeatable key sequence, i.e. both can start a key sequence:
>>
>>   M-g n n p ...
>>   M-g p p n ...
>
> I don't understand why you think what I said is related to what you say.
> Some code somewhere installs a transient keymap.  *That* code can do
> some extra work to stash that keymap somewhere else as well so you can
> look it up from there instead of having to check the `repeat-map` property.

The key word is "somewhere".  The first somewhere that installs
a transient keymap in post-command-hook needs to get that keymap.
Currently it gets it from the symbol property of this-command.

The second somewhere is the place where it's already stashed by
set-transient-map in overriding-terminal-local-map.  That keymap
could be checked, but there is no need since it would be easier
just to use the KEEP-PRED arg.

Re: master 12409c9: New transient mode 'repeat-mode' to allow shorter key sequences (bug#46515)

[Stefan Monnier]

>> I don't understand why you think what I said is related to what you say.
>> Some code somewhere installs a transient keymap.  *That* code can do
>> some extra work to stash that keymap somewhere else as well so you can
>> look it up from there instead of having to check the `repeat-map` property.
>
> The key word is "somewhere".  The first somewhere that installs
> a transient keymap in post-command-hook needs to get that keymap.
> Currently it gets it from the symbol property of this-command.

Aahhh.... now I understand.

So the `repeat-map` property is actually not redundant: it records which
transient keymap to use after that command and that keymap could
conceivably not include that command.  Also, not all commands in that
keymap should trigger the transient use of that keymap.

I understand quickly but only after a long explanation.
Sorry 'bout that,


        Stefan