bug#72995: 31.0.50; widget-move fails when widget starts at the second character in the buffer

[Stephen Berman via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

On Mon, 2 Sep 2024 23:36:35 -0500 Dale <dale@codefu.org> wrote:

> I think changes in commit 94dec95 (bug#69943) broke `widget-move' in a
> customize buffer when trying to move to the first widget in a buffer when that
> first widget starts at the second character in the buffer.  Here's some code
> to reproduce (tested in IELM):
>
> (let* ((first-wid (progn (widget-forward 1) (point))))
>   (print (format "First widget at %S" first-wid))
>   (cl-assert (and (numberp first-wid) (>= first-wid 1)))
>   (with-current-buffer (customize-group 'editing)
>     (narrow-to-region (1- first-wid) (point-max))
>     (goto-char (point-min))
>     (widget-forward 1)
>     (print (format "Expected to be at %S, point=%S" first-wid (point)))))
>
> On my Emacs I get:
>
> "First widget at 33"
>
> "Expected to be at 33, point=32"
>
> I think this happens because of this code near the end of `widget-move' (which
> is called by `widget-forward'):
>
>     (let ((new (widget-tabable-at)))
>       (while (and (eq (widget-tabable-at) new) (not (bobp)))
> 	(backward-char)))
>     (unless (bobp) (forward-char)))
>
> In my test case, as we enter the while loop point is at the start of the first
> widget (AKA "new").  We are not yet at beginning of buffer, so it moves point
> back one character.  Now we are at beginning of buffer, but that doesn't
> matter: the `eq' test fails first, and the loop ends.
>
> However, the `forward-char' never runs because we are indeed at beginning of
> buffer now.  I think this `forward-char' should have been run to put point
> back on the start of the widget.
>
> Bug#70594 also recently modified code around here, but I don't *think* that's
> relevant.
>
> In case you're wondering, this comes up because I use link-hint[1], which
> narrows a customize buffer in exactly the way shown above.
>
> [1]: https://github.com/noctuid/link-hint.el
>
> Please let me know if I can provide any more information!

Thanks for the bug report, which indeed shows a use case that the change
in commit 94dec95 fails to account for.  The reason that commit
conditionalized the call to forward-char was to ensure that tabbing puts
point at the start of the widget, but your case shows using just (bobp)
as the condition is insufficient (I used that because the existing unit
test for widget-move has the first widget starting at BOB).  I think the
following patch closes this gap:

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index e7e6351fcab..7eec06297ce 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -1336,7 +1336,7 @@ widget-move
     (let ((new (widget-tabable-at)))
       (while (and (eq (widget-tabable-at) new) (not (bobp)))
 	(backward-char)))
-    (unless (bobp) (forward-char)))
+    (unless (and (widget-tabable-at) (bobp)) (forward-char)))
   (unless suppress-echo
     (widget-echo-help (point)))
   (run-hooks 'widget-move-hook))

Can you confirm that this fixes your use case?  If so, I think it should
go into emacs-30, since that's where the faulty commit first appeared.
I'll also add a unit test for this case.

Steve Berman