bug#69666: [PATCH] (vtable-update-object): Make old-object argument optional

bug#69666: [PATCH] (vtable-update-object): Make old-object argument optional

[Adam Porter]

[-- Attachment #1: Type: text/plain, Size: 225 bytes --]

Hi,

Please see the attached patch which makes `vtable-update-object' easier 
to use in the common case of updating an existing object's 
representation in a table (rather than replacing it with another object).

Thanks,
Adam

[-- Attachment #2: 0001-vtable-update-object-Make-old-object-argument-option.patch --]
[-- Type: text/x-patch, Size: 2337 bytes --]

From ebb5a2a6ea5bc0437fd39d0e87406fe723183e5a Mon Sep 17 00:00:00 2001
From: Adam Porter <adam@alphapapa.net>
Date: Fri, 8 Mar 2024 23:43:14 -0600
Subject: [PATCH] (vtable-update-object): Make old-object argument optional

* lisp/emacs-lisp/vtable.el (vtable-update-object): Make 'old-object'
argument optional.
* doc/misc/vtable.texi (Interface Functions): Update documentation.

It's often necessary to update the representation of a single object in
a table (e.g a struct, whose identity does not change when its slots'
values are changed).  To do so, now the function may be called like
this:

  (vtable-update-object table foo)

Instead of like this:

  (vtable-update-object table foo foo)
---
 doc/misc/vtable.texi      | 7 ++++---
 lisp/emacs-lisp/vtable.el | 7 +++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/doc/misc/vtable.texi b/doc/misc/vtable.texi
index a4f2ed29d93..f6a2ae05818 100644
--- a/doc/misc/vtable.texi
+++ b/doc/misc/vtable.texi
@@ -554,9 +554,10 @@ Interface Functions
 also updates the displayed table.
 @end defun
 
-@defun vtable-update-object table object old-object
-Change @var{old-object} into @var{object} in @var{table}.  This also
-updates the displayed table.
+@defun vtable-update-object table object &optional old-object
+Change @var{old-object} into @var{object} in @var{table}; or, without
+@var{old-object}, update existing @var{object} in @var{table}.  This
+also updates the displayed table.
 
 This has the same effect as calling @code{vtable-remove-object} and
 then @code{vtable-insert-object}, but is more efficient.
diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
index 02020552e7f..ae6a5296cef 100644
--- a/lisp/emacs-lisp/vtable.el
+++ b/lisp/emacs-lisp/vtable.el
@@ -283,8 +283,11 @@ vtable-goto-column
       (goto-char (prop-match-beginning match))
     (end-of-line)))
 
-(defun vtable-update-object (table object old-object)
-  "Replace OLD-OBJECT in TABLE with OBJECT."
+(defun vtable-update-object (table object &optional old-object)
+  "Replace OLD-OBJECT in TABLE with OBJECT.
+Without OLD-OBJECT, just update existing OBJECT in 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.
-- 
2.30.2

bug#69666: [PATCH] (vtable-update-object): Make old-object argument optional

[Eli Zaretskii]

> Date: Fri, 8 Mar 2024 23:51:33 -0600
> From: Adam Porter <adam@alphapapa.net>
> 
> Please see the attached patch which makes `vtable-update-object' easier 
> to use in the common case of updating an existing object's 
> representation in a table (rather than replacing it with another object).

Thanks, I have some minor comments below.

> Subject: [PATCH] (vtable-update-object): Make old-object argument optional

Since this changes the API of a public function, we need to call this
out in NEWS.

> +@defun vtable-update-object table object &optional old-object
> +Change @var{old-object} into @var{object} in @var{table}; or, without
> +@var{old-object}, update existing @var{object} in @var{table}.  This
> +also updates the displayed table.

This is backwards: the documentation should first say what happens if
the function is called with just 2 arguments, and then what happens if
the 3rd one is supplied.  Like this:

  Update @var{object} in @var{table} and redisplay @var{table}.
  Optional argument @var{old-object}, if non-@code{nil}, means to
  change @var{old-object} into @var{object}.

> +(defun vtable-update-object (table object &optional old-object)
> +  "Replace OLD-OBJECT in TABLE with OBJECT.
> +Without OLD-OBJECT, just update existing OBJECT in TABLE."

Same here.

bug#69666: [PATCH] (vtable-update-object): Make old-object argument optional

[Adam Porter]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

Hi Eli,

On 3/14/24 04:00, Eli Zaretskii wrote:
>> Date: Fri, 8 Mar 2024 23:51:33 -0600
>> From: Adam Porter <adam@alphapapa.net>
>>
>> Please see the attached patch which makes `vtable-update-object' easier
>> to use in the common case of updating an existing object's
>> representation in a table (rather than replacing it with another object).
> 
> Thanks, I have some minor comments below.

Thanks for your review.  Please see the attached patch which addresses 
those three items and is rebased on current master.  Please let me know 
if I need to make any further changes.

Thanks,
Adam

[-- Attachment #2: 0001-vtable-update-object-Make-old-object-argument-option.patch --]
[-- Type: text/x-patch, Size: 3527 bytes --]

From 4b762015ebb82d1861c2efe51c1e902ac329bc8e Mon Sep 17 00:00:00 2001
From: Adam Porter <adam@alphapapa.net>
Date: Fri, 8 Mar 2024 23:43:14 -0600
Subject: [PATCH] (vtable-update-object): Make old-object argument optional
 (Bug#69666)

* lisp/emacs-lisp/vtable.el (vtable-update-object): Make 'old-object'
argument optional.
* doc/misc/vtable.texi (Interface Functions): Update documentation.
* etc/NEWS: Add news entry.

It's often necessary to update the representation of a single object in
a table (e.g a struct, whose identity does not change when its slots'
values are changed).  To do so, now the function may be called like
this:

  (vtable-update-object table object)

Instead of like this:

  (vtable-update-object table object object)
---
 doc/misc/vtable.texi      | 9 ++++++---
 etc/NEWS                  | 9 +++++++++
 lisp/emacs-lisp/vtable.el | 9 +++++++--
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/doc/misc/vtable.texi b/doc/misc/vtable.texi
index a4f2ed29d93..6881e9663c4 100644
--- a/doc/misc/vtable.texi
+++ b/doc/misc/vtable.texi
@@ -554,9 +554,12 @@ Interface Functions
 also updates the displayed table.
 @end defun
 
-@defun vtable-update-object table object old-object
-Change @var{old-object} into @var{object} in @var{table}.  This also
-updates the displayed table.
+@defun vtable-update-object table object &optional old-object
+Update @var{object}'s representation in @var{table}.  Optional argument
+@var{old-object}, if non-@code{nil}, means to replace @var{old-object}
+with @var{object} and redisplay the associated row in the table.  In
+either case, if the existing object is not found in the table (being
+compared with @code{equal}), signal an error.
 
 This has the same effect as calling @code{vtable-remove-object} and
 then @code{vtable-insert-object}, but is more efficient.
diff --git a/etc/NEWS b/etc/NEWS
index a654d2d8d79..85751074aaa 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2175,6 +2175,15 @@ aforementioned functions:
     (and (arrayp executing-kbd-macro)
          (>= executing-kbd-macro-index (length executing-kbd-macro))))
 
++++
+** 'vtable-update-object' updates an existing object with just two arguments.
+It is now possible to update the representation of an object in a vtable
+by calling 'vtable-update-object' with just the vtable and the object as
+arguments.  (Previously the 'old-object' argument was required which, in
+this case, would mean repeating the object in the argument list.)  When
+replacing an object with a different one, passing both the new and old
+objects is still necessary.
+
 \f
 * Changes in Emacs 30.1 on Non-Free Operating Systems
 
diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
index 15a430f5c26..dfcfa434135 100644
--- a/lisp/emacs-lisp/vtable.el
+++ b/lisp/emacs-lisp/vtable.el
@@ -283,8 +283,13 @@ vtable-goto-column
       (goto-char (prop-match-beginning match))
     (end-of-line)))
 
-(defun vtable-update-object (table object old-object)
-  "Replace OLD-OBJECT in TABLE with OBJECT."
+(defun vtable-update-object (table object &optional old-object)
+  "Update OBJECT's representation in TABLE.
+When 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."
+  (unless old-object
+    (setq old-object object))
   (let* ((objects (vtable-objects table))
          (inhibit-read-only t))
     ;; First replace the object in the object storage.
-- 
2.30.2

bug#69666: [PATCH] (vtable-update-object): Make old-object argument optional

[Eli Zaretskii]

> Date: Fri, 15 Mar 2024 19:41:33 -0500
> Cc: 69666@debbugs.gnu.org
> From: Adam Porter <adam@alphapapa.net>
> 
> > Thanks, I have some minor comments below.
> 
> Thanks for your review.  Please see the attached patch which addresses 
> those three items and is rebased on current master.  Please let me know 
> if I need to make any further changes.

Thanks, this LGTM.  Just a couple of minor nits:

> Subject: [PATCH] (vtable-update-object): Make old-object argument optional
>  (Bug#69666)

This is too long for the generated ChangeLog.  Since this just
reiterates what the log message below it says, I suggest:

  . make the heading more terse, like

     'vtable-update-object' can now be called with one argument

  . move the bug number to the actual log entry:

    * lisp/emacs-lisp/vtable.el (vtable-update-object): Make 'old-object'
    argument optional.  (Bug#69666)

> +(defun vtable-update-object (table object &optional old-object)
> +  "Update OBJECT's representation in TABLE.
> +When OLD-OBJECT is non-nil, replace OLD-OBJECT with OBJECT and display

"When" has a meaning of time, which is not what you mean here.  I
suggest to use "If" in these cases.

bug#69666: [PATCH] (vtable-update-object): Make old-object argument optional

[Adam Porter]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

Hi Eli,

Thanks, please see the attached patch.

On 3/16/24 05:29, Eli Zaretskii wrote:

> Thanks, this LGTM.  Just a couple of minor nits:
> 
>> Subject: [PATCH] (vtable-update-object): Make old-object argument
>> optional (Bug#69666)
> 
> This is too long for the generated ChangeLog.  Since this just 
> reiterates what the log message below it says, I suggest:
> 
> . make the heading more terse, like
> 
> 'vtable-update-object' can now be called with one argument
> 
> . move the bug number to the actual log entry:
> 
> * lisp/emacs-lisp/vtable.el (vtable-update-object): Make
> 'old-object' argument optional.  (Bug#69666)

Done.

For future reference, is there a way to know whether the heading line is
too long for the ChangeLog entry, other than manually counting?

> "When" has a meaning of time, which is not what you mean here.  I 
> suggest to use "If" in these cases.

Done.

Also, please note that I just discovered another limitation of this
function, which I've filed as bug#69837.  Since it seems quite
important, I've also updated the patch to mention the limitation in the
docstring and manual.

Thanks,
Adam

[-- Attachment #2: 0001-vtable-update-object-can-now-be-called-with-one-argu.patch --]
[-- Type: text/x-patch, Size: 4623 bytes --]

From 92e2d16b0da3dd1d19b7ac4e3a80fec7967f1259 Mon Sep 17 00:00:00 2001
From: Adam Porter <adam@alphapapa.net>
Date: Fri, 8 Mar 2024 23:43:14 -0600
Subject: [PATCH] 'vtable-update-object' can now be called with one argument

* lisp/emacs-lisp/vtable.el (vtable-update-object): Make 'old-object'
argument optional.  (Bug#69666)
* doc/misc/vtable.texi (Interface Functions): Update documentation.
* etc/NEWS: Add news entry.

It's often necessary to update the representation of a single object in
a table (e.g a struct, whose identity does not change when its slots'
values are changed).  To do so, now the function may be called like
this:

  (vtable-update-object table object)

Instead of like this:

  (vtable-update-object table object object)

This also documents the behavior of the just-discovered limitation filed
as bug#69837.
---
 doc/misc/vtable.texi      | 13 ++++++++++---
 etc/NEWS                  |  9 +++++++++
 lisp/emacs-lisp/vtable.el | 15 +++++++++++++--
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/doc/misc/vtable.texi b/doc/misc/vtable.texi
index a4f2ed29d93..dd5b70cf32f 100644
--- a/doc/misc/vtable.texi
+++ b/doc/misc/vtable.texi
@@ -554,12 +554,19 @@ Interface Functions
 also updates the displayed table.
 @end defun
 
-@defun vtable-update-object table object old-object
-Change @var{old-object} into @var{object} in @var{table}.  This also
-updates the displayed table.
+@defun vtable-update-object table object &optional old-object
+Update @var{object}'s representation in @var{table}.  Optional argument
+@var{old-object}, if non-@code{nil}, means to replace @var{old-object}
+with @var{object} and redisplay the associated row in the table.  In
+either case, if the existing object is not found in the table (being
+compared with @code{equal}), signal an error.
 
 This has the same effect as calling @code{vtable-remove-object} and
 then @code{vtable-insert-object}, but is more efficient.
+
+Note a limitation: if the 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.
 @end defun
 
 @defun vtable-column table index
diff --git a/etc/NEWS b/etc/NEWS
index a654d2d8d79..85751074aaa 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2175,6 +2175,15 @@ aforementioned functions:
     (and (arrayp executing-kbd-macro)
          (>= executing-kbd-macro-index (length executing-kbd-macro))))
 
++++
+** 'vtable-update-object' updates an existing object with just two arguments.
+It is now possible to update the representation of an object in a vtable
+by calling 'vtable-update-object' with just the vtable and the object as
+arguments.  (Previously the 'old-object' argument was required which, in
+this case, would mean repeating the object in the argument list.)  When
+replacing an object with a different one, passing both the new and old
+objects is still necessary.
+
 \f
 * Changes in Emacs 30.1 on Non-Free Operating Systems
 
diff --git a/lisp/emacs-lisp/vtable.el b/lisp/emacs-lisp/vtable.el
index 15a430f5c26..de52ab8b36f 100644
--- a/lisp/emacs-lisp/vtable.el
+++ b/lisp/emacs-lisp/vtable.el
@@ -283,8 +283,16 @@ vtable-goto-column
       (goto-char (prop-match-beginning match))
     (end-of-line)))
 
-(defun vtable-update-object (table object old-object)
-  "Replace OLD-OBJECT in TABLE with OBJECT."
+(defun vtable-update-object (table object &optional old-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."
+  (unless old-object
+    (setq old-object object))
   (let* ((objects (vtable-objects table))
          (inhibit-read-only t))
     ;; First replace the object in the object storage.
@@ -300,6 +308,9 @@ vtable-update-object
         (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))))
-- 
2.30.2

bug#69666: [PATCH] (vtable-update-object): Make old-object argument optional

[Eli Zaretskii]

> Date: Sat, 16 Mar 2024 23:29:52 -0500
> Cc: 69666@debbugs.gnu.org
> From: Adam Porter <adam@alphapapa.net>
> 
> Thanks, please see the attached patch.

Thanks, installed, and closing the bug.

> For future reference, is there a way to know whether the heading line is
> too long for the ChangeLog entry, other than manually counting?

I suggest to arrange for fill-column to be set in the buffers where
you edit the log messages, then M-q or auto-fill-mode will do this for
you.