Package: emacs;
Reported by: Stéphane Marks <shipmints <at> gmail.com>
Date: Thu, 19 Jun 2025 20:25:05 UTC
Severity: normal
Message #17 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: Sun, 22 Jun 2025 05:12:46 -0400
[Message part 1 (text/plain, inline)]
On Sun, Jun 22, 2025 at 4:59 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > Cc: Adam Porter <adam <at> alphapapa.net>, Spencer Baugh <
> sbaugh <at> janestreet.com>,
> > Lars Ingebrigtsen <larsi <at> gnus.org>, Augusto Stoffel <
> arstoffel <at> gmail.com>,
> > Kristoffer Balintona <krisbalintona <at> gmail.com>
> > From: Stéphane Marks <shipmints <at> gmail.com>
> > Date: Sat, 21 Jun 2025 11:41:50 -0400
> >
> > On Thu, Jun 19, 2025 at 4:25 PM Stéphane Marks <shipmints <at> gmail.com>
> wrote:
> >
> > Bug for a consolidated patch which is in the process of being prepared.
> >
> > Large patch attached. All the changes, aside from bug fixes, should be
> backward compatible. I've cc'd a few
> > of the major vtable users whose code bases helped inform many bug fixes
> and enhancements. Some of you
> > have helped test and provide feedback along the way. Please try this
> out (and review the updated texinfo
> > documentation if you want to).
>
> Thanks.
>
> Is it really not feasible to have separate patches for each bugfix and
> each enhancement (or coherent set of enhancements)? What if we decide
> to install just some of these changes -- you'd need to update the
> entire large patch, instead of dropping a relatively small one and
> leaving the others intact.
>
I hope we elect to adopt them all. It would be much more work to tease
them all out piecemeal and wouldn't really, in the end, produce a better
vtable.
For now, just a few comments from superficial first reading, mainly
> about the documentation.
>
I'll review the below and adjust. Much of the style was just carried over
from the original vtable doc. Happy to improve it.
WRT quoting symbols, there are examples in the texinfo doc collection that
suggest the convention is not enforced:
./misc/srecode.texi:such as the symbol @code{'prototype} for prototype
functions, or
./misc/ses.texi:@code{'ses-prin1}.
./misc/idlwave.texi:the single symbol @code{'everything}, all the copious
shell input is
./misc/auth.texi:Set this variable to @code{'trivia} to see lots of output
in
> > +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.
>
> > @table @code
> > +@item :name
> > +The default table name is `*vtable*`. Specify this if you have more
> ^^^^^^^^^^
> This should be @samp{*vtable*}.
>
> > -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.
>
> > +@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?
>
> > +@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.
>
> > +@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...
>
> > +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).
>
> > +that need special treatment, specify a larger guess increment. If nil,
> ^^^
> "nil" should have the @code markup.
>
> > +@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.
>
> > +@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).
>
> > +@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>.
>
> > +@findex vtable-goto-next-table
> > +@item <forward-paragraph>
>
> What's "<forward-paragraph>"? Are there keyboards with a key that has
> such a label?
>
> > --- 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?
>
Yessir.
[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.