Subject: Re: bug#69454: Not possible to insert an empty vtable

From: Joost Kremers <[email-masked]>
Date: Thu, 30 May 2024 23:40:29 +0200

Hi all,

>>>> Date: Wed, 28 Feb 2024 15:29:11 +0100
>>>> From: Eric Marsden <[email-masked]>
>>>> 
>>>> Hello,
>>>> 
>>>> The following generates an error. It seems to me that it would be 
>>>> preferable to insert the header line and show zero rows for the vtable.
>>>> 
>>>>     (require 'vtable)
>>>>     (make-vtable :columns '("tweedle" "dum") :objects (list))
>>>> 
>>>> Debugger entered--Lisp error: (wrong-number-of-arguments #<subr max> 0)
>>>>    max()
>>>>    apply(max nil)
>>>>    seq-max(nil)

I did some work on fixing this, and would like to suggest the attached patches.
There are three, which I could also squash into a single patch if preferred, but
although related, they are conceptually separate, I think. They are:

0001-Allow-empty-vtable.patch

This fixes the problem that if `make-vtable` is called without any objects and
the widths of (some of) the columns aren't specified, `vtable--compute-widths`
would error out. With the patch, columns without an explicit width get assigned
equal parts of the remaining window width.

0002-Fix-recomputing-of-vtable-column-alignment.patch

This one fixes what I believe to be another bug: `vtable--compute-columns`
should be able to recompute the columns, including setting the alignment
property. It didn't actually do the latter, though.

0003-Enable-inserting-new-objects-into-empty-vtable.patch

This patch makes it possible to insert an object into an empty vtable. It does
this by simply recreating and redisplaying the table, which I think makes the
most sense.

Comments?

TIA

-- 
Joost Kremers
Life has its moments

>From 45a84160d5557ee2004fa7e045f52854b9c8e3c5 Mon Sep 17 00:00:00 2001
From: Joost Kremers <[email-masked]>
Date: Thu, 30 May 2024 13:28:00 +0200
Subject: [PATCH 1/3] Allow empty vtable

* lisp/emacs-lisp/vtable.el (vtable--compute-widths): Set default width
  for columns that have no explicit width and no data.
---
 lisp/emacs-lisp/vtable.el | 67 ++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 26 deletions(-)

diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
index cb7ea397314..07ef7d20020 100644
--- a/lisp/emacs-lisp/vtable.el
+++ b/lisp/emacs-lisp/vtable.el
@@ -850,32 +850,47 @@ vtable--compute-width
     (error "Invalid spec: %s" spec))))
 
 (defun vtable--compute-widths (table cache)
-  "Compute the display widths for TABLE."
-  (seq-into
-   (seq-map-indexed
-    (lambda (column index)
-      (let ((width
-             (or
-              ;; Explicit widths.
-              (and (vtable-column-width column)
-                   (vtable--compute-width table (vtable-column-width column)))
-              ;; Compute based on the displayed widths of
-              ;; the data.
-              (seq-max (seq-map (lambda (elem)
-                                  (nth 1 (elt (cdr elem) index)))
-                                cache)))))
-        ;; Let min-width/max-width specs have their say.
-        (when-let ((min-width (and (vtable-column-min-width column)
-                                   (vtable--compute-width
-                                    table (vtable-column-min-width column)))))
-          (setq width (max width min-width)))
-        (when-let ((max-width (and (vtable-column-max-width column)
-                                   (vtable--compute-width
-                                    table (vtable-column-max-width column)))))
-          (setq width (min width max-width)))
-        width))
-    (vtable-columns table))
-   'vector))
+  "Compute the display widths for TABLE.
+CACHE is TABLE's cache data as returned by `vtable--compute-cache'."
+  (let ((widths (seq-map-indexed
+                 (lambda (column index)
+                   (let ((width
+                          (or
+                           ;; Explicit widths.
+                           (and (vtable-column-width column)
+                                (vtable--compute-width table (vtable-column-width column)))
+                           ;; If the vtable is empty and no explicit width is given,
+                           ;; set its width to 0 and deal with it below.
+                           (if (null cache)
+                               0)
+                           ;; Otherwise, compute based on the displayed widths of the
+                           ;; data.
+                           (seq-max (seq-map (lambda (elem)
+                                               (nth 1 (elt (cdr elem) index)))
+                                             cache)))))
+                     ;; Let min-width/max-width specs have their say.
+                     (when-let ((min-width (and (vtable-column-min-width column)
+                                                (vtable--compute-width
+                                                 table (vtable-column-min-width column)))))
+                       (setq width (max width min-width)))
+                     (when-let ((max-width (and (vtable-column-max-width column)
+                                                (vtable--compute-width
+                                                 table (vtable-column-max-width column)))))
+                       (setq width (min width max-width)))
+                     width))
+                 (vtable-columns table))))
+    ;; If there are any zero-width columns, divide the remaining window
+    ;; width evenly over them.
+    (when (member 0 widths)
+      (let* ((combined-width (apply #'+ widths))
+             (n-0cols (length (seq-keep #'zerop widths)))
+             (default-width (/ (- (window-width nil t) combined-width) n-0cols)))
+        (setq widths (mapcar (lambda (width)
+                               (if (zerop width)
+                                   default-width
+                                 width))
+                             widths))))
+    (seq-into widths 'vector)))
 
 (defun vtable--compute-cache (table)
   (seq-map
-- 
2.45.1

>From 34d179d791c10f1e7b8244542e4b59c58715ed47 Mon Sep 17 00:00:00 2001
From: Joost Kremers <[email-masked]>
Date: Thu, 30 May 2024 23:17:37 +0200
Subject: [PATCH 2/3] Fix recomputing of vtable column alignment

* lisp/emacs-lisp/vtable.el (vtable--compute-columns): Store new
  alignment.
---
 lisp/emacs-lisp/vtable.el | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
index 07ef7d20020..b97eaf0e3f3 100644
--- a/lisp/emacs-lisp/vtable.el
+++ b/lisp/emacs-lisp/vtable.el
@@ -485,11 +485,10 @@ vtable--compute-columns
     (seq-map-indexed
      (lambda (column index)
        ;; This is used when displaying.
-       (unless (vtable-column-align column)
-         (setf (vtable-column-align column)
-               (if (elt numerical index)
-                   'right
-                 'left)))
+       (setf (vtable-column-align column)
+             (if (elt numerical index)
+                 'right
+               'left))
        ;; This is used for sorting.
        (setf (vtable-column--numerical column)
              (elt numerical index))
-- 
2.45.1

>From 3c9a3dd95a7a82102bf721843ada3963144636fd Mon Sep 17 00:00:00 2001
From: Joost Kremers <[email-masked]>
Date: Thu, 30 May 2024 23:20:00 +0200
Subject: [PATCH 3/3] Enable inserting new objects into empty vtable

* lisp/emacs-lisp/vtable.el (vtable-insert-object): If the vtable is
  empty, add the new object and recreate + redisplay the table.
---
 lisp/emacs-lisp/vtable.el | 151 +++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 74 deletions(-)

diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
index b97eaf0e3f3..5d152d2d284 100644
--- a/lisp/emacs-lisp/vtable.el
+++ b/lisp/emacs-lisp/vtable.el
@@ -368,86 +368,89 @@ vtable-insert-object
 case.
 
 This also updates the displayed table."
-  ;; FIXME: Inserting an object into an empty vtable currently isn't
-  ;; possible. `nconc' fails silently (twice), and `setcar' on the cache
-  ;; raises an error.
+  ;; If the vtable is empty, just add the object and regenerate the
+  ;; table.
   (if (null (vtable-objects table))
-      (error "[vtable] Cannot insert object into empty vtable"))
-  ;; 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--cache 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))
-           (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)))
-           (line (cons object (vtable--compute-cached-line table object))))
-      (if (or before
+      (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 (car 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)
-                             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)))))
+          (nconc (vtable-objects table) (list object)))))
+    ;; Then adjust the cache and display.
+    (save-excursion
+      (vtable-goto-table table)
+      (let* ((cache (vtable--cache 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))
+             (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)))
+             (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 (car 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)
+                               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."
-- 
2.45.1