* bug#72995: 31.0.50; widget-move fails when widget starts at the second character in the buffer @ 2024-09-03 4:36 Dale 2024-09-03 11:11 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-09-03 12:35 ` Eli Zaretskii 0 siblings, 2 replies; 5+ messages in thread From: Dale @ 2024-09-03 4:36 UTC (permalink / raw) To: 72995 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'' ^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#72995: 31.0.50; widget-move fails when widget starts at the second character in the buffer 2024-09-03 4:36 bug#72995: 31.0.50; widget-move fails when widget starts at the second character in the buffer Dale @ 2024-09-03 11:11 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-09-14 7:44 ` Eli Zaretskii 2024-09-03 12:35 ` Eli Zaretskii 1 sibling, 1 reply; 5+ messages in thread From: Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-09-03 11:11 UTC (permalink / raw) To: Dale; +Cc: 72995 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#72995: 31.0.50; widget-move fails when widget starts at the second character in the buffer 2024-09-03 11:11 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-09-14 7:44 ` Eli Zaretskii 2024-09-14 10:47 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 5+ messages in thread From: Eli Zaretskii @ 2024-09-14 7:44 UTC (permalink / raw) To: Stephen Berman; +Cc: 72995, dale > 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#72995: 31.0.50; widget-move fails when widget starts at the second character in the buffer 2024-09-14 7:44 ` Eli Zaretskii @ 2024-09-14 10:47 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 0 replies; 5+ messages in thread From: Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-09-14 10:47 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 72995-done, dale 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#72995: 31.0.50; widget-move fails when widget starts at the second character in the buffer 2024-09-03 4:36 bug#72995: 31.0.50; widget-move fails when widget starts at the second character in the buffer Dale 2024-09-03 11:11 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-09-03 12:35 ` Eli Zaretskii 1 sibling, 0 replies; 5+ messages in thread From: Eli Zaretskii @ 2024-09-03 12:35 UTC (permalink / raw) To: Dale, Stephen Berman; +Cc: 72995 > 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? ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-09-14 10:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-03 4:36 bug#72995: 31.0.50; widget-move fails when widget starts at the second character in the buffer Dale 2024-09-03 11:11 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-09-14 7:44 ` Eli Zaretskii 2024-09-14 10:47 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-09-03 12:35 ` Eli Zaretskii
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.