GNU bug report logs - #73637
[PATCH] New hook 'help-setup-hook'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 73637 in the body.
You can then email your comments to 73637 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] New hook 'help-setup-hook'
Date: Sat, 05 Oct 2024 09:14:54 +0200

[Message part 1 (text/plain, inline)]

Tags: patch

Add a new hook that allows commands that make use of bespoke variable
settings in the *Help* buffer to clean up after themselves.  This
generalizes and improves the accuracy of the hard-coded clean up that
help-setup-xref currently performs.

[0001-New-hook-help-setup-hook.patch (text/patch, attachment)]

Message #8 received at 73637 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 73637 <at> debbugs.gnu.org
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Sat, 05 Oct 2024 10:40:56 +0300

> Date: Sat, 05 Oct 2024 09:14:54 +0200
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>

Stefan, any comments?

> --- a/lisp/help-fns.el
> +++ b/lisp/help-fns.el
> @@ -280,7 +280,12 @@ xref-backend-references
>  
>  (defun help-fns--setup-xref-backend ()
>    (add-hook 'xref-backend-functions #'elisp--xref-backend nil t)
> -  (setq-local semantic-symref-filepattern-alist '((help-mode "*.el"))))
> +  (setq-local semantic-symref-filepattern-alist '((help-mode "*.el")))
> +  (add-hook 'help-setup-hook
> +            (lambda ()
> +              (kill-local-variable 'xref-backend-functions)
> +              (kill-local-variable 'semantic-symref-filepattern-alist))
> +            nil t))

Please add comments here to explain why the hook does these things.

> --- a/lisp/help-mode.el
> +++ b/lisp/help-mode.el
> @@ -492,6 +492,13 @@ help-xref-url-regexp
>    (purecopy "\\<[Uu][Rr][Ll][ \t\n]+['`‘]\\([^'’]+\\)['’]")
>    "Regexp matching doc string references to a URL.")
>  
> +(defvar-local help-setup-hook nil
> +  "Functions to call with no arguments when populating \"*Help*\" buffers.

Please mention in the doc string which function(s) call(s) this hook.

> -    ;; Disable `outline-minor-mode' in a reused Help buffer
> -    ;; created by `describe-bindings' that enables this mode.

This useful comment is lost as result of your patch.  Is that on
purpose?

Finally, I think this hook should be added to the list of the standard
hooks in the ELisp manual.

Thanks.

Message #11 received at 73637 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 73637 <at> debbugs.gnu.org
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Sat, 05 Oct 2024 10:46:06 +0200

[Message part 1 (text/plain, inline)]

Hi,

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Date: Sat, 05 Oct 2024 09:14:54 +0200
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>
> Stefan, any comments?
>
>> --- a/lisp/help-fns.el
>> +++ b/lisp/help-fns.el
>> @@ -280,7 +280,12 @@ xref-backend-references
>>  
>>  (defun help-fns--setup-xref-backend ()
>>    (add-hook 'xref-backend-functions #'elisp--xref-backend nil t)
>> -  (setq-local semantic-symref-filepattern-alist '((help-mode "*.el"))))
>> +  (setq-local semantic-symref-filepattern-alist '((help-mode "*.el")))
>> +  (add-hook 'help-setup-hook
>> +            (lambda ()
>> +              (kill-local-variable 'xref-backend-functions)
>> +              (kill-local-variable 'semantic-symref-filepattern-alist))
>> +            nil t))
>
> Please add comments here to explain why the hook does these things.

OK, see updated patch below.

>> --- a/lisp/help-mode.el
>> +++ b/lisp/help-mode.el
>> @@ -492,6 +492,13 @@ help-xref-url-regexp
>>    (purecopy "\\<[Uu][Rr][Ll][ \t\n]+['`‘]\\([^'’]+\\)['’]")
>>    "Regexp matching doc string references to a URL.")
>>  
>> +(defvar-local help-setup-hook nil
>> +  "Functions to call with no arguments when populating \"*Help*\" buffers.
>
> Please mention in the doc string which function(s) call(s) this hook.

Done.

>> -    ;; Disable `outline-minor-mode' in a reused Help buffer
>> -    ;; created by `describe-bindings' that enables this mode.
>
> This useful comment is lost as result of your patch.  Is that on
> purpose?

Yes, this change obviates the need for this comment, as the code no
longer caters for describe-bindings specifically, it just runs a hook.

> Finally, I think this hook should be added to the list of the standard
> hooks in the ELisp manual.

Done in the patch below.  Thanks for taking a look.

[v2-0001-New-hook-help-setup-hook-bug-73637.patch (text/x-patch, attachment)]

Message #14 received at 73637 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eshel Yaron <me <at> eshelyaron.com>, 73637 <at> debbugs.gnu.org
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Sat, 5 Oct 2024 15:19:29 +0300

Hi again!

On 05/10/2024 10:14, Eshel Yaron via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Add a new hook that allows commands that make use of bespoke variable
> settings in the*Help* buffer to clean up after themselves.  This
> generalizes and improves the accuracy of the hard-coded clean up that
> help-setup-xref currently performs.

Thanks for looking into it.

Have you tried going in the other direction? Meaning, killing all locals 
by default but keeping the ones that are really needed.

This is less backward compatible, but ultimately a more predictable and 
common design. This seems to work okay here:

diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 7bb69a9389f..8e37c138588 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -505,30 +505,19 @@ help-setup-xref
 because we want to record the \"previous\" position of point so we can
 restore it properly when going back."
   (with-current-buffer (help-buffer)
-    ;; Disable `outline-minor-mode' in a reused Help buffer
-    ;; created by `describe-bindings' that enables this mode.
-    (when (bound-and-true-p outline-minor-mode)
-      (outline-minor-mode -1)
-      (mapc #'kill-local-variable
-            '(outline-search-function
-              outline-regexp
-              outline-heading-end-regexp
-              outline-level
-              outline-minor-mode-cycle
-              outline-minor-mode-highlight
-              outline-minor-mode-use-buttons
-              outline-default-state
-              outline-default-rules)))
-    (kill-local-variable 'xref-backend-functions)
-    (kill-local-variable 'semantic-symref-filepattern-alist)
-    (when help-xref-stack-item
-      (push (cons (point) help-xref-stack-item) help-xref-stack)
-      (setq help-xref-forward-stack nil))
-    (when interactive-p
-      (let ((tail (nthcdr 10 help-xref-stack)))
-	;; Truncate the stack.
-	(if tail (setcdr tail nil))))
-    (setq help-xref-stack-item item)))
+    (let ((stack-item help-xref-stack-item)
+          (stack help-xref-stack))
+      (kill-all-local-variables)
+      (setq help-xref-stack-item stack-item
+            help-xref-stack stack)
+      (when help-xref-stack-item
+        (push (cons (point) help-xref-stack-item) help-xref-stack)
+        (setq help-xref-forward-stack nil))
+      (when interactive-p
+        (let ((tail (nthcdr 10 help-xref-stack)))
+	  ;; Truncate the stack.
+	  (if tail (setcdr tail nil))))
+      (setq help-xref-stack-item item))))

 (defvar help-xref-following nil
   "Non-nil when following a help cross-reference.")

Message #17 received at 73637 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 73637 <at> debbugs.gnu.org
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Sat, 05 Oct 2024 09:10:59 -0400

>> Stefan, any comments?

Looking at the patch from a distance it makes me wonder whether we
shouldn't call `kill-all-local-variables` instead?
IOW instead of listing all the vars that need to be killed, change to
code to preserve the few vars that should not be killed.


        Stefan

Message #20 received at 73637 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dmitry Gutov <dmitry <at> gutov.dev>, Eli Zaretskii <eliz <at> gnu.org>,
 73637 <at> debbugs.gnu.org
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Sat, 05 Oct 2024 15:32:56 +0200

Dmitry Gutov <dmitry <at> gutov.dev> writes:

> Have you tried going in the other direction? Meaning, killing all locals 
> by default but keeping the ones that are really needed.
>
> This is less backward compatible, but ultimately a more predictable and 
> common design. This seems to work okay here:

Stefan Monnier writes:

> Looking at the patch from a distance it makes me wonder whether we
> shouldn't call `kill-all-local-variables` instead?
> IOW instead of listing all the vars that need to be killed, change to
> code to preserve the few vars that should not be killed.

SGTM, that works too and it's definitely a simpler approach.

Message #25 received at 73637-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eshel Yaron <me <at> eshelyaron.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 73637-done <at> debbugs.gnu.org
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Sun, 6 Oct 2024 04:34:38 +0300

On 05/10/2024 16:32, Eshel Yaron wrote:
> Dmitry Gutov<dmitry <at> gutov.dev> writes:
> 
>> Have you tried going in the other direction? Meaning, killing all locals
>> by default but keeping the ones that are really needed.
>>
>> This is less backward compatible, but ultimately a more predictable and
>> common design. This seems to work okay here:
> Stefan Monnier writes:
> 
>> Looking at the patch from a distance it makes me wonder whether we
>> shouldn't call `kill-all-local-variables` instead?
>> IOW instead of listing all the vars that need to be killed, change to
>> code to preserve the few vars that should not be killed.
> SGTM, that works too and it's definitely a simpler approach.

Great! I've pushed that patch, please report any new regressions.

Message #28 received at 73637 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: 73637 <at> debbugs.gnu.org
Cc: dmitry <at> gutov.dev, me <at> eshelyaron.com
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Sun, 06 Oct 2024 09:56:32 +0300

> Great! I've pushed that patch, please report any new regressions.

Maybe 'help-xref-forward-stack' and 'help-xref-stack-forward-item'
should be preserved as well?

Message #31 received at 73637 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Juri Linkov <juri <at> linkov.net>, 73637 <at> debbugs.gnu.org
Cc: me <at> eshelyaron.com
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Sun, 6 Oct 2024 17:41:02 +0300

On 06/10/2024 09:56, Juri Linkov wrote:
>> Great! I've pushed that patch, please report any new regressions.
> Maybe 'help-xref-forward-stack' and 'help-xref-stack-forward-item'
> should be preserved as well?

Good question - looks like both of these vars are marked as 
'permanent-local' already. And the ones I set up to be restored are too.

This suggests a further simplification, pushed to master now:

diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index a44749d1fe9..4ee4f4156a1 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -505,19 +505,15 @@ help-setup-xref
 because we want to record the \"previous\" position of point so we can
 restore it properly when going back."
   (with-current-buffer (help-buffer)
-    (let ((stack-item help-xref-stack-item)
-          (stack help-xref-stack))
-      (kill-all-local-variables)
-      (setq help-xref-stack-item stack-item
-            help-xref-stack stack)
-      (when help-xref-stack-item
-        (push (cons (point) help-xref-stack-item) help-xref-stack)
-        (setq help-xref-forward-stack nil))
-      (when interactive-p
-        (let ((tail (nthcdr 10 help-xref-stack)))
-      ;; Truncate the stack.
-      (if tail (setcdr tail nil))))
-      (setq help-xref-stack-item item))))
+    (kill-all-local-variables)
+    (when help-xref-stack-item
+      (push (cons (point) help-xref-stack-item) help-xref-stack)
+      (setq help-xref-forward-stack nil))
+    (when interactive-p
+      (let ((tail (nthcdr 10 help-xref-stack)))
+        ;; Truncate the stack.
+        (if tail (setcdr tail nil))))
+    (setq help-xref-stack-item item)))

 (defvar help-xref-following nil
   "Non-nil when following a help cross-reference.")

Message #34 received at 73637 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: me <at> eshelyaron.com, 73637 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Mon, 07 Oct 2024 19:53:33 -0700

Hi people,

Dmitry Gutov <dmitry <at> gutov.dev> writes:

> On 06/10/2024 09:56, Juri Linkov wrote:
>>> Great! I've pushed that patch, please report any new regressions.
>> Maybe 'help-xref-forward-stack' and 'help-xref-stack-forward-item'
>> should be preserved as well?
>
> Good question - looks like both of these vars are marked as 
> 'permanent-local' already. And the ones I set up to be restored are too.
>
> This suggests a further simplification, pushed to master now:
>
>
>   (defvar help-xref-following nil
>     "Non-nil when following a help cross-reference.")

Just linking to a possibly related bug:

  bug#73686: 31.0.50; ERC 5.6.1-git:
  back button gone from describe-face via erc-nicks-list-face

Thanks,
J.P.

Message #37 received at 73637 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: me <at> eshelyaron.com, 73637 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73637: [PATCH] New hook 'help-setup-hook'
Date: Tue, 08 Oct 2024 14:16:57 -0700

"J.P." <jp <at> neverwas.me> writes:

> Just linking to a possibly related bug:
>
>   bug#73686: 31.0.50; ERC 5.6.1-git:
>   back button gone from describe-face via erc-nicks-list-face

Looking into this a bit, I'm now pretty convinced this change introduced
a regression involving non-default names for help buffers. Details here:

  https://lists.gnu.org/archive/html/bug-gnu-emacs/2024-10/msg00385.html

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.