Package: emacs;
Reported by: Stéphane Marks <shipmints <at> gmail.com>
Date: Thu, 19 Jun 2025 20:25:05 UTC
Severity: normal
Message #41 received at 78843 <at> debbugs.gnu.org (full text, mbox):
From: Stéphane Marks <shipmints <at> gmail.com> To: Eli Zaretskii <eliz <at> gnu.org> Cc: sbaugh <at> janestreet.com, krisbalintona <at> gmail.com, 78843 <at> debbugs.gnu.org, adam <at> alphapapa.net, larsi <at> gnus.org, arstoffel <at> gmail.com Subject: Re: bug#78843: Various vtable bug fixes and enhancements Date: Mon, 23 Jun 2025 10:49:07 -0400
[Message part 1 (text/plain, inline)]
On Sun, Jun 22, 2025 at 4:59 AM Eli Zaretskii <eliz <at> gnu.org> 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
[Message part 2 (text/html, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.