Package: emacs;
Reported by: Stéphane Marks <shipmints <at> gmail.com>
Date: Thu, 19 Jun 2025 20:25:05 UTC
Severity: normal
View this message in rfc822 format
From: Eli Zaretskii <eliz <at> gnu.org> To: Stéphane Marks <shipmints <at> gmail.com> 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: bug#78843: Various vtable bug fixes and enhancements Date: Sun, 22 Jun 2025 11:59:21 +0300
> 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.
For now, just a few comments from superficial first reading, mainly
about the documentation.
> +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?
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.