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 Sat, 14 Sep 2024 10:44:42 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> 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?

Yes, and I intended to ask today if I should do that, but you beat me to
the punch :).  The reason I wanted confirmation is that I cannot
reproduce the problem via ielm as in the OP, so perhaps there's more
involved.  But with `M-x customize-group RET editing' and evaluating the
sexp in the Customize buffer, the problem is apparent, and is
reproducible without Customize but just by using simple widgets (that's
what the test I added does).  So it is definitely a regression and
should be fixed in emacs-30.

>                                                 If so, please
> install, and thanks.

Done in commit d509a356997 to emacs-30 and closing the bug.  (If there's
something I missed having to do with ielm, we can reopen the bug.)

Steve Berman