bug#30792: 26.0.91; improve docstring of with-help-window

bug#30792: 26.0.91; improve docstring of with-help-window

[Nick Helm]

I find the docstring of `with-help-window' a little hard to follow, so I
had a go at improving it. This also renames the var BUFFER-NAME to
BUFFER-OR-NAME to match `with-temp-buffer-window' and others.

--- a/lisp/help.el	2018-03-13 19:52:32.000000000 +1300
+++ b/lisp/help.el	2018-03-13 21:54:07.000000000 +1300
@@ -1370,15 +1370,20 @@
 
 ;; (4) A marker (`help-window-point-marker') to move point in the help
 ;;     window to an arbitrary buffer position.
-(defmacro with-help-window (buffer-name &rest body)
-  "Display buffer named BUFFER-NAME in a help window.
-Evaluate the forms in BODY with standard output bound to a buffer
-called BUFFER-NAME (creating it if it does not exist), put that
-buffer in `help-mode', display the buffer in a window (see
-`with-temp-buffer-window' for details) and issue a message how to
-deal with that \"help\" window when it's no more needed.  Select
-the help window if the current value of the user option
-`help-window-select' says so.  Return last value in BODY."
+(defmacro with-help-window (buffer-or-name &rest body)
+  "Show buffer BUFFER-OR-NAME with output of BODY in a help window.
+Make the buffer specified by BUFFER-OR-NAME empty (or create it
+if it does not exist).  Evaluate BODY with `standard-output' bound
+to that buffer, so that output from `prin1' and similar functions
+in BODY go into that buffer.  The buffer is not made current while
+BODY is evaluated.  Finally, display the buffer in a window and
+put it in `help-mode'.  Return the value returned by BODY.
+
+The help window will be selected if `help-window-select' is
+non-nil.  However, if the help window displays on a different
+frame, the window manager may automatically select that frame.
+
+See `with-temp-buffer-window' for more details."
   (declare (indent 1) (debug t))
   `(progn
      ;; Make `help-window-point-marker' point nowhere.  The only place

bug#30792: 26.0.91; improve docstring of with-help-window

[martin rudalics]

 > I find the docstring of `with-help-window' a little hard to follow, so I
 > had a go at improving it. This also renames the var BUFFER-NAME to
 > BUFFER-OR-NAME to match `with-temp-buffer-window' and others.

Thanks.  I can see only part of that renaming in your patch.  Am I
something missing?

martin


 > --- a/lisp/help.el	2018-03-13 19:52:32.000000000 +1300
 > +++ b/lisp/help.el	2018-03-13 21:54:07.000000000 +1300
 > @@ -1370,15 +1370,20 @@
 >
 >   ;; (4) A marker (`help-window-point-marker') to move point in the help
 >   ;;     window to an arbitrary buffer position.
 > -(defmacro with-help-window (buffer-name &rest body)
 > -  "Display buffer named BUFFER-NAME in a help window.
 > -Evaluate the forms in BODY with standard output bound to a buffer
 > -called BUFFER-NAME (creating it if it does not exist), put that
 > -buffer in `help-mode', display the buffer in a window (see
 > -`with-temp-buffer-window' for details) and issue a message how to
 > -deal with that \"help\" window when it's no more needed.  Select
 > -the help window if the current value of the user option
 > -`help-window-select' says so.  Return last value in BODY."
 > +(defmacro with-help-window (buffer-or-name &rest body)
 > +  "Show buffer BUFFER-OR-NAME with output of BODY in a help window.
 > +Make the buffer specified by BUFFER-OR-NAME empty (or create it
 > +if it does not exist).  Evaluate BODY with `standard-output' bound
 > +to that buffer, so that output from `prin1' and similar functions
 > +in BODY go into that buffer.  The buffer is not made current while
 > +BODY is evaluated.  Finally, display the buffer in a window and
 > +put it in `help-mode'.  Return the value returned by BODY.
 > +
 > +The help window will be selected if `help-window-select' is
 > +non-nil.  However, if the help window displays on a different
 > +frame, the window manager may automatically select that frame.
 > +
 > +See `with-temp-buffer-window' for more details."
 >     (declare (indent 1) (debug t))
 >     `(progn
 >        ;; Make `help-window-point-marker' point nowhere.  The only place

bug#30792: 26.0.91; improve docstring of with-help-window

[Nick Helm]

On Tue, 13 Mar 2018 at 23:17:17 +1300, martin rudalics wrote:

>> I find the docstring of `with-help-window' a little hard to follow, so I
>> had a go at improving it. This also renames the var BUFFER-NAME to
>> BUFFER-OR-NAME to match `with-temp-buffer-window' and others.
>
> Thanks.  I can see only part of that renaming in your patch.  Am I
> something missing?
>
> martin

Sorry, got a little ahead of myself. Try this.

--- a/lisp/help.el	2018-03-13 19:52:32.000000000 +1300
+++ b/lisp/help.el	2018-03-13 23:23:52.000000000 +1300
@@ -1370,15 +1370,20 @@
 
 ;; (4) A marker (`help-window-point-marker') to move point in the help
 ;;     window to an arbitrary buffer position.
-(defmacro with-help-window (buffer-name &rest body)
-  "Display buffer named BUFFER-NAME in a help window.
-Evaluate the forms in BODY with standard output bound to a buffer
-called BUFFER-NAME (creating it if it does not exist), put that
-buffer in `help-mode', display the buffer in a window (see
-`with-temp-buffer-window' for details) and issue a message how to
-deal with that \"help\" window when it's no more needed.  Select
-the help window if the current value of the user option
-`help-window-select' says so.  Return last value in BODY."
+(defmacro with-help-window (buffer-or-name &rest body)
+  "Show buffer BUFFER-OR-NAME with output of BODY in a help window.
+Make the buffer specified by BUFFER-OR-NAME empty (or create it
+if it does not exist).  Evaluate BODY with `standard-output' bound
+to that buffer, so that output from `prin1' and similar functions
+in BODY go into that buffer.  The buffer is not made current while
+BODY is evaluated.  Finally, display the buffer in a window and
+put it in `help-mode'.  Return the value returned by BODY.
+
+The help window will be selected if `help-window-select' is
+non-nil.  However, if the help window displays on a different
+frame, the window manager may automatically select that frame.
+
+See `with-temp-buffer-window' for more details."
   (declare (indent 1) (debug t))
   `(progn
      ;; Make `help-window-point-marker' point nowhere.  The only place
@@ -1390,7 +1395,7 @@
 	    (cons 'help-mode-finish temp-buffer-window-show-hook)))
        (setq help-window-old-frame (selected-frame))
        (with-temp-buffer-window
-	,buffer-name nil 'help-window-setup (progn ,@body)))))
+	,buffer-or-name nil 'help-window-setup (progn ,@body)))))
 
 ;; Called from C, on encountering `help-char' when reading a char.
 ;; Don't print to *Help*; that would clobber Help history.

bug#30792: 26.0.91; improve docstring of with-help-window

[Eli Zaretskii]

> From: Nick Helm <nick@tenpoint.co.nz>
> Date: Tue, 13 Mar 2018 23:31:01 +1300
> Cc: 30792@debbugs.gnu.org
> 
> Sorry, got a little ahead of myself. Try this.
> 
> --- a/lisp/help.el	2018-03-13 19:52:32.000000000 +1300
> +++ b/lisp/help.el	2018-03-13 23:23:52.000000000 +1300
> @@ -1370,15 +1370,20 @@
>  
>  ;; (4) A marker (`help-window-point-marker') to move point in the help
>  ;;     window to an arbitrary buffer position.
> -(defmacro with-help-window (buffer-name &rest body)
> -  "Display buffer named BUFFER-NAME in a help window.
> -Evaluate the forms in BODY with standard output bound to a buffer
> -called BUFFER-NAME (creating it if it does not exist), put that
> -buffer in `help-mode', display the buffer in a window (see
> -`with-temp-buffer-window' for details) and issue a message how to
> -deal with that \"help\" window when it's no more needed.  Select
> -the help window if the current value of the user option
> -`help-window-select' says so.  Return last value in BODY."
> +(defmacro with-help-window (buffer-or-name &rest body)
> +  "Show buffer BUFFER-OR-NAME with output of BODY in a help window.
> +Make the buffer specified by BUFFER-OR-NAME empty (or create it
> +if it does not exist).  Evaluate BODY with `standard-output' bound
> +to that buffer, so that output from `prin1' and similar functions
> +in BODY go into that buffer.  The buffer is not made current while
> +BODY is evaluated.  Finally, display the buffer in a window and
> +put it in `help-mode'.  Return the value returned by BODY.
> +
> +The help window will be selected if `help-window-select' is
> +non-nil.  However, if the help window displays on a different
> +frame, the window manager may automatically select that frame.
> +
> +See `with-temp-buffer-window' for more details."

Thanks.

It strikes me that instead of repeating most of what
with-temp-buffer-window's doc string says, it might be better to
simply refer to there.  Like this:

  Display the output produced by evaluating BODY, like
  `with-temp-buffer-window' does, then put the window in
  `help-mode' [...]

bug#30792: 26.0.91; improve docstring of with-help-window

[Nick Helm]

On Wed, 14 Mar 2018 at 05:53:32 +1300, Eli Zaretskii wrote:

> It strikes me that instead of repeating most of what
> with-temp-buffer-window's doc string says, it might be better to
> simply refer to there.  Like this:
>
>   Display the output produced by evaluating BODY, like
>   `with-temp-buffer-window' does, then put the window in
>   `help-mode' [...]

Ok, how's this? (wording borrowed from other macros that use
`with-temp-buffer-window'):

--- a/lisp/help.el	2018-03-14 13:03:14.000000000 +1300
+++ b/lisp/help.el	2018-03-14 13:06:03.000000000 +1300
@@ -1370,15 +1370,10 @@

 ;; (4) A marker (`help-window-point-marker') to move point in the help
 ;;     window to an arbitrary buffer position.
-(defmacro with-help-window (buffer-name &rest body)
-  "Display buffer named BUFFER-NAME in a help window.
-Evaluate the forms in BODY with standard output bound to a buffer
-called BUFFER-NAME (creating it if it does not exist), put that
-buffer in `help-mode', display the buffer in a window (see
-`with-temp-buffer-window' for details) and issue a message how to
-deal with that \"help\" window when it's no more needed.  Select
-the help window if the current value of the user option
-`help-window-select' says so.  Return last value in BODY."
+(defmacro with-help-window (buffer-or-name &rest body)
+  "Evaluate BODY, send output to BUFFER-OR-NAME and show in a help window.
+This construct is like `with-current-buffer-window' but unlike that
+puts the buffer specified by BUFFER-OR-NAME in `help-mode'."
   (declare (indent 1) (debug t))
   `(progn
      ;; Make `help-window-point-marker' point nowhere.  The only place
@@ -1390,7 +1385,7 @@
 	    (cons 'help-mode-finish temp-buffer-window-show-hook)))
        (setq help-window-old-frame (selected-frame))
        (with-temp-buffer-window
-	,buffer-name nil 'help-window-setup (progn ,@body)))))
+	,buffer-or-name nil 'help-window-setup (progn ,@body)))))

 ;; Called from C, on encountering `help-char' when reading a char.
 ;; Don't print to *Help*; that would clobber Help history.

bug#30792: 26.0.91; improve docstring of with-help-window

[martin rudalics]

 >> It strikes me that instead of repeating most of what
 >> with-temp-buffer-window's doc string says, it might be better to
 >> simply refer to there.  Like this:
 >>
 >>    Display the output produced by evaluating BODY, like
 >>    `with-temp-buffer-window' does, then put the window in
 >>    `help-mode' [...]

Neither `with-temp-buffer-window' nor `help-mode' are of any relevance
here.  The distinctive aspect of `with-help-window' is provided by
`help-window-setup' which sets up the help window for quitting it
later.  Earlier macros failed miserably in this regard.  (I have to
admit that many people are still ignorant of this fact and believe
they could resolve the quitting problem by saving and restoring window
configurations.  So the old doc-string probably failed to provide this
rather crucial information as well.)

 > +This construct is like `with-current-buffer-window' but unlike that

IIUC it does not make the buffer current when running BODY.

martin

bug#30792: 26.0.91; improve docstring of with-help-window

[Eli Zaretskii]

> From: Nick Helm <nick@tenpoint.co.nz>
> Cc: 30792@debbugs.gnu.org
> Date: Wed, 14 Mar 2018 13:09:15 +1300
> 
> On Wed, 14 Mar 2018 at 05:53:32 +1300, Eli Zaretskii wrote:
> 
> > It strikes me that instead of repeating most of what
> > with-temp-buffer-window's doc string says, it might be better to
> > simply refer to there.  Like this:
> >
> >   Display the output produced by evaluating BODY, like
> >   `with-temp-buffer-window' does, then put the window in
> >   `help-mode' [...]
> 
> Ok, how's this? (wording borrowed from other macros that use
> `with-temp-buffer-window'):

OK, but I think you should mention this part of the old doc string:

  and issue a message how to deal with that \"help\" window when it's
  no more needed

bug#30792: 26.0.91; improve docstring of with-help-window

[Eli Zaretskii]

> Date: Wed, 14 Mar 2018 09:14:33 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: 30792@debbugs.gnu.org
> 
>  >> It strikes me that instead of repeating most of what
>  >> with-temp-buffer-window's doc string says, it might be better to
>  >> simply refer to there.  Like this:
>  >>
>  >>    Display the output produced by evaluating BODY, like
>  >>    `with-temp-buffer-window' does, then put the window in
>  >>    `help-mode' [...]
> 
> Neither `with-temp-buffer-window' nor `help-mode' are of any relevance
> here.

??? with-help-window is a thin wrapper around with-temp-buffer-window.

> The distinctive aspect of `with-help-window' is provided by
> `help-window-setup' which sets up the help window for quitting it
> later.

Indeed, which is why I think the modified doc string should keep the
part of the existing doc string which tells about this special setup.

>  > +This construct is like `with-current-buffer-window' but unlike that
> 
> IIUC it does not make the buffer current when running BODY.

I think the reference to with-current-buffer-window is a copy/paste
mistake, it should instead refer to with-temp-buffer-window.

bug#30792: 26.0.91; improve docstring of with-help-window

[martin rudalics]

 >> Neither `with-temp-buffer-window' nor `help-mode' are of any relevance
 >> here.
 >
 > ??? with-help-window is a thin wrapper around with-temp-buffer-window.

I confused `with-temp-buffer-window' and `with-output-to-temp-buffer'.
It's the former that sets up the quitting mechanism.

 >> The distinctive aspect of `with-help-window' is provided by
 >> `help-window-setup' which sets up the help window for quitting it
 >> later.
 >
 > Indeed,

... not really, as mentioned abbove ...

 > which is why I think the modified doc string should keep the
 > part of the existing doc string which tells about this special setup.

The comment before the definition of `with-help-window' says what it
does additionally.  It think it's really not necessary to say it in
the doc-string.

But if we change BUFFER-NAME to BUFFER-OR-NAME this should be
reflected in the Elisp manual.

martin

bug#30792: 26.0.91; improve docstring of with-help-window

[Nick Helm]

On Thu, 15 Mar 2018 at 09:05:57 +1300, martin rudalics wrote:

>> which is why I think the modified doc string should keep the
>> part of the existing doc string which tells about this special setup.
>
> The comment before the definition of `with-help-window' says what it
> does additionally.  It think it's really not necessary to say it in
> the doc-string.

I don't think a user should have to read code comments to work out how
to use a macro. Besides, all of those comments (except (4)) appear in
the macro's manual entry, though not as clearly.

Isn't it sufficient that the docstring says it puts the buffer in
`help-mode'? If a user wants to find out what that means and how to
tweak the result, they can follow the link and read the help
documentation. If they're reading the docstring in the first place, they
almost certainly know how help works, and that pushing q quits a help
window by default.

> But if we change BUFFER-NAME to BUFFER-OR-NAME this should be
> reflected in the Elisp manual.

This was just a suggestion, as `with-help-window' simply passes the
value along to `with-temp-buffer-window'.

I think the expected use of `with-help-window' was to receive output
from `help-buffer', which returns an existing help buffer object or
"*Help*". But that's not the only way to use `with-help-window' and
`with-temp-buffer-window' can accept any existing buffer object or a
string name to create a new one. I think it would be useful if
`with-help-window' made that clear by using the same name.

Nick

bug#30792: 26.0.91; improve docstring of with-help-window

[martin rudalics]

 > Isn't it sufficient that the docstring says it puts the buffer in
 > `help-mode'? If a user wants to find out what that means and how to
 > tweak the result, they can follow the link and read the help
 > documentation. If they're reading the docstring in the first place, they
 > almost certainly know how help works, and that pushing q quits a help
 > window by default.

Agreed.  Please follow Eli's proposal in this regard.

 >> But if we change BUFFER-NAME to BUFFER-OR-NAME this should be
 >> reflected in the Elisp manual.
 >
 > This was just a suggestion, as `with-help-window' simply passes the
 > value along to `with-temp-buffer-window'.
 >
 > I think the expected use of `with-help-window' was to receive output
 > from `help-buffer', which returns an existing help buffer object or
 > "*Help*". But that's not the only way to use `with-help-window' and
 > `with-temp-buffer-window' can accept any existing buffer object or a
 > string name to create a new one. I think it would be useful if
 > `with-help-window' made that clear by using the same name.

Agreed as well.  But we should synchronize argument names in
definitions and manuals so please provide a corresponding change for
help.texi too.

Thanks, martin

bug#30792: 26.0.91; improve docstring of with-help-window

[Nick Helm]

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

Two patches attached.

Thanks,
Nick



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Change-variable-names.patch --]
[-- Type: text/x-patch, Size: 2441 bytes --]

From 3b96b7ddeb2dc0049b036e1f552b7029031042ec Mon Sep 17 00:00:00 2001
From: Nick Helm <nick@tenpoint.co.nz>
Date: Sun, 18 Mar 2018 17:26:34 +1300
Subject: [PATCH 1/2] Align `with-help-window' variable names with
 `with-temp-buffer-window'.

* doc/lispref/help.texi (Help Functions):
* lisp/help.el (with-help-window): Change variable name to buffer-or-name.
---
 doc/lispref/help.texi | 4 ++--
 lisp/help.el          | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/doc/lispref/help.texi b/doc/lispref/help.texi
index 33cc2f0d55..ea5e525735 100644
--- a/doc/lispref/help.texi
+++ b/doc/lispref/help.texi
@@ -744,10 +744,10 @@ This function returns the name of the help buffer, which is normally
 @end defun
 
 @vindex help-window-select
-@defmac with-help-window buffer-name body@dots{}
+@defmac with-help-window buffer-or-name body@dots{}
 This macro evaluates @var{body} like @code{with-output-to-temp-buffer}
 (@pxref{Temporary Displays}), inserting any output produced by its forms
-into a buffer named @var{buffer-name}.  (Usually, @var{buffer-name}
+into a buffer named @var{buffer-or-name}.  (Usually, @var{buffer-or-name}
 should be the value returned by the function @code{help-buffer}.)  It
 also puts the specified buffer into Help mode and displays a message
 telling the user how to quit and scroll the help window.  It selects the
diff --git a/lisp/help.el b/lisp/help.el
index 014af5141e..55abecc05d 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1370,7 +1370,7 @@ Return VALUE."
 
 ;; (4) A marker (`help-window-point-marker') to move point in the help
 ;;     window to an arbitrary buffer position.
-(defmacro with-help-window (buffer-name &rest body)
+(defmacro with-help-window (buffer-or-name &rest body)
   "Display buffer named BUFFER-NAME in a help window.
 Evaluate the forms in BODY with standard output bound to a buffer
 called BUFFER-NAME (creating it if it does not exist), put that
@@ -1390,7 +1390,7 @@ the help window if the current value of the user option
 	    (cons 'help-mode-finish temp-buffer-window-show-hook)))
        (setq help-window-old-frame (selected-frame))
        (with-temp-buffer-window
-	,buffer-name nil 'help-window-setup (progn ,@body)))))
+	,buffer-or-name nil 'help-window-setup (progn ,@body)))))
 
 ;; Called from C, on encountering `help-char' when reading a char.
 ;; Don't print to *Help*; that would clobber Help history.
-- 
2.14.3 (Apple Git-98)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Improve-docstring.patch --]
[-- Type: text/x-patch, Size: 1769 bytes --]

From cbbb8d229f2b34f944493ea3415c18a80b776359 Mon Sep 17 00:00:00 2001
From: Nick Helm <nick@tenpoint.co.nz>
Date: Sun, 18 Mar 2018 21:05:44 +1300
Subject: [PATCH 2/2] Improve `with-help-window' docstring.

* lisp/help.el (with-help-window): Rewrite docstring, adding reference to
`help-window-setup'.
---
 lisp/help.el | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lisp/help.el b/lisp/help.el
index 55abecc05d..958b06c095 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1371,14 +1371,13 @@ Return VALUE."
 ;; (4) A marker (`help-window-point-marker') to move point in the help
 ;;     window to an arbitrary buffer position.
 (defmacro with-help-window (buffer-or-name &rest body)
-  "Display buffer named BUFFER-NAME in a help window.
-Evaluate the forms in BODY with standard output bound to a buffer
-called BUFFER-NAME (creating it if it does not exist), put that
-buffer in `help-mode', display the buffer in a window (see
-`with-temp-buffer-window' for details) and issue a message how to
-deal with that \"help\" window when it's no more needed.  Select
-the help window if the current value of the user option
-`help-window-select' says so.  Return last value in BODY."
+  "Evaluate BODY, send output to BUFFER-OR-NAME and show in a help window.
+This construct is like `with-temp-buffer-window' but unlike that
+puts the buffer specified by BUFFER-OR-NAME in `help-mode' and
+displays a message about how to delete the help window when it's no
+longer needed.  The help window will be selected if
+`help-window-select' is non-nil.  See `help-window-setup' for
+more options."
   (declare (indent 1) (debug t))
   `(progn
      ;; Make `help-window-point-marker' point nowhere.  The only place
-- 
2.14.3 (Apple Git-98)

bug#30792: 26.0.91; improve docstring of with-help-window

[Eli Zaretskii]

> From: Nick Helm <nick@tenpoint.co.nz>
> Cc: Eli Zaretskii <eliz@gnu.org>, martin rudalics <rudalics@gmx.at>
> Date: Mon, 19 Mar 2018 00:19:03 +1300
> 
> Two patches attached.

Thanks, I pushed them to the emacs-26 branch.

Note that they needed "some work", please try to make sure these minor
nits are taken care of in your future contributions:

  . always mention the bug number in the commit log message
  . make sure the lines in the commit log message are shorter than 67
    characters, so that they don't overflow a line when converted to a
    ChangeLog file
  . be sure to read the docs after renaming variables, because
    frequently additional changes are needed to make the result clear
    and/or palatable
  . it's better not to break a changeset into separate parts, unless
    really necessary, so that the result is a single commit on the
    mainline

Keeping those rules will allow us to use "git am" instead of
committing manually.