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

From: Spencer Baugh <[email-masked]>
Date: Mon, 23 Jun 2025 13:15:06 -0400

Stéphane Marks <[email-masked]> writes:

> On Sun, Jun 22, 2025 at 6:35 AM Eli Zaretskii <[email-masked]> wrote:
>
>  > From: Stéphane Marks <[email-masked]>
>  > Date: Sun, 22 Jun 2025 06:24:39 -0400
>  > Cc: [email-masked], [email-masked], [email-masked], 
>  >       [email-masked], [email-masked], [email-masked]
>  > 
>  >  > Let's start with the vtable users I looped in actually using it.
>  > 
>  >  I don't see how this will help in reviewing the patch. It will allow
>  >  those folks to try the patch, but that's a separate job.  Patch review
>  >  has to happen regardless, because it might spot issues that no
>  >  reasonable amount of testing can.  This is why patch review is a
>  >  necessary step in our development process.
>  > 
>  >  > I can attach a vtable.el without the patch file and
>  >  > make it easier.
>  > 
>  >  Thanks, but it won't make it easier, no.
>  > 
>  > Of course.  I did say that I would submit a "big-bang" patch a few weeks ago.  The vtable users who are
>  > familiar with the code (having implemented their workarounds) should have an easier time with the review. 
>
>  That will help, but I would like to review it as well.  It's part of
>  my job.
>
>  Your patch lists many changes:
>
>  > Bug fixes
>  > 
>  > - vtable-update-object works with no visible or changed window
>  > width cache key (bug=69837)
>  > - Improve vtable--limit-string performance for long
>  > strings (bug=77684)
>  > - Keymap changes:
>  >   - Keymap spans the whole header column and header
>  >   dividers (bug=74701)
>  >   - Missing keymap and 'vtable-column property on body/header
>  >   dividers and newlines
>  >   - mouse-1 and close table bindings in vtable-header-line-map
>  >   keymap
>  >   - table-drag-resize-column-map keymap for use in body on
>  >   dividers (bug=74701)
>  >   - vtable-header-drag-resize-column-map consolidates
>  >   vtable-header-line-map and vtable-drag-resize-column-map
>  > - Infer column widths based on formatted elements, not raw
>  > elements
>  > - Compute columns works when there are no objects at
>  > initialization
>  > - text-scale-mode support (header and body elements are pixel
>  > aligned)
>  > - Header line handles display-line-numbers-mode
>  > - Redraw row colors on lines below an inserted or removed line
>  > - Cache is coherent with its objects (rather than by chance),
>  > invalidated where needed
>  > - All cache references now cache warming calls vs. mere cache
>  > get
>  > - Table mutations work when the table buffer is not the current
>  > buffer and if the table isn't current
>  > - Resize all tables in a buffer when their window
>  > resizes (debounced to accommodate frame size dragging)
>  > - Spacer goes after the column name on the header if right
>  > aligned
>  > - Column name defaults to right-aligned when its data are
>  > right-aligned
>  > - Improve column placeholder message when no objects and no
>  > columns are specified
>  > - Do not display divider after a row's final column
>  > - Option to display divider on the header (was always on)
>  > - Treat divider as a part of its preceding column when dragging
>  > or sorting
>  > - Append table faces to values and column heading names rather
>  > than override
>  > - Truncated string ellipsis text properties match those of the
>  > formatted column entry
>  > - Clicking on a header keeps point within table bounds
>  > - Use vtable-object-equal and do not assume eq (or via assq,
>  > memq)
>  > - Header line adjust final column name and sort indicator in a
>  > narrow window works as intended
>  > - Set window point where needed
>  > - Next/previous line remains in the current column
>  > - Signal an error if a table is inserted in more than one buffer
>  > or more than once in a buffer
>  > - vtable--cache-key handle if called when the selected window
>  > does not contain a table buffer
>  > - Restore "focus" after vtable-remove-object,
>  > vtable-update-object, vtable-redisplay-range, vtable-revert if
>  > table is current
>  > - vtable-revert works when reverted table is not the current
>  > table
>  > - Mutate the correct associated table in
>  > vtable-header-line-sort, vtable--drag-resize-column
>  > - vtable--recompute-cache considers the table's sort order
>  > - vtable--alter-column-width respects min-width and max-width
>  > - vtable-insert-object if integer location specified, does not
>  > assume the object list and line cache are in the same order
>  > - vtable-insert-object signal an error if integer insert
>  > location specified when the table is sorted
>  > - vtable-update-object does not assume objects are in the same
>  > order as the cache, which might be sorted
>  > - vtable-update-object no longer signal "setcar nil" on failed
>  > old-object search
>  > - vtable-update-object goto table before updating
>  > - vtable-remove-object updates numeric column type as needed
>  > after removing line
>  > - vtable-remove-object signals an error if the specified object
>  > is not found
>  > - vtable-remove-object leaves point within table bounds if table
>  > is current (and there are rows)
>  > - Hide event-handler interactive functions from M-x display (not
>  > really "commands")
>  > - Renamed private vtable-header-line-sort to
>  > vtable--header-line-sort
>  > - Ensured docstrings for all public functions with added
>  > clarifications where needed
>  > 
>  > Enhancements
>  > 
>  > - Custom column comparator for sorting types other than numerics
>  > and lexical strings
>  > https://lists.gnu.org/archive/html/emacs-devel/2024-10/msg00043.html
>  > - Column-width inference can optionally include column-name
>  > width vs. just data
>  > - Table name slot, defaults to "*vtable*", which is displayed in
>  > messages to differentiate vtables
>  > - Duplicate object ignore, warning or error, defaults to ignore
>  > duplicates
>  > - Explicitly identify columns as numeric or non-numeric to avoid
>  > the cost of inferring numericalness
>  > - Option to sort table after insert or update object
>  > - Option to select the newly inserted line if the table is
>  > current
>  > - Sort indicator characters specifiable
>  > - Sort indicator faces for ascending and descending
>  > - Unsort the table, toggling between unsorted and the sort-by at
>  > initialization
>  > - Unsort binding in vtable-map
>  > - Add tab to vtable-header-line-map to move point into the table
>  > - Functions to compute programmatic row/column colors
>  > - Header face independent of body face
>  > - Header column alignment independent of from body alignment,
>  > defaults to body alignment
>  > - Center alignment options
>  > - Truncation ellipsis specifiable
>  > - Option to make in-buffer header cursor-intangible
>  > - Option to make in-buffer "decor" cursor-intangible (spacers,
>  > dividers, indicator padding)
>  > - Object/line marking/unmarking functions, marked line face
>  > - Next/previous line functions that remain in table body bounds
>  > - Multi-table commands 'vtable-goto-next-table',
>  > 'vtable-goto-previous-table'
>  > - Optionally apply row text properties; e.g., for mouse-face,
>  > cursor-face
>  > - Optional "navigation" keymap adding several useful bindings
>  > including "q" to close the table (a la special-mode)
>  > - Display a message when interactive column resizing min-width
>  > and max-width limits reached
>  > - Optionally pulse an updated, inserted, removed line,
>  > defaulting to 'pulse-momentary-highlight-one-line'
>  > - Go to table beginning/end of body convenience functions
>  > - Table and object line-number convenience functions
>  > - Table close-action which defaults to 'quit-window'
>  > - Extra-data slots on both the table and columns for
>  > programmatic reference
>
>  It is nigh impossible to review such a large number of changes
>  together: how am I supposed to find out which design and
>  implementation decisions did you take, when faced with such a large
>  number of separate changes?  Without that, the review is not useful,
>  even impossible.
>
>  So please subdivide the patch into smaller, preferably independent
>  parts, to make the patch review possible, let alone practical.
>
> Let's see what the vtable user feedback is first and then I'll think
> about it.  It will be a trade-off between your pain and mine.

As a vtable user: Please either split this patch up into smaller
patches, or add many automated tests of vtable (both for new and old
functionality).  Preferably both.

Without doing at least one of those things, I don't believe this patch
should be merged, because it is too likely to break things.

Yes, I know you have tested it manually.  That is not sufficient.  Good
software engineering practice demands more than that.