bug#51173: 28.0.60; gnus-article-describe-key doesn't work

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Katsumi Yamaoka]

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

Hi,

The following example program returns `sh-case' on Emacs 27 and
olders, however to make it work on Emacs 28 and 29 the third
line has to be uncommented.  So does `describe-key' case.

(with-temp-buffer
  (sh-mode)
  ;;(set-window-buffer nil (current-buffer))
  (describe-key-briefly "\C-c\C-c"))

I don't know when/why those commands were changed to require the
buffer (where the keymap is) to be visited in the selected window,
but now `gnus-article-describe-key\(-briefly\)?' doesn't work
because of this.  A patch to gnus-art.el is attached, though the
one that should be fixed might be help.el.

Thanks.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 2826 bytes --]

--- gnus-art.el~	2021-10-06 01:11:50.777999500 +0000
+++ gnus-art.el	2021-10-13 01:04:18.073219200 +0000
@@ -6854,19 +6854,24 @@
 		       (read-key-sequence "Describe key: ")))
 	       gnus-article-mode)
   (gnus-article-check-buffer)
-  (if (memq (key-binding key t) '(gnus-article-read-summary-keys
-				  gnus-article-read-summary-send-keys))
-      (with-current-buffer gnus-article-current-summary
-	(setq unread-command-events
-	      (nconc
-	       (mapcar (lambda (x) (if (and (integerp x) (>= x 128))
-				       (list 'meta (- x 128))
-				     x))
-		       key)
-	       unread-command-events))
-	(let ((cursor-in-echo-area t)
-	      gnus-pick-mode)
-	  (describe-key (read-key-sequence nil t))))
+  (if (and (memq (key-binding key t) '(gnus-article-read-summary-keys
+				       gnus-article-read-summary-send-keys))
+	   (buffer-live-p gnus-article-current-summary))
+      (let ((artbuf (current-buffer)))
+	(unwind-protect
+	    (progn
+	      (set-window-buffer nil gnus-article-current-summary)
+	      (setq unread-command-events
+		    (nconc
+		     (mapcar (lambda (x) (if (and (integerp x) (>= x 128))
+					     (list 'meta (- x 128))
+					   x))
+			     key)
+		     unread-command-events))
+	      (let ((cursor-in-echo-area t)
+		    gnus-pick-mode)
+		(describe-key (read-key-sequence nil t))))
+	  (set-window-buffer nil artbuf)))
     (describe-key key)))
 
 (defun gnus-article-describe-key-briefly (key &optional insert)
@@ -6877,19 +6882,24 @@
 		     current-prefix-arg)
 	       gnus-article-mode)
   (gnus-article-check-buffer)
-  (if (memq (key-binding key t) '(gnus-article-read-summary-keys
-				  gnus-article-read-summary-send-keys))
-      (with-current-buffer gnus-article-current-summary
-	(setq unread-command-events
-	      (nconc
-	       (mapcar (lambda (x) (if (and (integerp x) (>= x 128))
-				       (list 'meta (- x 128))
-				     x))
-		       key)
-	       unread-command-events))
-	(let ((cursor-in-echo-area t)
-	      gnus-pick-mode)
-	  (describe-key-briefly (read-key-sequence nil t) insert)))
+  (if (and (memq (key-binding key t) '(gnus-article-read-summary-keys
+				       gnus-article-read-summary-send-keys))
+	   (buffer-live-p gnus-article-current-summary))
+      (let ((artbuf (current-buffer)))
+	(unwind-protect
+	    (progn
+	      (set-window-buffer nil gnus-article-current-summary)
+	      (setq unread-command-events
+		    (nconc
+		     (mapcar (lambda (x) (if (and (integerp x) (>= x 128))
+					     (list 'meta (- x 128))
+					   x))
+			     key)
+		     unread-command-events))
+	      (let ((cursor-in-echo-area t)
+		    gnus-pick-mode)
+		(describe-key-briefly (read-key-sequence nil t) insert)))
+	  (set-window-buffer nil artbuf)))
     (describe-key-briefly key insert)))
 
 ;;`gnus-agent-mode' in gnus-agent.el will define it.

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Lars Ingebrigtsen]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

> The following example program returns `sh-case' on Emacs 27 and
> olders, however to make it work on Emacs 28 and 29 the third
> line has to be uncommented.  So does `describe-key' case.
>
> (with-temp-buffer
>   (sh-mode)
>   ;;(set-window-buffer nil (current-buffer))
>   (describe-key-briefly "\C-c\C-c"))
>
> I don't know when/why those commands were changed to require the
> buffer (where the keymap is) to be visited in the selected window,
> but now `gnus-article-describe-key\(-briefly\)?' doesn't work
> because of this.  A patch to gnus-art.el is attached, though the
> one that should be fixed might be help.el.

I think this sounds like a bug in describe-key*, so perhaps it should be
fixed there?  Looking at the history, I'm not at all sure what caused
this regression, but perhaps it's:

commit 9d4af3e6bdfac374f6c9591566c010e6a1514751
Author:     Stefan Monnier <monnier@iro.umontreal.ca>
AuthorDate: Tue Jan 30 11:57:40 2018 -0500

I've added Stefan to the CCs.

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

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

>> The following example program returns `sh-case' on Emacs 27 and
>> olders, however to make it work on Emacs 28 and 29 the third
>> line has to be uncommented.  So does `describe-key' case.
>>
>> (with-temp-buffer
>>   (sh-mode)
>>   ;;(set-window-buffer nil (current-buffer))
>>   (describe-key-briefly "\C-c\C-c"))
>>
>> I don't know when/why those commands were changed to require the
>> buffer (where the keymap is) to be visited in the selected window,
>> but now `gnus-article-describe-key\(-briefly\)?' doesn't work
>> because of this.  A patch to gnus-art.el is attached, though the
>> one that should be fixed might be help.el.
>
> I think this sounds like a bug in describe-key*, so perhaps it should be
> fixed there?  Looking at the history, I'm not at all sure what caused
> this regression, but perhaps it's:
>
> commit 9d4af3e6bdfac374f6c9591566c010e6a1514751
> Author:     Stefan Monnier <monnier@iro.umontreal.ca>
> AuthorDate: Tue Jan 30 11:57:40 2018 -0500
>
> I've added Stefan to the CCs.

I think this is mea culpa - commit 2d1564103e.
It was changed to handle context menu clicks
in the displayed window.

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

> I think this is mea culpa - commit 2d1564103e.
> It was changed to handle context menu clicks
> in the displayed window.

This patch should handle only mouse events specially:

diff --git a/lisp/help.el b/lisp/help.el
index fa4eaee417..956a3d0d32 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -699,7 +699,10 @@ help--analyze-key
          ;; is selected from the context menu that should describe KEY
          ;; at the position of mouse click that opened the context menu.
          ;; When no mouse was involved, it defaults to window-point.
-	 (defn (save-excursion (mouse-set-point event) (key-binding key t))))
+         (defn (if (consp event)
+                   (save-excursion
+                     (mouse-set-point event) (key-binding key t))
+                 (key-binding key t))))
     ;; Handle the case where we faked an entry in "Select and Paste" menu.
     (when (and (eq defn nil)
 	       (stringp (aref key (1- (length key))))
-- 

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> This patch should handle only mouse events specially:

The patch makes both the with-temp-buffer test case as well as `C-h k'
work in Gnus article buffers, so it seems to fix the problem.

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

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

close 51173 28.0.60
quit

>> This patch should handle only mouse events specially:
>
> The patch makes both the with-temp-buffer test case as well as `C-h k'
> work in Gnus article buffers, so it seems to fix the problem.

So now pushed to emacs-28.

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I think this is mea culpa - commit 2d1564103e.
>> It was changed to handle context menu clicks
>> in the displayed window.
>
> This patch should handle only mouse events specially:
>
> diff --git a/lisp/help.el b/lisp/help.el
> index fa4eaee417..956a3d0d32 100644
> --- a/lisp/help.el
> +++ b/lisp/help.el
> @@ -699,7 +699,10 @@ help--analyze-key
>           ;; is selected from the context menu that should describe KEY
>           ;; at the position of mouse click that opened the context menu.
>           ;; When no mouse was involved, it defaults to window-point.
> -	 (defn (save-excursion (mouse-set-point event) (key-binding key t))))
> +         (defn (if (consp event)
> +                   (save-excursion
> +                     (mouse-set-point event) (key-binding key t))
> +                 (key-binding key t))))
>      ;; Handle the case where we faked an entry in "Select and Paste" menu.
>      (when (and (eq defn nil)
>  	       (stringp (aref key (1- (length key))))

But this will still use "the wrong buffer" for mouse clicks, no?

BTW, maybe a cleaner fix would be as follows:
- Add a `buffer` argument to `describe-key(-briefly)`.
- Pass that argument from `gnus-article-describe-key`.
And to get the behavior that Juri just pushed that `buffer` argument
would default to (if (consp event) (window-buffer (posn-window
(event-start event))) (current-buffere)).


        Stefan

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

>> +         (defn (if (consp event)
>> +                   (save-excursion
>> +                     (mouse-set-point event) (key-binding key t))
>> +                 (key-binding key t))))
>>      ;; Handle the case where we faked an entry in "Select and Paste" menu.
>>      (when (and (eq defn nil)
>>  	       (stringp (aref key (1- (length key))))
>
> But this will still use "the wrong buffer" for mouse clicks, no?

It seems all mouse clicks expect the buffer where they were clicked
to be displayed in a window.  I can't imagine how a mouse click
could originate from a hidden buffer.

> BTW, maybe a cleaner fix would be as follows:
> - Add a `buffer` argument to `describe-key(-briefly)`.
> - Pass that argument from `gnus-article-describe-key`.
> And to get the behavior that Juri just pushed that `buffer` argument
> would default to (if (consp event) (window-buffer (posn-window
> (event-start event))) (current-buffere)).

I don't know.  All reported cases work now after Eli fixed
mouse-minibuffer-check not to raise an error when called from
mouse-set-point, so now it's safe to use it in help--analyze-key.

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> +         (defn (if (consp event)
>>> +                   (save-excursion
>>> +                     (mouse-set-point event) (key-binding key t))
>>> +                 (key-binding key t))))
>>>      ;; Handle the case where we faked an entry in "Select and Paste" menu.
>>>      (when (and (eq defn nil)
>>>  	       (stringp (aref key (1- (length key))))
>>
>> But this will still use "the wrong buffer" for mouse clicks, no?
>
> It seems all mouse clicks expect the buffer where they were clicked
> to be displayed in a window.  I can't imagine how a mouse click
> could originate from a hidden buffer.

`gnus-article-describe-key` is used for those keybindings which Gnus
redirects from the buffer in which they occurred to some other buffer.
So if you use such a redirection for mouse-clicks,
`gnus-article-describe-key` would also want to look them up in the
other buffer.


        Stefan

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

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

> But this will still use "the wrong buffer" for mouse clicks, no?
>
> BTW, maybe a cleaner fix would be as follows:
> - Add a `buffer` argument to `describe-key(-briefly)`.
> - Pass that argument from `gnus-article-describe-key`.
> And to get the behavior that Juri just pushed that `buffer` argument
> would default to (if (consp event) (window-buffer (posn-window
> (event-start event))) (current-buffere)).

Maybe something like this (but currently I have no idea how to test all cases):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gnus-article-describe-key.patch --]
[-- Type: text/x-diff, Size: 3969 bytes --]

diff --git a/lisp/help.el b/lisp/help.el
index 9666ef9805..08a293c3dc 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -731,7 +731,7 @@ help--filter-info-list
    ;; If nothing left, then keep one (the last one).
    (last info-list)))
 
-(defun describe-key-briefly (&optional key-list insert untranslated)
+(defun describe-key-briefly (&optional key-list insert buffer)
   "Print the name of the functions KEY-LIST invokes.
 KEY-LIST is a list of pairs (SEQ . RAW-SEQ) of key sequences, where
 RAW-SEQ is the untranslated form of the key sequence SEQ.
@@ -739,8 +739,10 @@ describe-key-briefly
 
 While reading KEY-LIST interactively, this command temporarily enables
 menu items or tool-bar buttons that are disabled to allow getting help
-on them."
-  (declare (advertised-calling-convention (key-list &optional insert) "27.1"))
+on them.
+
+BUFFER is the buffer in which to lookup those keys; it defaults to the
+current buffer."
   (interactive
    ;; Ignore mouse movement events because it's too easy to miss the
    ;; message while moving the mouse.
@@ -748,15 +750,13 @@ describe-key-briefly
      `(,key-list ,current-prefix-arg)))
   (when (arrayp key-list)
     ;; Old calling convention, changed
-    (setq key-list (list (cons key-list
-                               (if (numberp untranslated)
-                                   (this-single-command-raw-keys)
-                                 untranslated)))))
-  (let* ((info-list (mapcar (lambda (kr)
-                              (help--analyze-key (car kr) (cdr kr)))
-                            key-list))
-         (msg (mapconcat #'car (help--filter-info-list info-list 1) "\n")))
-    (if insert (insert msg) (message "%s" msg))))
+    (setq key-list (list (cons key-list nil))))
+  (with-current-buffer (or buffer (current-buffer))
+    (let* ((info-list (mapcar (lambda (kr)
+                                (help--analyze-key (car kr) (cdr kr)))
+                              key-list))
+           (msg (mapconcat #'car (help--filter-info-list info-list 1) "\n")))
+      (if insert (insert msg) (message "%s" msg)))))
 
 (defun help--key-binding-keymap (key &optional accept-default no-remap position)
   "Return a keymap holding a binding for KEY within current keymaps.
diff --git a/lisp/gnus/gnus-art.el b/lisp/gnus/gnus-art.el
index bb466b9400..bbb452279a 100644
--- a/lisp/gnus/gnus-art.el
+++ b/lisp/gnus/gnus-art.el
@@ -6865,8 +6865,14 @@ gnus-article-describe-key
 	       unread-command-events))
 	(let ((cursor-in-echo-area t)
 	      gnus-pick-mode)
-	  (describe-key (read-key-sequence nil t))))
-    (describe-key key)))
+	  (let* ((key (read-key-sequence nil t))
+                 (buffer (if (consp key)
+                             (window-buffer (posn-window (event-start key)))
+                           (current-buffer))))
+            (describe-key key buffer))))
+    (describe-key key (if (consp key)
+                          (window-buffer (posn-window (event-start key)))
+                        (current-buffer)))))
 
 (defun gnus-article-describe-key-briefly (key &optional insert)
   "Display documentation of the function invoked by KEY.
@@ -6888,8 +6894,15 @@ gnus-article-describe-key-briefly
 	       unread-command-events))
 	(let ((cursor-in-echo-area t)
 	      gnus-pick-mode)
-	  (describe-key-briefly (read-key-sequence nil t) insert)))
-    (describe-key-briefly key insert)))
+	  (let ((key (read-key-sequence nil t))
+                (buffer (if (consp key)
+                            (window-buffer (posn-window (event-start key)))
+                          (current-buffer))))
+            (describe-key-briefly key insert buffer))))
+    (describe-key-briefly key insert
+                          (if (consp key)
+                              (window-buffer (posn-window (event-start key)))
+                            (current-buffer)))))
 
 ;;`gnus-agent-mode' in gnus-agent.el will define it.
 (defvar gnus-agent-summary-mode)

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> +	  (let* ((key (read-key-sequence nil t))
> +                 (buffer (if (consp key)
> +                             (window-buffer (posn-window (event-start key)))
> +                           (current-buffer))))
> +            (describe-key key buffer))))
> +    (describe-key key (if (consp key)
> +                          (window-buffer (posn-window (event-start key)))
> +                        (current-buffer)))))

I think the first `describe-key` above should always use
`current-buffer` (that's the whole point of the function: to lookup the
keybinding in that other buffer).

And for the second, it's supposed to be a fallback that does whatever
`describe-key` does normally, so I don't see why we'd need/want this
(if ...) construction.

OTOH we should probably try and change the `key` arg to use the new
key-list format expected by `describe-key` (i.e. a list of (SEQ
. RAW-SEQ) pairs).

> +	  (let ((key (read-key-sequence nil t))
> +                (buffer (if (consp key)
> +                            (window-buffer (posn-window (event-start key)))
> +                          (current-buffer))))
> +            (describe-key-briefly key insert buffer))))
> +    (describe-key-briefly key insert
> +                          (if (consp key)
> +                              (window-buffer (posn-window (event-start key)))
> +                            (current-buffer)))))

Same here.


        Stefan

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

>> +	  (let* ((key (read-key-sequence nil t))
>> +                 (buffer (if (consp key)
>> +                             (window-buffer (posn-window (event-start key)))
>> +                           (current-buffer))))
>> +            (describe-key key buffer))))
>
> I think the first `describe-key` above should always use
> `current-buffer` (that's the whole point of the function: to lookup the
> keybinding in that other buffer).

There is still something missing: `gnus-article-describe-key`
already selects the required buffer with `with-current-buffer`.
Then why should it provide the same buffer as an argument
to `describe-key` to select it again in `describe-key`?

Maybe you intended to add a new argument `buffer` to pass it down
to `help--analyze-key` that could use it somehow in this condition:

         (defn (if (consp event)
                   (save-excursion (mouse-set-point event) (key-binding key t))
                 (key-binding key t)))

But I have no idea how.  Maybe not to set point (that also selects
another window) when the `buffer` arg is provided?  E.g.

         (defn (if (not buffer)
                   (save-excursion (mouse-set-point event) (key-binding key t))
                 (key-binding key t)))

IOW, the semantics of `buffer` is not clear here.

> OTOH we should probably try and change the `key` arg to use the new
> key-list format expected by `describe-key` (i.e. a list of (SEQ
> . RAW-SEQ) pairs).

`gnus-article-describe-key` just passes down the value
that `read-key-sequence` returns.

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> +	  (let* ((key (read-key-sequence nil t))
>>> +                 (buffer (if (consp key)
>>> +                             (window-buffer (posn-window (event-start key)))
>>> +                           (current-buffer))))
>>> +            (describe-key key buffer))))
>>
>> I think the first `describe-key` above should always use
>> `current-buffer` (that's the whole point of the function: to lookup the
>> keybinding in that other buffer).
>
> There is still something missing: `gnus-article-describe-key`
> already selects the required buffer with `with-current-buffer`.
> Then why should it provide the same buffer as an argument
> to `describe-key` to select it again in `describe-key`?

Because `describe-key` otherwise uses the buffer of the window
associated with the event (this is in done in `help--analyse-key` where
we currently use `mouse-set-point`).  So we need to pass an explicit
buffer to tell `describe-key` to ignore the event's window (and we need
to change this part of `describe-key/help--analyse-key` to obey such a
buffer argument).

> Maybe you intended to add a new argument `buffer` to pass it down
> to `help--analyze-key` that could use it somehow in this condition:
>
>          (defn (if (consp event)
>                    (save-excursion (mouse-set-point event) (key-binding key t))
>                  (key-binding key t)))

Yes.

> But I have no idea how.  Maybe not to set point (that also selects
> another window) when the `buffer` arg is provided?

Exactly.

>          (defn (if (not buffer)
>                    (save-excursion (mouse-set-point event) (key-binding key t))
>                  (key-binding key t)))
>
> IOW, the semantics of `buffer` is not clear here.

If nil it means "defaults to the (window-buffer (posn-window (event-end event)))"

>> OTOH we should probably try and change the `key` arg to use the new
>> key-list format expected by `describe-key` (i.e. a list of (SEQ
>> . RAW-SEQ) pairs).
>
> `gnus-article-describe-key` just passes down the value
> that `read-key-sequence` returns.

I know.  This is the old calling convention of `describe-key`; we
should move to the new one.


        Stefan

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

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

>> There is still something missing: `gnus-article-describe-key`
>> already selects the required buffer with `with-current-buffer`.
>> Then why should it provide the same buffer as an argument
>> to `describe-key` to select it again in `describe-key`?
>
> Because `describe-key` otherwise uses the buffer of the window
> associated with the event (this is in done in `help--analyse-key` where
> we currently use `mouse-set-point`).  So we need to pass an explicit
> buffer to tell `describe-key` to ignore the event's window (and we need
> to change this part of `describe-key/help--analyse-key` to obey such a
> buffer argument).
>
>>          (defn (if (not buffer)
>>                    (save-excursion (mouse-set-point event) (key-binding key t))
>>                  (key-binding key t)))
>>
>> IOW, the semantics of `buffer` is not clear here.
>
> If nil it means "defaults to the (window-buffer (posn-window (event-end event)))"
>
>>> OTOH we should probably try and change the `key` arg to use the new
>>> key-list format expected by `describe-key` (i.e. a list of (SEQ
>>> . RAW-SEQ) pairs).
>>
>> `gnus-article-describe-key` just passes down the value
>> that `read-key-sequence` returns.
>
> I know.  This is the old calling convention of `describe-key`; we
> should move to the new one.

Is seems everything is covered by this patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: help--analyze-key.patch --]
[-- Type: text/x-diff, Size: 5139 bytes --]

diff --git a/lisp/gnus/gnus-art.el b/lisp/gnus/gnus-art.el
index bb466b9400..930c739a73 100644
--- a/lisp/gnus/gnus-art.el
+++ b/lisp/gnus/gnus-art.el
@@ -6865,7 +6865,9 @@ gnus-article-describe-key
 	       unread-command-events))
 	(let ((cursor-in-echo-area t)
 	      gnus-pick-mode)
-	  (describe-key (read-key-sequence nil t))))
+	  (describe-key (cons (read-key-sequence nil t)
+			      (this-single-command-raw-keys))
+			(current-buffer))))
     (describe-key key)))
 
 (defun gnus-article-describe-key-briefly (key &optional insert)
@@ -6888,7 +6890,9 @@ gnus-article-describe-key-briefly
 	       unread-command-events))
 	(let ((cursor-in-echo-area t)
 	      gnus-pick-mode)
-	  (describe-key-briefly (read-key-sequence nil t) insert)))
+	  (describe-key-briefly (cons (read-key-sequence nil t)
+				      (this-single-command-raw-keys))
+				insert (current-buffer))))
     (describe-key-briefly key insert)))
 
 ;;`gnus-agent-mode' in gnus-agent.el will define it.
diff --git a/lisp/help.el b/lisp/help.el
index 9666ef9805..a7084d29ce 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -677,9 +677,11 @@ help-key-description
 (defun help--binding-undefined-p (defn)
   (or (null defn) (integerp defn) (equal defn 'undefined)))
 
-(defun help--analyze-key (key untranslated)
+(defun help--analyze-key (key untranslated &optional buffer)
   "Get information about KEY its corresponding UNTRANSLATED events.
-Returns a list of the form (BRIEF-DESC DEFN EVENT MOUSE-MSG)."
+Returns a list of the form (BRIEF-DESC DEFN EVENT MOUSE-MSG).
+When BUFFER is nil, it defaults to the
+`(window-buffer (posn-window (event-end event)))'."
   (if (numberp untranslated)
       (error "Missing `untranslated'!"))
   (let* ((event (when (> (length key) 0)
@@ -699,9 +701,8 @@ help--analyze-key
          ;; is selected from the context menu that should describe KEY
          ;; at the position of mouse click that opened the context menu.
          ;; When no mouse was involved, don't use `mouse-set-point'.
-         (defn (if (consp event)
-                   (save-excursion (mouse-set-point event) (key-binding key t))
-                 (key-binding key t))))
+         (defn (if buffer (key-binding key t)
+                 (save-excursion (mouse-set-point event) (key-binding key t)))))
     ;; Handle the case where we faked an entry in "Select and Paste" menu.
     (when (and (eq defn nil)
 	       (stringp (aref key (1- (length key))))
@@ -731,7 +732,7 @@ help--filter-info-list
    ;; If nothing left, then keep one (the last one).
    (last info-list)))
 
-(defun describe-key-briefly (&optional key-list insert untranslated)
+(defun describe-key-briefly (&optional key-list insert buffer)
   "Print the name of the functions KEY-LIST invokes.
 KEY-LIST is a list of pairs (SEQ . RAW-SEQ) of key sequences, where
 RAW-SEQ is the untranslated form of the key sequence SEQ.
@@ -739,8 +740,10 @@ describe-key-briefly
 
 While reading KEY-LIST interactively, this command temporarily enables
 menu items or tool-bar buttons that are disabled to allow getting help
-on them."
-  (declare (advertised-calling-convention (key-list &optional insert) "27.1"))
+on them.
+
+BUFFER is the buffer in which to lookup those keys; it defaults to the
+current buffer."
   (interactive
    ;; Ignore mouse movement events because it's too easy to miss the
    ;; message while moving the mouse.
@@ -748,15 +751,13 @@ describe-key-briefly
      `(,key-list ,current-prefix-arg)))
   (when (arrayp key-list)
     ;; Old calling convention, changed
-    (setq key-list (list (cons key-list
-                               (if (numberp untranslated)
-                                   (this-single-command-raw-keys)
-                                 untranslated)))))
-  (let* ((info-list (mapcar (lambda (kr)
-                              (help--analyze-key (car kr) (cdr kr)))
-                            key-list))
-         (msg (mapconcat #'car (help--filter-info-list info-list 1) "\n")))
-    (if insert (insert msg) (message "%s" msg))))
+    (setq key-list (list (cons key-list nil))))
+  (with-current-buffer (if (buffer-live-p buffer) buffer (current-buffer))
+    (let* ((info-list (mapcar (lambda (kr)
+                                (help--analyze-key (car kr) (cdr kr) buffer))
+                              key-list))
+           (msg (mapconcat #'car (help--filter-info-list info-list 1) "\n")))
+      (if insert (insert msg) (message "%s" msg)))))
 
 (defun help--key-binding-keymap (key &optional accept-default no-remap position)
   "Return a keymap holding a binding for KEY within current keymaps.
@@ -916,7 +917,7 @@ describe-key
              (mapcar (lambda (x)
                        (pcase-let* ((`(,seq . ,raw-seq) x)
                                     (`(,brief-desc ,defn ,event ,_mouse-msg)
-                                     (help--analyze-key seq raw-seq))
+                                     (help--analyze-key seq raw-seq buffer))
                                     (locus
                                      (help--binding-locus
                                       seq (event-start event))))

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

>>>> OTOH we should probably try and change the `key` arg to use the new
>>>> key-list format expected by `describe-key` (i.e. a list of (SEQ
>>>> . RAW-SEQ) pairs).
>>>
>>> `gnus-article-describe-key` just passes down the value
>>> that `read-key-sequence` returns.
>>
>> I know.  This is the old calling convention of `describe-key`; we
>> should move to the new one.
>
> Is seems everything is covered by this patch:

So this is pushed now.

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>>>> OTOH we should probably try and change the `key` arg to use the new
>>>>> key-list format expected by `describe-key` (i.e. a list of (SEQ
>>>>> . RAW-SEQ) pairs).
>>>>
>>>> `gnus-article-describe-key` just passes down the value
>>>> that `read-key-sequence` returns.
>>>
>>> I know.  This is the old calling convention of `describe-key`; we
>>> should move to the new one.
>>
>> Is seems everything is covered by this patch:
>
> So this is pushed now.

Thanks, and sorry I didn't get to it earlier.  FWIW, it does look good,


        Stefan

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

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

>>>>>> OTOH we should probably try and change the `key` arg to use the new
>>>>>> key-list format expected by `describe-key` (i.e. a list of (SEQ
>>>>>> . RAW-SEQ) pairs).
>>>>>
>>>>> `gnus-article-describe-key` just passes down the value
>>>>> that `read-key-sequence` returns.
>>>>
>>>> I know.  This is the old calling convention of `describe-key`; we
>>>> should move to the new one.
>>>
>>> Is seems everything is covered by this patch:
>>
>> So this is pushed now.
>
> Thanks, and sorry I didn't get to it earlier.  FWIW, it does look good,

Oh, another regression:

 C-s                 ;; isearch-forward
 C-h k               ;; isearch-describe-key

then typing any key to describe in isearch-mode
leaves isearch in a broken state: it displays the
search prompt, but no isearch indicator on the mode-line.
So something exits isearch.  Adding a breakpoint
in isearch-done reveals this backtrace:

  isearch-done()
  isearch-mouse-leave-buffer()
  mouse-minibuffer-check(19)
  mouse-set-point(19)
  help--analyze-key("\23" [19] nil)
  describe-key((("\23" . [19])))
  funcall-interactively(describe-key (("\23" . [19])))
  isearch-describe-key()
  funcall-interactively(isearch-describe-key)
  command-execute(isearch-describe-key)

I don't know if more functions using describe-key are broken,
but copying the same code from gnus-article-describe-key
that gives the buffer argument to describe-key fixes this.
I have no idea for a better way to avoid such problems.

More precautions are added to this patch:
when isearch-mouse-commands contains isearch-describe-key,
isearch-mouse-leave-buffer won't leave isearch-mode.

Also to avoid a broken state, isearch-update
should be used only when isearch-mode is active:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: isearch-describe-key.patch --]
[-- Type: text/x-diff, Size: 1265 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 0a041b7a1e..043d2c016e 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -525,15 +525,17 @@ isearch-describe-key
   "Display documentation of the function invoked by isearch key."
   (interactive)
   (let ((display-buffer-overriding-action isearch--display-help-action))
-    (call-interactively 'describe-key))
-  (isearch-update))
+    (describe-key (list (cons (read-key-sequence nil t)
+                              (this-single-command-raw-keys)))
+                  (current-buffer)))
+  (when isearch-mode (isearch-update)))
 
 (defun isearch-describe-mode ()
   "Display documentation of Isearch mode."
   (interactive)
   (let ((display-buffer-overriding-action isearch--display-help-action))
     (describe-function 'isearch-forward))
-  (isearch-update))
+  (when isearch-mode (isearch-update)))
 
 (defalias 'isearch-mode-help 'isearch-describe-mode)
 
@@ -1498,7 +1500,7 @@ isearch-done
 
   (and (not edit) isearch-recursive-edit (exit-recursive-edit)))
 
-(defvar isearch-mouse-commands '(mouse-minor-mode-menu)
+(defvar isearch-mouse-commands '(mouse-minor-mode-menu isearch-describe-key)
   "List of mouse commands that are allowed during Isearch.")
 
 (defun isearch-mouse-leave-buffer ()

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>   mouse-minibuffer-check(19)
>   mouse-set-point(19)
>   help--analyze-key("\23" [19] nil)

Hpw 'bout the patch below?

Or maybe the use of `mouse-minibuffer-check` in `mouse-set-point` should
be moved to its interactive spec?


        Stefan


diff --git a/lisp/help.el b/lisp/help.el
index 1917ef425d..e175421d02 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -715,7 +715,8 @@ help--analyze-key
                        (and (not (windowp (posn-window (event-start event))))
                             (not (framep (posn-window (event-start event))))))
                    (key-binding key t)
-                 (save-excursion (mouse-set-point event) (key-binding key t)))))
+                 (save-excursion (posn-set-point (event-end event))
+                                 (key-binding key t)))))
     ;; Handle the case where we faked an entry in "Select and Paste" menu.
     (when (and (eq defn nil)
 	       (stringp (aref key (1- (length key))))

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Eli Zaretskii]

> Cc: 51173@debbugs.gnu.org
> Date: Mon, 29 Nov 2021 15:57:51 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> >   mouse-minibuffer-check(19)
> >   mouse-set-point(19)
> >   help--analyze-key("\23" [19] nil)
> 
> Hpw 'bout the patch below?
> 
> Or maybe the use of `mouse-minibuffer-check` in `mouse-set-point` should
> be moved to its interactive spec?

If you want this fixed on the release branch, I'd prefer a localized
change in Gnus (with a more thorough change on master).

Thanks.

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

>>   mouse-minibuffer-check(19)
>>   mouse-set-point(19)
>>   help--analyze-key("\23" [19] nil)
>
> Hpw 'bout the patch below?

I confirm that the patch fixes this regression,
while not breaking other cases.

Maybe this patch also obsoletes this code in the same place:

                       ;; Clicks on the menu bar produce "event" that
                       ;; is just '(menu-bar)', for which
                       ;; `mouse-set-point' is not useful.
                       (and (not (windowp (posn-window (event-start event))))
                            (not (framep (posn-window (event-start event)))))

While testing with this code removed, clicks on the menu bar still work
without errors.

> Or maybe the use of `mouse-minibuffer-check` in `mouse-set-point` should
> be moved to its interactive spec?

I agree with Eli that more cardinal changes could be tried only in master.

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> >   mouse-minibuffer-check(19)
>> >   mouse-set-point(19)
>> >   help--analyze-key("\23" [19] nil)
>> 
>> Hpw 'bout the patch below?
>> 
>> Or maybe the use of `mouse-minibuffer-check` in `mouse-set-point` should
>> be moved to its interactive spec?
>
> If you want this fixed on the release branch, I'd prefer a localized
> change in Gnus (with a more thorough change on master).

Indeed my patch is not intended for `master` (nor is my suggestion to
move the `mouse-minibuffer-check` call to the interactive spec of
`mouse-set-point`).


        Stefan

bug#51173: 28.0.60; gnus-article-describe-key doesn't work

[Juri Linkov]

>> Hpw 'bout the patch below?
>
> I confirm that the patch fixes this regression,
> while not breaking other cases.
>
> Maybe this patch also obsoletes this code in the same place:
>
>                        ;; Clicks on the menu bar produce "event" that
>                        ;; is just '(menu-bar)', for which
>                        ;; `mouse-set-point' is not useful.
>                        (and (not (windowp (posn-window (event-start event))))
>                             (not (framep (posn-window (event-start event)))))
>
> While testing with this code removed, clicks on the menu bar still work
> without errors.

Now the fix was pushed to emacs-28.