GNU bug report logs - #76789
31.0.50; [PATCH] speedbar: New speedbar-window-mode

Previous Next

Full log

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vincenzo Pupillo <v.pupillo <at> gmail.com>
Cc: 76789 <at> debbugs.gnu.org
Subject: Re: bug#76789: 31.0.50; [PATCH] speedbar: New speedbar-window-mode
Date: Sun, 09 Mar 2025 08:53:50 +0200

> From: Vincenzo Pupillo <v.pupillo <at> gmail.com>
> Cc: 76789 <at> debbugs.gnu.org
> Date: Sat, 08 Mar 2025 23:06:13 +0100
> 
> > Speedbar has its own manual.  Did you consider updating that manual
> > with this new feature?
> I wrote something in speedbar.texi (not included in this patch), but I also 
> saw that the emacs manual has section 18.9 Speedbar Frame (chapter 18 Frames 
> and Graphical Display), and I am not sure how to edit it.

Just add short text there saying that speedbar can optionally be
displayed as a window, not a frame.  That section has a reference to
the Speedbar manual, so the details are covered by that.

Thanks.  Please see a few more comments below.

> +*** The new command 'speedbar-window-mode' open Speedbar in a window instead
> +of a frame.                                ^^^^

"opens"

> +*** New alias 'speedbar-window' is an alias for 'speedbar-window-mode'.

"New command 'speedbar-window' is an alias for 'speedbar-window-mode'."

> +*** The new user option 'speedbar-prefer-window', tell 'speedbar' to open
> +a side window instead of a frame.                 ^^^^

"tells"

> +(defcustom speedbar-prefer-window nil
> +  "If t, the command `speedbar' open the speedbar in a window."
                                   ^^^^
"opens"

> +(defcustom speedbar-window-dedicated-window t
> +  "Make the `speedbar-window' dedicated."

"Whether to make the `speedbar-window' dedicated."

> +  :group 'speedbar
> +  :type 'boolean
> +  :version "31.1")
> +
> +(defcustom speedbar-window-side 'left
> +  "Show the `speedbar-window' on the `left', `right', `top' or `bottom'.
> +See `display-buffer-in-side-window' for more details."

Our style is to make the first line of the doc string a kind of
summary:

    "Control the side of the frame on which to show the speedbar window.
  The value can be `left', `right', `top' or `bottom'.
  See `display-buffer-in-side-window' for more details."

> +(defcustom speedbar-window-default-width 20
> +  "Initial width in characters of `speedbar-window' under window system.

Why "under window system"?  Doesn't this work on TTY frames in the
same way?

> +(defcustom speedbar-window-max-width 40
> +  "The maximum allowed width in characters of the `speedbar-window'."

This begs the question: what happens with wider items?  I suggest to
tell that in the doc string.

> +(defun speedbar-easymenu-definition-trailer ()
> +  "Menu items appearing at the end of the speedbar menu."

I guess you meant "Return menu items appearing..."?

> +A nil ARG means toggle.  If `speedbar-prefer-window' is t, open the
> +speedbar in a window istead of in a frame."
                        ^^^^^^^^^^^^^^^^^^^^
"...instead of in a separate frame" is better here, because any Emacs
display is always "in a frame", even if it's in some window.

> +(defun speedbar-is-frame-or-window-p ()

This function is not a predicate, since its value is not a boolean.
So its name should be something like speedbar-frame-or-window, without
"is" and without "-p".

> +(defun speedbar-window-mode (&optional arg)
> +  "Enable or disable speedbar window.

I suggest

  Enable or disable speedbar window mode.

or

  "Enable or disable speedbar display in a separate window.

> +(defsubst speedbar-window--window-live-p ()
> +    "Return non-nil if  `speedbar--window' is defined and live."
                         ^^
Excess whitespace there.

> +(defsubst speedbar-window--buffer-live-p ()
> +    "Return non-nil `speedbar-buffer' is defined and live."
                      ^^
"if" is missing there.  Also, what do you mean by "buffer is defined"?
I suggest to remove it and leave only "buffer is live".

> +(defun speedbar-window--live-p ()
> +  "Return t if `speedbar-window' is opened."
                                    ^^^^^^^^^
I suggest "is live" or "is displayed".

> +(defsubst speedbar-window-current-window ()
> +  "Return t if the current windows is the `speedbar--window'."

"Return t if the selected window is the `speedbar--window'."

> +(defsubst speedbar-window--width ()
> +  "Return the width of `speedbar-window' WINDOW."
                                            ^^^^^^
This function has no argument named WINDOW.

> +(defun speedbar-width ()
> +  "Returns the width of the `speedbar'.
      ^^^^^^^
"Return"

> +(defun speedbar--speedbar-live-p ()
> +  "Return non-nil if `speedbar-window-mode' or `speedbar-frame-mode' are open."
                                                                        ^^^^^^^^^

"are active", I think?

Previous Next

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