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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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.





Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Nick Helm <[hidden email]>
> Date: Tue, 13 Mar 2018 23:31:01 +1300
> Cc: [hidden email]
>
> 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' [...]



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Nick Helm
> From: Nick Helm <[hidden email]>
> Cc: [hidden email]
> 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



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by martin rudalics
> Date: Wed, 14 Mar 2018 09:14:33 +0100
> From: martin rudalics <[hidden email]>
> CC: [hidden email]
>
>  >> 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.



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Nick Helm
In reply to this post by martin rudalics
Two patches attached.

Thanks,
Nick



0001-Change-variable-names.patch (2K) Download Attachment
0002-Improve-docstring.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Nick Helm <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>, martin rudalics <[hidden email]>
> 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.