On Sun, Jun 22, 2025 at 4:59 AM Eli Zaretskii 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 > > 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 . > Done. > +@findex vtable-goto-next-table > > +@item > > What's ""? 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 " " #'vtable-goto-next-table " " #'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