Package: emacs;
Reported by: Adam Porter <adam <at> alphapapa.net>
Date: Sun, 17 Mar 2024 03:43:02 UTC
Severity: normal
Tags: patch
Found in version 29.2
View this message in rfc822 format
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stéphane Marks <shipmints <at> gmail.com> Cc: Adam Porter <adam <at> alphapapa.net>, 69837 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, Stefan Kangas <stefankangas <at> gmail.com> Subject: bug#69837: 29.2; vtable-update-object only works in visible windows Date: Thu, 19 Jun 2025 18:02:59 -0400
[Message part 1 (text/plain, inline)]
Stéphane Marks <shipmints <at> gmail.com> writes: > On Thu, Jun 19, 2025 at 5:37 PM Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors > <bug-gnu-emacs <at> gnu.org> wrote: > > Stefan Kangas <stefankangas <at> gmail.com> writes: > > > Eli Zaretskii <eliz <at> gnu.org> writes: > > > >>> Date: Thu, 21 Mar 2024 03:36:02 -0500 > >>> Cc: 69837 <at> debbugs.gnu.org > >>> From: Adam Porter <adam <at> alphapapa.net> > >>> > >>> FWIW, I did some hacking on listen.el attempting to further understand > >>> and work around this problem, and I've found what seems to be a usable > >>> workaround (for my purposes, anyway). > >>> > >>> The attached code defines a macro within which I call > >>> `listen-queue--vtable-update-object' (which merely incorporates the fix > >>> to `vtable-update-object' from bug#69664). As well, I wrap > >>> `make-vtable' in a function that also sets two buffer-local variables to > >>> the values of `frame-terminal' and `window-width' at the time of the > >>> vtable's creation. The macro locally overrides the functions > >>> `frame-terminal' and `window-width' to return those saved values. > >>> > >>> This allows the cache key to match unconditionally, which allows the > >>> vtable's objects to be updated even when its buffer is not visible. > >>> > >>> In my limited testing, it seems to work fine. In my estimation, the > >>> consequences of doing this in the worst case would be that the rows for > >>> the updated objects might be drawn with some columns at a slightly > >>> incorrect width, which is easily rectified by reverting the table > >>> (usually bound to "g"). As well, that worst case (e.g. imagining a > >>> vtable whose buffer might be initially displayed on one terminal/monitor > >>> and later on another with different characteristics) would seem to be > >>> relatively rare (so for my project, it seems like an obviously good > >>> thing to do). > >>> > >>> For Emacs itself, I'm not sure what the best fix would be. I suppose a > >>> workaround like this could be implemented as a fallback in case the > >>> cache key misses; it would seem better to update the object potentially > >>> sub-optimally than to error and not update it at all. > >>> > >>> Another possibility would be to ignore the frame-terminal and > >>> window-width in the cache key altogether (i.e. so they would always be > >>> assumed to be the same), but I'm sure that Lars did it this way for a > >>> reason, so that would seem unwise. > >>> > >>> Let me know how you'd like me to proceed. > >> > >> I think you should install your workaround. I don't see how it could > >> be worse than what we have now. > >> > >> P.S. And sorry for a long silence. > > > > Adam, could you please install the workaround? Or maybe you did > > already? > > I ran into this same problem. I think Adam's workaround is on the right > track, but the issue is present in a number of other functions in > vtable.el, not just vtable-update-object. > > The root of the issue is that when we're interacting with the text of an > inserted vtable, we should use the "cache" object that was last used to > insert that vtable, rather than one based on the current selected window > and terminal. Otherwise we will experience various inconsistencies, > e.g. inserting a new line that has different column widths from the rest > of the vtable text. > > My attached patch solves this by saving the "cache" object in a text > property on the vtable text, and updating all the relevant vtable > functions to access the cache via that text property rather than by > computing a cache key from the current selected window/terminal. > > Adam, could you check that this solves the problem for your use case? > > I have a very big patch coming for vtable under https://debbugs.gnu.org/cgi/bugreport.cgi?bug=78843 which addresses this issue, > and a ton of other bugs (and features). That's coming in the next day or so, if you can wait? Certainly I can wait. Cc me when you send the patch. Though if your patch is very large, maybe this relatively small patch should land first. (Also, I hope that you're adding tests in your very large patch?) BTW, an updated version of my patch is attached, just fixing a silly bug that was in the first version.
[0001-Fix-implicit-usage-of-the-current-window-width-in-vt.patch (text/x-patch, inline)]
From 5607bb42202e050edc5e33b8fa9c1c96d65d8746 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Thu, 19 Jun 2025 17:20:12 -0400
Subject: [PATCH] Fix implicit usage of the current window-width in vtable.el
Previously, many functions in vtable.el called vtable--cache,
which computed vtable--cache-key based on the current selected
window and frame; this could cause vtable functions to fail or
misbehave if they were not called from the selected window and
frame that vtable-insert was last called in.
Now, the vtable cache is stored with the text of the vtable, so
that functions which need to interact with some vtable text can
do so reliably without having to use the same selected window
and frame.
Also, vtable-update-object has always required TABLE to be
present at point in the current buffer; now its docstring states
this.
* lisp/emacs-lisp/vtable.el (vtable--current-cache)
(vtable--cache-widths, vtable--cache-lines): Add.
(vtable-insert): Save cache in 'vtable-cache.
(vtable--ensure-cache, vtable--recompute-cache): Inline into
vtable-insert.
(vtable--widths, vtable--cache): Delete.
(vtable-update-object): Use vtable--current-cache and update
docstring. (bug#69837)
(vtable-remove-object, vtable-insert-object): Use
vtable--current-cache and save cache in 'vtable-cache.
(vtable--sort, vtable--alter-column-width)
(vtable-previous-column, vtable-next-column): Use
vtable--current-cache.
---
lisp/emacs-lisp/vtable.el | 111 ++++++++++++++++++++------------------
1 file changed, 59 insertions(+), 52 deletions(-)
diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
index 00785113edb..d6be6d361f7 100644
--- a/lisp/emacs-lisp/vtable.el
+++ b/lisp/emacs-lisp/vtable.el
@@ -282,10 +282,9 @@ vtable-update-object
"Update OBJECT's representation in TABLE.
If OLD-OBJECT is non-nil, replace OLD-OBJECT with OBJECT and display it.
In either case, if the existing object is not found in the table (being
-compared with `equal'), signal an error. Note a limitation: if TABLE's
-buffer is not in a visible window, or if its window has changed width
-since it was updated, updating the TABLE is not possible, and an error
-is signaled."
+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))
@@ -302,14 +301,13 @@ vtable-update-object
(unless objects
(error "Can't find the old object"))
(setcar (cdr objects) object))
- ;; Then update the cache...
- ;; FIXME: If the table's buffer has no visible window, or if its
- ;; width has changed since the table was updated, the cache key will
- ;; not match and the object can't be updated. (Bug #69837).
- (if-let* ((line-number (seq-position (car (vtable--cache table)) old-object
- (lambda (a b)
- (equal (car a) b))))
- (line (elt (car (vtable--cache table)) line-number)))
+ ;; 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))
@@ -320,10 +318,11 @@ vtable-update-object
(start (point)))
(delete-line)
(vtable--insert-line table line line-number
- (nth 1 (vtable--cache table))
+ (vtable--cache-widths cache)
(vtable--spacer table))
(add-text-properties start (point) (list 'keymap keymap
- 'vtable table))))
+ '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)))
@@ -335,11 +334,12 @@ vtable-remove-object
;; First remove from the objects.
(setf (vtable-objects table) (delq object (vtable-objects table)))
;; Then adjust the cache and display.
- (let ((cache (vtable--cache table))
- (inhibit-read-only t))
- (setcar cache (delq (assq object (car cache)) (car cache)))
- (save-excursion
- (vtable-goto-table table)
+ (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)))))
@@ -400,7 +400,7 @@ vtable-insert-object
;; Then adjust the cache and display.
(save-excursion
(vtable-goto-table table)
- (let* ((cache (vtable--cache table))
+ (let* ((cache (vtable--current-cache))
(inhibit-read-only t)
(keymap (get-text-property (point) 'keymap))
(ellipsis (if (vtable-ellipsis table)
@@ -408,13 +408,14 @@ vtable-insert-object
'face (vtable-face table))
""))
(ellipsis-width (string-pixel-width ellipsis))
- (elem (if location ; This binding mirrors the binding of `pos' above.
+ (lines (vtable--cache-lines cache))
+ (elem (if location ; This binding mirrors the binding of `pos' above.
(if (integerp location)
- (nth location (car cache))
- (or (assq location (car cache))
- (and before (caar cache))))
- (if before (caar cache))))
- (pos (memq elem (car cache)))
+ (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)))
@@ -433,13 +434,13 @@ vtable-insert-object
(forward-line 1) ; Insert *after*.
(vtable-end-of-table)))
;; Otherwise, append the object.
- (setcar cache (nconc (car cache) (list line)))
+ (setcar cache (nconc (vtable--cache-lines cache) (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
- (nth 1 cache) (vtable--spacer table)
+ (vtable--cache-widths cache) (vtable--spacer table)
ellipsis ellipsis-width)
(add-text-properties start (point) (list 'keymap keymap
'vtable table)))
@@ -512,15 +513,11 @@ vtable--compute-columns
(defun vtable--spacer (table)
(vtable--compute-width table (vtable-separator-width table)))
-(defun vtable--recompute-cache (table)
- (let* ((data (vtable--compute-cache table))
- (widths (vtable--compute-widths table data)))
- (setf (gethash (vtable--cache-key) (slot-value table '-cache))
- (list data widths))))
+(defun vtable--cache-widths (cache)
+ (nth 1 cache))
-(defun vtable--ensure-cache (table)
- (or (vtable--cache table)
- (vtable--recompute-cache table)))
+(defun vtable--cache-lines (cache)
+ (car cache))
(defun vtable-insert (table)
(let* ((spacer (vtable--spacer table))
@@ -533,7 +530,12 @@ vtable-insert
;; 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).
- (widths (nth 1 (vtable--ensure-cache table))))
+ (cache (or (gethash (vtable--cache-key) (slot-value table '-cache))
+ (let* ((data (vtable--compute-cache table))
+ (widths (vtable--compute-widths table data)))
+ (setf (gethash (vtable--cache-key) (slot-value table '-cache))
+ (list data widths)))))
+ (widths (vtable--cache-widths cache)))
;; Don't insert any header or header line if the user hasn't
;; specified the columns.
(when (slot-value table '-has-column-spec)
@@ -546,18 +548,20 @@ vtable-insert
(add-text-properties start (point)
(list 'keymap vtable-header-line-map
'rear-nonsticky t
- 'vtable table))
+ 'vtable table
+ 'vtable-cache cache))
(setq start (point))))
- (vtable--sort table)
+ (vtable--sort table cache)
;; Insert the data.
(let ((line-number 0))
- (dolist (line (car (vtable--cache table)))
+ (dolist (line (vtable--cache-lines cache))
(vtable--insert-line table line line-number widths spacer
ellipsis ellipsis-width)
(setq line-number (1+ line-number))))
(add-text-properties start (point)
(list 'rear-nonsticky t
- 'vtable table))
+ 'vtable table
+ 'vtable-cache cache))
(goto-char start)))
(defun vtable--insert-line (table line line-number widths spacer
@@ -659,16 +663,22 @@ vtable--insert-line
(defun vtable--cache-key ()
(cons (frame-terminal) (window-width)))
-(defun vtable--cache (table)
- (gethash (vtable--cache-key) (slot-value table '-cache)))
+(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))
-(defun vtable--sort (table)
+(defun vtable--sort (table cache)
(pcase-dolist (`(,index . ,direction) (vtable-sort-by table))
- (let ((cache (vtable--cache table))
- (numerical (vtable-column--numerical
+ (let ((numerical (vtable-column--numerical
(elt (vtable-columns table) index)))
(numcomp (if (eq direction 'descend)
#'> #'<))
@@ -971,9 +981,6 @@ vtable-revert
(when column
(vtable-goto-column column))))
-(defun vtable--widths (table)
- (nth 1 (vtable--ensure-cache table)))
-
;;; Commands.
(defvar-keymap vtable-header-mode-map
@@ -998,7 +1005,7 @@ vtable-narrow-current-column
(- (* (vtable--char-width table) (or n 1))))))
(defun vtable--alter-column-width (table column delta)
- (let ((widths (vtable--widths table)))
+ (let ((widths (vtable--cache-widths (vtable--current-cache))))
(setf (aref widths column)
(max (* (vtable--char-width table) 2)
(+ (aref widths column) delta)))
@@ -1020,14 +1027,14 @@ vtable-previous-column
(interactive)
(vtable-goto-column
(max 0 (1- (or (vtable-current-column)
- (length (vtable--widths (vtable-current-table))))))))
+ (length (vtable--cache-widths (vtable--current-cache))))))))
(defun vtable-next-column ()
"Go to the next column."
(interactive)
(when (vtable-current-column)
(vtable-goto-column
- (min (1- (length (vtable--widths (vtable-current-table))))
+ (min (1- (length (vtable--cache-widths (vtable--current-cache))))
(1+ (vtable-current-column))))))
(defun vtable-revert-command ()
--
2.39.3
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.