Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Robert Pluim]

>>>>> On Sat, 23 Oct 2021 21:31:06 +0300, Juri Linkov <juri@linkov.net> said:

    >> Something like this for master? emacs-28 would be nice, but this is
    >> not a bugfix.
    >> 
    >> +            (princ (format-message " `%s' (bound to %s)\n" command
    >> +                                   (mapconcat
    >> +                                    (lambda (key)
    >> +                                      (format "'%s'" (key-description key)))
    >> + (where-is-internal command (symbol-value (car keymap)))
    >> +                                    " ,"))))

    Juri> Thanks, I tried this, but it shows global keybindings,
    Juri> not shorter repeatable keybindings from the repeat-map.
    Juri> For example,

    Juri>   ‘undo-repeat-map’ keymap is repeatable by these commands:
    Juri>    ‘undo’ (bound to 'u' ,'C-_' ,'<undo>' ,'C-/' ,'C-x u' ,'<menu-bar> <edit> <undo>')

    Juri> I expected that it will show only 'u' from ‘undo-repeat-map’.

Since somebody has liberally used 'defalias' in tab-bar.el, it becomes
this :-)

diff --git a/lisp/repeat.el b/lisp/repeat.el
index ac08952eaa..931615ed4c 100644
--- a/lisp/repeat.el
+++ b/lisp/repeat.el
@@ -516,7 +516,12 @@ describe-repeat-maps
           (princ (format-message "`%s' keymap is repeatable by these commands:\n"
                                  (car keymap)))
           (dolist (command (sort (cdr keymap) 'string-lessp))
-            (princ (format-message " `%s'\n" command)))
+            (let* ((info (help-fns--analyze-function command))
+                   (map (list (symbol-value (car keymap))))
+                   (desc (key-description
+                          (or (where-is-internal command map t)
+                              (where-is-internal (nth 3 info) map t)))))
+              (princ (format-message " `%s' (bound to '%s')\n" command desc))))
           (princ "\n"))))))
 
 (provide 'repeat)

Note that this doesnʼt find the bindings for 'O' in
other-window-repeat-map nor 'M' in tab-bar-move-repeat map, because
the commands theyʼre bound to donʼt have the 'repeat-map' property,
which we might want to fix.

Juri> PS: the long lines could be refilled, but this is a minor problem.

The lines should be short enough with the above.

Robert
-- 

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Juri Linkov]

>     >> Something like this for master? emacs-28 would be nice, but this is
>     >> not a bugfix.
>     >>
>     >> +            (princ (format-message " `%s' (bound to %s)\n" command
>     >> +                                   (mapconcat
>     >> +                                    (lambda (key)
>     >> +                                      (format "'%s'" (key-description key)))
>     >> + (where-is-internal command (symbol-value (car keymap)))
>     >> +                                    " ,"))))
>
>     Juri> Thanks, I tried this, but it shows global keybindings,
>     Juri> not shorter repeatable keybindings from the repeat-map.
>     Juri> For example,
>
>     Juri>   ‘undo-repeat-map’ keymap is repeatable by these commands:
>     Juri>    ‘undo’ (bound to 'u' ,'C-_' ,'<undo>' ,'C-/' ,'C-x u' ,'<menu-bar> <edit> <undo>')
>
>     Juri> I expected that it will show only 'u' from ‘undo-repeat-map’.
>
> Since somebody has liberally used 'defalias' in tab-bar.el, it becomes
> this :-)

I fixed aliases for tab-bar.el some time ago, but was busy with fixing
other problems in repeat.el, so sorry for not pushing the fixes earlier.
Now I'll push them together with the unfinished test suite for repeat.el  :-)

> diff --git a/lisp/repeat.el b/lisp/repeat.el
> index ac08952eaa..931615ed4c 100644
> --- a/lisp/repeat.el
> +++ b/lisp/repeat.el
> @@ -516,7 +516,12 @@ describe-repeat-maps
>            (princ (format-message "`%s' keymap is repeatable by these commands:\n"
>                                   (car keymap)))
>            (dolist (command (sort (cdr keymap) 'string-lessp))
> -            (princ (format-message " `%s'\n" command)))
> +            (let* ((info (help-fns--analyze-function command))
> +                   (map (list (symbol-value (car keymap))))
> +                   (desc (key-description
> +                          (or (where-is-internal command map t)
> +                              (where-is-internal (nth 3 info) map t)))))
> +              (princ (format-message " `%s' (bound to '%s')\n" command desc))))
>            (princ "\n"))))))

Thanks, I tried this out, and everything looks nice.
I could push this in the next few days after trying to solve
remaining problems:

> Note that this doesnʼt find the bindings for 'O' in
> other-window-repeat-map nor 'M' in tab-bar-move-repeat map, because
> the commands theyʼre bound to donʼt have the 'repeat-map' property,
> which we might want to fix.

Maybe it would be sufficient just to say that a key is bound to a lambda?

> Juri> PS: the long lines could be refilled, but this is a minor problem.
>
> The lines should be short enough with the above.

I agree, a long repeat-map is too unusual.

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Robert Pluim]

>>>>> On Sun, 24 Oct 2021 22:12:55 +0300, Juri Linkov <juri@linkov.net> said:
    Juri> I fixed aliases for tab-bar.el some time ago, but was busy with fixing
    Juri> other problems in repeat.el, so sorry for not pushing the fixes earlier.
    Juri> Now I'll push them together with the unfinished test suite for repeat.el  :-)

    >> diff --git a/lisp/repeat.el b/lisp/repeat.el
    >> index ac08952eaa..931615ed4c 100644
    >> --- a/lisp/repeat.el
    >> +++ b/lisp/repeat.el
    >> @@ -516,7 +516,12 @@ describe-repeat-maps
    >> (princ (format-message "`%s' keymap is repeatable by these commands:\n"
    >> (car keymap)))
    >> (dolist (command (sort (cdr keymap) 'string-lessp))
    >> -            (princ (format-message " `%s'\n" command)))
    >> +            (let* ((info (help-fns--analyze-function command))
    >> +                   (map (list (symbol-value (car keymap))))
    >> +                   (desc (key-description
    >> +                          (or (where-is-internal command map t)
    >> +                              (where-is-internal (nth 3 info) map t)))))
    >> +              (princ (format-message " `%s' (bound to '%s')\n" command desc))))
    >> (princ "\n"))))))

    Juri> Thanks, I tried this out, and everything looks nice.
    Juri> I could push this in the next few days after trying to solve
    Juri> remaining problems:

Thereʼs no rush

    >> Note that this doesnʼt find the bindings for 'O' in
    >> other-window-repeat-map nor 'M' in tab-bar-move-repeat map, because
    >> the commands theyʼre bound to donʼt have the 'repeat-map' property,
    >> which we might want to fix.

    Juri> Maybe it would be sufficient just to say that a key is bound to a lambda?

Iʼd rather have a real function name to refer to than a lambda, but
itʼs up to you, I guess.

Robert
-- 

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Juri Linkov]

>     >> Note that this doesnʼt find the bindings for 'O' in
>     >> other-window-repeat-map nor 'M' in tab-bar-move-repeat map, because
>     >> the commands theyʼre bound to donʼt have the 'repeat-map' property,
>     >> which we might want to fix.
>
>     Juri> Maybe it would be sufficient just to say that a key is bound to a lambda?
>
> Iʼd rather have a real function name to refer to than a lambda, but
> itʼs up to you, I guess.

But this means adding a new command with a name e.g. 'other-window-backward'
that will cycle windows in the opposite direction.

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Robert Pluim]

>>>>> On Mon, 25 Oct 2021 10:41:44 +0300, Juri Linkov <juri@linkov.net> said:

    >> >> Note that this doesnʼt find the bindings for 'O' in
    >> >> other-window-repeat-map nor 'M' in tab-bar-move-repeat map, because
    >> >> the commands theyʼre bound to donʼt have the 'repeat-map' property,
    >> >> which we might want to fix.
    >> 
    Juri> Maybe it would be sufficient just to say that a key is
    >> bound to a lambda?
    >> 
    >> Iʼd rather have a real function name to refer to than a lambda, but
    >> itʼs up to you, I guess.

    Juri> But this means adding a new command with a name e.g. 'other-window-backward'
    Juri> that will cycle windows in the opposite direction.

As I said: itʼs up to you.

BTW, I tried using 'substitute-command-keys' to do this, since itʼs
more elegant, but it gave an unusual result for undo:

    (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
    =>#("C-x u" 0 5
      (font-lock-face help-key-binding face help-key-binding))

which is the global binding, not the repeat-map binding. Any ideas?

Robert
-- 

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Stefan Kangas]

Robert Pluim <rpluim@gmail.com> writes:

> BTW, I tried using 'substitute-command-keys' to do this, since itʼs
> more elegant, but it gave an unusual result for undo:
>
>     (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
>     =>#("C-x u" 0 5
>       (font-lock-face help-key-binding face help-key-binding))

That looks like a bug to me.

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Robert Pluim]

>>>>> On Mon, 25 Oct 2021 11:21:50 +0200, Stefan Kangas <stefan@marxist.se> said:

    Stefan> Robert Pluim <rpluim@gmail.com> writes:
    >> BTW, I tried using 'substitute-command-keys' to do this, since itʼs
    >> more elegant, but it gave an unusual result for undo:
    >> 
    >> (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
    >> =>#("C-x u" 0 5
    >> (font-lock-face help-key-binding face help-key-binding))

    Stefan> That looks like a bug to me.

Reported as bug#51384

Robert
-- 

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Andreas Schwab]

On Okt 25 2021, Stefan Kangas wrote:

> Robert Pluim <rpluim@gmail.com> writes:
>
>> BTW, I tried using 'substitute-command-keys' to do this, since itʼs
>> more elegant, but it gave an unusual result for undo:
>>
>>     (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
>>     =>#("C-x u" 0 5
>>       (font-lock-face help-key-binding face help-key-binding))
>
> That looks like a bug to me.

This is due to :advertised-binding.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Juri Linkov]

>>> BTW, I tried using 'substitute-command-keys' to do this, since itʼs
>>> more elegant, but it gave an unusual result for undo:
>>>
>>>     (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
>>>     =>#("C-x u" 0 5
>>>       (font-lock-face help-key-binding face help-key-binding))
>>
>> That looks like a bug to me.
>
> This is due to :advertised-binding.

Maybe 'where-is-internal' should check if the map contains
the advertised binding, only then return it?

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Juri Linkov]

>> But this means adding a new command with a name e.g.
>> 'other-window-backward' that will cycle windows in the opposite direction.
>
> As I said: itʼs up to you.

I think this command could be added only when someone will request it as
a standalone command useful on its own, without connection to repeat-mode.

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Stefan Monnier]

Juri Linkov [2021-10-25 20:54:03] wrote:
>>>> BTW, I tried using 'substitute-command-keys' to do this, since itʼs
>>>> more elegant, but it gave an unusual result for undo:
>>>>
>>>>     (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
>>>>     =>#("C-x u" 0 5
>>>>       (font-lock-face help-key-binding face help-key-binding))
>>>
>>> That looks like a bug to me.
>>
>> This is due to :advertised-binding.
>
> Maybe 'where-is-internal' should check if the map contains
> the advertised binding, only then return it?

The \\<...> doesn't override all other maps, so the `undo` command is
still found to be bound to `C-x u` in the global map.

But I agree that maybe `where-is-internal` could be told here to give
precedence to bindings found in the \\<...> map.


        Stefan

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Robert Pluim]

>>>>> On Mon, 25 Oct 2021 14:08:42 -0400, Stefan Monnier <monnier@iro.umontreal.ca> said:

    Stefan> Juri Linkov [2021-10-25 20:54:03] wrote:
    >>>>> BTW, I tried using 'substitute-command-keys' to do this, since itʼs
    >>>>> more elegant, but it gave an unusual result for undo:
    >>>>> 
    >>>>> (substitute-command-keys "\\<undo-repeat-map>\\[undo]")
    >>>>> =>#("C-x u" 0 5
    >>>>> (font-lock-face help-key-binding face help-key-binding))
    >>>> 
    >>>> That looks like a bug to me.
    >>> 
    >>> This is due to :advertised-binding.
    >> 
    >> Maybe 'where-is-internal' should check if the map contains
    >> the advertised binding, only then return it?

    Stefan> The \\<...> doesn't override all other maps, so the `undo` command is
    Stefan> still found to be bound to `C-x u` in the global map.

Right. I think it should look *only* in the specified map, otherwise
what's the point of specifying it?

    Stefan> But I agree that maybe `where-is-internal` could be told here to give
    Stefan> precedence to bindings found in the \\<...> map.

`where-is-internal' is not the issue. If you pass it (list keymap) it
will look only in 'keymap'. But substitute-command-keys passes it
'keymap', which allows it to look in the global map as well.

Perhaps something like this?

diff --git a/lisp/help.el b/lisp/help.el
index 7e2e492a36..38ecde7f8f 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1124,7 +1124,9 @@ substitute-command-keys
                 (delete-char 2)
                 (let* ((fun (intern (buffer-substring (point) (1- end-point))))
                        (key (with-current-buffer orig-buf
-                              (where-is-internal fun keymap t))))
+                              (or
+                               (where-is-internal fun (list keymap) t)
+                               (where-is-internal fun (list global-map) t)))))
                   ;; If this a command remap, we need to follow it.
                   (when (and (vectorp key)
                              (> (length key) 1)
@@ -1132,7 +1134,9 @@ substitute-command-keys
                              (symbolp (aref key 1)))
                     (setq fun (aref key 1))
                     (setq key (with-current-buffer orig-buf
-                                (where-is-internal fun keymap t))))
+                                (or
+                                 (where-is-internal fun (list keymap) t)
+                                 (where-is-internal fun (list global-map) t)))))
                   (if (not key)
                       ;; Function is not on any key.
                       (let ((op (point)))


Otherwise, extending 'substitute-command-keys' to allow '\(map)' to
mean "look only in map" is fairly trivial as well, but I donʼt see why
weʼd want to add more syntax when we could fix the existing one.

Robert
-- 

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Stefan Monnier]

>     Stefan> The \\<...> doesn't override all other maps, so the `undo` command is
>     Stefan> still found to be bound to `C-x u` in the global map.
> Right. I think it should look *only* in the specified map, otherwise
> what's the point of specifying it?

The common case is to specify the keymap that will likely be active when
the command is used.

Maybe you're right that we shouldn't look elsewhere, but I'd be
surprised if there aren't docstrings that rely on the current behavior.

>     Stefan> But I agree that maybe `where-is-internal` could be told
>     Stefan> here to give precedence to bindings found in the
>     Stefan> \\<...> map.
>
> `where-is-internal' is not the issue. If you pass it (list keymap) it
> will look only in 'keymap'. But substitute-command-keys passes it
> 'keymap', which allows it to look in the global map as well.
>
> Perhaps something like this?

Sounds about right, tho it disregards other keymaps than `keymap` and
`global-map`.  Maybe we should do

    (or (where-is-internal fun (list keymap) t)
        (where-is-internal fun nil t))

instead to avoid this problem.


        Stefan

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Robert Pluim]

>>>>> On Mon, 25 Oct 2021 16:57:19 -0400, Stefan Monnier <monnier@iro.umontreal.ca> said:

    Stefan> The \\<...> doesn't override all other maps, so the `undo` command is
    Stefan> still found to be bound to `C-x u` in the global map.
    >> Right. I think it should look *only* in the specified map, otherwise
    >> what's the point of specifying it?

    Stefan> The common case is to specify the keymap that will likely be active when
    Stefan> the command is used.

OK, so I guess that pleads for a fallback to the current behaviour.

    Stefan> Maybe you're right that we shouldn't look elsewhere, but I'd be
    Stefan> surprised if there aren't docstrings that rely on the current behavior.

That may be true, and I canʼt think of an easy way of finding them.

    Stefan> But I agree that maybe `where-is-internal` could be told
    Stefan> here to give precedence to bindings found in the
    Stefan> \\<...> map.
    >> 
    >> `where-is-internal' is not the issue. If you pass it (list keymap) it
    >> will look only in 'keymap'. But substitute-command-keys passes it
    >> 'keymap', which allows it to look in the global map as well.
    >> 
    >> Perhaps something like this?

    Stefan> Sounds about right, tho it disregards other keymaps than `keymap` and
    Stefan> `global-map`.  Maybe we should do

    Stefan>     (or (where-is-internal fun (list keymap) t)
    Stefan>         (where-is-internal fun nil t))

    Stefan> instead to avoid this problem.

I think the minimal change from the current behaviour would be this,
since 'keymap' starts out as 'overriding-local-map', but then can get
set to 'nil' or a specific keymap based on the contents of the string.

Or we could add an optional argument to 'substitute-command-keys' to
mean 'only look in MAPVAR'.

diff --git a/lisp/help.el b/lisp/help.el
index 9666ef9805..55e58e20e5 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1124,7 +1124,9 @@ substitute-command-keys
                 (delete-char 2)
                 (let* ((fun (intern (buffer-substring (point) (1- end-point))))
                        (key (with-current-buffer orig-buf
-                              (where-is-internal fun keymap t))))
+                              (or
+                               (where-is-internal fun (list keymap) t)
+                               (where-is-internal fun keymap t)))))
                   ;; If this a command remap, we need to follow it.
                   (when (and (vectorp key)
                              (> (length key) 1)
@@ -1132,7 +1134,9 @@ substitute-command-keys
                              (symbolp (aref key 1)))
                     (setq fun (aref key 1))
                     (setq key (with-current-buffer orig-buf
-                                (where-is-internal fun keymap t))))
+                                (or
+                                 (where-is-internal fun (list keymap) t)
+                                 (where-is-internal fun keymap t)))))
                   (if (not key)
                       ;; Function is not on any key.
                       (let ((op (point)))

Robert
-- 

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Robert Pluim]

>>>>> On Tue, 26 Oct 2021 12:06:28 +0200, Robert Pluim <rpluim@gmail.com> said:

    Robert> I think the minimal change from the current behaviour would be this,
    Robert> since 'keymap' starts out as 'overriding-local-map', but then can get
    Robert> set to 'nil' or a specific keymap based on the contents of the string.

Actually, no. Thatʼs a good way to break your Emacs. Back to the
drawing board.

Robert
-- 

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Stefan Monnier]

> -                              (where-is-internal fun keymap t))))
> +                              (or
> +                               (where-is-internal fun (list keymap) t)
> +                               (where-is-internal fun keymap t)))))

I think this is right, except you need to check `(if keymap` before
using `(list keymap)`.


        Stefan "based on your subsequent statement that it breaks Emacs ;-)"

Re: bug#49265: [External] : bug#49265: 28.0.50; repeat mode feature request

[Robert Pluim]

>>>>> On Tue, 26 Oct 2021 08:39:48 -0400, Stefan Monnier <monnier@iro.umontreal.ca> said:

    >> -                              (where-is-internal fun keymap t))))
    >> +                              (or
    >> +                               (where-is-internal fun (list keymap) t)
    >> +                               (where-is-internal fun keymap t)))))

    Stefan> I think this is right, except you need to check `(if keymap` before
    Stefan> using `(list keymap)`.

Yes

    Stefan>         Stefan "based on your subsequent statement that it breaks Emacs ;-)"

Well, only if you byte-compile stuff. Interactively it works fine :-)

Robert
--