bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Kevin Foley]

Allows users to control the `noconfirm' parameter of
`help-mode-revert-buffer'.

I've assigned copyright to FSF but this is my first patch on this list
so please let me know if I've missed any standards.

---
 lisp/help-mode.el | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 0dc6c9ffae..458b959aec 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -98,6 +98,13 @@ The format is (FUNCTION ARGS...).")
   "Hook run by `help-mode'."
   :type 'hook
   :group 'help)
+
+(defcustom help-mode-revert-buffer-noconfirm nil
+  "Indicates whether to prompt for confirmation when reverting a
+help buffer."
+  :type 'boolean
+  :group 'help
+  :version "28.1")
 \f
 ;; Button types used by help
 
@@ -757,7 +764,12 @@ Show all docs for that symbol as either a variable, function or face."
       (user-error "No symbol here"))))
 
 (defun help-mode-revert-buffer (_ignore-auto noconfirm)
-  (when (or noconfirm (yes-or-no-p "Revert help buffer? "))
+  "Revert help-mode buffer.  See
+`help-mode-revert-buffer-noconfirm' to control whether user is
+prompted for confirmation."
+  (when (or noconfirm
+            help-mode-revert-buffer-noconfirm
+            (yes-or-no-p "Revert help buffer? "))
     (let ((pos (point))
 	  (item help-xref-stack-item)
 	  ;; Pretend there is no current item to add to the history.
-- 
2.28.0

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Drew Adams]

> +(defcustom help-mode-revert-buffer-noconfirm nil
> +  "Indicates whether to prompt for confirmation when reverting a
> +help buffer."

A minor comment (and others may disagree with me).

I think the first line of the doc string should
say what a true value means, because the option
name does that.

Or at least the doc string should somehow make
clear which Boolean value means which behavior.

E.g.:

Non-nil means do not prompt for confirmation when reverting a help buffer.

If you just say "whether" there's no way to know
which value means what, except to guess from the
option name, in which case the doc string doesn't
add anything.

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Stefan Kangas]

Kevin Foley <kevin@kevinjfoley.me> writes:

> Allows users to control the `noconfirm' parameter of
> `help-mode-revert-buffer'.
>
> I've assigned copyright to FSF but this is my first patch on this list
> so please let me know if I've missed any standards.

Thanks, it looks useful.  I have added my comments below.

Your patch seems to be lacking ChangeLog entries as described in
etc/CONTRIBUTE.  You would help us if you could add such entries,
especially if you are planning to do more changes.

> +(defcustom help-mode-revert-buffer-noconfirm nil

This would change the default, right?  Any rationale for that?

Perhaps we could keep this new behavior optional for now?  We could
consider changing it later once we get some experience with it.

> +  "Indicates whether to prompt for confirmation when reverting a
> +help buffer."

The first sentence of a doc string should fit on one line.

Perhaps you could use something like:

    "If non-nil, prompt before reverting a help buffer."

>  (defun help-mode-revert-buffer (_ignore-auto noconfirm)
> -  (when (or noconfirm (yes-or-no-p "Revert help buffer? "))
> +  "Revert help-mode buffer.  See
> +`help-mode-revert-buffer-noconfirm' to control whether user is
> +prompted for confirmation."

The first line should be one sentence only, and the second sentence
start on the second line.

Other than those nits, the patch looks good to me.

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Kevin Foley]

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

Drew Adams <drew.adams@oracle.com> writes:

> I think the first line of the doc string should
> say what a true value means, because the option
> name does that.

> Or at least the doc string should somehow make
> clear which Boolean value means which behavior.

> E.g.:

> Non-nil means do not prompt for confirmation when reverting a help
> buffer.

I agree this is better and also matches the recommendation in
`(elisp)Documentation Tips', I've updated the patch to reflect this.


Stefan Kangas <stefan@marxist.se> writes:

> Your patch seems to be lacking ChangeLog entries as described in
> etc/CONTRIBUTE.  You would help us if you could add such entries,
> especially if you are planning to do more changes.

I've updated the commit message per the CONTRIBUTE file, let me know if
there are any changes needed.

As a side note, it may be helpful to mention this file in the
`(emacs)Sending Patches for GNU Emacs' info node.  That was the page I
was referencing for info before submitting this.

>> +(defcustom help-mode-revert-buffer-noconfirm nil
>
> This would change the default, right?  Any rationale for that?

The default should be the same.  I've updated the docstring which I
think makes this clearer.

>> +  "Indicates whether to prompt for confirmation when reverting a
>> +help buffer."
>
> The first sentence of a doc string should fit on one line.

Updated with Drew's suggestion.

>>  (defun help-mode-revert-buffer (_ignore-auto noconfirm)
>> -  (when (or noconfirm (yes-or-no-p "Revert help buffer? "))
>> +  "Revert help-mode buffer.  See
>> +`help-mode-revert-buffer-noconfirm' to control whether user is
>> +prompted for confirmation."
>
> The first line should be one sentence only, and the second sentence
> start on the second line.

Updated to match these conventions.

Thank you both for the feedback, let me know if there any other changes
that should be made.


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

From 42f226804ac20c2650ccfb063668e24e915b2cb2 Mon Sep 17 00:00:00 2001
From: "Kevin J. Foley" <kevin@kevinjfoley.me>
Date: Sat, 24 Oct 2020 16:14:31 -0400
Subject: [PATCH] Add option to disable confirmation prompt when reverting help
 buffer

    * lisp/help-mode.el (help-mode-revert-buffer-noconfirm):
    (help-mode-revert-buffer): Add variable and reference it in revert
    function.
---
 lisp/help-mode.el | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 0dc6c9ffae..729529e4db 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -98,6 +98,12 @@ The format is (FUNCTION ARGS...).")
   "Hook run by `help-mode'."
   :type 'hook
   :group 'help)
+
+(defcustom help-mode-revert-buffer-noconfirm nil
+  "Non-nil means do not prompt for confirmation when reverting a help buffer."
+  :type 'boolean
+  :group 'help
+  :version "28.1")
 \f
 ;; Button types used by help
 
@@ -757,7 +763,11 @@ Show all docs for that symbol as either a variable, function or face."
       (user-error "No symbol here"))))
 
 (defun help-mode-revert-buffer (_ignore-auto noconfirm)
-  (when (or noconfirm (yes-or-no-p "Revert help buffer? "))
+  "Revert help-mode buffer.
+See `help-mode-revert-buffer-noconfirm' to control confirmation prompt."
+  (when (or noconfirm
+            help-mode-revert-buffer-noconfirm
+            (yes-or-no-p "Revert help buffer? "))
     (let ((pos (point))
 	  (item help-xref-stack-item)
 	  ;; Pretend there is no current item to add to the history.
-- 
2.28.0

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Lars Ingebrigtsen]

Kevin Foley <kevin@kevinjfoley.me> writes:

> +(defcustom help-mode-revert-buffer-noconfirm nil

In general, I think "negative" variables should be avoided, because
doing the "not not" logic confuses people.  So I'd rather have
`help-mode-revert-buffer-confirm' (defaulting to t).

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

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Kevin Foley]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> +(defcustom help-mode-revert-buffer-noconfirm nil
>
> In general, I think "negative" variables should be avoided, because
> doing the "not not" logic confuses people.  So I'd rather have
> `help-mode-revert-buffer-confirm' (defaulting to t).

I was trying to match the noconfirm argument of the revert function but
I agree, `help-mode-revert-buffer-confirm' is less confusing.

I've attached an updated patch.

Kevin Foley


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

From 9d5c29fc084d13837a5e8fb27f3948953b923605 Mon Sep 17 00:00:00 2001
From: "Kevin J. Foley" <kevin@kevinjfoley.me>
Date: Sat, 24 Oct 2020 16:14:31 -0400
Subject: [PATCH] Add option to disable confirmation prompt when reverting help
 buffer

    * lisp/help-mode.el (help-mode-revert-buffer-confirm):
    (help-mode-revert-buffer): Add variable and reference it in revert
    function.
---
 lisp/help-mode.el | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lisp/help-mode.el b/lisp/help-mode.el
index 0dc6c9ffae..716924f312 100644
--- a/lisp/help-mode.el
+++ b/lisp/help-mode.el
@@ -98,6 +98,12 @@ The format is (FUNCTION ARGS...).")
   "Hook run by `help-mode'."
   :type 'hook
   :group 'help)
+
+(defcustom help-mode-revert-buffer-confirm t
+  "Non-nil means prompt for confirmation when reverting a help buffer."
+  :type 'boolean
+  :group 'help
+  :version "28.1")
 \f
 ;; Button types used by help
 
@@ -757,7 +763,11 @@ Show all docs for that symbol as either a variable, function or face."
       (user-error "No symbol here"))))
 
 (defun help-mode-revert-buffer (_ignore-auto noconfirm)
-  (when (or noconfirm (yes-or-no-p "Revert help buffer? "))
+  "Revert help-mode buffer.
+See `help-mode-revert-buffer-confirm' to control confirmation prompt."
+  (when (or noconfirm
+            (not help-mode-revert-buffer-confirm)
+            (yes-or-no-p "Revert help buffer? "))
     (let ((pos (point))
 	  (item help-xref-stack-item)
 	  ;; Pretend there is no current item to add to the history.
-- 
2.28.0

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Lars Ingebrigtsen]

Kevin Foley <kevin@kevinjfoley.me> writes:

> I was trying to match the noconfirm argument of the revert function but
> I agree, `help-mode-revert-buffer-confirm' is less confusing.
>
> I've attached an updated patch.

Thanks.

Looking at the code a bit more, I found it a bit odd that there'd be a
need for this since there are other mechanisms to control this in
general -- revert-without-query, in particular:

---
Optional second argument NOCONFIRM means don't ask for confirmation
at all.  (The variable `revert-without-query' offers another way to
revert buffers without querying for confirmation.)
---

But the problem is that revert-buffer calls help-mode-revert-buffer,
which doesn't heed that variable.

Now, the documentation for revert-without-query says that it's about
reverting files, so on one level that makes sense since the help buffers
don't visit files.

But...  I think it would be helpful if help-mode-revert-buffer (and
other similar functions) did heed that variable -- then there'd be one
simple way to control all this.  Or even perhaps have revert-buffer look
at that variable and do the right thing instead of punting to
revert-buffer--default.

That is, it would look at file names for buffers visiting files, and
buffer names for other buffers.

Any opinions?

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

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But...  I think it would be helpful if help-mode-revert-buffer (and
> other similar functions) did heed that variable -- then there'd be one
> simple way to control all this.

Agreed.  It would seem logical to just be able to say:

(add-to-list 'revert-without-query "\\`\\*Help\\*\\'")

> Or even perhaps have revert-buffer look at that variable and do the
> right thing instead of punting to revert-buffer--default.
>
> That is, it would look at file names for buffers visiting files, and
> buffer names for other buffers.

If we could get `revert-buffer' to handle this, it seems to me that it
would be even better.

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Kevin Foley]

Stefan Kangas <stefan@marxist.se> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> But...  I think it would be helpful if help-mode-revert-buffer (and
>> other similar functions) did heed that variable -- then there'd be one
>> simple way to control all this.
>
> Agreed.  It would seem logical to just be able to say:
>
> (add-to-list 'revert-without-query "\\`\\*Help\\*\\'")

I also agree with this.  I think this was how I originally tried to
solve this problem (it was a while ago) but it didn't seem supported.

>> Or even perhaps have revert-buffer look at that variable and do the
>> right thing instead of punting to revert-buffer--default.
>>
>> That is, it would look at file names for buffers visiting files, and
>> buffer names for other buffers.
>
> If we could get `revert-buffer' to handle this, it seems to me that it
> would be even better.

I could update `revert-buffer' to include the logic for IGNORE-AUTO and
NOCONFIRM that are used in `revert-buffer--default' with some additional
logic to handle buffers without associated files.

Should I leave this logic in `revert-buffer--default' as well or have it
only exist in `revert-buffer'?

I believe that would also require changing `help-mode' to use
`revert-buffer-function' and `revert-buffer' instead of binding
`help-mode-revert-buffer', right?

Kevin Foley

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Lars Ingebrigtsen]

Kevin Foley <kevin@kevinjfoley.me> writes:

> I could update `revert-buffer' to include the logic for IGNORE-AUTO and
> NOCONFIRM that are used in `revert-buffer--default' with some additional
> logic to handle buffers without associated files.

Let's see...  That would be this bit from --default, I guess:

            ((or noconfirm
                 (and (not (buffer-modified-p))
                      (catch 'found
                        (dolist (regexp revert-without-query)
                          (when (string-match regexp file-name)
                            (throw 'found t)))))
                 (yes-or-no-p (format "Revert buffer from file %s? "
                                      file-name)))

I am a bit worried about incompatibles by hoisting that code up to
`revert-buffer'...

I've looked at some of the different usages of `revert-buffer-function',
and the help-functions one seems to be an outlier -- very few of the
non-file ones ask at all (I haven't found one yet, but I've only looked
at a handful).  Which makes me wonder why the *Help* one does?  I've
always found that pretty odd.  What's the use case for prompting the
user when the user hits `g' here?

> I believe that would also require changing `help-mode' to use
> `revert-buffer-function' and `revert-buffer' instead of binding
> `help-mode-revert-buffer', right?

`help-mode' does bind `g' to `revert-buffer'?  So I think I don't quite
understand what you're asking.

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

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I've looked at some of the different usages of `revert-buffer-function',
> and the help-functions one seems to be an outlier -- very few of the
> non-file ones ask at all (I haven't found one yet, but I've only looked
> at a handful).  Which makes me wonder why the *Help* one does?  I've
> always found that pretty odd.  What's the use case for prompting the
> user when the user hits `g' here?

Thank you for looking into this more deeply.  I think not prompting
here could work, unless there is some important reason not to.

> I am a bit worried about incompatibles by hoisting that code up to
> `revert-buffer'...

Indeed, I missed to mention that in my previous email.  So it might
not be worth it if we can do the above instead.

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Kevin Foley]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I am a bit worried about incompatibles by hoisting that code up to
> `revert-buffer'...

Fair point, that probably isn't a good approach.

> `help-mode' does bind `g' to `revert-buffer'?  So I think I don't quite
> understand what you're asking.

Never mind, I was mistaken about which function was bound.

Stefan Kangas <stefan@marxist.se> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> I've looked at some of the different usages of `revert-buffer-function',
>> and the help-functions one seems to be an outlier -- very few of the
>> non-file ones ask at all (I haven't found one yet, but I've only looked
>> at a handful).  Which makes me wonder why the *Help* one does?  I've
>> always found that pretty odd.  What's the use case for prompting the
>> user when the user hits `g' here?
>
> Thank you for looking into this more deeply.  I think not prompting
> here could work, unless there is some important reason not to.

I agree, I've never found this useful which was the inspiration for the
original patch.

So would the solution be to remove the prompt all together?  Or would it
the approach used in the patch to provide an option to control it
specifically for *Help*?

Kevin Foley

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Lars Ingebrigtsen]

Kevin Foley <kevin@kevinjfoley.me> writes:

> So would the solution be to remove the prompt all together?  Or would it
> the approach used in the patch to provide an option to control it
> specifically for *Help*?

I looked at the VC log for the *Help* function to see whether there was
a particular reason this function required confirmation (when most other
non-file buffer reverting functions didn't), and I couldn't find
anything.  My guess is that since the revert-buffer functions have a
NOCONFIRM parameter, then it was just implemented that way without,
since that seems logical.  (And it would be, if the NOCONFIRM parameter
was more...  logical.)

So I've just removed the prompting in *Help* unconditionally on the
trunk now.

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

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> So I've just removed the prompting in *Help* unconditionally on the
> trunk now.

I like this change.  But I have now run into the scenario where I type
`g' to see the new value of a variable (where I expected a change) and
nothing seemed to happen.  The problem, of course, was that the value of
the variable did not change as I expected.

For me, this is fine, but I think this risks being confusing to
beginners.  And now I'm starting to think that perhaps this is why the
prompt was added in the first place.

So I wonder if it would perhaps make sense to show a message when the
buffer is reverted?  That way the user will get at least some feedback.

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> I like this change.  But I have now run into the scenario where I type
> `g' to see the new value of a variable (where I expected a change) and
> nothing seemed to happen.  The problem, of course, was that the value of
> the variable did not change as I expected.

I don't quite follow you -- the only change was to remove the user
query -- nothing else has changed.  A `g' should update the buffer with
the new value of the variable, as before.

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

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> I like this change.  But I have now run into the scenario where I type
>> `g' to see the new value of a variable (where I expected a change) and
>> nothing seemed to happen.  The problem, of course, was that the value of
>> the variable did not change as I expected.
>
> I don't quite follow you -- the only change was to remove the user
> query -- nothing else has changed.  A `g' should update the buffer with
> the new value of the variable, as before.

Absolutely, yes.  But: My dumb brain was just used to seeing the
confirmation and had to pause for half a second when I did not see it,
thinking something was wrong.  My idea was simply that we could perhaps
show a message saying that the buffer was reverted, to not give other
users reason for pause.  WDYT?

bug#44202: [PATCH] Add variable to control confirmation of help-mode-revert-buffer

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Absolutely, yes.  But: My dumb brain was just used to seeing the
> confirmation and had to pause for half a second when I did not see it,
> thinking something was wrong.  My idea was simply that we could perhaps
> show a message saying that the buffer was reverted, to not give other
> users reason for pause.  WDYT?

I'm not against showing a message, but I don't really see the need: The
user hit the `g' command, and having Emacs do what you ask surely
shouldn't come as a surprise.  :-)  

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