Subject: bug#77118: 31.0.50; help-key-binding face in help buffer

From: Stephen Berman <[email-masked]>
Date: Fri, 11 Apr 2025 12:23:55 +0200

On Fri, 11 Apr 2025 11:57:39 +0300 Eli Zaretskii <[email-masked]> wrote:

>> From: Stephen Berman <[email-masked]>
>> Cc: Eli Zaretskii <[email-masked]>,  Arash Esbati <[email-masked]>,  Stefan
>>  Monnier <[email-masked]>,  [email-masked]
>> Date: Fri, 11 Apr 2025 10:19:35 +0200
>>
>> On Thu, 20 Mar 2025 08:57:52 +0100 Stephen Berman <[email-masked]> wrote:
>>
>> > On Wed, 19 Mar 2025 12:15:40 -0700 Stefan Kangas <[email-masked]> wrote:
>> >
>> >> Eli Zaretskii <[email-masked]> writes:
>> >>
>> >>>> From: Arash Esbati <[email-masked]>
>> >>>> Date: Wed, 19 Mar 2025 14:05:21 +0100
>> >>>>
>> >>>> when I eval the following code:
>> >>>>
>> >>>> --8<---------------cut here---------------start------------->8---
>> >>>> (let ((help-form (substitute-command-keys "\
>> >>>> Select with a key:
>> >>>> \\`h'   do this,
>> >>>> \\`k'   do that,
>> >>>> \\`RET' do nothing.")))
>> >>>>   (read-char-choice (substitute-command-keys
>> >>>>                      (format "\
>> >>>> This (\\`h'), That (\\`k'), Nothing (\\`RET'), Help (\\`%s'): "
>> >>>>                              (key-description (vector help-char))))
>> >>>>                     '(?h ?k ?\r)))
>> >>>> --8<---------------cut here---------------end--------------->8---
>> >>>>
>> >>>> and do 'C-h', h, k and RET don't receive the help-key-binding face in
>> >>>> the help buffer which pops up.  What I see with 'emacs -Q' is attached.
>> >>>> Note the difference in the minibuffer.
>> >>>>
>> >>>> The reason seems to be in the implementation of `help-form-show' using
>> >>>> `princ' which strips properties.  If this is intentional, it doesn't
>> >>>> match definitions of `help-form' in Emacs tree.  Take for example
>> >>>> dired-aux.el which pushes the string for `help-form' also through
>> >>>> `substitute-command-keys'[1,2,3].
>> >>>>
>> >>>> In case this report is confirmed, a possible solution is provided
>> >>>> here[4].
>> >>>
>> >>> Stefan, any comments or reasons why we use princ there?
>> >>
>> >> There is no deep reason.  We want to stay in the buffer that we invoked
>> >> help from, to get the keybindings from the right keymaps, but we also
>> >> want to insert the documentation in the *Help* buffer.  Setting
>> >> `standard-output` to the *Help* buffer and using `princ` is just a
>> >> convenient way to do that.
>> >>
>> >> The fix is to switch things around so we can use `insert` instead.
>> >> We have done that elsewhere in help, and should do the same in
>> >> `read-char-choice` also.
>> >
>> > Is the patch I posted to help-gnu-emacs -- linked in the OP of this bug
>> > thread and repeated below for convenience -- adequate?
>> >
>> > Steve Berman
>> >
>> > diff --git a/lisp/help.el b/lisp/help.el
>> > index 835e47fec43..d77894b3372 100644
>> > --- a/lisp/help.el
>> > +++ b/lisp/help.el
>> > @@ -2190,8 +2190,8 @@ help-form-show
>> >    "Display the output of a non-nil `help-form'."
>> >    (let ((msg (eval help-form t)))
>> >      (if (stringp msg)
>> > -       (with-output-to-temp-buffer " *Char Help*"
>> > -         (princ msg)))))
>> > +        (with-help-window " *Char Help*"
>> > +         (insert msg)))))
>> >
>> >  (defun help--append-keystrokes-help (str)
>> >    (let* ((keys (this-single-command-keys))
>>
>> This bug thread seems to have stalled; is my patch acceptable or is
>> someone working on the alternative suggested upthread by Stefan Kangas
>> (a version of princ that preserves string properties)?  If my patch is
>> acceptable, should it be installed in emacs-30 or master?
>
> TBH, this patch scares me, even if we only install it on master, let
> alone on the release branch.  with-help-window's implementation has
> nothing in common with that of with-output-to-temp-buffer, so who
> knows what unintended changes in behavior this deceptively-simple
> change will cause?  E.g., with-help-window obeys the value of
> help-window-select wrt selecting the window: do we want that here?
> And that's just the tip of what I fear is a very large iceberg.  All
> that just to support some obscure use case?
>
> Can't we come up with some change that has more predictable
> consequences?  Like some creative use of with-current-buffer around
> the call to 'insert'?  That'd have only local effect, which is well
> understood, and thus make this change easier to agree to and install
> on the release branch.

Is the patch below creative enough?

Steve Berman

diff --git a/lisp/help.el b/lisp/help.el
index b0c003ed16a..c7419b0d0ad 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -2190,8 +2190,11 @@ help-form-show
   "Display the output of a non-nil `help-form'."
   (let ((msg (eval help-form t)))
     (if (stringp msg)
-	(with-output-to-temp-buffer " *Char Help*"
-	  (princ msg)))))
+	(let ((bufname " *Char Help*"))
+          (with-output-to-temp-buffer bufname)
+          (with-current-buffer bufname
+            (let (buffer-read-only)
+              (insert msg)))))))

 (defun help--append-keystrokes-help (str)
   (let* ((keys (this-single-command-keys))