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

[Eli Zaretskii]

> Cc: 72995@debbugs.gnu.org
> Date: Tue, 03 Sep 2024 13:11:40 +0200
> From:  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, do you think this should be installed on the emacs-30 branch
now, even though we had no response from Dale?  If so, please
install, and thanks.