GNU bug report logs - #79982
[PATCH] Add vtable buffer slot

Previous Next

To reply to this bug, email your comments to 79982 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 Visuwesh <visuweshm <at> gmail.com>, Adam Porter <adam <at> alphapapa.net>,
 Lars Ingebrigtsen <larsi <at> gnus.org>, Augusto Stoffel <arstoffel <at> gmail.com>
Subject: [PATCH] Add vtable buffer slot
Date: Wed, 10 Dec 2025 09:54:36 -0500

[Message part 1 (text/plain, inline)]

This solves for background vtable mutations, i.e., updates initiated from
buffers other than the vtable buffer, and for buffer-adjusted
string-pixel-width computations.

Many more patches for fixes and enhancements coming on the back of this
one, once agreed.  I'd love to see all of these in Emacs 31 and I'm now
generally available to pick up the pace.

P.S. I've cc'd the same group as the last vtable discussions/patches.  If
you do not want to be cc'd on these, let me know.

-Stéphane

[Message part 2 (text/html, inline)]

[0001-Add-vtable-buffer-slot.patch (application/octet-stream, attachment)]

Message #8 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: 79982 <at> debbugs.gnu.org
Cc: Spencer Baugh <sbaugh <at> janestreet.com>,
 Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 Visuwesh <visuweshm <at> gmail.com>, Adam Porter <adam <at> alphapapa.net>,
 Lars Ingebrigtsen <larsi <at> gnus.org>, Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot
Date: Wed, 10 Dec 2025 09:59:37 -0500

[Message part 1 (text/plain, inline)]

+ Spencer (now in my copy/paste list)

On Wed, Dec 10, 2025 at 9:55 AM Stéphane Marks <shipmints <at> gmail.com> wrote:

> This solves for background vtable mutations, i.e., updates initiated from
> buffers other than the vtable buffer, and for buffer-adjusted
> string-pixel-width computations.
>
> Many more patches for fixes and enhancements coming on the back of this
> one, once agreed.  I'd love to see all of these in Emacs 31 and I'm now
> generally available to pick up the pace.
>
> P.S. I've cc'd the same group as the last vtable discussions/patches.  If
> you do not want to be cc'd on these, let me know.
>
> -Stéphane
>

[Message part 2 (text/html, inline)]

Message #11 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot
Date: Wed, 10 Dec 2025 10:12:00 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:
> This solves for background vtable mutations, i.e., updates initiated from buffers other than the vtable buffer, and for
> buffer-adjusted string-pixel-width computations.
>
> Many more patches for fixes and enhancements coming on the back of this one, once agreed.  I'd love to see all of these in Emacs
> 31 and I'm now generally available to pick up the pace.
>
> P.S. I've cc'd the same group as the last vtable discussions/patches.  If you do not want to be cc'd on these, let me know.
>
> -Stéphane 

Please add a test which passes with this patch and doesn't pass without
this patch.

Message #14 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot
Date: Wed, 10 Dec 2025 12:50:38 -0500

[Message part 1 (text/plain, inline)]

On Wed, Dec 10, 2025 at 10:12 AM Spencer Baugh <sbaugh <at> janestreet.com>
wrote:

> Stéphane Marks <shipmints <at> gmail.com> writes:
> > This solves for background vtable mutations, i.e., updates initiated
> from buffers other than the vtable buffer, and for
> > buffer-adjusted string-pixel-width computations.
> >
> > Many more patches for fixes and enhancements coming on the back of this
> one, once agreed.  I'd love to see all of these in Emacs
> > 31 and I'm now generally available to pick up the pace.
> >
> > P.S. I've cc'd the same group as the last vtable discussions/patches.
> If you do not want to be cc'd on these, let me know.
> >
> > -Stéphane
>
> Please add a test which passes with this patch and doesn't pass without
> this patch.
>

Try the new tests in this updated patch.

[Message part 2 (text/html, inline)]

[0001-Add-vtable-buffer-slot.patch (application/octet-stream, attachment)]

Message #17 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot
Date: Wed, 10 Dec 2025 13:54:56 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:

> On Wed, Dec 10, 2025 at 10:12 AM Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>  Please add a test which passes with this patch and doesn't pass without
>  this patch.
>
> Try the new tests in this updated patch.

Thanks, these tests look good, this will help a lot.

> From 6e33b8cbf2063e1e7689f231bb0c6c6450520bd3 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?St=C3=A9phane=20Marks?= <shipmints <at> gmail.com>
> Date: Wed, 10 Dec 2025 09:20:02 -0500
> Subject: [PATCH] Add vtable buffer slot
>
> This solves for background vtable mutations, i.e., updates
> initiated from buffers other than the vtable buffer, and for
> buffer-adjusted string-pixel-width computations.
>
> * lisp/emacs-lisp/vtable.el (vtable): New '-buffer' slot.
> (vtable-buffer):
> (vtable-set-buffer): New function.
> (vtable-update-object):
> (vtable-remove-object):
> (vtable-insert-object): Wrap operation with the vtable buffer.
> (vtable--insert): Split from old 'vtable-insert'.
> (vtable-insert): Insert table and record the buffer.
> (vtable--insert-line):
> (vtable--insert-header-line): Use 'vtable-buffer' for pixel-width computation.
> (vtable--limit-string):
> (vtable--char-width): Pass buffer to 'string-pixel-width'.
> (vtable-revert): New optional table argument.
> (vtable--alter-column-width):
> (vtable-revert-command):
> (vtable-sort-by-current-column): Call 'vtable-revert' with the table.
>
> * test/lisp/emacs-lisp/vtable-tests.el
> (vtable-tests--make-no-header-2-object-table): New helper
> function.
> (test-vstable-compute-columns): Correct typo in test name.  Use
> new helper function.
> (test-vtable-unique-buffer)
> (test-vtable-non-current-buffer-insert-object)
> (test-vtable-non-current-buffer-remove-object)
> (test-vtable-non-current-buffer-update-object)
> (test-vtable--limit-string-with-face-remapped-buffer): New test.
> ---
>  lisp/emacs-lisp/vtable.el            | 389 +++++++++++++++------------
>  test/lisp/emacs-lisp/vtable-tests.el |  78 +++++-
>  2 files changed, 291 insertions(+), 176 deletions(-)
>
> diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
> index bcdd280fb92..fe4bae01077 100644
> --- a/lisp/emacs-lisp/vtable.el
> +++ b/lisp/emacs-lisp/vtable.el
> @@ -68,6 +68,7 @@ vtable
>     (column-colors :initarg :column-colors :accessor vtable-column-colors)
>     (row-colors :initarg :row-colors :accessor vtable-row-colors)
>     (-cached-colors :initform nil)
> +   (-buffer :initform nil)
>     (-cache :initform (make-hash-table :test #'equal))
>     (-cached-keymap :initform nil)
>     (-has-column-spec :initform nil))
> @@ -221,6 +222,20 @@ vtable--face-color
>  
>  ;;; Interface utility functions.
>  
> +(defun vtable-buffer (&optional table)
> +  "Return the buffer associated with TABLE.
> +If TABLE is nil, use the table under point.  Return nil if the table has
> +not been inserted into a buffer."
> +  (slot-value (or table (vtable-current-table))
> +              '-buffer))
> +
> +(defun vtable-set-buffer (buffer &optional table)
> +  "Associate BUFFER with TABLE.
> +If TABLE is nil, use the table under point."
> +  (setf (slot-value (or table (vtable-current-table))
> +                    '-buffer)
> +        buffer))
> +
>  (defun vtable-current-table ()
>    "Return the table under point."
>    (get-text-property (point) 'vtable))
> @@ -285,63 +300,65 @@ vtable-update-object
>  compared with `equal'), signal an error.
>  
>  TABLE must be at point in the current buffer."
> -  (unless old-object
> -    (setq old-object object))
> -  (let* ((objects (vtable-objects table))
> -         (inhibit-read-only t))
> -    ;; First replace the object in the object storage.
> -    (if (eq old-object (car objects))
> -        ;; It's at the head, so replace it there.
> -        (setf (vtable-objects table)
> -              (cons object (cdr objects)))
> -      ;; Otherwise splice into the list.
> -      (while (and (cdr objects)
> -                  (not (eq (cadr objects) old-object)))
> -        (setq objects (cdr objects)))
> -      (unless (cdr objects)
> -        (error "Can't find the old object"))
> -      (setcar (cdr objects) object))
> -    ;; Then update the rendered vtable in the current buffer.
> -    (if-let* ((cache (vtable--current-cache))
> -             (line-number (seq-position (vtable--cache-lines cache)
> -                                        old-object
> -                                        (lambda (a b)
> -                                          (equal (car a) b))))
> -             (line (elt (vtable--cache-lines cache) line-number)))
> -        (progn
> -          (setcar line object)
> -          (setcdr line (vtable--compute-cached-line table object))
> -          ;; ... and redisplay the line in question.
> -          (save-excursion
> -            (vtable-goto-object old-object)
> -            (let ((keymap (get-text-property (point) 'keymap))
> -                  (start (point)))
> -              (delete-line)
> -              (vtable--insert-line table line line-number
> -                                   (vtable--cache-widths cache)
> -                                   (vtable--spacer table))
> -              (add-text-properties start (point) (list 'keymap keymap
> -                                                       'vtable table
> -                                                       'vtable-cache cache))))
> -          ;; We may have inserted a non-numerical value into a previously
> -          ;; all-numerical table, so recompute.
> -          (vtable--recompute-numerical table (cdr line)))
> -      (error "Can't find cached object in vtable"))))
> +  (with-current-buffer (vtable-buffer table)
> +    (unless old-object
> +      (setq old-object object))
> +    (let* ((objects (vtable-objects table))
> +           (inhibit-read-only t))
> +      ;; First replace the object in the object storage.
> +      (if (eq old-object (car objects))
> +          ;; It's at the head, so replace it there.
> +          (setf (vtable-objects table)
> +                (cons object (cdr objects)))
> +        ;; Otherwise splice into the list.
> +        (while (and (cdr objects)
> +                    (not (eq (cadr objects) old-object)))
> +          (setq objects (cdr objects)))
> +        (unless (cdr objects)
> +          (error "Can't find the old object"))
> +        (setcar (cdr objects) object))
> +      ;; Then update the rendered vtable in the current buffer.
> +      (if-let* ((cache (vtable--current-cache))
> +                (line-number (seq-position (vtable--cache-lines cache)
> +                                           old-object
> +                                           (lambda (a b)
> +                                             (equal (car a) b))))
> +                (line (elt (vtable--cache-lines cache) line-number)))
> +          (progn
> +            (setcar line object)
> +            (setcdr line (vtable--compute-cached-line table object))
> +            ;; ... and redisplay the line in question.
> +            (save-excursion
> +              (vtable-goto-object old-object)
> +              (let ((keymap (get-text-property (point) 'keymap))
> +                    (start (point)))
> +                (delete-line)
> +                (vtable--insert-line table line line-number
> +                                     (vtable--cache-widths cache)
> +                                     (vtable--spacer table))
> +                (add-text-properties start (point) (list 'keymap keymap
> +                                                         'vtable table
> +                                                         'vtable-cache cache))))
> +            ;; We may have inserted a non-numerical value into a previously
> +            ;; all-numerical table, so recompute.
> +            (vtable--recompute-numerical table (cdr line)))
> +        (error "Can't find cached object in vtable")))))
>  
>  (defun vtable-remove-object (table object)
>    "Remove OBJECT from TABLE.
>  This will also remove the displayed line."
> -  ;; First remove from the objects.
> -  (setf (vtable-objects table) (delq object (vtable-objects table)))
> -  ;; Then adjust the cache and display.
> -  (save-excursion
> -    (vtable-goto-table table)
> -    (let ((cache (vtable--current-cache))
> -          (inhibit-read-only t))
> -      (setcar cache (delq (assq object (vtable--cache-lines cache))
> -                          (vtable--cache-lines cache)))
> -      (when (vtable-goto-object object)
> -        (delete-line)))))
> +  (with-current-buffer (vtable-buffer table)
> +    ;; First remove from the objects.
> +    (setf (vtable-objects table) (delq object (vtable-objects table)))
> +    ;; Then adjust the cache and display.
> +    (save-excursion
> +      (vtable-goto-table table)
> +      (let ((cache (vtable--current-cache))
> +            (inhibit-read-only t))
> +        (setcar cache (delq (assq object (vtable--cache-lines cache))
> +                            (vtable--cache-lines cache)))
> +        (when (vtable-goto-object object)
> +          (delete-line))))))
>  
>  ;; FIXME: The fact that the `location' argument of
>  ;; `vtable-insert-object' can be an integer and is then interpreted as
> @@ -363,91 +380,92 @@ vtable-insert-object
>  case.
>  
>  This also updates the displayed table."
> -  ;; If the vtable is empty, just add the object and regenerate the
> -  ;; table.
> -  (if (null (vtable-objects table))
> -      (progn
> -        (setf (vtable-objects table) (list object))
> -        (vtable--recompute-numerical table (vtable--compute-cached-line table object))
> -        (vtable-goto-table table)
> -        (vtable-revert-command))
> -    ;; First insert into the objects.
> -    (let ((pos (if location
> -                   (if (integerp location)
> -                       (prog1
> -                           (nthcdr location (vtable-objects table))
> -                         ;; Do not prepend if index is too large:
> -                         (setq before nil))
> -                     (or (memq location (vtable-objects table))
> -                         ;; Prepend if `location' is not found and
> -                         ;; `before' is non-nil:
> -                         (and before (vtable-objects table))))
> -                 ;; If `location' is nil and `before' is non-nil, we
> -                 ;; prepend the new object.
> -                 (if before (vtable-objects table)))))
> -      (if (or before  ; If `before' is non-nil, `pos' should be, as well.
> -              (and pos (integerp location)))
> -          ;; Add the new object before.
> -          (let ((old-object (car pos)))
> -            (setcar pos object)
> -            (setcdr pos (cons old-object (cdr pos))))
> -        ;; Otherwise, add the object after.
> -        (if pos
> -            ;; Splice the object into the list.
> -            (setcdr pos (cons object (cdr pos)))
> -          ;; Otherwise, append the object.
> -          (nconc (vtable-objects table) (list object)))))
> -    ;; Then adjust the cache and display.
> -    (save-excursion
> -      (vtable-goto-table table)
> -      (let* ((cache (vtable--current-cache))
> -             (inhibit-read-only t)
> -             (keymap (get-text-property (point) 'keymap))
> -             (ellipsis (if (vtable-ellipsis table)
> -                           (propertize (truncate-string-ellipsis)
> -                                       'face (vtable-face table))
> -                         ""))
> -             (ellipsis-width (string-pixel-width ellipsis))
> -             (lines (vtable--cache-lines cache))
> -             (elem (if location  ; This binding mirrors the binding of `pos' above.
> -                       (if (integerp location)
> -                           (nth location lines)
> -                         (or (assq location lines)
> -                             (and before (car lines))))
> -                     (if before (car lines))))
> -             (pos (memq elem lines))
> -             (line (cons object (vtable--compute-cached-line table object))))
> -        (if (or before
> +  (with-current-buffer (vtable-buffer table)
> +    ;; If the vtable is empty, just add the object and regenerate the
> +    ;; table.
> +    (if (null (vtable-objects table))
> +        (progn
> +          (setf (vtable-objects table) (list object))
> +          (vtable--recompute-numerical table (vtable--compute-cached-line table object))
> +          (vtable-goto-table table)
> +          (vtable-revert-command))
> +      ;; First insert into the objects.
> +      (let ((pos (if location
> +                     (if (integerp location)
> +                         (prog1
> +                             (nthcdr location (vtable-objects table))
> +                           ;; Do not prepend if index is too large:
> +                           (setq before nil))
> +                       (or (memq location (vtable-objects table))
> +                           ;; Prepend if `location' is not found and
> +                           ;; `before' is non-nil:
> +                           (and before (vtable-objects table))))
> +                   ;; If `location' is nil and `before' is non-nil, we
> +                   ;; prepend the new object.
> +                   (if before (vtable-objects table)))))
> +        (if (or before  ; If `before' is non-nil, `pos' should be, as well.
>                  (and pos (integerp location)))
> -            ;; Add the new object before:.
> -            (let ((old-line (car pos)))
> -              (setcar pos line)
> -              (setcdr pos (cons old-line (cdr pos)))
> -              (unless (vtable-goto-object (car elem))
> -                (vtable-beginning-of-table)))
> +            ;; Add the new object before.
> +            (let ((old-object (car pos)))
> +              (setcar pos object)
> +              (setcdr pos (cons old-object (cdr pos))))
>            ;; Otherwise, add the object after.
>            (if pos
>                ;; Splice the object into the list.
> -              (progn
> -                (setcdr pos (cons line (cdr pos)))
> -                (if (vtable-goto-object location)
> -                    (forward-line 1)  ; Insert *after*.
> -                  (vtable-end-of-table)))
> +              (setcdr pos (cons object (cdr pos)))
>              ;; Otherwise, append the object.
> -            (setcar cache (nconc lines (list line)))
> -            (vtable-end-of-table)))
> -        (let ((start (point)))
> -          ;; FIXME: We have to adjust colors in lines below this if we
> -          ;; have :row-colors.
> -          (vtable--insert-line table line 0
> -                               (vtable--cache-widths cache) (vtable--spacer table)
> -                               ellipsis ellipsis-width)
> -          (add-text-properties start (point) (list 'keymap keymap
> -                                                   'vtable table
> -                                                   'vtable-cache cache)))
> -        ;; We may have inserted a non-numerical value into a previously
> -        ;; all-numerical table, so recompute.
> -        (vtable--recompute-numerical table (cdr line))))))
> +            (nconc (vtable-objects table) (list object)))))
> +      ;; Then adjust the cache and display.
> +      (save-excursion
> +        (vtable-goto-table table)
> +        (let* ((cache (vtable--current-cache))
> +               (inhibit-read-only t)
> +               (keymap (get-text-property (point) 'keymap))
> +               (ellipsis (if (vtable-ellipsis table)
> +                             (propertize (truncate-string-ellipsis)
> +                                         'face (vtable-face table))
> +                           ""))
> +               (ellipsis-width (string-pixel-width ellipsis (current-buffer)))
> +               (lines (vtable--cache-lines cache))
> +               (elem (if location  ; This binding mirrors the binding of `pos' above.
> +                         (if (integerp location)
> +                             (nth location lines)
> +                           (or (assq location lines)
> +                               (and before (car lines))))
> +                       (if before (car lines))))
> +               (pos (memq elem lines))
> +               (line (cons object (vtable--compute-cached-line table object))))
> +          (if (or before
> +                  (and pos (integerp location)))
> +              ;; Add the new object before:.
> +              (let ((old-line (car pos)))
> +                (setcar pos line)
> +                (setcdr pos (cons old-line (cdr pos)))
> +                (unless (vtable-goto-object (car elem))
> +                  (vtable-beginning-of-table)))
> +            ;; Otherwise, add the object after.
> +            (if pos
> +                ;; Splice the object into the list.
> +                (progn
> +                  (setcdr pos (cons line (cdr pos)))
> +                  (if (vtable-goto-object location)
> +                      (forward-line 1)  ; Insert *after*.
> +                    (vtable-end-of-table)))
> +              ;; Otherwise, append the object.
> +              (setcar cache (nconc lines (list line)))
> +              (vtable-end-of-table)))
> +          (let ((start (point)))
> +            ;; FIXME: We have to adjust colors in lines below this if we
> +            ;; have :row-colors.
> +            (vtable--insert-line table line 0
> +                                 (vtable--cache-widths cache) (vtable--spacer table)
> +                                 ellipsis ellipsis-width)
> +            (add-text-properties start (point) (list 'keymap keymap
> +                                                     'vtable table
> +                                                     'vtable-cache cache)))
> +          ;; We may have inserted a non-numerical value into a previously
> +          ;; all-numerical table, so recompute.
> +          (vtable--recompute-numerical table (cdr line)))))))

Is it possible to avoid these massive diffs in vtable-update-object,
vtable-remove-object, and vtable-insert-object?  These will make the
history worse, and also makes it harder to review.  Maybe we only need
to wrap with-current-buffer around a smaller part of these functions?

FYI, in general, you should change try to make changes in a way which
minimizes the size of the diff.

>  (defun vtable-column (table index)
>    "Return the name of the INDEXth column in TABLE."
> @@ -520,14 +538,14 @@ vtable--cache-widths
>  (defun vtable--cache-lines (cache)
>    (car cache))
>  
> -(defun vtable-insert (table)
> +(defun vtable--insert (table)
>    (let* ((spacer (vtable--spacer table))
>           (start (point))
>           (ellipsis (if (vtable-ellipsis table)
>                         (propertize (truncate-string-ellipsis)
>                                     'face (vtable-face table))
>                       ""))
> -         (ellipsis-width (string-pixel-width ellipsis))
> +         (ellipsis-width (string-pixel-width ellipsis (current-buffer)))
>           ;; We maintain a cache per screen/window width, so that we render
>           ;; correctly if Emacs is open on two different screens (or the
>           ;; user resizes the frame).
> @@ -565,9 +583,24 @@ vtable-insert
>                                 'vtable-cache cache))
>      (goto-char start)))
>  
> +(defun vtable-insert (table)
> +  "Insert TABLE into the current buffer.
> +The current buffer will be recorded as TABLE's buffer.  If this is done
> +more than once, or if the table is attempted to be inserted more than
> +once into the same buffer, signal an error."
> +  (if-let* ((table-buffer (vtable-buffer table)))
> +      (if (eq table-buffer (current-buffer))
> +          (error "A vtable cannot be inserted more than once into a buffer")
> +        (error "A vtable cannot be inserted into more than one buffer")))
> +  (vtable-set-buffer (current-buffer) table)
> +  (let ((inhibit-read-only t)
> +        (inhibit-modification-hooks t))
> +    (vtable--insert table)))
> +
>  (defun vtable--insert-line (table line line-number widths spacer
>                                    &optional ellipsis ellipsis-width)
>    (let ((start (point))
> +        (buffer (vtable-buffer table))
>          (columns (vtable-columns table))
>          (column-colors
>           (and (vtable-column-colors table)
> @@ -607,16 +640,18 @@ vtable--insert-line
>                        (concat
>                         (vtable--limit-string
>                          pre-computed (- (elt widths index)
> -                                        (or ellipsis-width 0)))
> +                                        (or ellipsis-width 0))
> +                        buffer)
>                         ellipsis)
>                      pre-computed))
>                   ;; Recompute widths.
>                   (t
> -                  (if (> (string-pixel-width value) (elt widths index))
> +                  (if (> (string-pixel-width value buffer) (elt widths index))
>                        (concat
>                         (vtable--limit-string
>                          value (- (elt widths index)
> -                                 (or ellipsis-width 0)))
> +                                 (or ellipsis-width 0))
> +                        buffer)
>                         ellipsis)
>                      value))))
>                 (start (point))
> @@ -630,14 +665,15 @@ vtable--insert-line
>                            (list 'space
>                                  :width (list
>                                          (+ (- (elt widths index)
> -                                              (string-pixel-width displayed))
> +                                              (string-pixel-width
> +                                               displayed buffer))
>                                             (if last 0 spacer)))))))
>               ;; Align to the right.
>               (insert (propertize " " 'display
>                                   (list 'space
>                                         :width (list (- (elt widths index)
>                                                         (string-pixel-width
> -                                                        displayed)))))
> +                                                        displayed buffer)))))
>                       displayed)
>               (unless last
>                 (insert (propertize " " 'display
> @@ -718,6 +754,7 @@ vtable--indicator
>  (defun vtable--insert-header-line (table widths spacer)
>    ;; Insert the header directly into the buffer.
>    (let ((start (point))
> +        (buffer (vtable-buffer table))
>          (divider (vtable-divider table))
>          (cmap (define-keymap
>                  "<header-line> <drag-mouse-1>" #'vtable--drag-resize-column
> @@ -737,14 +774,15 @@ vtable--insert-header-line
>                       'keymap cmap))
>                (start (point))
>                (indicator (vtable--indicator table index))
> -              (indicator-width (string-pixel-width indicator))
> +              (indicator-width (string-pixel-width indicator buffer))
>                (last (= index (1- (length (vtable-columns table)))))
>                displayed)
>           (setq displayed
> -               (if (> (string-pixel-width name)
> +               (if (> (string-pixel-width name buffer)
>                        (- (elt widths index) indicator-width))
>                     (vtable--limit-string
> -                    name (- (elt widths index) indicator-width))
> +                    name (- (elt widths index) indicator-width)
> +                    buffer)
>                   name))
>           (let* ((indicator-lead-width
>                   ;; We want the indicator to not be quite flush right.
> @@ -753,7 +791,7 @@ vtable--insert-header-line
>                                          indicator-lead-width))
>                  (fill-width
>                   (+ (- (elt widths index)
> -                       (string-pixel-width displayed)
> +                       (string-pixel-width displayed buffer)
>                         indicator-width
>                         indicator-lead-width)
>                      (if last 0 spacer))))
> @@ -771,7 +809,8 @@ vtable--insert-header-line
>               ;; This is the final column, and we have a sorting
>               ;; indicator, and the table is too wide for the window.
>               (let* ((pre-indicator (string-pixel-width
> -                                    (buffer-substring (point-min) (point))))
> +                                    (buffer-substring (point-min) (point))
> +                                    buffer))
>                      (pre-fill
>                       (- (window-width nil t)
>                          pre-indicator
> @@ -850,14 +889,16 @@ vtable--set-header-line
>             (buffer-substring (point-min) (1- (point-max))))))
>    (vtable-header-mode 1))
>  
> -(defun vtable--limit-string (string pixels)
> +
> +(defun vtable--limit-string (string pixels buffer)
>    (while (and (length> string 0)
> -              (> (string-pixel-width string) pixels))
> +              (> (string-pixel-width string buffer) pixels))
>      (setq string (substring string 0 (1- (length string)))))
>    string)
>  
>  (defun vtable--char-width (table)
> -  (string-pixel-width (propertize "x" 'face (vtable-face table))))
> +  (string-pixel-width (propertize "x" 'face (vtable-face table))
> +                      (vtable-buffer table)))
>  
>  (defun vtable--compute-width (table spec)
>    (cond
> @@ -967,20 +1008,24 @@ vtable--make-keymap
>            (vtable-keymap table))
>        map)))
>  
> -(defun vtable-revert ()
> -  "Regenerate the table under point."
> -  (let ((table (vtable-current-table))
> -        (object (vtable-current-object))
> -        (column (vtable-current-column))
> -        (inhibit-read-only t))
> -    (unless table
> -      (user-error "No table under point"))
> -    (delete-region (vtable-beginning-of-table) (vtable-end-of-table))
> -    (vtable-insert table)
> -    (when object
> -      (vtable-goto-object object))
> -    (when column
> -      (vtable-goto-column column))))
> +(defun vtable-revert (&optional table)
> +  "Regenerate the table under point.
> +If TABLE is nil, use the table under point."

This repeats itself.  It should say "Regenerate TABLE, defaulting to the table under point."

> +  (setq table (or table (vtable-current-table)))
> +  (unless table
> +    (user-error "No table found"))

I don't think this error message should change.

> +  (with-current-buffer (vtable-buffer table)
> +    (let ((object (vtable-current-object))
> +          (column (vtable-current-column))

So we still implicitly depend on the value of point in the buffer?  How
does this interact with window-point?  This seems like a potential
foot-gun.

> +          (inhibit-read-only t))
> +      (unless table
> +        (user-error "No table under point"))
> +      (delete-region (vtable-beginning-of-table) (vtable-end-of-table))
> +      (vtable--insert table)
> +      (when object
> +        (vtable-goto-object object))
> +      (when column
> +        (vtable-goto-column column)))))
>
>  ;;; Commands.
>  
> @@ -1013,7 +1058,7 @@ vtable--alter-column-width
>      ;; Store the width so it'll be respected on a revert.
>      (setf (vtable-column-width (elt (vtable-columns table) column))
>            (format "%dpx" (aref widths column)))
> -    (vtable-revert)))
> +    (vtable-revert table)))
>  
>  (defun vtable-widen-current-column (&optional n)
>    "Widen the current column by N characters.
> @@ -1038,14 +1083,16 @@ vtable-next-column
>       (min (1- (length (vtable--cache-widths (vtable--current-cache))))
>            (1+ (vtable-current-column))))))
>  
> -(defun vtable-revert-command ()
> +(defun vtable-revert-command (&optional table)
>    "Re-query data and regenerate the table under point."
>    (interactive)
> -  (let ((table (vtable-current-table)))
> -    (when (vtable-objects-function table)
> -      (setf (vtable-objects table) (funcall (vtable-objects-function table))))
> -    (vtable--clear-cache table))
> -  (vtable-revert))
> +  (setq table (or table (vtable-current-table)))
> +  (unless table
> +    (user-error "No table found"))
> +  (when (vtable-objects-function table)
> +    (setf (vtable-objects table) (funcall (vtable-objects-function table))))
> +  (vtable--clear-cache table)
> +  (vtable-revert table))
>  
>  (defun vtable-sort-by-current-column ()
>    "Sort the table under point by the column under point."
> @@ -1067,8 +1114,8 @@ vtable-sort-by-current-column
>                                    (if (eq (cdr last) 'ascend)
>                                        'descend
>                                      'ascend)
> -                                'ascend))))))
> -  (vtable-revert))
> +                                'ascend)))))
> +    (vtable-revert table)))
>  
>  (defun vtable-header-line-sort (e)
>    "Sort a vtable from the header line."
> diff --git a/test/lisp/emacs-lisp/vtable-tests.el b/test/lisp/emacs-lisp/vtable-tests.el
> index 74fb8cc8139..83f826ea353 100644
> --- a/test/lisp/emacs-lisp/vtable-tests.el
> +++ b/test/lisp/emacs-lisp/vtable-tests.el
> @@ -27,16 +27,19 @@
>  (require 'ert)
>  (require 'ert-x)
>  
> -(ert-deftest test-vstable-compute-columns ()
> +(defun vtable-tests--make-no-header-2-object-table ()
> +  (make-vtable :columns '("a" "b" "c")
> +               :objects '(("foo" 1 2)
> +                          ("bar" 3 :zot))
> +               :insert nil))
> +
> +(ert-deftest test-vtable-compute-columns ()
>    (should
>     (equal (mapcar
>             (lambda (column)
>               (vtable-column-align column))
>             (vtable--compute-columns
> -            (make-vtable :columns '("a" "b" "c")
> -                         :objects '(("foo" 1 2)
> -                                    ("bar" 3 :zot))
> -                         :insert nil)))
> +            (vtable-tests--make-no-header-2-object-table)))
>            '(left right left))))
>  
>  (ert-deftest test-vtable-insert-object ()
> @@ -69,4 +72,69 @@ test-vtable-insert-object
>                (mapcar #'cadr (vtable-objects table))))
>            (number-sequence 0 11))))
>  
> +(ert-deftest test-vtable-unique-buffer ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      ;; This will run but fail on Emacs pre 31 vtable.
> +      (should-error (vtable-insert table))
> +      ;; This will run only on Emacs 31+ vtable.
> +      (when (> emacs-major-version 30)
> +        (should-error (vtable-set-buffer table (current-buffer)))))))
> +
> +(ert-deftest test-vtable-non-current-buffer-insert-object ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table))
> +        (obj '("baz" 4 5)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      (should (= (count-lines (point-min) (point-max)) 2))
> +      (with-temp-buffer
> +        (vtable-insert-object table obj))
> +      (should (= (count-lines (point-min) (point-max)) 3)))))
> +
> +(ert-deftest test-vtable-non-current-buffer-remove-object ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table))
> +        (obj '("baz" 4 5)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      (vtable-insert-object table obj)
> +      (should (= (count-lines (point-min) (point-max)) 3))
> +      (with-temp-buffer
> +        (vtable-remove-object table obj))
> +      (should (= (count-lines (point-min) (point-max)) 2)))))
> +
> +(ert-deftest test-vtable-non-current-buffer-update-object ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table))
> +        (obj '("baz" 4 5))
> +        (obj-2 '("qux" 6 7)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      (vtable-insert-object table obj)
> +      (should (= (count-lines (point-min) (point-max)) 3))
> +      (let ((line-2 (progn
> +                      (goto-char (point-min))
> +                      (forward-line 2)
> +                      (buffer-substring (point) (point-max)))))
> +        (with-temp-buffer
> +          (vtable-update-object table obj-2 obj))
> +        (let ((line-2-new (progn
> +                            (goto-char (point-min))
> +                            (forward-line 2)
> +                            (buffer-substring (point) (point-max)))))
> +          (should (= (count-lines (point-min) (point-max)) 3))
> +          (should (not (string= line-2 line-2-new))))))))
> +
> +(ert-deftest test-vtable--limit-string-with-face-remapped-buffer ()
> +  (with-temp-buffer
> +    (let ((text (propertize "XXXXX"
> +                            'face 'variable-pitch)))
> +      (face-remap-add-relative 'default :height 1.5)
> +      (if (> emacs-major-version 30)
> +          (should (eq
> +                   2
> +                   (length (vtable--limit-string text 50 (current-buffer)))))
> +        (should (eq
> +                 2
> +                 (length (vtable--limit-string text 50))))))))
> +
>  ;;; vtable-tests.el ends here

Message #20 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot
Date: Wed, 10 Dec 2025 17:18:31 -0500

[Message part 1 (text/plain, inline)]

On Wed, Dec 10, 2025 at 1:54 PM Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stéphane Marks <shipmints <at> gmail.com> writes:
>
> > On Wed, Dec 10, 2025 at 10:12 AM Spencer Baugh <sbaugh <at> janestreet.com>
> wrote:
> >  Please add a test which passes with this patch and doesn't pass without
> >  this patch.
> >
> > Try the new tests in this updated patch.
>
> Thanks, these tests look good, this will help a lot.
>
> > From 6e33b8cbf2063e1e7689f231bb0c6c6450520bd3 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?St=C3=A9phane=20Marks?= <shipmints <at> gmail.com>
> > Date: Wed, 10 Dec 2025 09:20:02 -0500
> > Subject: [PATCH] Add vtable buffer slot
> >
> > This solves for background vtable mutations, i.e., updates
> > initiated from buffers other than the vtable buffer, and for
> > buffer-adjusted string-pixel-width computations.
> >
> > * lisp/emacs-lisp/vtable.el (vtable): New '-buffer' slot.
> > (vtable-buffer):
> > (vtable-set-buffer): New function.
> > (vtable-update-object):
> > (vtable-remove-object):
> > (vtable-insert-object): Wrap operation with the vtable buffer.
> > (vtable--insert): Split from old 'vtable-insert'.
> > (vtable-insert): Insert table and record the buffer.
> > (vtable--insert-line):
> > (vtable--insert-header-line): Use 'vtable-buffer' for pixel-width
> computation.
> > (vtable--limit-string):
> > (vtable--char-width): Pass buffer to 'string-pixel-width'.
> > (vtable-revert): New optional table argument.
> > (vtable--alter-column-width):
> > (vtable-revert-command):
> > (vtable-sort-by-current-column): Call 'vtable-revert' with the table.
> >
> > * test/lisp/emacs-lisp/vtable-tests.el
> > (vtable-tests--make-no-header-2-object-table): New helper
> > function.
> > (test-vstable-compute-columns): Correct typo in test name.  Use
> > new helper function.
> > (test-vtable-unique-buffer)
> > (test-vtable-non-current-buffer-insert-object)
> > (test-vtable-non-current-buffer-remove-object)
> > (test-vtable-non-current-buffer-update-object)
> > (test-vtable--limit-string-with-face-remapped-buffer): New test.
> > ---
> >  lisp/emacs-lisp/vtable.el            | 389 +++++++++++++++------------
> >  test/lisp/emacs-lisp/vtable-tests.el |  78 +++++-
> >  2 files changed, 291 insertions(+), 176 deletions(-)
> >
> > diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
> > index bcdd280fb92..fe4bae01077 100644
> > --- a/lisp/emacs-lisp/vtable.el
> > +++ b/lisp/emacs-lisp/vtable.el
> > @@ -68,6 +68,7 @@ vtable
> >     (column-colors :initarg :column-colors :accessor
> vtable-column-colors)
> >     (row-colors :initarg :row-colors :accessor vtable-row-colors)
> >     (-cached-colors :initform nil)
> > +   (-buffer :initform nil)
> >     (-cache :initform (make-hash-table :test #'equal))
> >     (-cached-keymap :initform nil)
> >     (-has-column-spec :initform nil))
> > @@ -221,6 +222,20 @@ vtable--face-color
> >
> >  ;;; Interface utility functions.
> >
> > +(defun vtable-buffer (&optional table)
> > +  "Return the buffer associated with TABLE.
> > +If TABLE is nil, use the table under point.  Return nil if the table has
> > +not been inserted into a buffer."
> > +  (slot-value (or table (vtable-current-table))
> > +              '-buffer))
> > +
> > +(defun vtable-set-buffer (buffer &optional table)
> > +  "Associate BUFFER with TABLE.
> > +If TABLE is nil, use the table under point."
> > +  (setf (slot-value (or table (vtable-current-table))
> > +                    '-buffer)
> > +        buffer))
> > +
> >  (defun vtable-current-table ()
> >    "Return the table under point."
> >    (get-text-property (point) 'vtable))
> > @@ -285,63 +300,65 @@ vtable-update-object
> >  compared with `equal'), signal an error.
> >
> >  TABLE must be at point in the current buffer."
> > -  (unless old-object
> > -    (setq old-object object))
> > -  (let* ((objects (vtable-objects table))
> > -         (inhibit-read-only t))
> > -    ;; First replace the object in the object storage.
> > -    (if (eq old-object (car objects))
> > -        ;; It's at the head, so replace it there.
> > -        (setf (vtable-objects table)
> > -              (cons object (cdr objects)))
> > -      ;; Otherwise splice into the list.
> > -      (while (and (cdr objects)
> > -                  (not (eq (cadr objects) old-object)))
> > -        (setq objects (cdr objects)))
> > -      (unless (cdr objects)
> > -        (error "Can't find the old object"))
> > -      (setcar (cdr objects) object))
> > -    ;; Then update the rendered vtable in the current buffer.
> > -    (if-let* ((cache (vtable--current-cache))
> > -             (line-number (seq-position (vtable--cache-lines cache)
> > -                                        old-object
> > -                                        (lambda (a b)
> > -                                          (equal (car a) b))))
> > -             (line (elt (vtable--cache-lines cache) line-number)))
> > -        (progn
> > -          (setcar line object)
> > -          (setcdr line (vtable--compute-cached-line table object))
> > -          ;; ... and redisplay the line in question.
> > -          (save-excursion
> > -            (vtable-goto-object old-object)
> > -            (let ((keymap (get-text-property (point) 'keymap))
> > -                  (start (point)))
> > -              (delete-line)
> > -              (vtable--insert-line table line line-number
> > -                                   (vtable--cache-widths cache)
> > -                                   (vtable--spacer table))
> > -              (add-text-properties start (point) (list 'keymap keymap
> > -                                                       'vtable table
> > -                                                       'vtable-cache
> cache))))
> > -          ;; We may have inserted a non-numerical value into a
> previously
> > -          ;; all-numerical table, so recompute.
> > -          (vtable--recompute-numerical table (cdr line)))
> > -      (error "Can't find cached object in vtable"))))
> > +  (with-current-buffer (vtable-buffer table)
> > +    (unless old-object
> > +      (setq old-object object))
> > +    (let* ((objects (vtable-objects table))
> > +           (inhibit-read-only t))
> > +      ;; First replace the object in the object storage.
> > +      (if (eq old-object (car objects))
> > +          ;; It's at the head, so replace it there.
> > +          (setf (vtable-objects table)
> > +                (cons object (cdr objects)))
> > +        ;; Otherwise splice into the list.
> > +        (while (and (cdr objects)
> > +                    (not (eq (cadr objects) old-object)))
> > +          (setq objects (cdr objects)))
> > +        (unless (cdr objects)
> > +          (error "Can't find the old object"))
> > +        (setcar (cdr objects) object))
> > +      ;; Then update the rendered vtable in the current buffer.
> > +      (if-let* ((cache (vtable--current-cache))
> > +                (line-number (seq-position (vtable--cache-lines cache)
> > +                                           old-object
> > +                                           (lambda (a b)
> > +                                             (equal (car a) b))))
> > +                (line (elt (vtable--cache-lines cache) line-number)))
> > +          (progn
> > +            (setcar line object)
> > +            (setcdr line (vtable--compute-cached-line table object))
> > +            ;; ... and redisplay the line in question.
> > +            (save-excursion
> > +              (vtable-goto-object old-object)
> > +              (let ((keymap (get-text-property (point) 'keymap))
> > +                    (start (point)))
> > +                (delete-line)
> > +                (vtable--insert-line table line line-number
> > +                                     (vtable--cache-widths cache)
> > +                                     (vtable--spacer table))
> > +                (add-text-properties start (point) (list 'keymap keymap
> > +                                                         'vtable table
> > +                                                         'vtable-cache
> cache))))
> > +            ;; We may have inserted a non-numerical value into a
> previously
> > +            ;; all-numerical table, so recompute.
> > +            (vtable--recompute-numerical table (cdr line)))
> > +        (error "Can't find cached object in vtable")))))
> >
> >  (defun vtable-remove-object (table object)
> >    "Remove OBJECT from TABLE.
> >  This will also remove the displayed line."
> > -  ;; First remove from the objects.
> > -  (setf (vtable-objects table) (delq object (vtable-objects table)))
> > -  ;; Then adjust the cache and display.
> > -  (save-excursion
> > -    (vtable-goto-table table)
> > -    (let ((cache (vtable--current-cache))
> > -          (inhibit-read-only t))
> > -      (setcar cache (delq (assq object (vtable--cache-lines cache))
> > -                          (vtable--cache-lines cache)))
> > -      (when (vtable-goto-object object)
> > -        (delete-line)))))
> > +  (with-current-buffer (vtable-buffer table)
> > +    ;; First remove from the objects.
> > +    (setf (vtable-objects table) (delq object (vtable-objects table)))
> > +    ;; Then adjust the cache and display.
> > +    (save-excursion
> > +      (vtable-goto-table table)
> > +      (let ((cache (vtable--current-cache))
> > +            (inhibit-read-only t))
> > +        (setcar cache (delq (assq object (vtable--cache-lines cache))
> > +                            (vtable--cache-lines cache)))
> > +        (when (vtable-goto-object object)
> > +          (delete-line))))))
> >
> >  ;; FIXME: The fact that the `location' argument of
> >  ;; `vtable-insert-object' can be an integer and is then interpreted as
> > @@ -363,91 +380,92 @@ vtable-insert-object
> >  case.
> >
> >  This also updates the displayed table."
> > -  ;; If the vtable is empty, just add the object and regenerate the
> > -  ;; table.
> > -  (if (null (vtable-objects table))
> > -      (progn
> > -        (setf (vtable-objects table) (list object))
> > -        (vtable--recompute-numerical table (vtable--compute-cached-line
> table object))
> > -        (vtable-goto-table table)
> > -        (vtable-revert-command))
> > -    ;; First insert into the objects.
> > -    (let ((pos (if location
> > -                   (if (integerp location)
> > -                       (prog1
> > -                           (nthcdr location (vtable-objects table))
> > -                         ;; Do not prepend if index is too large:
> > -                         (setq before nil))
> > -                     (or (memq location (vtable-objects table))
> > -                         ;; Prepend if `location' is not found and
> > -                         ;; `before' is non-nil:
> > -                         (and before (vtable-objects table))))
> > -                 ;; If `location' is nil and `before' is non-nil, we
> > -                 ;; prepend the new object.
> > -                 (if before (vtable-objects table)))))
> > -      (if (or before  ; If `before' is non-nil, `pos' should be, as
> well.
> > -              (and pos (integerp location)))
> > -          ;; Add the new object before.
> > -          (let ((old-object (car pos)))
> > -            (setcar pos object)
> > -            (setcdr pos (cons old-object (cdr pos))))
> > -        ;; Otherwise, add the object after.
> > -        (if pos
> > -            ;; Splice the object into the list.
> > -            (setcdr pos (cons object (cdr pos)))
> > -          ;; Otherwise, append the object.
> > -          (nconc (vtable-objects table) (list object)))))
> > -    ;; Then adjust the cache and display.
> > -    (save-excursion
> > -      (vtable-goto-table table)
> > -      (let* ((cache (vtable--current-cache))
> > -             (inhibit-read-only t)
> > -             (keymap (get-text-property (point) 'keymap))
> > -             (ellipsis (if (vtable-ellipsis table)
> > -                           (propertize (truncate-string-ellipsis)
> > -                                       'face (vtable-face table))
> > -                         ""))
> > -             (ellipsis-width (string-pixel-width ellipsis))
> > -             (lines (vtable--cache-lines cache))
> > -             (elem (if location  ; This binding mirrors the binding of
> `pos' above.
> > -                       (if (integerp location)
> > -                           (nth location lines)
> > -                         (or (assq location lines)
> > -                             (and before (car lines))))
> > -                     (if before (car lines))))
> > -             (pos (memq elem lines))
> > -             (line (cons object (vtable--compute-cached-line table
> object))))
> > -        (if (or before
> > +  (with-current-buffer (vtable-buffer table)
> > +    ;; If the vtable is empty, just add the object and regenerate the
> > +    ;; table.
> > +    (if (null (vtable-objects table))
> > +        (progn
> > +          (setf (vtable-objects table) (list object))
> > +          (vtable--recompute-numerical table
> (vtable--compute-cached-line table object))
> > +          (vtable-goto-table table)
> > +          (vtable-revert-command))
> > +      ;; First insert into the objects.
> > +      (let ((pos (if location
> > +                     (if (integerp location)
> > +                         (prog1
> > +                             (nthcdr location (vtable-objects table))
> > +                           ;; Do not prepend if index is too large:
> > +                           (setq before nil))
> > +                       (or (memq location (vtable-objects table))
> > +                           ;; Prepend if `location' is not found and
> > +                           ;; `before' is non-nil:
> > +                           (and before (vtable-objects table))))
> > +                   ;; If `location' is nil and `before' is non-nil, we
> > +                   ;; prepend the new object.
> > +                   (if before (vtable-objects table)))))
> > +        (if (or before  ; If `before' is non-nil, `pos' should be, as
> well.
> >                  (and pos (integerp location)))
> > -            ;; Add the new object before:.
> > -            (let ((old-line (car pos)))
> > -              (setcar pos line)
> > -              (setcdr pos (cons old-line (cdr pos)))
> > -              (unless (vtable-goto-object (car elem))
> > -                (vtable-beginning-of-table)))
> > +            ;; Add the new object before.
> > +            (let ((old-object (car pos)))
> > +              (setcar pos object)
> > +              (setcdr pos (cons old-object (cdr pos))))
> >            ;; Otherwise, add the object after.
> >            (if pos
> >                ;; Splice the object into the list.
> > -              (progn
> > -                (setcdr pos (cons line (cdr pos)))
> > -                (if (vtable-goto-object location)
> > -                    (forward-line 1)  ; Insert *after*.
> > -                  (vtable-end-of-table)))
> > +              (setcdr pos (cons object (cdr pos)))
> >              ;; Otherwise, append the object.
> > -            (setcar cache (nconc lines (list line)))
> > -            (vtable-end-of-table)))
> > -        (let ((start (point)))
> > -          ;; FIXME: We have to adjust colors in lines below this if we
> > -          ;; have :row-colors.
> > -          (vtable--insert-line table line 0
> > -                               (vtable--cache-widths cache)
> (vtable--spacer table)
> > -                               ellipsis ellipsis-width)
> > -          (add-text-properties start (point) (list 'keymap keymap
> > -                                                   'vtable table
> > -                                                   'vtable-cache
> cache)))
> > -        ;; We may have inserted a non-numerical value into a previously
> > -        ;; all-numerical table, so recompute.
> > -        (vtable--recompute-numerical table (cdr line))))))
> > +            (nconc (vtable-objects table) (list object)))))
> > +      ;; Then adjust the cache and display.
> > +      (save-excursion
> > +        (vtable-goto-table table)
> > +        (let* ((cache (vtable--current-cache))
> > +               (inhibit-read-only t)
> > +               (keymap (get-text-property (point) 'keymap))
> > +               (ellipsis (if (vtable-ellipsis table)
> > +                             (propertize (truncate-string-ellipsis)
> > +                                         'face (vtable-face table))
> > +                           ""))
> > +               (ellipsis-width (string-pixel-width ellipsis
> (current-buffer)))
> > +               (lines (vtable--cache-lines cache))
> > +               (elem (if location  ; This binding mirrors the binding
> of `pos' above.
> > +                         (if (integerp location)
> > +                             (nth location lines)
> > +                           (or (assq location lines)
> > +                               (and before (car lines))))
> > +                       (if before (car lines))))
> > +               (pos (memq elem lines))
> > +               (line (cons object (vtable--compute-cached-line table
> object))))
> > +          (if (or before
> > +                  (and pos (integerp location)))
> > +              ;; Add the new object before:.
> > +              (let ((old-line (car pos)))
> > +                (setcar pos line)
> > +                (setcdr pos (cons old-line (cdr pos)))
> > +                (unless (vtable-goto-object (car elem))
> > +                  (vtable-beginning-of-table)))
> > +            ;; Otherwise, add the object after.
> > +            (if pos
> > +                ;; Splice the object into the list.
> > +                (progn
> > +                  (setcdr pos (cons line (cdr pos)))
> > +                  (if (vtable-goto-object location)
> > +                      (forward-line 1)  ; Insert *after*.
> > +                    (vtable-end-of-table)))
> > +              ;; Otherwise, append the object.
> > +              (setcar cache (nconc lines (list line)))
> > +              (vtable-end-of-table)))
> > +          (let ((start (point)))
> > +            ;; FIXME: We have to adjust colors in lines below this if we
> > +            ;; have :row-colors.
> > +            (vtable--insert-line table line 0
> > +                                 (vtable--cache-widths cache)
> (vtable--spacer table)
> > +                                 ellipsis ellipsis-width)
> > +            (add-text-properties start (point) (list 'keymap keymap
> > +                                                     'vtable table
> > +                                                     'vtable-cache
> cache)))
> > +          ;; We may have inserted a non-numerical value into a
> previously
> > +          ;; all-numerical table, so recompute.
> > +          (vtable--recompute-numerical table (cdr line)))))))
>
> Is it possible to avoid these massive diffs in vtable-update-object,
> vtable-remove-object, and vtable-insert-object?  These will make the
> history worse, and also makes it harder to review.  Maybe we only need
> to wrap with-current-buffer around a smaller part of these functions?
>
> FYI, in general, you should change try to make changes in a way which
> minimizes the size of the diff.
>
> >  (defun vtable-column (table index)
> >    "Return the name of the INDEXth column in TABLE."
> > @@ -520,14 +538,14 @@ vtable--cache-widths
> >  (defun vtable--cache-lines (cache)
> >    (car cache))
> >
> > -(defun vtable-insert (table)
> > +(defun vtable--insert (table)
> >    (let* ((spacer (vtable--spacer table))
> >           (start (point))
> >           (ellipsis (if (vtable-ellipsis table)
> >                         (propertize (truncate-string-ellipsis)
> >                                     'face (vtable-face table))
> >                       ""))
> > -         (ellipsis-width (string-pixel-width ellipsis))
> > +         (ellipsis-width (string-pixel-width ellipsis (current-buffer)))
> >           ;; We maintain a cache per screen/window width, so that we
> render
> >           ;; correctly if Emacs is open on two different screens (or the
> >           ;; user resizes the frame).
> > @@ -565,9 +583,24 @@ vtable-insert
> >                                 'vtable-cache cache))
> >      (goto-char start)))
> >
> > +(defun vtable-insert (table)
> > +  "Insert TABLE into the current buffer.
> > +The current buffer will be recorded as TABLE's buffer.  If this is done
> > +more than once, or if the table is attempted to be inserted more than
> > +once into the same buffer, signal an error."
> > +  (if-let* ((table-buffer (vtable-buffer table)))
> > +      (if (eq table-buffer (current-buffer))
> > +          (error "A vtable cannot be inserted more than once into a
> buffer")
> > +        (error "A vtable cannot be inserted into more than one
> buffer")))
> > +  (vtable-set-buffer (current-buffer) table)
> > +  (let ((inhibit-read-only t)
> > +        (inhibit-modification-hooks t))
> > +    (vtable--insert table)))
> > +
> >  (defun vtable--insert-line (table line line-number widths spacer
> >                                    &optional ellipsis ellipsis-width)
> >    (let ((start (point))
> > +        (buffer (vtable-buffer table))
> >          (columns (vtable-columns table))
> >          (column-colors
> >           (and (vtable-column-colors table)
> > @@ -607,16 +640,18 @@ vtable--insert-line
> >                        (concat
> >                         (vtable--limit-string
> >                          pre-computed (- (elt widths index)
> > -                                        (or ellipsis-width 0)))
> > +                                        (or ellipsis-width 0))
> > +                        buffer)
> >                         ellipsis)
> >                      pre-computed))
> >                   ;; Recompute widths.
> >                   (t
> > -                  (if (> (string-pixel-width value) (elt widths index))
> > +                  (if (> (string-pixel-width value buffer) (elt widths
> index))
> >                        (concat
> >                         (vtable--limit-string
> >                          value (- (elt widths index)
> > -                                 (or ellipsis-width 0)))
> > +                                 (or ellipsis-width 0))
> > +                        buffer)
> >                         ellipsis)
> >                      value))))
> >                 (start (point))
> > @@ -630,14 +665,15 @@ vtable--insert-line
> >                            (list 'space
> >                                  :width (list
> >                                          (+ (- (elt widths index)
> > -                                              (string-pixel-width
> displayed))
> > +                                              (string-pixel-width
> > +                                               displayed buffer))
> >                                             (if last 0 spacer)))))))
> >               ;; Align to the right.
> >               (insert (propertize " " 'display
> >                                   (list 'space
> >                                         :width (list (- (elt widths
> index)
> >
>  (string-pixel-width
> > -                                                        displayed)))))
> > +                                                        displayed
> buffer)))))
> >                       displayed)
> >               (unless last
> >                 (insert (propertize " " 'display
> > @@ -718,6 +754,7 @@ vtable--indicator
> >  (defun vtable--insert-header-line (table widths spacer)
> >    ;; Insert the header directly into the buffer.
> >    (let ((start (point))
> > +        (buffer (vtable-buffer table))
> >          (divider (vtable-divider table))
> >          (cmap (define-keymap
> >                  "<header-line> <drag-mouse-1>"
> #'vtable--drag-resize-column
> > @@ -737,14 +774,15 @@ vtable--insert-header-line
> >                       'keymap cmap))
> >                (start (point))
> >                (indicator (vtable--indicator table index))
> > -              (indicator-width (string-pixel-width indicator))
> > +              (indicator-width (string-pixel-width indicator buffer))
> >                (last (= index (1- (length (vtable-columns table)))))
> >                displayed)
> >           (setq displayed
> > -               (if (> (string-pixel-width name)
> > +               (if (> (string-pixel-width name buffer)
> >                        (- (elt widths index) indicator-width))
> >                     (vtable--limit-string
> > -                    name (- (elt widths index) indicator-width))
> > +                    name (- (elt widths index) indicator-width)
> > +                    buffer)
> >                   name))
> >           (let* ((indicator-lead-width
> >                   ;; We want the indicator to not be quite flush right.
> > @@ -753,7 +791,7 @@ vtable--insert-header-line
> >                                          indicator-lead-width))
> >                  (fill-width
> >                   (+ (- (elt widths index)
> > -                       (string-pixel-width displayed)
> > +                       (string-pixel-width displayed buffer)
> >                         indicator-width
> >                         indicator-lead-width)
> >                      (if last 0 spacer))))
> > @@ -771,7 +809,8 @@ vtable--insert-header-line
> >               ;; This is the final column, and we have a sorting
> >               ;; indicator, and the table is too wide for the window.
> >               (let* ((pre-indicator (string-pixel-width
> > -                                    (buffer-substring (point-min)
> (point))))
> > +                                    (buffer-substring (point-min)
> (point))
> > +                                    buffer))
> >                      (pre-fill
> >                       (- (window-width nil t)
> >                          pre-indicator
> > @@ -850,14 +889,16 @@ vtable--set-header-line
> >             (buffer-substring (point-min) (1- (point-max))))))
> >    (vtable-header-mode 1))
> >
> > -(defun vtable--limit-string (string pixels)
> > +
> > +(defun vtable--limit-string (string pixels buffer)
> >    (while (and (length> string 0)
> > -              (> (string-pixel-width string) pixels))
> > +              (> (string-pixel-width string buffer) pixels))
> >      (setq string (substring string 0 (1- (length string)))))
> >    string)
> >
> >  (defun vtable--char-width (table)
> > -  (string-pixel-width (propertize "x" 'face (vtable-face table))))
> > +  (string-pixel-width (propertize "x" 'face (vtable-face table))
> > +                      (vtable-buffer table)))
> >
> >  (defun vtable--compute-width (table spec)
> >    (cond
> > @@ -967,20 +1008,24 @@ vtable--make-keymap
> >            (vtable-keymap table))
> >        map)))
> >
> > -(defun vtable-revert ()
> > -  "Regenerate the table under point."
> > -  (let ((table (vtable-current-table))
> > -        (object (vtable-current-object))
> > -        (column (vtable-current-column))
> > -        (inhibit-read-only t))
> > -    (unless table
> > -      (user-error "No table under point"))
> > -    (delete-region (vtable-beginning-of-table) (vtable-end-of-table))
> > -    (vtable-insert table)
> > -    (when object
> > -      (vtable-goto-object object))
> > -    (when column
> > -      (vtable-goto-column column))))
> > +(defun vtable-revert (&optional table)
> > +  "Regenerate the table under point.
> > +If TABLE is nil, use the table under point."
>
> This repeats itself.  It should say "Regenerate TABLE, defaulting to the
> table under point."
>
> > +  (setq table (or table (vtable-current-table)))
> > +  (unless table
> > +    (user-error "No table found"))
>
> I don't think this error message should change.
>
> > +  (with-current-buffer (vtable-buffer table)
> > +    (let ((object (vtable-current-object))
> > +          (column (vtable-current-column))
>
> So we still implicitly depend on the value of point in the buffer?  How
> does this interact with window-point?  This seems like a potential
> foot-gun.
>
> > +          (inhibit-read-only t))
> > +      (unless table
> > +        (user-error "No table under point"))
> > +      (delete-region (vtable-beginning-of-table) (vtable-end-of-table))
> > +      (vtable--insert table)
> > +      (when object
> > +        (vtable-goto-object object))
> > +      (when column
> > +        (vtable-goto-column column)))))
> >
> >  ;;; Commands.
> >
> > @@ -1013,7 +1058,7 @@ vtable--alter-column-width
> >      ;; Store the width so it'll be respected on a revert.
> >      (setf (vtable-column-width (elt (vtable-columns table) column))
> >            (format "%dpx" (aref widths column)))
> > -    (vtable-revert)))
> > +    (vtable-revert table)))
> >
> >  (defun vtable-widen-current-column (&optional n)
> >    "Widen the current column by N characters.
> > @@ -1038,14 +1083,16 @@ vtable-next-column
> >       (min (1- (length (vtable--cache-widths (vtable--current-cache))))
> >            (1+ (vtable-current-column))))))
> >
> > -(defun vtable-revert-command ()
> > +(defun vtable-revert-command (&optional table)
> >    "Re-query data and regenerate the table under point."
> >    (interactive)
> > -  (let ((table (vtable-current-table)))
> > -    (when (vtable-objects-function table)
> > -      (setf (vtable-objects table) (funcall (vtable-objects-function
> table))))
> > -    (vtable--clear-cache table))
> > -  (vtable-revert))
> > +  (setq table (or table (vtable-current-table)))
> > +  (unless table
> > +    (user-error "No table found"))
> > +  (when (vtable-objects-function table)
> > +    (setf (vtable-objects table) (funcall (vtable-objects-function
> table))))
> > +  (vtable--clear-cache table)
> > +  (vtable-revert table))
> >
> >  (defun vtable-sort-by-current-column ()
> >    "Sort the table under point by the column under point."
> > @@ -1067,8 +1114,8 @@ vtable-sort-by-current-column
> >                                    (if (eq (cdr last) 'ascend)
> >                                        'descend
> >                                      'ascend)
> > -                                'ascend))))))
> > -  (vtable-revert))
> > +                                'ascend)))))
> > +    (vtable-revert table)))
> >
> >  (defun vtable-header-line-sort (e)
> >    "Sort a vtable from the header line."
> > diff --git a/test/lisp/emacs-lisp/vtable-tests.el
> b/test/lisp/emacs-lisp/vtable-tests.el
> > index 74fb8cc8139..83f826ea353 100644
> > --- a/test/lisp/emacs-lisp/vtable-tests.el
> > +++ b/test/lisp/emacs-lisp/vtable-tests.el
> > @@ -27,16 +27,19 @@
> >  (require 'ert)
> >  (require 'ert-x)
> >
> > -(ert-deftest test-vstable-compute-columns ()
> > +(defun vtable-tests--make-no-header-2-object-table ()
> > +  (make-vtable :columns '("a" "b" "c")
> > +               :objects '(("foo" 1 2)
> > +                          ("bar" 3 :zot))
> > +               :insert nil))
> > +
> > +(ert-deftest test-vtable-compute-columns ()
> >    (should
> >     (equal (mapcar
> >             (lambda (column)
> >               (vtable-column-align column))
> >             (vtable--compute-columns
> > -            (make-vtable :columns '("a" "b" "c")
> > -                         :objects '(("foo" 1 2)
> > -                                    ("bar" 3 :zot))
> > -                         :insert nil)))
> > +            (vtable-tests--make-no-header-2-object-table)))
> >            '(left right left))))
> >
> >  (ert-deftest test-vtable-insert-object ()
> > @@ -69,4 +72,69 @@ test-vtable-insert-object
> >                (mapcar #'cadr (vtable-objects table))))
> >            (number-sequence 0 11))))
> >
> > +(ert-deftest test-vtable-unique-buffer ()
> > +  (let ((table (vtable-tests--make-no-header-2-object-table)))
> > +    (with-temp-buffer
> > +      (vtable-insert table)
> > +      ;; This will run but fail on Emacs pre 31 vtable.
> > +      (should-error (vtable-insert table))
> > +      ;; This will run only on Emacs 31+ vtable.
> > +      (when (> emacs-major-version 30)
> > +        (should-error (vtable-set-buffer table (current-buffer)))))))
> > +
> > +(ert-deftest test-vtable-non-current-buffer-insert-object ()
> > +  (let ((table (vtable-tests--make-no-header-2-object-table))
> > +        (obj '("baz" 4 5)))
> > +    (with-temp-buffer
> > +      (vtable-insert table)
> > +      (should (= (count-lines (point-min) (point-max)) 2))
> > +      (with-temp-buffer
> > +        (vtable-insert-object table obj))
> > +      (should (= (count-lines (point-min) (point-max)) 3)))))
> > +
> > +(ert-deftest test-vtable-non-current-buffer-remove-object ()
> > +  (let ((table (vtable-tests--make-no-header-2-object-table))
> > +        (obj '("baz" 4 5)))
> > +    (with-temp-buffer
> > +      (vtable-insert table)
> > +      (vtable-insert-object table obj)
> > +      (should (= (count-lines (point-min) (point-max)) 3))
> > +      (with-temp-buffer
> > +        (vtable-remove-object table obj))
> > +      (should (= (count-lines (point-min) (point-max)) 2)))))
> > +
> > +(ert-deftest test-vtable-non-current-buffer-update-object ()
> > +  (let ((table (vtable-tests--make-no-header-2-object-table))
> > +        (obj '("baz" 4 5))
> > +        (obj-2 '("qux" 6 7)))
> > +    (with-temp-buffer
> > +      (vtable-insert table)
> > +      (vtable-insert-object table obj)
> > +      (should (= (count-lines (point-min) (point-max)) 3))
> > +      (let ((line-2 (progn
> > +                      (goto-char (point-min))
> > +                      (forward-line 2)
> > +                      (buffer-substring (point) (point-max)))))
> > +        (with-temp-buffer
> > +          (vtable-update-object table obj-2 obj))
> > +        (let ((line-2-new (progn
> > +                            (goto-char (point-min))
> > +                            (forward-line 2)
> > +                            (buffer-substring (point) (point-max)))))
> > +          (should (= (count-lines (point-min) (point-max)) 3))
> > +          (should (not (string= line-2 line-2-new))))))))
> > +
> > +(ert-deftest test-vtable--limit-string-with-face-remapped-buffer ()
> > +  (with-temp-buffer
> > +    (let ((text (propertize "XXXXX"
> > +                            'face 'variable-pitch)))
> > +      (face-remap-add-relative 'default :height 1.5)
> > +      (if (> emacs-major-version 30)
> > +          (should (eq
> > +                   2
> > +                   (length (vtable--limit-string text 50
> (current-buffer)))))
> > +        (should (eq
> > +                 2
> > +                 (length (vtable--limit-string text 50))))))))
> > +
> >  ;;; vtable-tests.el ends here
>

Thanks.  Revised patch, attached.  With regard to point vs. window-point,
the code seems no worse off than before?  There is a suite of additional
fixes coming that address keeping point stable during operations and we can
see if there are any ill effects when we get to testing those changes.

-Stéphane

[Message part 2 (text/html, inline)]

[0001-Add-vtable-buffer-slot.patch (application/octet-stream, attachment)]

Message #23 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot
Date: Wed, 10 Dec 2025 17:34:37 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:
> Thanks.  Revised patch, attached.

(BTW, you shouldn't include the entire previous patch as email context
if you're not replying to individual parts of it, it makes things a bit
harder to read)

> With regard to point vs. window-point, the code seems no worse off
> than before?  There is a suite of additional fixes coming that address
> keeping point stable during operations and we can see if there are any
> ill effects when we get to testing those changes.
>
> -Stéphane 
>
> From cf2260c055bed43be6e7208422f9713b2c083d86 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?St=C3=A9phane=20Marks?= <shipmints <at> gmail.com>
> Date: Wed, 10 Dec 2025 09:20:02 -0500
> Subject: [PATCH] Add vtable buffer slot
>
> This solves for background vtable mutations, i.e., updates
> initiated from buffers other than the vtable buffer, and for
> buffer-adjusted string-pixel-width computations.
>
> * lisp/emacs-lisp/vtable.el (vtable): New '-buffer' slot.
> (vtable-buffer):
> (vtable-set-buffer): New function.
> (vtable-update-object):
> (vtable-remove-object):
> (vtable-insert-object): Wrap operation with the vtable buffer.
> (vtable--insert): Split from old 'vtable-insert'.
> (vtable-insert): Insert table and record the buffer.
> (vtable--insert-line):
> (vtable--insert-header-line): Use 'vtable-buffer' for pixel-width computation.
> (vtable--limit-string):
> (vtable--char-width): Pass buffer to 'string-pixel-width'.
> (vtable-revert): New optional table argument.
> (vtable--alter-column-width):
> (vtable-revert-command):
> (vtable-sort-by-current-column): Call 'vtable-revert' with the table.
>
> * test/lisp/emacs-lisp/vtable-tests.el
> (vtable-tests--make-no-header-2-object-table): New helper
> function.
> (test-vstable-compute-columns): Correct typo in test name.  Use
> new helper function.
> (test-vtable-unique-buffer)
> (test-vtable-non-current-buffer-insert-object)
> (test-vtable-non-current-buffer-remove-object)
> (test-vtable-non-current-buffer-update-object)
> (test-vtable--limit-string-with-face-remapped-buffer): New test.
> ---
>  lisp/emacs-lisp/vtable.el            | 293 +++++++++++++++------------
>  test/lisp/emacs-lisp/vtable-tests.el |  78 ++++++-
>  2 files changed, 242 insertions(+), 129 deletions(-)
>
> diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
> index bcdd280fb92..94f3199f12b 100644
> --- a/lisp/emacs-lisp/vtable.el
> +++ b/lisp/emacs-lisp/vtable.el
> @@ -68,6 +68,7 @@ vtable
>     (column-colors :initarg :column-colors :accessor vtable-column-colors)
>     (row-colors :initarg :row-colors :accessor vtable-row-colors)
>     (-cached-colors :initform nil)
> +   (-buffer :initform nil)
>     (-cache :initform (make-hash-table :test #'equal))
>     (-cached-keymap :initform nil)
>     (-has-column-spec :initform nil))
> @@ -221,6 +222,20 @@ vtable--face-color
>  
>  ;;; Interface utility functions.
>  
> +(defun vtable-buffer (&optional table)
> +  "Return the buffer associated with TABLE.
> +If TABLE is nil, use the table under point.  Return nil if the table has
> +not been inserted into a buffer."
> +  (slot-value (or table (vtable-current-table))
> +              '-buffer))
> +
> +(defun vtable-set-buffer (buffer &optional table)
> +  "Associate BUFFER with TABLE.
> +If TABLE is nil, use the table under point."
> +  (setf (slot-value (or table (vtable-current-table))
> +                    '-buffer)
> +        buffer))
> +
>  (defun vtable-current-table ()
>    "Return the table under point."
>    (get-text-property (point) 'vtable))
> @@ -301,32 +316,33 @@ vtable-update-object
>        (unless (cdr objects)
>          (error "Can't find the old object"))
>        (setcar (cdr objects) object))
> -    ;; Then update the rendered vtable in the current buffer.
> -    (if-let* ((cache (vtable--current-cache))
> -             (line-number (seq-position (vtable--cache-lines cache)
> -                                        old-object
> -                                        (lambda (a b)
> -                                          (equal (car a) b))))
> -             (line (elt (vtable--cache-lines cache) line-number)))
> -        (progn
> -          (setcar line object)
> -          (setcdr line (vtable--compute-cached-line table object))
> -          ;; ... and redisplay the line in question.
> -          (save-excursion
> -            (vtable-goto-object old-object)
> -            (let ((keymap (get-text-property (point) 'keymap))
> -                  (start (point)))
> -              (delete-line)
> -              (vtable--insert-line table line line-number
> -                                   (vtable--cache-widths cache)
> -                                   (vtable--spacer table))
> -              (add-text-properties start (point) (list 'keymap keymap
> -                                                       'vtable table
> -                                                       'vtable-cache cache))))
> -          ;; We may have inserted a non-numerical value into a previously
> -          ;; all-numerical table, so recompute.
> -          (vtable--recompute-numerical table (cdr line)))
> -      (error "Can't find cached object in vtable"))))
> +    ;; Then update the rendered vtable in its buffer.
> +    (with-current-buffer (vtable-buffer table)
> +      (if-let* ((cache (vtable--current-cache))

Since we're restricting a vtable to only be inserted in a single buffer
now, we can make vtable--current-cache take the table as an explicit
argument, and just pull the cache out of there rather than the current
buffer.  Actually, we can stop setting the vtable-cache text property
entirely: it should only be pulled from the table.

> +                (line-number (seq-position (vtable--cache-lines cache)
> +                                           old-object
> +                                           (lambda (a b)
> +                                             (equal (car a) b))))
> +                (line (elt (vtable--cache-lines cache) line-number)))
> +          (progn

With the above-mentioned change to vtable--current-cache, you could have
the change just turn this progn into a with-current-buffer.

> +            (setcar line object)
> +            (setcdr line (vtable--compute-cached-line table object))
> +            ;; ... and redisplay the line in question.
> +            (save-excursion
> +              (vtable-goto-object old-object)
> +              (let ((keymap (get-text-property (point) 'keymap))
> +                    (start (point)))
> +                (delete-line)
> +                (vtable--insert-line table line line-number
> +                                     (vtable--cache-widths cache)
> +                                     (vtable--spacer table))
> +                (add-text-properties start (point) (list 'keymap keymap
> +                                                         'vtable table
> +                                                         'vtable-cache cache))))
> +            ;; We may have inserted a non-numerical value into a previously
> +            ;; all-numerical table, so recompute.
> +            (vtable--recompute-numerical table (cdr line)))
> +        (error "Can't find cached object in vtable")))))
>  
>  (defun vtable-remove-object (table object)
>    "Remove OBJECT from TABLE.
> @@ -334,14 +350,15 @@ vtable-remove-object
>    ;; First remove from the objects.
>    (setf (vtable-objects table) (delq object (vtable-objects table)))
>    ;; Then adjust the cache and display.
> -  (save-excursion
> -    (vtable-goto-table table)
> -    (let ((cache (vtable--current-cache))
> -          (inhibit-read-only t))
> -      (setcar cache (delq (assq object (vtable--cache-lines cache))
> -                          (vtable--cache-lines cache)))
> -      (when (vtable-goto-object object)
> -        (delete-line)))))
> +  (with-current-buffer (vtable-buffer table)
> +    (save-excursion
> +      (vtable-goto-table table)
> +      (let ((cache (vtable--current-cache))

Maybe we won't need this vtable-goto-table if we make
vtable--current-cache take the table?

> +            (inhibit-read-only t))
> +        (setcar cache (delq (assq object (vtable--cache-lines cache))
> +                            (vtable--cache-lines cache)))
> +        (when (vtable-goto-object object)
> +          (delete-line))))))
>  
>  ;; FIXME: The fact that the `location' argument of
>  ;; `vtable-insert-object' can be an integer and is then interpreted as
> @@ -369,8 +386,9 @@ vtable-insert-object
>        (progn
>          (setf (vtable-objects table) (list object))
>          (vtable--recompute-numerical table (vtable--compute-cached-line table object))
> -        (vtable-goto-table table)
> -        (vtable-revert-command))
> +        (with-current-buffer (vtable-buffer table)
> +          (vtable-goto-table table)
> +          (vtable-revert-command)))
>      ;; First insert into the objects.
>      (let ((pos (if location
>                     (if (integerp location)
> @@ -398,56 +416,57 @@ vtable-insert-object
>            ;; Otherwise, append the object.
>            (nconc (vtable-objects table) (list object)))))
>      ;; Then adjust the cache and display.
> -    (save-excursion
> -      (vtable-goto-table table)
> -      (let* ((cache (vtable--current-cache))
> -             (inhibit-read-only t)
> -             (keymap (get-text-property (point) 'keymap))
> -             (ellipsis (if (vtable-ellipsis table)
> -                           (propertize (truncate-string-ellipsis)
> -                                       'face (vtable-face table))
> -                         ""))
> -             (ellipsis-width (string-pixel-width ellipsis))
> -             (lines (vtable--cache-lines cache))
> -             (elem (if location  ; This binding mirrors the binding of `pos' above.
> -                       (if (integerp location)
> -                           (nth location lines)
> -                         (or (assq location lines)
> -                             (and before (car lines))))
> -                     (if before (car lines))))
> -             (pos (memq elem lines))
> -             (line (cons object (vtable--compute-cached-line table object))))
> -        (if (or before
> -                (and pos (integerp location)))
> -            ;; Add the new object before:.
> -            (let ((old-line (car pos)))
> -              (setcar pos line)
> -              (setcdr pos (cons old-line (cdr pos)))
> -              (unless (vtable-goto-object (car elem))
> -                (vtable-beginning-of-table)))
> -          ;; Otherwise, add the object after.
> -          (if pos
> -              ;; Splice the object into the list.
> -              (progn
> -                (setcdr pos (cons line (cdr pos)))
> -                (if (vtable-goto-object location)
> -                    (forward-line 1)  ; Insert *after*.
> -                  (vtable-end-of-table)))
> -            ;; Otherwise, append the object.
> -            (setcar cache (nconc lines (list line)))
> -            (vtable-end-of-table)))
> -        (let ((start (point)))
> -          ;; FIXME: We have to adjust colors in lines below this if we
> -          ;; have :row-colors.
> -          (vtable--insert-line table line 0
> -                               (vtable--cache-widths cache) (vtable--spacer table)
> -                               ellipsis ellipsis-width)
> -          (add-text-properties start (point) (list 'keymap keymap
> -                                                   'vtable table
> -                                                   'vtable-cache cache)))
> -        ;; We may have inserted a non-numerical value into a previously
> -        ;; all-numerical table, so recompute.
> -        (vtable--recompute-numerical table (cdr line))))))
> +    (with-current-buffer (vtable-buffer table)
> +      (save-excursion
> +        (vtable-goto-table table)
> +        (let* ((cache (vtable--current-cache))

> +               (inhibit-read-only t)
> +               (keymap (get-text-property (point) 'keymap))
> +               (ellipsis (if (vtable-ellipsis table)
> +                             (propertize (truncate-string-ellipsis)
> +                                         'face (vtable-face table))
> +                           ""))
> +               (ellipsis-width (string-pixel-width ellipsis (current-buffer)))
> +               (lines (vtable--cache-lines cache))
> +               (elem (if location  ; This binding mirrors the binding of `pos' above.
> +                         (if (integerp location)
> +                             (nth location lines)
> +                           (or (assq location lines)
> +                               (and before (car lines))))
> +                       (if before (car lines))))
> +               (pos (memq elem lines))
> +               (line (cons object (vtable--compute-cached-line table object))))

Hmm... I think this is the only thing in these let-bindings which
depends on the current buffer (besides vtable--current-cache which
should take the buffer as an argument).

If we made vtable--compute-cached-line take the buffer and explicitly
passed it to string-pixel-width in there, I think we'd be able to bind
with-current-buffer around much less code.

> +          (if (or before
> +                  (and pos (integerp location)))
> +              ;; Add the new object before:.
> +              (let ((old-line (car pos)))
> +                (setcar pos line)
> +                (setcdr pos (cons old-line (cdr pos)))
> +                (unless (vtable-goto-object (car elem))
> +                  (vtable-beginning-of-table)))
> +            ;; Otherwise, add the object after.
> +            (if pos
> +                ;; Splice the object into the list.
> +                (progn
> +                  (setcdr pos (cons line (cdr pos)))
> +                  (if (vtable-goto-object location)
> +                      (forward-line 1)  ; Insert *after*.
> +                    (vtable-end-of-table)))
> +              ;; Otherwise, append the object.
> +              (setcar cache (nconc lines (list line)))
> +              (vtable-end-of-table)))
> +          (let ((start (point)))
> +            ;; FIXME: We have to adjust colors in lines below this if we
> +            ;; have :row-colors.
> +            (vtable--insert-line table line 0
> +                                 (vtable--cache-widths cache) (vtable--spacer table)
> +                                 ellipsis ellipsis-width)
> +            (add-text-properties start (point) (list 'keymap keymap
> +                                                     'vtable table
> +                                                     'vtable-cache cache)))
> +          ;; We may have inserted a non-numerical value into a previously
> +          ;; all-numerical table, so recompute.
> +          (vtable--recompute-numerical table (cdr line)))))))

Message #26 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot
Date: Thu, 11 Dec 2025 11:59:43 -0500

[Message part 1 (text/plain, inline)]

Revised patch, attached.

On Wed, Dec 10, 2025 at 5:34 PM Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stéphane Marks <shipmints <at> gmail.com> writes:
>
> Since we're restricting a vtable to only be inserted in a single buffer
> now, we can make vtable--current-cache take the table as an explicit
> argument, and just pull the cache out of there rather than the current
> buffer.  Actually, we can stop setting the vtable-cache text property
> entirely: it should only be pulled from the table.
>

Good idea.

> +                (line-number (seq-position (vtable--cache-lines cache)
> > +                                           old-object
> > +                                           (lambda (a b)
> > +                                             (equal (car a) b))))
> > +                (line (elt (vtable--cache-lines cache) line-number)))
> > +          (progn
>
> With the above-mentioned change to vtable--current-cache, you could have
> the change just turn this progn into a with-current-buffer.
>

Good idea.

>  (defun vtable-remove-object (table object)
> >    "Remove OBJECT from TABLE.
> > @@ -334,14 +350,15 @@ vtable-remove-object
> >    ;; First remove from the objects.
> >    (setf (vtable-objects table) (delq object (vtable-objects table)))
> >    ;; Then adjust the cache and display.
> > -  (save-excursion
> > -    (vtable-goto-table table)
> > -    (let ((cache (vtable--current-cache))
> > -          (inhibit-read-only t))
> > -      (setcar cache (delq (assq object (vtable--cache-lines cache))
> > -                          (vtable--cache-lines cache)))
> > -      (when (vtable-goto-object object)
> > -        (delete-line)))))
> > +  (with-current-buffer (vtable-buffer table)
> > +    (save-excursion
> > +      (vtable-goto-table table)
> > +      (let ((cache (vtable--current-cache))
>
> Maybe we won't need this vtable-goto-table if we make
> vtable--current-cache take the table?
>

The goto-object calls require point being in the table.


> > +               (inhibit-read-only t)
> > +               (keymap (get-text-property (point) 'keymap))
> > +               (ellipsis (if (vtable-ellipsis table)
> > +                             (propertize (truncate-string-ellipsis)
> > +                                         'face (vtable-face table))
> > +                           ""))
> > +               (ellipsis-width (string-pixel-width ellipsis
> (current-buffer)))
> > +               (lines (vtable--cache-lines cache))
> > +               (elem (if location  ; This binding mirrors the binding
> of `pos' above.
> > +                         (if (integerp location)
> > +                             (nth location lines)
> > +                           (or (assq location lines)
> > +                               (and before (car lines))))
> > +                       (if before (car lines))))
> > +               (pos (memq elem lines))
> > +               (line (cons object (vtable--compute-cached-line table
> object))))
>
> Hmm... I think this is the only thing in these let-bindings which
> depends on the current buffer (besides vtable--current-cache which
> should take the buffer as an argument).
>

I reworked that code a bit to narrow the w-c-b scope.

If we made vtable--compute-cached-line take the buffer and explicitly
> passed it to string-pixel-width in there, I think we'd be able to bind
> with-current-buffer around much less code.
>

Not needed. I changed the string-pixel-width call to include vtable-buffer.

-Stéphane

[Message part 2 (text/html, inline)]

[0001-Add-vtable-buffer-slot.patch (application/octet-stream, attachment)]

Message #29 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer
 slot
Date: Thu, 11 Dec 2025 13:23:01 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:
>  >  (defun vtable-remove-object (table object)
>  >    "Remove OBJECT from TABLE.
>  > @@ -334,14 +350,15 @@ vtable-remove-object
>  >    ;; First remove from the objects.
>  >    (setf (vtable-objects table) (delq object (vtable-objects table)))
>  >    ;; Then adjust the cache and display.
>  > -  (save-excursion
>  > -    (vtable-goto-table table)
>  > -    (let ((cache (vtable--current-cache))
>  > -          (inhibit-read-only t))
>  > -      (setcar cache (delq (assq object (vtable--cache-lines cache))
>  > -                          (vtable--cache-lines cache)))
>  > -      (when (vtable-goto-object object)
>  > -        (delete-line)))))
>  > +  (with-current-buffer (vtable-buffer table)
>  > +    (save-excursion
>  > +      (vtable-goto-table table)
>  > +      (let ((cache (vtable--current-cache))
>
>  Maybe we won't need this vtable-goto-table if we make
>  vtable--current-cache take the table?
>
> The goto-object calls require point being in the table.
>  
>  > +               (inhibit-read-only t)
>  > +               (keymap (get-text-property (point) 'keymap))
>  > +               (ellipsis (if (vtable-ellipsis table)
>  > +                             (propertize (truncate-string-ellipsis)
>  > +                                         'face (vtable-face table))
>  > +                           ""))
>  > +               (ellipsis-width (string-pixel-width ellipsis (current-buffer)))
>  > +               (lines (vtable--cache-lines cache))
>  > +               (elem (if location  ; This binding mirrors the binding of `pos' above.
>  > +                         (if (integerp location)
>  > +                             (nth location lines)
>  > +                           (or (assq location lines)
>  > +                               (and before (car lines))))
>  > +                       (if before (car lines))))
>  > +               (pos (memq elem lines))
>  > +               (line (cons object (vtable--compute-cached-line table object))))
>
>  Hmm... I think this is the only thing in these let-bindings which
>  depends on the current buffer (besides vtable--current-cache which
>  should take the buffer as an argument).
>
> I reworked that code a bit to narrow the w-c-b scope.
>
>  If we made vtable--compute-cached-line take the buffer and explicitly
>  passed it to string-pixel-width in there, I think we'd be able to bind
>  with-current-buffer around much less code.
>
> Not needed. I changed the string-pixel-width call to include vtable-buffer.

Oh, smart, much better.

> -Stéphane 
>
> From 1c104f1b29aa9e396005ae73394a2f49104361c6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?St=C3=A9phane=20Marks?= <shipmints <at> gmail.com>
> Date: Wed, 10 Dec 2025 09:20:02 -0500
> Subject: [PATCH] Add vtable buffer slot
>
> This solves for background vtable mutations, i.e., updates
> initiated from buffers other than the vtable buffer, and for
> buffer-adjusted string-pixel-width computations.
>
> * lisp/emacs-lisp/vtable.el (vtable): New '-buffer' slot.
> (vtable-buffer):
> (vtable-set-buffer): New function.
> (vtable-update-object):
> (vtable-remove-object):
> (vtable-insert-object): Wrap operation with the vtable buffer.
> (vtable--insert): Split from old 'vtable-insert'.
> (vtable-insert): Insert table and record the buffer.
> (vtable--insert-line):
> (vtable--insert-header-line): Use 'vtable-buffer' for pixel-width computation.
> (vtable--limit-string):
> (vtable--char-width): Pass buffer to 'string-pixel-width'.
> (vtable-revert): New optional table argument.
> (vtable--alter-column-width):
> (vtable-revert-command):
> (vtable-sort-by-current-column): Call 'vtable-revert' with the table.
>
> * test/lisp/emacs-lisp/vtable-tests.el
> (vtable-tests--make-no-header-2-object-table): New helper
> function.
> (test-vstable-compute-columns): Correct typo in test name.  Use
> new helper function.
> (test-vtable-unique-buffer)
> (test-vtable-non-current-buffer-insert-object)
> (test-vtable-non-current-buffer-remove-object)
> (test-vtable-non-current-buffer-update-object)
> (test-vtable--limit-string-with-face-remapped-buffer): New test.
> ---
>  lisp/emacs-lisp/vtable.el            | 284 ++++++++++++++++-----------
>  test/lisp/emacs-lisp/vtable-tests.el |  78 +++++++-
>  2 files changed, 239 insertions(+), 123 deletions(-)
>
> diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
> index bcdd280fb92..f4d87581dfa 100644
> --- a/lisp/emacs-lisp/vtable.el
> +++ b/lisp/emacs-lisp/vtable.el
> @@ -68,7 +68,9 @@ vtable
>     (column-colors :initarg :column-colors :accessor vtable-column-colors)
>     (row-colors :initarg :row-colors :accessor vtable-row-colors)
>     (-cached-colors :initform nil)
> +   (-buffer :initform nil)
>     (-cache :initform (make-hash-table :test #'equal))
> +   (-current-cache :initform nil)

Nice.

One alternative is to save -current-cache-key instead and look it up in
-cache, but saving -current-cache is fine.

A thought for the future, not for this patch: We probably should have a
-current-window-width slot of the vtable, which is set whenever we
vtable-revert, and then use it in all the places that currently call
window-width.  That would make us more correct in a bunch of cases, I
think.  Then the cache key would be (cons (frame-terminal)
-current-window-width), so if we just saved -current-frame-terminal as
well, we would be able to compute (vtable--current-cache-key table), and
remove -current-cache/-current-cache-key.

>     (-cached-keymap :initform nil)
>     (-has-column-spec :initform nil))
>    "An object to hold the data for a table.")
> @@ -221,6 +223,20 @@ vtable--face-color
>  
>  ;;; Interface utility functions.
>  
> +(defun vtable-buffer (&optional table)

This should be a mandatory argument.

And if it's a mandatory argument then this can just be defined with a
:accessor keyword argument in the defclass.

Also while we're at it, I think the slot name should be just 'buffer not
'-buffer.

> +  "Return the buffer associated with TABLE.
> +If TABLE is nil, use the table under point.  Return nil if the table has
> +not been inserted into a buffer."
> +  (slot-value (or table (vtable-current-table))
> +              '-buffer))
> +
> +(defun vtable-set-buffer (buffer &optional table)

Again this should be a mandatory argument.

But also I think this then just becomes a trivial

(setf (vtable-buffer table) buffer)

and probably that doesn't need a helper function.

> +  "Associate BUFFER with TABLE.
> +If TABLE is nil, use the table under point."
> +  (setf (slot-value (or table (vtable-current-table))
> +                    '-buffer)
> +        buffer))
> +
>  (defun vtable-current-table ()
>    "Return the table under point."
>    (get-text-property (point) 'vtable))
> @@ -301,14 +317,14 @@ vtable-update-object
>        (unless (cdr objects)
>          (error "Can't find the old object"))
>        (setcar (cdr objects) object))
> -    ;; Then update the rendered vtable in the current buffer.
> -    (if-let* ((cache (vtable--current-cache))
> -             (line-number (seq-position (vtable--cache-lines cache)
> -                                        old-object
> -                                        (lambda (a b)
> -                                          (equal (car a) b))))
> -             (line (elt (vtable--cache-lines cache) line-number)))
> -        (progn
> +    ;; Then update the rendered vtable in its buffer.
> +    (if-let* ((cache (vtable--current-cache table))
> +              (line-number (seq-position (vtable--cache-lines cache)
> +                                         old-object
> +                                         (lambda (a b)
> +                                           (equal (car a) b))))
> +              (line (elt (vtable--cache-lines cache) line-number)))
> +        (with-current-buffer (vtable-buffer table)
>            (setcar line object)
>            (setcdr line (vtable--compute-cached-line table object))
>            ;; ... and redisplay the line in question.
> @@ -321,8 +337,7 @@ vtable-update-object
>                                     (vtable--cache-widths cache)
>                                     (vtable--spacer table))
>                (add-text-properties start (point) (list 'keymap keymap
> -                                                       'vtable table
> -                                                       'vtable-cache cache))))
> +                                                       'vtable table))))
>            ;; We may have inserted a non-numerical value into a previously
>            ;; all-numerical table, so recompute.
>            (vtable--recompute-numerical table (cdr line)))
> @@ -334,14 +349,15 @@ vtable-remove-object
>    ;; First remove from the objects.
>    (setf (vtable-objects table) (delq object (vtable-objects table)))
>    ;; Then adjust the cache and display.
> -  (save-excursion
> -    (vtable-goto-table table)
> -    (let ((cache (vtable--current-cache))
> -          (inhibit-read-only t))
> -      (setcar cache (delq (assq object (vtable--cache-lines cache))
> -                          (vtable--cache-lines cache)))
> -      (when (vtable-goto-object object)
> -        (delete-line)))))
> +  (with-current-buffer (vtable-buffer table)
> +    (save-excursion
> +      (vtable-goto-table table)
> +      (let ((cache (vtable--current-cache table))
> +            (inhibit-read-only t))
> +        (setcar cache (delq (assq object (vtable--cache-lines cache))
> +                            (vtable--cache-lines cache)))
> +        (when (vtable-goto-object object)
> +          (delete-line))))))
>  
>  ;; FIXME: The fact that the `location' argument of
>  ;; `vtable-insert-object' can be an integer and is then interpreted as
> @@ -369,8 +385,9 @@ vtable-insert-object
>        (progn
>          (setf (vtable-objects table) (list object))
>          (vtable--recompute-numerical table (vtable--compute-cached-line table object))
> -        (vtable-goto-table table)
> -        (vtable-revert-command))
> +        (with-current-buffer (vtable-buffer table)
> +          (vtable-goto-table table)
> +          (vtable-revert-command)))
>      ;; First insert into the objects.
>      (let ((pos (if location
>                     (if (integerp location)
> @@ -398,56 +415,57 @@ vtable-insert-object
>            ;; Otherwise, append the object.
>            (nconc (vtable-objects table) (list object)))))
>      ;; Then adjust the cache and display.
> -    (save-excursion
> -      (vtable-goto-table table)
> -      (let* ((cache (vtable--current-cache))
> -             (inhibit-read-only t)
> -             (keymap (get-text-property (point) 'keymap))
> -             (ellipsis (if (vtable-ellipsis table)
> -                           (propertize (truncate-string-ellipsis)
> -                                       'face (vtable-face table))
> -                         ""))
> -             (ellipsis-width (string-pixel-width ellipsis))
> -             (lines (vtable--cache-lines cache))
> -             (elem (if location  ; This binding mirrors the binding of `pos' above.
> -                       (if (integerp location)
> -                           (nth location lines)
> -                         (or (assq location lines)
> -                             (and before (car lines))))
> -                     (if before (car lines))))
> -             (pos (memq elem lines))
> -             (line (cons object (vtable--compute-cached-line table object))))
> -        (if (or before
> -                (and pos (integerp location)))
> -            ;; Add the new object before:.
> -            (let ((old-line (car pos)))
> -              (setcar pos line)
> -              (setcdr pos (cons old-line (cdr pos)))
> -              (unless (vtable-goto-object (car elem))
> -                (vtable-beginning-of-table)))
> -          ;; Otherwise, add the object after.
> -          (if pos
> -              ;; Splice the object into the list.
> -              (progn
> -                (setcdr pos (cons line (cdr pos)))
> -                (if (vtable-goto-object location)
> -                    (forward-line 1)  ; Insert *after*.
> -                  (vtable-end-of-table)))
> -            ;; Otherwise, append the object.
> -            (setcar cache (nconc lines (list line)))
> -            (vtable-end-of-table)))
> -        (let ((start (point)))
> -          ;; FIXME: We have to adjust colors in lines below this if we
> -          ;; have :row-colors.
> -          (vtable--insert-line table line 0
> -                               (vtable--cache-widths cache) (vtable--spacer table)
> -                               ellipsis ellipsis-width)
> -          (add-text-properties start (point) (list 'keymap keymap
> -                                                   'vtable table
> -                                                   'vtable-cache cache)))
> -        ;; We may have inserted a non-numerical value into a previously
> -        ;; all-numerical table, so recompute.
> -        (vtable--recompute-numerical table (cdr line))))))
> +    (let* ((cache (vtable--current-cache table))
> +           (inhibit-read-only t)
> +           (lines (vtable--cache-lines cache))
> +           (elem (if location  ; This binding mirrors the binding of `pos' above.
> +                     (if (integerp location)
> +                         (nth location lines)
> +                       (or (assq location lines)
> +                           (and before (car lines))))
> +                   (if before (car lines))))
> +           (pos (memq elem lines))
> +           (line (cons object (vtable--compute-cached-line table object))))
> +      (with-current-buffer (vtable-buffer table)
> +        (save-excursion
> +          (vtable-goto-table table)
> +          (let* ((ellipsis (if (vtable-ellipsis table)
> +                               (propertize (truncate-string-ellipsis)
> +                                           'face (vtable-face table))
> +                             ""))
> +                 (ellipsis-width (string-pixel-width ellipsis (current-buffer)))

Can we move these let-bindings out to the outer let-binding now?

> +                 (keymap (get-text-property (point) 'keymap)))

I guess we could get this from vtable-keymap now?  In which case it can
also move to the outer let-binding?

> +            (if (or before
> +                    (and pos (integerp location)))
> +                ;; Add the new object before:.
> +                (let ((old-line (car pos)))
> +                  (setcar pos line)
> +                  (setcdr pos (cons old-line (cdr pos)))
> +                  (unless (vtable-goto-object (car elem))
> +                    (vtable-beginning-of-table)))
> +              ;; Otherwise, add the object after.
> +              (if pos
> +                  ;; Splice the object into the list.
> +                  (progn
> +                    (setcdr pos (cons line (cdr pos)))
> +                    (if (vtable-goto-object location)
> +                        (forward-line 1)  ; Insert *after*.
> +                      (vtable-end-of-table)))
> +                ;; Otherwise, append the object.
> +                (setcar cache (nconc lines (list line)))
> +                (vtable-end-of-table)))
> +            (let ((start (point)))
> +              ;; FIXME: We have to adjust colors in lines below this if we
> +              ;; have :row-colors.
> +              (vtable--insert-line table line 0
> +                                   (vtable--cache-widths cache)
> +                                   (vtable--spacer table)
> +                                   ellipsis ellipsis-width)
> +              (add-text-properties start (point) (list 'keymap keymap
> +                                                       'vtable table)))
> +            ;; We may have inserted a non-numerical value into a previously
> +            ;; all-numerical table, so recompute.
> +            (vtable--recompute-numerical table (cdr line))))))))
>  
>  (defun vtable-column (table index)
>    "Return the name of the INDEXth column in TABLE."
> @@ -520,14 +538,14 @@ vtable--cache-widths
>  (defun vtable--cache-lines (cache)
>    (car cache))
>  
> -(defun vtable-insert (table)
> +(defun vtable--insert (table)
>    (let* ((spacer (vtable--spacer table))
>           (start (point))
>           (ellipsis (if (vtable-ellipsis table)
>                         (propertize (truncate-string-ellipsis)
>                                     'face (vtable-face table))
>                       ""))
> -         (ellipsis-width (string-pixel-width ellipsis))
> +         (ellipsis-width (string-pixel-width ellipsis (current-buffer)))

Should we use vtable-buffer rather than current-buffer here, I guess?

>           ;; We maintain a cache per screen/window width, so that we render
>           ;; correctly if Emacs is open on two different screens (or the
>           ;; user resizes the frame).
> @@ -549,8 +567,7 @@ vtable-insert
>          (add-text-properties start (point)
>                               (list 'keymap vtable-header-line-map
>                                     'rear-nonsticky t
> -                                   'vtable table
> -                                   'vtable-cache cache))
> +                                   'vtable table))
>          (setq start (point))))
>      (vtable--sort table cache)
>      ;; Insert the data.
> @@ -561,13 +578,28 @@ vtable-insert
>          (setq line-number (1+ line-number))))
>      (add-text-properties start (point)
>                           (list 'rear-nonsticky t
> -                               'vtable table
> -                               'vtable-cache cache))
> +                               'vtable table))
> +    (vtable--set-current-cache table cache)
>      (goto-char start)))
>  
> +(defun vtable-insert (table)
> +  "Insert TABLE into the current buffer.
> +The current buffer will be recorded as TABLE's buffer.  If this is done
> +more than once, or if the table is attempted to be inserted more than
> +once into the same buffer, signal an error."
> +  (if-let* ((table-buffer (vtable-buffer table)))
> +      (if (eq table-buffer (current-buffer))
> +          (error "A vtable cannot be inserted more than once into a buffer")

I don't think this should be an error.  It's reasonable for Lisp code to
clear a buffer and then want to reinsert a vtable into it.  If we're
inserting into the same buffer, there's no problem.

I guess your concern is inserting a vtable when it already is present.
I don't think there's a good way to prevent that right now,
unfortunately, without also breaking the legitimate use case of clearing
a buffer then reinserting the vtable.

> +        (error "A vtable cannot be inserted into more than one buffer")))

I agree that this should be an error, of course.

> +  (vtable-set-buffer (current-buffer) table)
> +  (let ((inhibit-read-only t)
> +        (inhibit-modification-hooks t))
> +    (vtable--insert table)))
> +
>  (defun vtable--insert-line (table line line-number widths spacer
>                                    &optional ellipsis ellipsis-width)
>    (let ((start (point))
> +        (buffer (vtable-buffer table))
>          (columns (vtable-columns table))
>          (column-colors
>           (and (vtable-column-colors table)
> @@ -607,16 +639,18 @@ vtable--insert-line
>                        (concat
>                         (vtable--limit-string
>                          pre-computed (- (elt widths index)
> -                                        (or ellipsis-width 0)))
> +                                        (or ellipsis-width 0))
> +                        buffer)
>                         ellipsis)
>                      pre-computed))
>                   ;; Recompute widths.
>                   (t
> -                  (if (> (string-pixel-width value) (elt widths index))
> +                  (if (> (string-pixel-width value buffer) (elt widths index))
>                        (concat
>                         (vtable--limit-string
>                          value (- (elt widths index)
> -                                 (or ellipsis-width 0)))
> +                                 (or ellipsis-width 0))
> +                        buffer)
>                         ellipsis)
>                      value))))
>                 (start (point))
> @@ -630,14 +664,15 @@ vtable--insert-line
>                            (list 'space
>                                  :width (list
>                                          (+ (- (elt widths index)
> -                                              (string-pixel-width displayed))
> +                                              (string-pixel-width
> +                                               displayed buffer))
>                                             (if last 0 spacer)))))))
>               ;; Align to the right.
>               (insert (propertize " " 'display
>                                   (list 'space
>                                         :width (list (- (elt widths index)
>                                                         (string-pixel-width
> -                                                        displayed)))))
> +                                                        displayed buffer)))))
>                       displayed)
>               (unless last
>                 (insert (propertize " " 'display
> @@ -664,15 +699,19 @@ vtable--insert-line
>  (defun vtable--cache-key ()
>    (cons (frame-terminal) (window-width)))
>  
> -(defun vtable--current-cache ()
> -  "Return the current cache for the table at point.
> +(defun vtable--current-cache (&optional table)
> +  "Return the current cache for TABLE or the table under point.
>  
>  In `vtable-insert', the lines and widths of the vtable text are computed
>  based on the current selected frame and window and stored in a cache.
>  Subsequent interaction with the text of the vtable should use that cache
>  via this function rather than by calling `vtable--cache-key' to look up
>  the cache."
> -  (get-text-property (point) 'vtable-cache))
> +  (slot-value (or table (vtable-current-table)) '-current-cache))

Can we make TABLE just be a mandatory argument?  It looks to me like
this would just require vtable-next-column and vtable-previous-column to
run (vtable--current-cache (vtable-current-table))

Then this function just becomes another :accessor.

> +
> +(defun vtable--set-current-cache (table cache)
> +  "Set the current cache for the table."
> +  (setf (slot-value table '-current-cache) cache))
>  
>  (defun vtable--clear-cache (table)
>    (setf (gethash (vtable--cache-key) (slot-value table '-cache)) nil))
> @@ -718,6 +757,7 @@ vtable--indicator
>  (defun vtable--insert-header-line (table widths spacer)
>    ;; Insert the header directly into the buffer.
>    (let ((start (point))
> +        (buffer (vtable-buffer table))
>          (divider (vtable-divider table))
>          (cmap (define-keymap
>                  "<header-line> <drag-mouse-1>" #'vtable--drag-resize-column
> @@ -737,14 +777,15 @@ vtable--insert-header-line
>                       'keymap cmap))
>                (start (point))
>                (indicator (vtable--indicator table index))
> -              (indicator-width (string-pixel-width indicator))
> +              (indicator-width (string-pixel-width indicator buffer))
>                (last (= index (1- (length (vtable-columns table)))))
>                displayed)
>           (setq displayed
> -               (if (> (string-pixel-width name)
> +               (if (> (string-pixel-width name buffer)
>                        (- (elt widths index) indicator-width))
>                     (vtable--limit-string
> -                    name (- (elt widths index) indicator-width))
> +                    name (- (elt widths index) indicator-width)
> +                    buffer)
>                   name))
>           (let* ((indicator-lead-width
>                   ;; We want the indicator to not be quite flush right.
> @@ -753,7 +794,7 @@ vtable--insert-header-line
>                                          indicator-lead-width))
>                  (fill-width
>                   (+ (- (elt widths index)
> -                       (string-pixel-width displayed)
> +                       (string-pixel-width displayed buffer)
>                         indicator-width
>                         indicator-lead-width)
>                      (if last 0 spacer))))
> @@ -771,7 +812,8 @@ vtable--insert-header-line
>               ;; This is the final column, and we have a sorting
>               ;; indicator, and the table is too wide for the window.
>               (let* ((pre-indicator (string-pixel-width
> -                                    (buffer-substring (point-min) (point))))
> +                                    (buffer-substring (point-min) (point))
> +                                    buffer))
>                      (pre-fill
>                       (- (window-width nil t)
>                          pre-indicator
> @@ -850,14 +892,16 @@ vtable--set-header-line
>             (buffer-substring (point-min) (1- (point-max))))))
>    (vtable-header-mode 1))
>  
> -(defun vtable--limit-string (string pixels)
> +
> +(defun vtable--limit-string (string pixels buffer)
>    (while (and (length> string 0)
> -              (> (string-pixel-width string) pixels))
> +              (> (string-pixel-width string buffer) pixels))
>      (setq string (substring string 0 (1- (length string)))))
>    string)
>  
>  (defun vtable--char-width (table)
> -  (string-pixel-width (propertize "x" 'face (vtable-face table))))
> +  (string-pixel-width (propertize "x" 'face (vtable-face table))
> +                      (vtable-buffer table)))
>  
>  (defun vtable--compute-width (table spec)
>    (cond
> @@ -936,7 +980,7 @@ vtable--compute-cached-line
>         ;; We stash the computed width and string here -- if there are
>         ;; no formatters/displayers, we'll be using the string, and
>         ;; then won't have to recreate it.
> -       (list value (string-pixel-width string) string)))
> +       (list value (string-pixel-width string (vtable-buffer table)) string)))
>     (vtable-columns table)))
>  
>  (defun vtable--make-keymap (table)
> @@ -967,20 +1011,21 @@ vtable--make-keymap
>            (vtable-keymap table))
>        map)))
>  
> -(defun vtable-revert ()
> -  "Regenerate the table under point."
> -  (let ((table (vtable-current-table))
> -        (object (vtable-current-object))
> -        (column (vtable-current-column))
> -        (inhibit-read-only t))
> -    (unless table
> -      (user-error "No table under point"))
> -    (delete-region (vtable-beginning-of-table) (vtable-end-of-table))
> -    (vtable-insert table)
> -    (when object
> -      (vtable-goto-object object))
> -    (when column
> -      (vtable-goto-column column))))
> +(defun vtable-revert (&optional table)
> +  "Regenerate TABLE, defaulting to the table under point."
> +  (setq table (or table (vtable-current-table)))
> +  (unless table
> +    (user-error "No table under point"))
> +  (with-current-buffer (vtable-buffer table)
> +    (let ((object (vtable-current-object))
> +          (column (vtable-current-column))
> +          (inhibit-read-only t))
> +      (delete-region (vtable-beginning-of-table) (vtable-end-of-table))
> +      (vtable--insert table)
> +      (when object
> +        (vtable-goto-object object))
> +      (when column
> +        (vtable-goto-column column)))))
>  
>  ;;; Commands.
>  
> @@ -1006,14 +1051,14 @@ vtable-narrow-current-column
>                                  (- (* (vtable--char-width table) (or n 1))))))
>  
>  (defun vtable--alter-column-width (table column delta)
> -  (let ((widths (vtable--cache-widths (vtable--current-cache))))
> +  (let ((widths (vtable--cache-widths (vtable--current-cache table))))
>      (setf (aref widths column)
>            (max (* (vtable--char-width table) 2)
>                 (+ (aref widths column) delta)))
>      ;; Store the width so it'll be respected on a revert.
>      (setf (vtable-column-width (elt (vtable-columns table) column))
>            (format "%dpx" (aref widths column)))
> -    (vtable-revert)))
> +    (vtable-revert table)))
>  
>  (defun vtable-widen-current-column (&optional n)
>    "Widen the current column by N characters.
> @@ -1028,7 +1073,8 @@ vtable-previous-column
>    (interactive)
>    (vtable-goto-column
>     (max 0 (1- (or (vtable-current-column)
> -                  (length (vtable--cache-widths (vtable--current-cache))))))))
> +                  (length (vtable--cache-widths
> +                           (vtable--current-cache))))))))
>  
>  (defun vtable-next-column ()
>    "Go to the next column."
> @@ -1038,14 +1084,16 @@ vtable-next-column
>       (min (1- (length (vtable--cache-widths (vtable--current-cache))))
>            (1+ (vtable-current-column))))))
>  
> -(defun vtable-revert-command ()
> +(defun vtable-revert-command (&optional table)
>    "Re-query data and regenerate the table under point."
>    (interactive)
> -  (let ((table (vtable-current-table)))
> -    (when (vtable-objects-function table)
> -      (setf (vtable-objects table) (funcall (vtable-objects-function table))))
> -    (vtable--clear-cache table))
> -  (vtable-revert))
> +  (setq table (or table (vtable-current-table)))
> +  (unless table
> +    (user-error "No table found"))
> +  (when (vtable-objects-function table)
> +    (setf (vtable-objects table) (funcall (vtable-objects-function table))))
> +  (vtable--clear-cache table)
> +  (vtable-revert table))
>  
>  (defun vtable-sort-by-current-column ()
>    "Sort the table under point by the column under point."
> @@ -1067,8 +1115,8 @@ vtable-sort-by-current-column
>                                    (if (eq (cdr last) 'ascend)
>                                        'descend
>                                      'ascend)
> -                                'ascend))))))
> -  (vtable-revert))
> +                                'ascend)))))
> +    (vtable-revert table)))
>  
>  (defun vtable-header-line-sort (e)
>    "Sort a vtable from the header line."
> diff --git a/test/lisp/emacs-lisp/vtable-tests.el b/test/lisp/emacs-lisp/vtable-tests.el
> index 74fb8cc8139..83f826ea353 100644
> --- a/test/lisp/emacs-lisp/vtable-tests.el
> +++ b/test/lisp/emacs-lisp/vtable-tests.el
> @@ -27,16 +27,19 @@
>  (require 'ert)
>  (require 'ert-x)
>  
> -(ert-deftest test-vstable-compute-columns ()
> +(defun vtable-tests--make-no-header-2-object-table ()
> +  (make-vtable :columns '("a" "b" "c")
> +               :objects '(("foo" 1 2)
> +                          ("bar" 3 :zot))
> +               :insert nil))
> +
> +(ert-deftest test-vtable-compute-columns ()
>    (should
>     (equal (mapcar
>             (lambda (column)
>               (vtable-column-align column))
>             (vtable--compute-columns
> -            (make-vtable :columns '("a" "b" "c")
> -                         :objects '(("foo" 1 2)
> -                                    ("bar" 3 :zot))
> -                         :insert nil)))
> +            (vtable-tests--make-no-header-2-object-table)))
>            '(left right left))))
>  
>  (ert-deftest test-vtable-insert-object ()
> @@ -69,4 +72,69 @@ test-vtable-insert-object
>                (mapcar #'cadr (vtable-objects table))))
>            (number-sequence 0 11))))
>  
> +(ert-deftest test-vtable-unique-buffer ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      ;; This will run but fail on Emacs pre 31 vtable.
> +      (should-error (vtable-insert table))
> +      ;; This will run only on Emacs 31+ vtable.
> +      (when (> emacs-major-version 30)
> +        (should-error (vtable-set-buffer table (current-buffer)))))))
> +
> +(ert-deftest test-vtable-non-current-buffer-insert-object ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table))
> +        (obj '("baz" 4 5)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      (should (= (count-lines (point-min) (point-max)) 2))
> +      (with-temp-buffer
> +        (vtable-insert-object table obj))
> +      (should (= (count-lines (point-min) (point-max)) 3)))))
> +
> +(ert-deftest test-vtable-non-current-buffer-remove-object ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table))
> +        (obj '("baz" 4 5)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      (vtable-insert-object table obj)
> +      (should (= (count-lines (point-min) (point-max)) 3))
> +      (with-temp-buffer
> +        (vtable-remove-object table obj))
> +      (should (= (count-lines (point-min) (point-max)) 2)))))
> +
> +(ert-deftest test-vtable-non-current-buffer-update-object ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table))
> +        (obj '("baz" 4 5))
> +        (obj-2 '("qux" 6 7)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      (vtable-insert-object table obj)
> +      (should (= (count-lines (point-min) (point-max)) 3))
> +      (let ((line-2 (progn
> +                      (goto-char (point-min))
> +                      (forward-line 2)
> +                      (buffer-substring (point) (point-max)))))
> +        (with-temp-buffer
> +          (vtable-update-object table obj-2 obj))
> +        (let ((line-2-new (progn
> +                            (goto-char (point-min))
> +                            (forward-line 2)
> +                            (buffer-substring (point) (point-max)))))
> +          (should (= (count-lines (point-min) (point-max)) 3))
> +          (should (not (string= line-2 line-2-new))))))))
> +
> +(ert-deftest test-vtable--limit-string-with-face-remapped-buffer ()
> +  (with-temp-buffer
> +    (let ((text (propertize "XXXXX"
> +                            'face 'variable-pitch)))
> +      (face-remap-add-relative 'default :height 1.5)
> +      (if (> emacs-major-version 30)
> +          (should (eq
> +                   2
> +                   (length (vtable--limit-string text 50 (current-buffer)))))
> +        (should (eq
> +                 2
> +                 (length (vtable--limit-string text 50))))))))
> +
>  ;;; vtable-tests.el ends here

Message #32 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Thu, 11 Dec 2025 14:31:26 -0500

[Message part 1 (text/plain, inline)]

On Thu, Dec 11, 2025 at 1:23 PM Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stéphane Marks <shipmints <at> gmail.com> writes:
>
> One alternative is to save -current-cache-key instead and look it up in
> -cache, but saving -current-cache is fine.
>
> A thought for the future, not for this patch: We probably should have a
> -current-window-width slot of the vtable, which is set whenever we
> vtable-revert, and then use it in all the places that currently call
> window-width.  That would make us more correct in a bunch of cases, I
> think.  Then the cache key would be (cons (frame-terminal)
> -current-window-width), so if we just saved -current-frame-terminal as
> well, we would be able to compute (vtable--current-cache-key table), and
> remove -current-cache/-current-cache-key.
>

We can refine these later, for sure.  It's an internal detail.

>     (-cached-keymap :initform nil)
> >     (-has-column-spec :initform nil))
> >    "An object to hold the data for a table.")
> > @@ -221,6 +223,20 @@ vtable--face-color
> >
> >  ;;; Interface utility functions.
> >
> > +(defun vtable-buffer (&optional table)
>
> This should be a mandatory argument.
>
> And if it's a mandatory argument then this can just be defined with a
> :accessor keyword argument in the defclass.

> +(defun vtable-set-buffer (buffer &optional table)

> But also I think this then just becomes a trivial
>
> (setf (vtable-buffer table) buffer)
>  Also while we're at it, I think the slot name should be just 'buffer not
> '-buffer.
>

Right, good, done.

<snip>

> +    (let* ((cache (vtable--current-cache table))
> > +           (inhibit-read-only t)
> > +           (lines (vtable--cache-lines cache))
> > +           (elem (if location  ; This binding mirrors the binding of
> `pos' above.
> > +                     (if (integerp location)
> > +                         (nth location lines)
> > +                       (or (assq location lines)
> > +                           (and before (car lines))))
> > +                   (if before (car lines))))
> > +           (pos (memq elem lines))
> > +           (line (cons object (vtable--compute-cached-line table
> object))))
> > +      (with-current-buffer (vtable-buffer table)
> > +        (save-excursion
> > +          (vtable-goto-table table)
> > +          (let* ((ellipsis (if (vtable-ellipsis table)
> > +                               (propertize (truncate-string-ellipsis)
> > +                                           'face (vtable-face table))
> > +                             ""))
> > +                 (ellipsis-width (string-pixel-width ellipsis
> (current-buffer)))
>
> Can we move these let-bindings out to the outer let-binding now?
>

I'd prefer if they stay close to where they're being used, at least here,
where the cache update is clearly separate from the buffer update.

> +                 (keymap (get-text-property (point) 'keymap)))
>
> I guess we could get this from vtable-keymap now?  In which case it can
> also move to the outer let-binding?
>

It would have to be from the -cached-keymap slot.  Let's leave the keymaps
as is for the time being.  We have patches to refine and expand their
functionality coming.

> -(defun vtable-insert (table)
> > +(defun vtable--insert (table)
> >    (let* ((spacer (vtable--spacer table))
> >           (start (point))
> >           (ellipsis (if (vtable-ellipsis table)
> >                         (propertize (truncate-string-ellipsis)
> >                                     'face (vtable-face table))
> >                       ""))
> > -         (ellipsis-width (string-pixel-width ellipsis))
> > +         (ellipsis-width (string-pixel-width ellipsis (current-buffer)))
>
> Should we use vtable-buffer rather than current-buffer here, I guess?
>

Easy enough.

>
> > +  (if-let* ((table-buffer (vtable-buffer table)))
> > +      (if (eq table-buffer (current-buffer))
> > +          (error "A vtable cannot be inserted more than once into a
> buffer")
>
> I don't think this should be an error.  It's reasonable for Lisp code to
> clear a buffer and then want to reinsert a vtable into it.  If we're
> inserting into the same buffer, there's no problem.
>
> I guess your concern is inserting a vtable when it already is present.
> I don't think there's a good way to prevent that right now,
> unfortunately, without also breaking the legitimate use case of clearing
> a buffer then reinserting the vtable.
>
> > +        (error "A vtable cannot be inserted into more than one
> buffer")))
>
> I agree that this should be an error, of course.
>

Duly refined and the docstring now reads "A table may be reinserted into
its own buffer, but insert only one instance per buffer.  This restriction
needs to be enforced by the caller."  We can build a mechanic later that
enforces this.

> +(defun vtable--current-cache (&optional table)
> > +  "Return the current cache for TABLE or the table under point.
> >
> >  In `vtable-insert', the lines and widths of the vtable text are computed
> >  based on the current selected frame and window and stored in a cache.
> >  Subsequent interaction with the text of the vtable should use that cache
> >  via this function rather than by calling `vtable--cache-key' to look up
> >  the cache."
> > -  (get-text-property (point) 'vtable-cache))
> > +  (slot-value (or table (vtable-current-table)) '-current-cache))
>
> Can we make TABLE just be a mandatory argument?  It looks to me like
> this would just require vtable-next-column and vtable-previous-column to
> run (vtable--current-cache (vtable-current-table))
>
> Then this function just becomes another :accessor.
>

Good, done.

> +(defun vtable--set-current-cache (table cache)
> > +  "Set the current cache for the table."
> > +  (setf (slot-value table '-current-cache) cache))
>

I've removed that function, too.

Fresh patch attached.

-Stéphane

[Message part 2 (text/html, inline)]

[0001-Add-vtable-buffer-slot.patch (application/octet-stream, attachment)]

Message #35 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer
 slot, [PATCH] Add vtable buffer slot
Date: Thu, 11 Dec 2025 14:46:10 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:

> On Thu, Dec 11, 2025 at 1:23 PM Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>  > +    (let* ((cache (vtable--current-cache table))
>  > +           (inhibit-read-only t)
>  > +           (lines (vtable--cache-lines cache))
>  > +           (elem (if location  ; This binding mirrors the binding of `pos' above.
>  > +                     (if (integerp location)
>  > +                         (nth location lines)
>  > +                       (or (assq location lines)
>  > +                           (and before (car lines))))
>  > +                   (if before (car lines))))
>  > +           (pos (memq elem lines))
>  > +           (line (cons object (vtable--compute-cached-line table object))))
>  > +      (with-current-buffer (vtable-buffer table)
>  > +        (save-excursion
>  > +          (vtable-goto-table table)
>  > +          (let* ((ellipsis (if (vtable-ellipsis table)
>  > +                               (propertize (truncate-string-ellipsis)
>  > +                                           'face (vtable-face table))
>  > +                             ""))
>  > +                 (ellipsis-width (string-pixel-width ellipsis (current-buffer)))
>
>  Can we move these let-bindings out to the outer let-binding now?
>
> I'd prefer if they stay close to where they're being used, at least here, where the cache update is clearly separate from the buffer
> update.

Good point.  Actually, so these bindings actually probably should be in
the same let-binding as (let ((start (point))) ...), a few lines later.
Since they're only used in that part of the code.

>  > + (keymap (get-text-property (point) 'keymap)))
>
>  I guess we could get this from vtable-keymap now?  In which case it can
>  also move to the outer let-binding?
>
> It would have to be from the -cached-keymap slot.  Let's leave the keymaps as is for the time being.  We have patches to refine
> and expand their functionality coming.

That's fair, but I think if we move these let-bindings down to the (let
((start (point)))) binding, that avoids reindenting the rest of this
function.  Which makes this diff much smaller, which is nice.

> From 4505c63efaa9ce124a57c79317533fa3f0298f2c Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?St=C3=A9phane=20Marks?= <shipmints <at> gmail.com>
> Date: Wed, 10 Dec 2025 09:20:02 -0500
> Subject: [PATCH] Add vtable buffer slot
>
> This solves for background vtable mutations, i.e., updates
> initiated from buffers other than the vtable buffer, and for
> buffer-adjusted string-pixel-width computations.
>
> * lisp/emacs-lisp/vtable.el (vtable): New '-buffer' slot.
> (vtable-buffer):
> (vtable-set-buffer): New function.
> (vtable-update-object):
> (vtable-remove-object):
> (vtable-insert-object): Wrap operation with the vtable buffer.
> (vtable--insert): Split from old 'vtable-insert'.
> (vtable-insert): Insert table and record the buffer.
> (vtable--insert-line):
> (vtable--insert-header-line): Use 'vtable-buffer' for pixel-width computation.
> (vtable--limit-string):
> (vtable--char-width): Pass buffer to 'string-pixel-width'.
> (vtable-revert): New optional table argument.
> (vtable--alter-column-width):
> (vtable-revert-command):
> (vtable-sort-by-current-column): Call 'vtable-revert' with the table.
>
> * test/lisp/emacs-lisp/vtable-tests.el
> (vtable-tests--make-no-header-2-object-table): New helper
> function.
> (test-vstable-compute-columns): Correct typo in test name.  Use
> new helper function.
> (test-vtable-unique-buffer)
> (test-vtable-non-current-buffer-insert-object)
> (test-vtable-non-current-buffer-remove-object)
> (test-vtable-non-current-buffer-update-object)
> (test-vtable--limit-string-with-face-remapped-buffer): New test.
> ---
>  lisp/emacs-lisp/vtable.el            | 274 +++++++++++++++------------
>  test/lisp/emacs-lisp/vtable-tests.el |  78 +++++++-
>  2 files changed, 221 insertions(+), 131 deletions(-)
>
> diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
> index bcdd280fb92..ccdde49830e 100644
> --- a/lisp/emacs-lisp/vtable.el
> +++ b/lisp/emacs-lisp/vtable.el
> @@ -67,8 +67,10 @@ vtable
>     (ellipsis :initarg :ellipsis :accessor vtable-ellipsis)
>     (column-colors :initarg :column-colors :accessor vtable-column-colors)
>     (row-colors :initarg :row-colors :accessor vtable-row-colors)
> +   (buffer :initform nil :accessor vtable-buffer)
>     (-cached-colors :initform nil)
>     (-cache :initform (make-hash-table :test #'equal))
> +   (-current-cache :initform nil :accessor vtable--current-cache)
>     (-cached-keymap :initform nil)
>     (-has-column-spec :initform nil))
>    "An object to hold the data for a table.")
> @@ -301,14 +303,14 @@ vtable-update-object
>        (unless (cdr objects)
>          (error "Can't find the old object"))
>        (setcar (cdr objects) object))
> -    ;; Then update the rendered vtable in the current buffer.
> -    (if-let* ((cache (vtable--current-cache))
> -             (line-number (seq-position (vtable--cache-lines cache)
> -                                        old-object
> -                                        (lambda (a b)
> -                                          (equal (car a) b))))
> -             (line (elt (vtable--cache-lines cache) line-number)))
> -        (progn
> +    ;; Then update the rendered vtable in its buffer.
> +    (if-let* ((cache (vtable--current-cache table))
> +              (line-number (seq-position (vtable--cache-lines cache)
> +                                         old-object
> +                                         (lambda (a b)
> +                                           (equal (car a) b))))
> +              (line (elt (vtable--cache-lines cache) line-number)))
> +        (with-current-buffer (vtable-buffer table)
>            (setcar line object)
>            (setcdr line (vtable--compute-cached-line table object))
>            ;; ... and redisplay the line in question.
> @@ -321,8 +323,7 @@ vtable-update-object
>                                     (vtable--cache-widths cache)
>                                     (vtable--spacer table))
>                (add-text-properties start (point) (list 'keymap keymap
> -                                                       'vtable table
> -                                                       'vtable-cache cache))))
> +                                                       'vtable table))))
>            ;; We may have inserted a non-numerical value into a previously
>            ;; all-numerical table, so recompute.
>            (vtable--recompute-numerical table (cdr line)))
> @@ -334,14 +335,15 @@ vtable-remove-object
>    ;; First remove from the objects.
>    (setf (vtable-objects table) (delq object (vtable-objects table)))
>    ;; Then adjust the cache and display.
> -  (save-excursion
> -    (vtable-goto-table table)
> -    (let ((cache (vtable--current-cache))
> -          (inhibit-read-only t))
> -      (setcar cache (delq (assq object (vtable--cache-lines cache))
> -                          (vtable--cache-lines cache)))
> -      (when (vtable-goto-object object)
> -        (delete-line)))))
> +  (with-current-buffer (vtable-buffer table)
> +    (save-excursion
> +      (vtable-goto-table table)
> +      (let ((cache (vtable--current-cache table))
> +            (inhibit-read-only t))
> +        (setcar cache (delq (assq object (vtable--cache-lines cache))
> +                            (vtable--cache-lines cache)))
> +        (when (vtable-goto-object object)
> +          (delete-line))))))
>  
>  ;; FIXME: The fact that the `location' argument of
>  ;; `vtable-insert-object' can be an integer and is then interpreted as
> @@ -369,8 +371,9 @@ vtable-insert-object
>        (progn
>          (setf (vtable-objects table) (list object))
>          (vtable--recompute-numerical table (vtable--compute-cached-line table object))
> -        (vtable-goto-table table)
> -        (vtable-revert-command))
> +        (with-current-buffer (vtable-buffer table)
> +          (vtable-goto-table table)
> +          (vtable-revert-command)))
>      ;; First insert into the objects.
>      (let ((pos (if location
>                     (if (integerp location)
> @@ -398,56 +401,57 @@ vtable-insert-object
>            ;; Otherwise, append the object.
>            (nconc (vtable-objects table) (list object)))))
>      ;; Then adjust the cache and display.
> -    (save-excursion
> -      (vtable-goto-table table)
> -      (let* ((cache (vtable--current-cache))
> -             (inhibit-read-only t)
> -             (keymap (get-text-property (point) 'keymap))
> -             (ellipsis (if (vtable-ellipsis table)
> -                           (propertize (truncate-string-ellipsis)
> -                                       'face (vtable-face table))
> -                         ""))
> -             (ellipsis-width (string-pixel-width ellipsis))
> -             (lines (vtable--cache-lines cache))
> -             (elem (if location  ; This binding mirrors the binding of `pos' above.
> -                       (if (integerp location)
> -                           (nth location lines)
> -                         (or (assq location lines)
> -                             (and before (car lines))))
> -                     (if before (car lines))))
> -             (pos (memq elem lines))
> -             (line (cons object (vtable--compute-cached-line table object))))
> -        (if (or before
> -                (and pos (integerp location)))
> -            ;; Add the new object before:.
> -            (let ((old-line (car pos)))
> -              (setcar pos line)
> -              (setcdr pos (cons old-line (cdr pos)))
> -              (unless (vtable-goto-object (car elem))
> -                (vtable-beginning-of-table)))
> -          ;; Otherwise, add the object after.
> -          (if pos
> -              ;; Splice the object into the list.
> -              (progn
> -                (setcdr pos (cons line (cdr pos)))
> -                (if (vtable-goto-object location)
> -                    (forward-line 1)  ; Insert *after*.
> -                  (vtable-end-of-table)))
> -            ;; Otherwise, append the object.
> -            (setcar cache (nconc lines (list line)))
> -            (vtable-end-of-table)))
> -        (let ((start (point)))
> -          ;; FIXME: We have to adjust colors in lines below this if we
> -          ;; have :row-colors.
> -          (vtable--insert-line table line 0
> -                               (vtable--cache-widths cache) (vtable--spacer table)
> -                               ellipsis ellipsis-width)
> -          (add-text-properties start (point) (list 'keymap keymap
> -                                                   'vtable table
> -                                                   'vtable-cache cache)))
> -        ;; We may have inserted a non-numerical value into a previously
> -        ;; all-numerical table, so recompute.
> -        (vtable--recompute-numerical table (cdr line))))))
> +    (let* ((cache (vtable--current-cache table))
> +           (inhibit-read-only t)
> +           (lines (vtable--cache-lines cache))
> +           (elem (if location  ; This binding mirrors the binding of `pos' above.
> +                     (if (integerp location)
> +                         (nth location lines)
> +                       (or (assq location lines)
> +                           (and before (car lines))))
> +                   (if before (car lines))))
> +           (pos (memq elem lines))
> +           (line (cons object (vtable--compute-cached-line table object))))
> +      (with-current-buffer (vtable-buffer table)
> +        (save-excursion
> +          (vtable-goto-table table)
> +          (let* ((ellipsis (if (vtable-ellipsis table)
> +                               (propertize (truncate-string-ellipsis)
> +                                           'face (vtable-face table))
> +                             ""))
> +                 (ellipsis-width (string-pixel-width ellipsis (current-buffer)))
> +                 (keymap (get-text-property (point) 'keymap)))
> +            (if (or before
> +                    (and pos (integerp location)))
> +                ;; Add the new object before:.
> +                (let ((old-line (car pos)))
> +                  (setcar pos line)
> +                  (setcdr pos (cons old-line (cdr pos)))
> +                  (unless (vtable-goto-object (car elem))
> +                    (vtable-beginning-of-table)))
> +              ;; Otherwise, add the object after.
> +              (if pos
> +                  ;; Splice the object into the list.
> +                  (progn
> +                    (setcdr pos (cons line (cdr pos)))
> +                    (if (vtable-goto-object location)
> +                        (forward-line 1)  ; Insert *after*.
> +                      (vtable-end-of-table)))
> +                ;; Otherwise, append the object.
> +                (setcar cache (nconc lines (list line)))
> +                (vtable-end-of-table)))
> +            (let ((start (point)))
> +              ;; FIXME: We have to adjust colors in lines below this if we
> +              ;; have :row-colors.
> +              (vtable--insert-line table line 0
> +                                   (vtable--cache-widths cache)
> +                                   (vtable--spacer table)
> +                                   ellipsis ellipsis-width)
> +              (add-text-properties start (point) (list 'keymap keymap
> +                                                       'vtable table)))
> +            ;; We may have inserted a non-numerical value into a previously
> +            ;; all-numerical table, so recompute.
> +            (vtable--recompute-numerical table (cdr line))))))))
>  
>  (defun vtable-column (table index)
>    "Return the name of the INDEXth column in TABLE."
> @@ -520,14 +524,14 @@ vtable--cache-widths
>  (defun vtable--cache-lines (cache)
>    (car cache))
>  
> -(defun vtable-insert (table)
> +(defun vtable--insert (table)
>    (let* ((spacer (vtable--spacer table))
>           (start (point))
>           (ellipsis (if (vtable-ellipsis table)
>                         (propertize (truncate-string-ellipsis)
>                                     'face (vtable-face table))
>                       ""))
> -         (ellipsis-width (string-pixel-width ellipsis))
> +         (ellipsis-width (string-pixel-width ellipsis (vtable-buffer table)))
>           ;; We maintain a cache per screen/window width, so that we render
>           ;; correctly if Emacs is open on two different screens (or the
>           ;; user resizes the frame).
> @@ -549,8 +553,7 @@ vtable-insert
>          (add-text-properties start (point)
>                               (list 'keymap vtable-header-line-map
>                                     'rear-nonsticky t
> -                                   'vtable table
> -                                   'vtable-cache cache))
> +                                   'vtable table))
>          (setq start (point))))
>      (vtable--sort table cache)
>      ;; Insert the data.
> @@ -561,13 +564,29 @@ vtable-insert
>          (setq line-number (1+ line-number))))
>      (add-text-properties start (point)
>                           (list 'rear-nonsticky t
> -                               'vtable table
> -                               'vtable-cache cache))
> +                               'vtable table))
> +    (setf (vtable--current-cache table) cache)
>      (goto-char start)))
>  
> +(defun vtable-insert (table)
> +  "Insert TABLE into the current buffer.
> +The current buffer will be recorded as TABLE's buffer.  If the table is
> +inserted into a buffer other than its originating buffer, signal an
> +error.  A table may be reinserted into its own buffer, but insert only
> +one instance per buffer.  This restriction needs to be enforced by the
> +caller."
> +  (and-let* ((table-buffer (vtable-buffer table))
> +             (not (eq table-buffer (current-buffer))))
> +    (error "A vtable cannot be inserted into more than one buffer"))
> +  (setf (vtable-buffer table) (current-buffer))
> +  (let ((inhibit-read-only t)
> +        (inhibit-modification-hooks t))

Hm, why do we need these inhibit-read-only and
inhibit-modification-hooks?  Those are new, right?  They make sense,
though.  Could we cover it with a test?

> +    (vtable--insert table)))
> +
>  (defun vtable--insert-line (table line line-number widths spacer
>                                    &optional ellipsis ellipsis-width)
>    (let ((start (point))
> +        (buffer (vtable-buffer table))
>          (columns (vtable-columns table))
>          (column-colors
>           (and (vtable-column-colors table)
> @@ -607,16 +626,18 @@ vtable--insert-line
>                        (concat
>                         (vtable--limit-string
>                          pre-computed (- (elt widths index)
> -                                        (or ellipsis-width 0)))
> +                                        (or ellipsis-width 0))
> +                        buffer)
>                         ellipsis)
>                      pre-computed))
>                   ;; Recompute widths.
>                   (t
> -                  (if (> (string-pixel-width value) (elt widths index))
> +                  (if (> (string-pixel-width value buffer) (elt widths index))
>                        (concat
>                         (vtable--limit-string
>                          value (- (elt widths index)
> -                                 (or ellipsis-width 0)))
> +                                 (or ellipsis-width 0))
> +                        buffer)
>                         ellipsis)
>                      value))))
>                 (start (point))
> @@ -630,14 +651,15 @@ vtable--insert-line
>                            (list 'space
>                                  :width (list
>                                          (+ (- (elt widths index)
> -                                              (string-pixel-width displayed))
> +                                              (string-pixel-width
> +                                               displayed buffer))
>                                             (if last 0 spacer)))))))
>               ;; Align to the right.
>               (insert (propertize " " 'display
>                                   (list 'space
>                                         :width (list (- (elt widths index)
>                                                         (string-pixel-width
> -                                                        displayed)))))
> +                                                        displayed buffer)))))
>                       displayed)
>               (unless last
>                 (insert (propertize " " 'display
> @@ -664,16 +686,6 @@ vtable--insert-line
>  (defun vtable--cache-key ()
>    (cons (frame-terminal) (window-width)))
>  
> -(defun vtable--current-cache ()
> -  "Return the current cache for the table at point.
> -
> -In `vtable-insert', the lines and widths of the vtable text are computed
> -based on the current selected frame and window and stored in a cache.
> -Subsequent interaction with the text of the vtable should use that cache
> -via this function rather than by calling `vtable--cache-key' to look up
> -the cache."
> -  (get-text-property (point) 'vtable-cache))
> -
>  (defun vtable--clear-cache (table)
>    (setf (gethash (vtable--cache-key) (slot-value table '-cache)) nil))
>  
> @@ -718,6 +730,7 @@ vtable--indicator
>  (defun vtable--insert-header-line (table widths spacer)
>    ;; Insert the header directly into the buffer.
>    (let ((start (point))
> +        (buffer (vtable-buffer table))
>          (divider (vtable-divider table))
>          (cmap (define-keymap
>                  "<header-line> <drag-mouse-1>" #'vtable--drag-resize-column
> @@ -737,14 +750,15 @@ vtable--insert-header-line
>                       'keymap cmap))
>                (start (point))
>                (indicator (vtable--indicator table index))
> -              (indicator-width (string-pixel-width indicator))
> +              (indicator-width (string-pixel-width indicator buffer))
>                (last (= index (1- (length (vtable-columns table)))))
>                displayed)
>           (setq displayed
> -               (if (> (string-pixel-width name)
> +               (if (> (string-pixel-width name buffer)
>                        (- (elt widths index) indicator-width))
>                     (vtable--limit-string
> -                    name (- (elt widths index) indicator-width))
> +                    name (- (elt widths index) indicator-width)
> +                    buffer)
>                   name))
>           (let* ((indicator-lead-width
>                   ;; We want the indicator to not be quite flush right.
> @@ -753,7 +767,7 @@ vtable--insert-header-line
>                                          indicator-lead-width))
>                  (fill-width
>                   (+ (- (elt widths index)
> -                       (string-pixel-width displayed)
> +                       (string-pixel-width displayed buffer)
>                         indicator-width
>                         indicator-lead-width)
>                      (if last 0 spacer))))
> @@ -771,7 +785,8 @@ vtable--insert-header-line
>               ;; This is the final column, and we have a sorting
>               ;; indicator, and the table is too wide for the window.
>               (let* ((pre-indicator (string-pixel-width
> -                                    (buffer-substring (point-min) (point))))
> +                                    (buffer-substring (point-min) (point))
> +                                    buffer))
>                      (pre-fill
>                       (- (window-width nil t)
>                          pre-indicator
> @@ -850,14 +865,16 @@ vtable--set-header-line
>             (buffer-substring (point-min) (1- (point-max))))))
>    (vtable-header-mode 1))
>  
> -(defun vtable--limit-string (string pixels)
> +
> +(defun vtable--limit-string (string pixels buffer)
>    (while (and (length> string 0)
> -              (> (string-pixel-width string) pixels))
> +              (> (string-pixel-width string buffer) pixels))
>      (setq string (substring string 0 (1- (length string)))))
>    string)
>  
>  (defun vtable--char-width (table)
> -  (string-pixel-width (propertize "x" 'face (vtable-face table))))
> +  (string-pixel-width (propertize "x" 'face (vtable-face table))
> +                      (vtable-buffer table)))
>  
>  (defun vtable--compute-width (table spec)
>    (cond
> @@ -936,7 +953,7 @@ vtable--compute-cached-line
>         ;; We stash the computed width and string here -- if there are
>         ;; no formatters/displayers, we'll be using the string, and
>         ;; then won't have to recreate it.
> -       (list value (string-pixel-width string) string)))
> +       (list value (string-pixel-width string (vtable-buffer table)) string)))
>     (vtable-columns table)))
>  
>  (defun vtable--make-keymap (table)
> @@ -967,20 +984,21 @@ vtable--make-keymap
>            (vtable-keymap table))
>        map)))
>  
> -(defun vtable-revert ()
> -  "Regenerate the table under point."
> -  (let ((table (vtable-current-table))
> -        (object (vtable-current-object))
> -        (column (vtable-current-column))
> -        (inhibit-read-only t))
> -    (unless table
> -      (user-error "No table under point"))
> -    (delete-region (vtable-beginning-of-table) (vtable-end-of-table))
> -    (vtable-insert table)
> -    (when object
> -      (vtable-goto-object object))
> -    (when column
> -      (vtable-goto-column column))))
> +(defun vtable-revert (&optional table)
> +  "Regenerate TABLE, defaulting to the table under point."
> +  (setq table (or table (vtable-current-table)))
> +  (unless table
> +    (user-error "No table under point"))
> +  (with-current-buffer (vtable-buffer table)
> +    (let ((object (vtable-current-object))
> +          (column (vtable-current-column))
> +          (inhibit-read-only t))
> +      (delete-region (vtable-beginning-of-table) (vtable-end-of-table))
> +      (vtable--insert table)
> +      (when object
> +        (vtable-goto-object object))
> +      (when column
> +        (vtable-goto-column column)))))
>  
>  ;;; Commands.
>  
> @@ -1006,14 +1024,14 @@ vtable-narrow-current-column
>                                  (- (* (vtable--char-width table) (or n 1))))))
>  
>  (defun vtable--alter-column-width (table column delta)
> -  (let ((widths (vtable--cache-widths (vtable--current-cache))))
> +  (let ((widths (vtable--cache-widths (vtable--current-cache table))))
>      (setf (aref widths column)
>            (max (* (vtable--char-width table) 2)
>                 (+ (aref widths column) delta)))
>      ;; Store the width so it'll be respected on a revert.
>      (setf (vtable-column-width (elt (vtable-columns table) column))
>            (format "%dpx" (aref widths column)))
> -    (vtable-revert)))
> +    (vtable-revert table)))
>  
>  (defun vtable-widen-current-column (&optional n)
>    "Widen the current column by N characters.
> @@ -1028,24 +1046,28 @@ vtable-previous-column
>    (interactive)
>    (vtable-goto-column
>     (max 0 (1- (or (vtable-current-column)
> -                  (length (vtable--cache-widths (vtable--current-cache))))))))
> +                  (length (vtable--cache-widths
> +                           (vtable--current-cache (vtable-current-table)))))))))
>  
>  (defun vtable-next-column ()
>    "Go to the next column."
>    (interactive)
>    (when (vtable-current-column)
>      (vtable-goto-column
> -     (min (1- (length (vtable--cache-widths (vtable--current-cache))))
> +     (min (1- (length (vtable--cache-widths
> +                       (vtable--current-cache (vtable-current-table)))))
>            (1+ (vtable-current-column))))))
>  
> -(defun vtable-revert-command ()
> +(defun vtable-revert-command (&optional table)
>    "Re-query data and regenerate the table under point."

This docstring should mention TABLE.

>    (interactive)
> -  (let ((table (vtable-current-table)))
> -    (when (vtable-objects-function table)
> -      (setf (vtable-objects table) (funcall (vtable-objects-function table))))
> -    (vtable--clear-cache table))
> -  (vtable-revert))
> +  (setq table (or table (vtable-current-table)))
> +  (unless table
> +    (user-error "No table found"))
> +  (when (vtable-objects-function table)
> +    (setf (vtable-objects table) (funcall (vtable-objects-function table))))
> +  (vtable--clear-cache table)
> +  (vtable-revert table))
>  
>  (defun vtable-sort-by-current-column ()
>    "Sort the table under point by the column under point."
> @@ -1067,8 +1089,8 @@ vtable-sort-by-current-column
>                                    (if (eq (cdr last) 'ascend)
>                                        'descend
>                                      'ascend)
> -                                'ascend))))))
> -  (vtable-revert))
> +                                'ascend)))))
> +    (vtable-revert table)))
>  
>  (defun vtable-header-line-sort (e)
>    "Sort a vtable from the header line."
> diff --git a/test/lisp/emacs-lisp/vtable-tests.el b/test/lisp/emacs-lisp/vtable-tests.el
> index 74fb8cc8139..83f826ea353 100644
> --- a/test/lisp/emacs-lisp/vtable-tests.el
> +++ b/test/lisp/emacs-lisp/vtable-tests.el
> @@ -27,16 +27,19 @@
>  (require 'ert)
>  (require 'ert-x)
>  
> -(ert-deftest test-vstable-compute-columns ()
> +(defun vtable-tests--make-no-header-2-object-table ()
> +  (make-vtable :columns '("a" "b" "c")
> +               :objects '(("foo" 1 2)
> +                          ("bar" 3 :zot))
> +               :insert nil))
> +
> +(ert-deftest test-vtable-compute-columns ()
>    (should
>     (equal (mapcar
>             (lambda (column)
>               (vtable-column-align column))
>             (vtable--compute-columns
> -            (make-vtable :columns '("a" "b" "c")
> -                         :objects '(("foo" 1 2)
> -                                    ("bar" 3 :zot))
> -                         :insert nil)))
> +            (vtable-tests--make-no-header-2-object-table)))
>            '(left right left))))
>  
>  (ert-deftest test-vtable-insert-object ()
> @@ -69,4 +72,69 @@ test-vtable-insert-object
>                (mapcar #'cadr (vtable-objects table))))
>            (number-sequence 0 11))))
>  
> +(ert-deftest test-vtable-unique-buffer ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      ;; This will run but fail on Emacs pre 31 vtable.
> +      (should-error (vtable-insert table))
> +      ;; This will run only on Emacs 31+ vtable.
> +      (when (> emacs-major-version 30)
> +        (should-error (vtable-set-buffer table (current-buffer)))))))
> +
> +(ert-deftest test-vtable-non-current-buffer-insert-object ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table))
> +        (obj '("baz" 4 5)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      (should (= (count-lines (point-min) (point-max)) 2))
> +      (with-temp-buffer
> +        (vtable-insert-object table obj))
> +      (should (= (count-lines (point-min) (point-max)) 3)))))
> +
> +(ert-deftest test-vtable-non-current-buffer-remove-object ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table))
> +        (obj '("baz" 4 5)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      (vtable-insert-object table obj)
> +      (should (= (count-lines (point-min) (point-max)) 3))
> +      (with-temp-buffer
> +        (vtable-remove-object table obj))
> +      (should (= (count-lines (point-min) (point-max)) 2)))))
> +
> +(ert-deftest test-vtable-non-current-buffer-update-object ()
> +  (let ((table (vtable-tests--make-no-header-2-object-table))
> +        (obj '("baz" 4 5))
> +        (obj-2 '("qux" 6 7)))
> +    (with-temp-buffer
> +      (vtable-insert table)
> +      (vtable-insert-object table obj)
> +      (should (= (count-lines (point-min) (point-max)) 3))
> +      (let ((line-2 (progn
> +                      (goto-char (point-min))
> +                      (forward-line 2)
> +                      (buffer-substring (point) (point-max)))))
> +        (with-temp-buffer
> +          (vtable-update-object table obj-2 obj))
> +        (let ((line-2-new (progn
> +                            (goto-char (point-min))
> +                            (forward-line 2)
> +                            (buffer-substring (point) (point-max)))))
> +          (should (= (count-lines (point-min) (point-max)) 3))
> +          (should (not (string= line-2 line-2-new))))))))
> +
> +(ert-deftest test-vtable--limit-string-with-face-remapped-buffer ()
> +  (with-temp-buffer
> +    (let ((text (propertize "XXXXX"
> +                            'face 'variable-pitch)))
> +      (face-remap-add-relative 'default :height 1.5)
> +      (if (> emacs-major-version 30)
> +          (should (eq
> +                   2
> +                   (length (vtable--limit-string text 50 (current-buffer)))))
> +        (should (eq
> +                 2
> +                 (length (vtable--limit-string text 50))))))))
> +
>  ;;; vtable-tests.el ends here

Message #38 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot,
 [PATCH] Add vtable buffer slot
Date: Thu, 11 Dec 2025 16:07:24 -0500

[Message part 1 (text/plain, inline)]

On Thu, Dec 11, 2025 at 2:46 PM Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stéphane Marks <shipmints <at> gmail.com> writes:
>
> > On Thu, Dec 11, 2025 at 1:23 PM Spencer Baugh <sbaugh <at> janestreet.com>
> wrote:
> >  Can we move these let-bindings out to the outer let-binding now?
> >
> > I'd prefer if they stay close to where they're being used, at least
> here, where the cache update is clearly separate from the buffer
> > update.
>
> Good point.  Actually, so these bindings actually probably should be in
> the same let-binding as (let ((start (point))) ...), a few lines later.
> Since they're only used in that part of the code.
>

Sure.

>  > + (keymap (get-text-property (point) 'keymap)))
> >
> >  I guess we could get this from vtable-keymap now?  In which case it can
> >  also move to the outer let-binding?
> >
> > It would have to be from the -cached-keymap slot.  Let's leave the
> keymaps as is for the time being.  We have patches to refine
> > and expand their functionality coming.
>
> That's fair, but I think if we move these let-bindings down to the (let
> ((start (point)))) binding, that avoids reindenting the rest of this
> function.  Which makes this diff much smaller, which is nice.
>

I put the vtable-set-buffer function back in because I recalled
that Augusto's comint-mime implementation uses a multi-buffer approach to
get a vtable into a comint buffer.  It first inserts into a temp buffer and
then blasts the buffer-string into the comint buffer and it expects to be
able to manipulate that vtable in that comint buffer despite its foreign
origin.  comint-mime would invoke this to reset the buffer (yes, this is
not backward compatible for his use case but he's the only one I've found
and I'm aware of and it's been brought up).


> > +(defun vtable-insert (table)
> > +  "Insert TABLE into the current buffer.
> > +The current buffer will be recorded as TABLE's buffer.  If the table is
> > +inserted into a buffer other than its originating buffer, signal an
> > +error.  A table may be reinserted into its own buffer, but insert only
> > +one instance per buffer.  This restriction needs to be enforced by the
> > +caller."
> > +  (and-let* ((table-buffer (vtable-buffer table))
> > +             (not (eq table-buffer (current-buffer))))
> > +    (error "A vtable cannot be inserted into more than one buffer"))
> > +  (setf (vtable-buffer table) (current-buffer))
> > +  (let ((inhibit-read-only t)
> > +        (inhibit-modification-hooks t))
>
> Hm, why do we need these inhibit-read-only and
> inhibit-modification-hooks?  Those are new, right?  They make sense,
> though.  Could we cover it with a test?
>

Test added, at least for the read-only case.  Mod hooks are a performance
issue and we could have a pedantic test for that but it's fussy for me at
the moment.

I also went ahead and put inhibit-modification-hooks in the other places it
needs to be (those were in my mega patch).

> -(defun vtable-revert-command ()
> > +(defun vtable-revert-command (&optional table)
> >    "Re-query data and regenerate the table under point."
>
> This docstring should mention TABLE.


Yep.

Patch attached.

-Stéphane

[Message part 2 (text/html, inline)]

[0001-Add-vtable-buffer-slot.patch (application/octet-stream, attachment)]

Message #41 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Spencer Baugh <sbaugh <at> janestreet.com>,
 Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer
 slot, [PATCH] Add vtable buffer slot
Date: Fri, 12 Dec 2025 11:55:57 +0100

On Thu, 11 Dec 2025 at 16:07, Stéphane Marks <shipmints <at> gmail.com> wrote:

> I put the vtable-set-buffer function back in because I recalled that
> Augusto's comint-mime implementation uses a multi-buffer approach to
> get a vtable into a comint buffer.  It first inserts into a temp
> buffer and then blasts the buffer-string into the comint buffer and it
> expects to be able to manipulate that vtable in that comint buffer
> despite its foreign origin.  comint-mime would invoke this to reset
> the buffer (yes, this is not backward compatible for his use case but
> he's the only one I've found and I'm aware of and it's been brought
> up).

I implemented it that way because it "just worked", but it seems
perfectly reasonable to declare that a vtable is tied to a buffer,
period.  I recommend picking the approach that simplifies the
implementation and future uses of vtable.  comint-mime can be changed
(in fact, it has to be changed anyway).

While I'm not fully into the loop regarding this discussion, it seems to
me that the vtable's buffer should not part of the public interface and
the ability to move a vtable between buffers should not be a supported
feature.

Message #44 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: Spencer Baugh <sbaugh <at> janestreet.com>,
 Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot,
 [PATCH] Add vtable buffer slot
Date: Fri, 12 Dec 2025 06:36:58 -0500

[Message part 1 (text/plain, inline)]

On Fri, Dec 12, 2025 at 5:55 AM Augusto Stoffel <arstoffel <at> gmail.com> wrote:

> On Thu, 11 Dec 2025 at 16:07, Stéphane Marks <shipmints <at> gmail.com> wrote:
>
> > I put the vtable-set-buffer function back in because I recalled that
> > Augusto's comint-mime implementation uses a multi-buffer approach to
> > get a vtable into a comint buffer.  It first inserts into a temp
> > buffer and then blasts the buffer-string into the comint buffer and it
> > expects to be able to manipulate that vtable in that comint buffer
> > despite its foreign origin.  comint-mime would invoke this to reset
> > the buffer (yes, this is not backward compatible for his use case but
> > he's the only one I've found and I'm aware of and it's been brought
> > up).
>
> I implemented it that way because it "just worked", but it seems
> perfectly reasonable to declare that a vtable is tied to a buffer,
> period.  I recommend picking the approach that simplifies the
> implementation and future uses of vtable.  comint-mime can be changed
> (in fact, it has to be changed anyway).
>
> While I'm not fully into the loop regarding this discussion, it seems to
> me that the vtable's buffer should not part of the public interface and
> the ability to move a vtable between buffers should not be a supported
> feature.
>

I don't mind the safety hatch, though.  Other people might do something
clever like you did with the HTML renderer.  I wrote a mild admonishment in
the docstring.

[Message part 2 (text/html, inline)]

Message #47 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer
 slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Fri, 12 Dec 2025 08:23:17 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:
> --- a/lisp/emacs-lisp/vtable.el
> +++ b/lisp/emacs-lisp/vtable.el
> @@ -398,56 +403,58 @@ vtable-insert-object
>            ;; Otherwise, append the object.
>            (nconc (vtable-objects table) (list object)))))
>      ;; Then adjust the cache and display.
> -    (save-excursion
> -      (vtable-goto-table table)
> -      (let* ((cache (vtable--current-cache))
> -             (inhibit-read-only t)
> -             (keymap (get-text-property (point) 'keymap))
> -             (ellipsis (if (vtable-ellipsis table)
> -                           (propertize (truncate-string-ellipsis)
> -                                       'face (vtable-face table))
> -                         ""))
> -             (ellipsis-width (string-pixel-width ellipsis))
> -             (lines (vtable--cache-lines cache))
> -             (elem (if location  ; This binding mirrors the binding of `pos' above.
> -                       (if (integerp location)
> -                           (nth location lines)
> -                         (or (assq location lines)
> -                             (and before (car lines))))
> -                     (if before (car lines))))
> -             (pos (memq elem lines))
> -             (line (cons object (vtable--compute-cached-line table object))))
> -        (if (or before
> -                (and pos (integerp location)))
> -            ;; Add the new object before:.
> -            (let ((old-line (car pos)))
> -              (setcar pos line)
> -              (setcdr pos (cons old-line (cdr pos)))
> -              (unless (vtable-goto-object (car elem))
> -                (vtable-beginning-of-table)))
> -          ;; Otherwise, add the object after.
> -          (if pos
> -              ;; Splice the object into the list.
> -              (progn
> -                (setcdr pos (cons line (cdr pos)))
> -                (if (vtable-goto-object location)
> -                    (forward-line 1)  ; Insert *after*.
> -                  (vtable-end-of-table)))
> -            ;; Otherwise, append the object.
> -            (setcar cache (nconc lines (list line)))
> -            (vtable-end-of-table)))
> -        (let ((start (point)))
> -          ;; FIXME: We have to adjust colors in lines below this if we
> -          ;; have :row-colors.
> -          (vtable--insert-line table line 0
> -                               (vtable--cache-widths cache) (vtable--spacer table)
> -                               ellipsis ellipsis-width)
> -          (add-text-properties start (point) (list 'keymap keymap
> -                                                   'vtable table
> -                                                   'vtable-cache cache)))
> -        ;; We may have inserted a non-numerical value into a previously
> -        ;; all-numerical table, so recompute.
> -        (vtable--recompute-numerical table (cdr line))))))
> +    (let* ((cache (vtable--current-cache table))
> +           (inhibit-read-only t)
> +           (inhibit-modification-hooks t)
> +           (lines (vtable--cache-lines cache))
> +           (elem (if location  ; This binding mirrors the binding of `pos' above.
> +                     (if (integerp location)
> +                         (nth location lines)
> +                       (or (assq location lines)
> +                           (and before (car lines))))
> +                   (if before (car lines))))
> +           (pos (memq elem lines))
> +           (line (cons object (vtable--compute-cached-line table object))))
> +      (with-current-buffer (vtable-buffer table)
> +        (save-excursion

So in the end I think the only way to keep the diff to vtable-insert-object small is
if you use (set-buffer (vtable-buffer table)) here instead of
with-current-buffer.  It's a bit less normal, but it at least avoids
some unnecessary indentation.

That is, instead of
(with-current-buffer (vtable-buffer table)
  (save-excursion
    ...))

do

(save-excursion
  (set-buffer (vtable-buffer table))
  ...)

(save-excursion saves and restores the current buffer)

Thanks for making all these changes.  I think after that we should be
good to go.

Message #50 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer
 slot, [PATCH] Add vtable buffer slot
Date: Fri, 12 Dec 2025 08:26:00 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:

> On Fri, Dec 12, 2025 at 5:55 AM Augusto Stoffel <arstoffel <at> gmail.com> wrote:
>
>  On Thu, 11 Dec 2025 at 16:07, Stéphane Marks <shipmints <at> gmail.com> wrote:
>
>  > I put the vtable-set-buffer function back in because I recalled that
>  > Augusto's comint-mime implementation uses a multi-buffer approach to
>  > get a vtable into a comint buffer.  It first inserts into a temp
>  > buffer and then blasts the buffer-string into the comint buffer and it
>  > expects to be able to manipulate that vtable in that comint buffer
>  > despite its foreign origin.  comint-mime would invoke this to reset
>  > the buffer (yes, this is not backward compatible for his use case but
>  > he's the only one I've found and I'm aware of and it's been brought
>  > up).
>
>  I implemented it that way because it "just worked", but it seems
>  perfectly reasonable to declare that a vtable is tied to a buffer,
>  period.  I recommend picking the approach that simplifies the
>  implementation and future uses of vtable.  comint-mime can be changed
>  (in fact, it has to be changed anyway).
>
>  While I'm not fully into the loop regarding this discussion, it seems to
>  me that the vtable's buffer should not part of the public interface and
>  the ability to move a vtable between buffers should not be a supported
>  feature.
>
> I don't mind the safety hatch, though.  Other people might do something clever like you did with the HTML renderer.  I wrote a mild
> admonishment in the docstring.

I agree with Augusto that eventually we probably shouldn't support
changing vtables between buffers.  But, at the moment, removing support
for changing vtables between buffers doesn't simplify the code at all.
So I agree with leaving it in for now and just writing an admonishment
in the docstring, as you did.

Message #53 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot,
 [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Fri, 12 Dec 2025 09:33:45 -0500

[Message part 1 (text/plain, inline)]

On Fri, Dec 12, 2025 at 8:23 AM Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stéphane Marks <shipmints <at> gmail.com> writes:
> > --- a/lisp/emacs-lisp/vtable.el
> > +++ b/lisp/emacs-lisp/vtable.el
> > @@ -398,56 +403,58 @@ vtable-insert-object
> ...
> > +      (with-current-buffer (vtable-buffer table)
> > +        (save-excursion
>
> So in the end I think the only way to keep the diff to
> vtable-insert-object small is
> if you use (set-buffer (vtable-buffer table)) here instead of
> with-current-buffer.  It's a bit less normal, but it at least avoids
> some unnecessary indentation.
>
> That is, instead of
> (with-current-buffer (vtable-buffer table)
>   (save-excursion
>     ...))
>
> do
>
> (save-excursion
>   (set-buffer (vtable-buffer table))
>   ...)
>
> (save-excursion saves and restores the current buffer)
>
> Thanks for making all these changes.  I think after that we should be
> good to go.
>

While I appreciate the desire for tiny diffs, I prefer that the stanzas
used in update insert remove functions remain consistent and we avoid a
special comment explaining why insert is special saying this was just for
indentation.  Hrumph.

[Message part 2 (text/html, inline)]

Message #56 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot,
 [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Fri, 12 Dec 2025 09:34:35 -0500

[Message part 1 (text/plain, inline)]

On Fri, Dec 12, 2025 at 9:33 AM Stéphane Marks <shipmints <at> gmail.com> wrote:

> On Fri, Dec 12, 2025 at 8:23 AM Spencer Baugh <sbaugh <at> janestreet.com>
> wrote:
>
>> Stéphane Marks <shipmints <at> gmail.com> writes:
>> > --- a/lisp/emacs-lisp/vtable.el
>> > +++ b/lisp/emacs-lisp/vtable.el
>> > @@ -398,56 +403,58 @@ vtable-insert-object
>> ...
>> > +      (with-current-buffer (vtable-buffer table)
>> > +        (save-excursion
>>
>> So in the end I think the only way to keep the diff to
>> vtable-insert-object small is
>> if you use (set-buffer (vtable-buffer table)) here instead of
>> with-current-buffer.  It's a bit less normal, but it at least avoids
>> some unnecessary indentation.
>>
>> That is, instead of
>> (with-current-buffer (vtable-buffer table)
>>   (save-excursion
>>     ...))
>>
>> do
>>
>> (save-excursion
>>   (set-buffer (vtable-buffer table))
>>   ...)
>>
>> (save-excursion saves and restores the current buffer)
>>
>> Thanks for making all these changes.  I think after that we should be
>> good to go.
>>
>
> While I appreciate the desire for tiny diffs, I prefer that the stanzas
> used in update insert remove functions remain consistent and we avoid a
> special comment explaining why insert is special saying this was just for
> indentation.  Hrumph.
>

The alternative would be to make them all save-excursion, set-buffer, if
you think that's worthy.

[Message part 2 (text/html, inline)]

Message #59 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer
 slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Fri, 12 Dec 2025 10:35:23 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:

> On Fri, Dec 12, 2025 at 9:33 AM Stéphane Marks <shipmints <at> gmail.com> wrote:
>
>  On Fri, Dec 12, 2025 at 8:23 AM Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>
>  Stéphane Marks <shipmints <at> gmail.com> writes:
>  > --- a/lisp/emacs-lisp/vtable.el
>  > +++ b/lisp/emacs-lisp/vtable.el
>  > @@ -398,56 +403,58 @@ vtable-insert-object
>  ...
>  > +      (with-current-buffer (vtable-buffer table)
>  > +        (save-excursion
>
>  So in the end I think the only way to keep the diff to vtable-insert-object small is
>  if you use (set-buffer (vtable-buffer table)) here instead of
>  with-current-buffer.  It's a bit less normal, but it at least avoids
>  some unnecessary indentation.
>
>  That is, instead of
>  (with-current-buffer (vtable-buffer table)
>    (save-excursion
>      ...))
>
>  do
>
>  (save-excursion
>    (set-buffer (vtable-buffer table))
>    ...)
>
>  (save-excursion saves and restores the current buffer)
>
>  Thanks for making all these changes.  I think after that we should be
>  good to go.
>
>  While I appreciate the desire for tiny diffs, I prefer that the stanzas used in update insert remove functions remain consistent
>  and we avoid a special comment explaining why insert is special saying this was just for indentation.  Hrumph.
>
> The alternative would be to make them all save-excursion, set-buffer, if you think that's worthy.

IMO that's worth doing.  It also saves indentation, so it's actually
somewhat useful in all the places we're currently adding a
with-current-buffer, even if it doesn't avoid a big reindent diff.

Message #62 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot,
 [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Fri, 12 Dec 2025 10:48:06 -0500

[Message part 1 (text/plain, inline)]

On Fri, Dec 12, 2025 at 10:35 AM Spencer Baugh <sbaugh <at> janestreet.com>
wrote:

> Stéphane Marks <shipmints <at> gmail.com> writes:
>
> > On Fri, Dec 12, 2025 at 9:33 AM Stéphane Marks <shipmints <at> gmail.com>
> wrote:
> >
> >  On Fri, Dec 12, 2025 at 8:23 AM Spencer Baugh <sbaugh <at> janestreet.com>
> wrote:
> >
> >  Stéphane Marks <shipmints <at> gmail.com> writes:
> >  > --- a/lisp/emacs-lisp/vtable.el
> >  > +++ b/lisp/emacs-lisp/vtable.el
> >  > @@ -398,56 +403,58 @@ vtable-insert-object
> >  ...
> >  > +      (with-current-buffer (vtable-buffer table)
> >  > +        (save-excursion
> >
> >  So in the end I think the only way to keep the diff to
> vtable-insert-object small is
> >  if you use (set-buffer (vtable-buffer table)) here instead of
> >  with-current-buffer.  It's a bit less normal, but it at least avoids
> >  some unnecessary indentation.
> >
> >  That is, instead of
> >  (with-current-buffer (vtable-buffer table)
> >    (save-excursion
> >      ...))
> >
> >  do
> >
> >  (save-excursion
> >    (set-buffer (vtable-buffer table))
> >    ...)
> >
> >  (save-excursion saves and restores the current buffer)
> >
> >  Thanks for making all these changes.  I think after that we should be
> >  good to go.
> >
> >  While I appreciate the desire for tiny diffs, I prefer that the stanzas
> used in update insert remove functions remain consistent
> >  and we avoid a special comment explaining why insert is special saying
> this was just for indentation.  Hrumph.
> >
> > The alternative would be to make them all save-excursion, set-buffer, if
> you think that's worthy.
>
> IMO that's worth doing.  It also saves indentation, so it's actually
> somewhat useful in all the places we're currently adding a
> with-current-buffer, even if it doesn't avoid a big reindent diff.
>

I forgot about the compiler warning

  emacs-lisp/vtable.el:376:10: Warning: Use ‘with-current-buffer’ rather
than save-excursion+set-buffer

We may also need save-excursion twice, once for what is the ambient current
buffer, and the second to ensure that point doesn't move around in the
vtable-buffer.  Both of these, I think, are satisfied via
with-current-buffer and we avoid the warning.  So, perhaps, we should bite
the bullet and swallow the extra indentation in vtable-insert-object and
use the more robust and simpler w-c-b.

[Message part 2 (text/html, inline)]

Message #65 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot,
 [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Mon, 15 Dec 2025 16:22:08 -0500

[Message part 1 (text/plain, inline)]

On Fri, Dec 12, 2025 at 10:48 AM Stéphane Marks <shipmints <at> gmail.com> wrote:

> On Fri, Dec 12, 2025 at 10:35 AM Spencer Baugh <sbaugh <at> janestreet.com>
> wrote:
>
>> Stéphane Marks <shipmints <at> gmail.com> writes:
>>
>> > On Fri, Dec 12, 2025 at 9:33 AM Stéphane Marks <shipmints <at> gmail.com>
>> wrote:
>> >
>> >  On Fri, Dec 12, 2025 at 8:23 AM Spencer Baugh <sbaugh <at> janestreet.com>
>> wrote:
>> >
>> >  Stéphane Marks <shipmints <at> gmail.com> writes:
>> >  > --- a/lisp/emacs-lisp/vtable.el
>> >  > +++ b/lisp/emacs-lisp/vtable.el
>> >  > @@ -398,56 +403,58 @@ vtable-insert-object
>> >  ...
>> >  > +      (with-current-buffer (vtable-buffer table)
>> >  > +        (save-excursion
>> >
>> >  So in the end I think the only way to keep the diff to
>> vtable-insert-object small is
>> >  if you use (set-buffer (vtable-buffer table)) here instead of
>> >  with-current-buffer.  It's a bit less normal, but it at least avoids
>> >  some unnecessary indentation.
>> >
>> >  That is, instead of
>> >  (with-current-buffer (vtable-buffer table)
>> >    (save-excursion
>> >      ...))
>> >
>> >  do
>> >
>> >  (save-excursion
>> >    (set-buffer (vtable-buffer table))
>> >    ...)
>> >
>> >  (save-excursion saves and restores the current buffer)
>> >
>> >  Thanks for making all these changes.  I think after that we should be
>> >  good to go.
>> >
>> >  While I appreciate the desire for tiny diffs, I prefer that the
>> stanzas used in update insert remove functions remain consistent
>> >  and we avoid a special comment explaining why insert is special saying
>> this was just for indentation.  Hrumph.
>> >
>> > The alternative would be to make them all save-excursion, set-buffer,
>> if you think that's worthy.
>>
>> IMO that's worth doing.  It also saves indentation, so it's actually
>> somewhat useful in all the places we're currently adding a
>> with-current-buffer, even if it doesn't avoid a big reindent diff.
>>
>
> I forgot about the compiler warning
>
>   emacs-lisp/vtable.el:376:10: Warning: Use ‘with-current-buffer’ rather
> than save-excursion+set-buffer
>
> We may also need save-excursion twice, once for what is the ambient
> current buffer, and the second to ensure that point doesn't move around in
> the vtable-buffer.  Both of these, I think, are satisfied via
> with-current-buffer and we avoid the warning.  So, perhaps, we should bite
> the bullet and swallow the extra indentation in vtable-insert-object and
> use the more robust and simpler w-c-b.
>

I can get a revised patch out quickly.  Can we agree on using
with-current-buffer and suffer the few extra indented diff lines?

[Message part 2 (text/html, inline)]

Message #68 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer
 slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Mon, 15 Dec 2025 16:59:40 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:
> @@ -398,56 +403,58 @@ vtable-insert-object
>            ;; Otherwise, append the object.
>            (nconc (vtable-objects table) (list object)))))
>      ;; Then adjust the cache and display.
> -    (save-excursion
> -      (vtable-goto-table table)
> -      (let* ((cache (vtable--current-cache))
> -             (inhibit-read-only t)
> -             (keymap (get-text-property (point) 'keymap))
> -             (ellipsis (if (vtable-ellipsis table)
> -                           (propertize (truncate-string-ellipsis)
> -                                       'face (vtable-face table))
> -                         ""))
> -             (ellipsis-width (string-pixel-width ellipsis))
> -             (lines (vtable--cache-lines cache))
> -             (elem (if location  ; This binding mirrors the binding of `pos' above.
> -                       (if (integerp location)
> -                           (nth location lines)
> -                         (or (assq location lines)
> -                             (and before (car lines))))
> -                     (if before (car lines))))
> -             (pos (memq elem lines))
> -             (line (cons object (vtable--compute-cached-line table object))))
> -        (if (or before
> -                (and pos (integerp location)))
> -            ;; Add the new object before:.
> -            (let ((old-line (car pos)))
> -              (setcar pos line)
> -              (setcdr pos (cons old-line (cdr pos)))
> -              (unless (vtable-goto-object (car elem))
> -                (vtable-beginning-of-table)))
> -          ;; Otherwise, add the object after.
> -          (if pos
> -              ;; Splice the object into the list.
> -              (progn
> -                (setcdr pos (cons line (cdr pos)))
> -                (if (vtable-goto-object location)
> -                    (forward-line 1)  ; Insert *after*.
> -                  (vtable-end-of-table)))
> -            ;; Otherwise, append the object.
> -            (setcar cache (nconc lines (list line)))
> -            (vtable-end-of-table)))
> -        (let ((start (point)))
> -          ;; FIXME: We have to adjust colors in lines below this if we
> -          ;; have :row-colors.
> -          (vtable--insert-line table line 0
> -                               (vtable--cache-widths cache) (vtable--spacer table)
> -                               ellipsis ellipsis-width)
> -          (add-text-properties start (point) (list 'keymap keymap
> -                                                   'vtable table
> -                                                   'vtable-cache cache)))
> -        ;; We may have inserted a non-numerical value into a previously
> -        ;; all-numerical table, so recompute.
> -        (vtable--recompute-numerical table (cdr line))))))
> +    (let* ((cache (vtable--current-cache table))
> +           (inhibit-read-only t)
> +           (inhibit-modification-hooks t)
> +           (lines (vtable--cache-lines cache))
> +           (elem (if location  ; This binding mirrors the binding of `pos' above.
> +                     (if (integerp location)
> +                         (nth location lines)
> +                       (or (assq location lines)
> +                           (and before (car lines))))
> +                   (if before (car lines))))
> +           (pos (memq elem lines))
> +           (line (cons object (vtable--compute-cached-line table object))))
> +      (with-current-buffer (vtable-buffer table)
> +        (save-excursion
> +          (vtable-goto-table table)
> +          (if (or before
> +                  (and pos (integerp location)))
> +              ;; Add the new object before:.
> +              (let ((old-line (car pos)))
> +                (setcar pos line)
> +                (setcdr pos (cons old-line (cdr pos)))
> +                (unless (vtable-goto-object (car elem))
> +                  (vtable-beginning-of-table)))
> +            ;; Otherwise, add the object after.
> +            (if pos
> +                ;; Splice the object into the list.
> +                (progn
> +                  (setcdr pos (cons line (cdr pos)))
> +                  (if (vtable-goto-object location)
> +                      (forward-line 1)  ; Insert *after*.
> +                    (vtable-end-of-table)))
> +              ;; Otherwise, append the object.
> +              (setcar cache (nconc lines (list line)))
> +              (vtable-end-of-table)))
> +          (let* ((start (point))
> +                 (ellipsis (if (vtable-ellipsis table)
> +                               (propertize (truncate-string-ellipsis)
> +                                           'face (vtable-face table))
> +                             ""))
> +                 (ellipsis-width (string-pixel-width ellipsis (current-buffer)))
> +                 (keymap (get-text-property (point) 'keymap)))

The inhibit-read-only and inhibit-modification-hooks bindings should be
here, inside the with-current-buffer, not in the outer let-binding.  The
bindings are ineffective when done in this order if there are
buffer-local bindings for inhibit-read-only or
inhibit-modification-hooks.  It might be nice to just have a single test
checking that things work if there are such buffer-local bindings.

Message #71 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stéphane Marks <shipmints <at> gmail.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer
 slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Mon, 15 Dec 2025 17:00:19 -0500

Stéphane Marks <shipmints <at> gmail.com> writes:

> On Fri, Dec 12, 2025 at 10:48 AM Stéphane Marks <shipmints <at> gmail.com> wrote:
>
>  On Fri, Dec 12, 2025 at 10:35 AM Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>
>  Stéphane Marks <shipmints <at> gmail.com> writes:
>
>  > On Fri, Dec 12, 2025 at 9:33 AM Stéphane Marks <shipmints <at> gmail.com> wrote:
>  >
>  >  On Fri, Dec 12, 2025 at 8:23 AM Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>  >
>  >  Stéphane Marks <shipmints <at> gmail.com> writes:
>  >  > --- a/lisp/emacs-lisp/vtable.el
>  >  > +++ b/lisp/emacs-lisp/vtable.el
>  >  > @@ -398,56 +403,58 @@ vtable-insert-object
>  >  ...
>  >  > +      (with-current-buffer (vtable-buffer table)
>  >  > +        (save-excursion
>  >
>  >  So in the end I think the only way to keep the diff to vtable-insert-object small is
>  >  if you use (set-buffer (vtable-buffer table)) here instead of
>  >  with-current-buffer.  It's a bit less normal, but it at least avoids
>  >  some unnecessary indentation.
>  >
>  >  That is, instead of
>  >  (with-current-buffer (vtable-buffer table)
>  >    (save-excursion
>  >      ...))
>  >
>  >  do
>  >
>  >  (save-excursion
>  >    (set-buffer (vtable-buffer table))
>  >    ...)
>  >
>  >  (save-excursion saves and restores the current buffer)
>  >
>  >  Thanks for making all these changes.  I think after that we should be
>  >  good to go.
>  >
>  >  While I appreciate the desire for tiny diffs, I prefer that the stanzas used in update insert remove functions remain
>  consistent
>  >  and we avoid a special comment explaining why insert is special saying this was just for indentation.  Hrumph.
>  >
>  > The alternative would be to make them all save-excursion, set-buffer, if you think that's worthy.
>
>  IMO that's worth doing.  It also saves indentation, so it's actually
>  somewhat useful in all the places we're currently adding a
>  with-current-buffer, even if it doesn't avoid a big reindent diff.
>
>  I forgot about the compiler warning
>
>    emacs-lisp/vtable.el:376:10: Warning: Use ‘with-current-buffer’ rather than save-excursion+set-buffer
>
>  We may also need save-excursion twice, once for what is the ambient current buffer, and the second to ensure that point doesn't
>  move around in the vtable-buffer.  Both of these, I think, are satisfied via with-current-buffer and we avoid the warning.  So,
>  perhaps, we should bite the bullet and swallow the extra indentation in vtable-insert-object and use the more robust and simpler
>  w-c-b.
>
> I can get a revised patch out quickly.  Can we agree on using with-current-buffer and suffer the few extra indented diff lines?

Yes, okay, I agree it's fine.  I don't think you need to revise anything
there, your most recent patch is fine.  (Except for the other issue I
just emailed about)

Message #74 received at 79982 <at> debbugs.gnu.org (full text, mbox):

From: Stéphane Marks <shipmints <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Kristoffer Balintona <krisbalintona <at> gmail.com>,
 Joost Kremers <joostkremers <at> fastmail.fm>, ijqq <at> protonmail.com,
 79982 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com>,
 Adam Porter <adam <at> alphapapa.net>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#79982: [PATCH] Add vtable buffer slot, [PATCH] Add vtable
 buffer slot, [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot,
 [PATCH] Add vtable buffer slot, [PATCH] Add vtable buffer slot
Date: Mon, 15 Dec 2025 18:29:36 -0500

[Message part 1 (text/plain, inline)]

On Mon, Dec 15, 2025 at 5:00 PM Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stéphane Marks <shipmints <at> gmail.com> writes:
>
> > On Fri, Dec 12, 2025 at 10:48 AM Stéphane Marks <shipmints <at> gmail.com>
> wrote:
> >
> >  On Fri, Dec 12, 2025 at 10:35 AM Spencer Baugh <sbaugh <at> janestreet.com>
> wrote:
> >
> >  Stéphane Marks <shipmints <at> gmail.com> writes:
> >
> >  > On Fri, Dec 12, 2025 at 9:33 AM Stéphane Marks <shipmints <at> gmail.com>
> wrote:
> >  >
> >  >  On Fri, Dec 12, 2025 at 8:23 AM Spencer Baugh <sbaugh <at> janestreet.com>
> wrote:
> >  >
> >  >  Stéphane Marks <shipmints <at> gmail.com> writes:
> >  >  > --- a/lisp/emacs-lisp/vtable.el
> >  >  > +++ b/lisp/emacs-lisp/vtable.el
> >  >  > @@ -398,56 +403,58 @@ vtable-insert-object
> >  >  ...
> >  >  > +      (with-current-buffer (vtable-buffer table)
> >  >  > +        (save-excursion
> >  >
> >  >  So in the end I think the only way to keep the diff to
> vtable-insert-object small is
> >  >  if you use (set-buffer (vtable-buffer table)) here instead of
> >  >  with-current-buffer.  It's a bit less normal, but it at least avoids
> >  >  some unnecessary indentation.
> >  >
> >  >  That is, instead of
> >  >  (with-current-buffer (vtable-buffer table)
> >  >    (save-excursion
> >  >      ...))
> >  >
> >  >  do
> >  >
> >  >  (save-excursion
> >  >    (set-buffer (vtable-buffer table))
> >  >    ...)
> >  >
> >  >  (save-excursion saves and restores the current buffer)
> >  >
> >  >  Thanks for making all these changes.  I think after that we should be
> >  >  good to go.
> >  >
> >  >  While I appreciate the desire for tiny diffs, I prefer that the
> stanzas used in update insert remove functions remain
> >  consistent
> >  >  and we avoid a special comment explaining why insert is special
> saying this was just for indentation.  Hrumph.
> >  >
> >  > The alternative would be to make them all save-excursion, set-buffer,
> if you think that's worthy.
> >
> >  IMO that's worth doing.  It also saves indentation, so it's actually
> >  somewhat useful in all the places we're currently adding a
> >  with-current-buffer, even if it doesn't avoid a big reindent diff.
> >
> >  I forgot about the compiler warning
> >
> >    emacs-lisp/vtable.el:376:10: Warning: Use ‘with-current-buffer’
> rather than save-excursion+set-buffer
> >
> >  We may also need save-excursion twice, once for what is the ambient
> current buffer, and the second to ensure that point doesn't
> >  move around in the vtable-buffer.  Both of these, I think, are
> satisfied via with-current-buffer and we avoid the warning.  So,
> >  perhaps, we should bite the bullet and swallow the extra indentation in
> vtable-insert-object and use the more robust and simpler
> >  w-c-b.
> >
> > I can get a revised patch out quickly.  Can we agree on using
> with-current-buffer and suffer the few extra indented diff lines?
>
> Yes, okay, I agree it's fine.  I don't think you need to revise anything
> there, your most recent patch is fine.  (Except for the other issue I
> just emailed about)
>

Thanks for catching the buffer-local let bindings.

Here's a revised patch.  Up next, a simpler patch optimizing
vtable--limit-string with a string-length heuristic, and which a bunch of
us have tested.

-Stéphane

[Message part 2 (text/html, inline)]

[0001-Add-vtable-buffer-slot.patch (application/octet-stream, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.