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

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

[Dale]

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!

Best regards,
Dale


In GNU Emacs 31.0.50 (build 1, aarch64-apple-darwin23.6.0, NS
appkit-2487.70 Version 14.6.1 (Build 23G93)) of 2024-09-01 built on
mymacRepository revision: 0f98515cec1298bc82c589b5f8565503900b806d
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2487
System Description:  macOS 14.6.1

Configured using:
 'configure --without-x --with-xwidgets --with-json --with-tree-sitter --without-imagemagick --with-xpm --with-jpeg --with-tiff --with-gif --with-png --with-rsvg --with-webp --with-sqlite3 --with-cairo --with-xml2 --with-gnutls --with-zlib --with-modules --with-threads --with-native-compilation --with-ns --enable-ns-self-contained 'CFLAGS= -D_DARWIN_UNLIMITED_SELECT=1 -DFD_SETSIZE=10240''

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]

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

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.

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]

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

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

[Eli Zaretskii]

> From: Dale <dale@codefu.org>
> Date: Mon, 2 Sep 2024 23:36:35 -0500
> 
> 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!
> 
> Best regards,
> Dale

Stephen, could you please look into this?