bug#51247: 28.0.60; Insufficient documentation of tab-bar.el internal functions

[Eli Zaretskii <eliz@gnu.org>]

> From: Juri Linkov <juri@linkov.net>
> Cc: 51247@debbugs.gnu.org
> Date: Sun, 17 Oct 2021 20:17:43 +0300
> 
> > I show below the places where IMO the lack of proper documentation is
> > particularly evident, or where the documentation is insufficient or
> > inaccurate.  Please fill at least those gaps; bonus points for
> > documenting more than this bare minimum.
> 
> I fixed all places that you noticed (please check)

Thanks.  A few questions/suggestions below:

   (defun tab-bar--key-to-number (key)
  +  "This function is used to interpret the key that represents a tab.
  +It returns `t' for the `nil' value, `nil' for the current tab,
  +returns the number for the symbol that begins with `tab-' like `tab-1',
  +and `t' for other values."

I'd rewrite as follows:

  (defun tab-bar--key-to-number (key)
    "Return the tab number represented by KEY.
  If KEY is a symbol 'tab-N', where N is a tab number, the value is N.
  If KEY is \\='current-tab, the value is nil.
  For any other value of KET, the value is t."

Is this correct?

   (defun tab-bar--event-to-item (posn)
  +  "This function extracts extra info from the mouse event POSN.
  +It returns a list that contains three elements: a key,
  +a key binding, and a boolean value whether the close button \"+\"
  +was clicked."

I'd rewrite as follows:

  (defun tab-bar--event-to-item (posn)
    "This function extracts extra info from the mouse event at position POSN.
  It returns a list of the form (KEY KEY-BINDING CLOSE-P), where:
   KEY is a symbol representing a tab, such as \\='tab-1 or \\='current-tab;
   KEY-BINDING is the binding of KEY;
   CLOSE-P is non-nil if the mouse event was a click on the close button \"x\",
     nil otherwise."

Is this correct?

   (defun tab-bar--format-tab-group (tab i &optional current-p)
  +  "Format TAB as a tab that represents a group of tabs.
  +Use the argument I as its index

Whose index is "its index" here?

   (defun tab-bar--current-tab-make (&optional tab)
  -  ;; `tab' here is an argument meaning "use tab as template".  This is
  -  ;; necessary when switching tabs, otherwise the destination tab
  -  ;; inherits the current tab's `explicit-name' parameter.
  +  "Make the current tab data structure from TAB.
  +TAB here is an argument meaning \"use tab as template\".  This is
  +necessary when switching tabs, otherwise the destination tab
  +inherits the current tab's `explicit-name' parameter."

I don't think I understand what do you mean by "use tab as template",
can you explain?

> except the following:
> 
> >   (defun tab-bar-format-tabs-groups ()
> >     "Show tabs with their groups."
> >
> > The doc string says "show", but this functions doesn't display
> > anything, AFAICT, it produces a list.  Please adjust the doc string
> > and add a description of the returned value.
> 
> This docstring is automatically displayed in the Customization buffer
> of `tab-bar-format' that looks like this:
> 
>   Hide Tab Bar Format:
>   Hook:
>   [ ] tab-bar-format-menu-global
>           Show global menu on clicking the Menu button.
>   [X] tab-bar-format-history
>           Show back and forward buttons when ‘tab-bar-history-mode’ is enabled. More
>   [ ] tab-bar-format-tabs
>           Show all tabs.
>   [X] tab-bar-format-tabs-groups
>           Show tabs with their groups.
>   [ ] tab-bar-separator
>           Separator between tabs.
>   [ ] tab-bar-format-add-tab
>           Button to add a new tab.
>   [X] tab-bar-format-align-right
>           Align the rest of tab bar items to the right.
>   [X] tab-bar-format-global
>           Format ‘global-mode-string’ to display it in the tab bar. More
>   [INS]
> 
> So here its docstring explains what this option is useful for.

Can't you provide a separate text for the Custom display?  The doc
string in its current form is simply misleading.

> >   (defun switch-to-buffer-other-tab (buffer-or-name &optional norecord)
> >     "Switch to buffer BUFFER-OR-NAME in another tab.
> >   Like \\[switch-to-buffer-other-frame] (which see), but creates a new tab.
> >   Interactively, prompt for the buffer to switch to."
> >
> > This command should document the NORECORD argument.
> 
> Oops, this was copy/paste from switch-to-buffer, switch-to-buffer-other-window,
> switch-to-buffer-other-frame that all just pass NORECORD down to 'pop-to-buffer'.
> But switch-to-buffer-other-tab doesn't use 'pop-to-buffer'.  It uses 'display-buffer'
> that has no NORECORD arg.  So this arg should be obsoleted, or maybe deleted right away.

Fine with me, please use advertised-calling-convention or somesuch to
hide that argument.