Subject: Re: bug#78843: Various vtable bug fixes and enhancements

From: Stéphane Marks <[email-masked]>
Date: Mon, 23 Jun 2025 10:49:07 -0400

On Sun, Jun 22, 2025 at 4:59 AM Eli Zaretskii <[email-masked]> wrote:

>
> For now, just a few comments from superficial first reading, mainly
> about the documentation.
>

I'm going through the documentation based on your feedback while I have a
few minutes today.

> +A vtable can be inserted into a single buffer only.  An error is
> > +signaled if a vtable is attempted to be inserted more than once into a
> > +single buffer, or into multiple buffers.
>
> The passive voice here is used way too much.  Please rephrase not to
> use it.
>

Done. I will review the rest of the language.

>  @table @code
> > +@item :name
> > +The default table name is `*vtable*`.  Specify this if you have more
>                              ^^^^^^^^^^
> This should be @samp{*vtable*}.
>

Done.

> -In the latter case, if @code{:columns} is non-@code{nil} and there's
> > -more elements in the sequence than there is in @code{:columns}, only
> > -the @code{:columns} first elements are displayed.
> > +In the latter case, if @code{:columns} is non-@code{nil} and there are
> > +more elements in the sequence than there are in @code{:columns}, the
> > +elements beyond those in @code{:columns} are not displayed.
>
> Since the elements are not in :columns, the modified text is
> confusing, IMO.  I suggest
>
>   ... the sequence elements beyond the number of elements in
>   @code{:columns} will not be shown.
>

Done.

> +@item :object-equal
> > +This function tests for the equality of two table objects.  It defaults
> > +to @code{eq}.
> > +
> > +@defun object-equal object1 object2
> > +Return non-nil if @var{object1} and @var{object2} are equal, and non-nil
> > +otherwise.
> > +@end defun
>
> I don't understand the purpose of this @defun here.  What did you
> intend to say by placing it here?
>

I was following Lars's original format for other "callback" functions.  If
I change this one, I should change them all for consistency.  What is your
preference?

> +@item :duplicate-objects
> > +A vtable assumes all objects inserted into the table are unique among
> > +themselves.  If the symbol @code{'allow}, the default, allow duplicate
> > +objects (the first object found during table operations has primacy), if
>
> First, please say "If the value is the symbol...", not just "If the
> symbol".  And second, the way we typeset symbols in a Texinfo manual
> is by using @code{allow}, without the apostrophe.
>

Done.

> +@code{'ignore}, silently ignore duplicates leaving the existing object
> > +intact, if @code{'ignore-warn} do the same thing as @code{'ignore} and
> > +produce a warning message, or if @code{'error}, signal an error if a
> > +duplicate is detected.
>
> Likewise here about the symbol names.  In addition, please use
> semi-colons to separate the description of the different values:
>
>   ...if it's |@code{ignore}, silently ignore duplicates leaving the
>   existing object intact; if it's @code{ignore-warn}, do the same...
>

Done.

> +This is a list where each element is either a string (the column name),
> > +a plist of keyword/values (to make a @code{vtable-column} object), or a
> > +@code{vtable-column} object (created by calling the function
> > +@code{make-vtable-column}).
>
> When you mention a function or a variable, like make-vtable-column
> here, it is usually a good idea to have a cross-reference to where
> they are described (unless they are described in the same node).
>

I will.

> +that need special treatment, specify a larger guess increment.  If nil,
>                                                                       ^^^
> "nil" should have the @code markup.
>

I corrected all the nil references.  It was copy/pasta from docstrings.

> +@item comparator
> > +This function will be called to compare column values.  Use this when
> > +your objects contain data that needs non-numeric or string collation,
> > +for example, dates.  Another case would be where you want the equivalent
> > +of @code{sort-fold-case}.
> > +
> > +@defun comparator value1 value2
> > +This function is called with two values to compare.  The return value
> > +should follow the semantics of @code{<} or, @code{string-lessp}.
> >  @end defun
>
> Same question here about this @defun, as above about object-equal.
>

Ditto.  Tell me which style you prefer.

> +@item :sort-indicator
> > +This is a list of two cons cells that specify the sort ascending and
> > +descending characters that are shown on the table header to indicate a
> > +sorted column.  The first character is the fancier ``graphical''
> > +character, and the second a text-only character.  If the first character
> > +cannot be displayed on the selected frame, the table will show the
> > +text-only character.  The default is
> > +@code{vtable-sort-indicator-default}.  For example:
> > +
> > +@lisp
> > +'((?▼ ?v) (?▲ ?^)) ; This is the default.
> > +'((?⬇ ?v) (?⬆ ?^)) ; Alternative arrows.
> > +@end lisp
>
> Using non-ASCII characters in a manual runs the risk of producing
> incorrect output in the printed manual, due to lack of support in the
> fonts used by TeX.  It is better to avoid that.  Here, you use them in
> the example, which seems unnecessary, so I suggest to rewrite the
> example using only ASCII characters (you can mention non-ASCII as a
> possibility without actually showing such an example).
>

Done.

> +@table @kbd
> > +@findex vtable-next-line
> > +@item n
> > +@item <down>
>
> Our conventions are to UPCASE arrow key names, so please use DOWN, UP,
> etc.
>
> Also, since "DOWN" is a label of the key, you should use @key{DOWN},
> not <DOWN>.
>

Done.

> +@findex vtable-goto-next-table
> > +@item <forward-paragraph>
>
> What's "<forward-paragraph>"?  Are there keyboards with a key that has
> such a label?
>

This is remapped in the code.  Is there a way you prefer to refer to
remapped keys?

(defvar-keymap vtable-navigation-map
  "<remap> <forward-paragraph>"  #'vtable-goto-next-table
  "<remap> <backward-paragraph>" #'vtable-goto-previous-table

> --- a/lisp/emacs-lisp/vtable.el
> > +++ b/lisp/emacs-lisp/vtable.el
> > @@ -30,77 +30,220 @@
> >
> >  (defface vtable
> >    '((t :inherit variable-pitch))
> > -  "Face used (by default) for vtables."
> > +  "Face used (by default) for vtable bodies."
> >    :version "29.1"
> >    :group 'faces)
> >
> > +(defface vtable-header
> > +  '((t :inherit (header-line vtable)))
> > +  "Face used (by default) for vtable headers."
> > +  :version "31.1"
> > +  :group 'faces)
> > +
> > +(defface vtable-marked
> > +  '((t :inherit region))
> > +  "Face used (by default) for marked vtable objects."
> > +  :version "31.1"
> > +  :group 'faces)
> > +
> > +(defface vtable-sort-indicator-ascend
> > +  '((t :inherit vtable-header))
> > +  "Face used (by default) for vtable ascend sort indicator."
> > +  :version "31.1"
> > +  :group 'faces)
> > +
> > +(defface vtable-sort-indicator-descend
> > +  '((t :inherit vtable-header))
> > +  "Face used (by default) for vtable descend sort indicator."
> > +  :version "31.1"
> > +  :group 'faces)
>
> Shouldn't these faces be in the vtable group instead?
>

Done.  I'd copied the original vtable.

Once we get feedback from users, I will split the patch into several
digestible smaller patches, but probably not too parsimoniously.

-Stéphane