Patch for beginning-of-visual-line and end-of-visual-line

Patch for beginning-of-visual-line and end-of-visual-line

[Justin Burkett]

Hi,

I found (what seems like) a bug in these two functions. The docstrings
advertise that they should stop if they reach the beginning or end of
buffer and they don't. As a test cast, try

(beginning-of-visual-line 2)

in a buffer with one line. According to the docstring, point should be
at the end of the buffer, but it ends up at the beginning for me in
Emacs 25.2.1.

Here's a patch that works for me.

Justin

diff --git a/lisp/simple.el b/lisp/simple.el
index 3d23fc3..04d8030 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -6682,9 +6682,10 @@ To ignore intangibility, bind
`inhibit-point-motion-hooks' to t."
   (if (/= n 1)
       (let ((line-move-visual t))
  (line-move (1- n) t)))
-  ;; Unlike `move-beginning-of-line', `move-end-of-line' doesn't
-  ;; constrain to field boundaries, so we don't either.
-  (vertical-motion (cons (window-width) 0)))
+  (unless (or (bobp) (eobp))
+    ;; Unlike `move-beginning-of-line', `move-end-of-line' doesn't
+    ;; constrain to field boundaries, so we don't either.
+    (vertical-motion (cons (window-width) 0))))

 (defun beginning-of-visual-line (&optional n)
   "Move point to beginning of current visual line.
@@ -6697,9 +6698,10 @@ To ignore intangibility, bind
`inhibit-point-motion-hooks' to t."
     (if (/= n 1)
  (let ((line-move-visual t))
   (line-move (1- n) t)))
-    (vertical-motion 0)
-    ;; Constrain to field boundaries, like `move-beginning-of-line'.
-    (goto-char (constrain-to-field (point) opoint (/= n 1)))))
+    (unless (or (bobp) (eobp))
+      (vertical-motion 0)
+      ;; Constrain to field boundaries, like `move-beginning-of-line'.
+      (goto-char (constrain-to-field (point) opoint (/= n 1))))))

 (defun kill-visual-line (&optional arg)
   "Kill the rest of the visual line.

Re: Patch for beginning-of-visual-line and end-of-visual-line

[Eli Zaretskii]

> From: Justin Burkett <justin@burkett.cc>
> Date: Fri, 11 Aug 2017 13:24:54 -0400
> 
> I found (what seems like) a bug in these two functions. The docstrings
> advertise that they should stop if they reach the beginning or end of
> buffer and they don't. As a test cast, try
> 
> (beginning-of-visual-line 2)
> 
> in a buffer with one line. According to the docstring, point should be
> at the end of the buffer, but it ends up at the beginning for me in
> Emacs 25.2.1.

I cannot reproduce what you see.  I created a buffer with one line,
and (beginning-of-visual-line 2) ends up at EOB for me, in both Emacs
25.2 and the current master.

Did you try this in "emacs -Q"?  If not, some of your customizations
could be the reason for the behavior you see.

Or maybe you should describe your recipe in more detail.

Re: Patch for beginning-of-visual-line and end-of-visual-line

[Justin Burkett]

> Did you try this in "emacs -Q"?  If not, some of your customizations
> could be the reason for the behavior you see.

Yes, I just tried it on a debian build of 25.1.1 as well. Here are
reproduction steps

$ emacs -Q
C-x b *test* RET
asdf
M-: (beginning-of-visual-line 2)

point ends up at the beginning of the buffer for me. Note this doesn't
happen with a final newline in the buffer.

Justin


On Fri, Aug 11, 2017 at 2:41 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Justin Burkett <justin@burkett.cc>
>> Date: Fri, 11 Aug 2017 13:24:54 -0400
>>
>> I found (what seems like) a bug in these two functions. The docstrings
>> advertise that they should stop if they reach the beginning or end of
>> buffer and they don't. As a test cast, try
>>
>> (beginning-of-visual-line 2)
>>
>> in a buffer with one line. According to the docstring, point should be
>> at the end of the buffer, but it ends up at the beginning for me in
>> Emacs 25.2.1.
>
> I cannot reproduce what you see.  I created a buffer with one line,
> and (beginning-of-visual-line 2) ends up at EOB for me, in both Emacs
> 25.2 and the current master.
>
> Did you try this in "emacs -Q"?  If not, some of your customizations
> could be the reason for the behavior you see.
>
> Or maybe you should describe your recipe in more detail.

Re: Patch for beginning-of-visual-line and end-of-visual-line

[Eli Zaretskii]

> From: Justin Burkett <justin@burkett.cc>
> Date: Fri, 11 Aug 2017 17:10:23 -0400
> Cc: emacs-devel@gnu.org
> 
> $ emacs -Q
> C-x b *test* RET
> asdf
> M-: (beginning-of-visual-line 2)
> 
> point ends up at the beginning of the buffer for me. Note this doesn't
> happen with a final newline in the buffer.

Ah, that was the missing bit.

So I think this behavior is correct, perhaps we should describe this
special case in the doc string.  This function must end up at the
beginning of a visual line, and in your case there's only one such
place.  Note that (beginning-of-visual-line 1) and
(beginning-of-visual-line) all end up in the same place in this case.

Re: Patch for beginning-of-visual-line and end-of-visual-line

[Justin Burkett]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

> So I think this behavior is correct, perhaps we should describe this
> special case in the doc string.  This function must end up at the
> beginning of a visual line, and in your case there's only one such
> place.  Note that (beginning-of-visual-line 1) and
> (beginning-of-visual-line) all end up in the same place in this case.


That's fine with me. While we're at it, move-beginning-of-line has the same
inconsistency in the docstring. Arguably it's not that special of a case.
With 50 lines in a buffer that doesn't end in a newline,
(beginning-of-visual-line 99) does not end up at the end of the buffer as
suggested by the docstring. In other words, it's the result of a buffer not
ending in a newline, not that there is only one line in the buffer.

Also, it would be nice to specify what the return value of these functions
is if the docstring will change.

Thanks,
Justin


On Sat, Aug 12, 2017 at 2:35 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Justin Burkett <justin@burkett.cc>
> > Date: Fri, 11 Aug 2017 17:10:23 -0400
> > Cc: emacs-devel@gnu.org
> >
> > $ emacs -Q
> > C-x b *test* RET
> > asdf
> > M-: (beginning-of-visual-line 2)
> >
> > point ends up at the beginning of the buffer for me. Note this doesn't
> > happen with a final newline in the buffer.
>
> Ah, that was the missing bit.
>
> So I think this behavior is correct, perhaps we should describe this
> special case in the doc string.  This function must end up at the
> beginning of a visual line, and in your case there's only one such
> place.  Note that (beginning-of-visual-line 1) and
> (beginning-of-visual-line) all end up in the same place in this case.
>

[-- Attachment #2: Type: text/html, Size: 2378 bytes --]

Re: Patch for beginning-of-visual-line and end-of-visual-line

[Eli Zaretskii]

> From: Justin Burkett <justin@burkett.cc>
> Date: Sat, 12 Aug 2017 07:54:16 -0400
> Cc: emacs-devel@gnu.org
> 
>  So I think this behavior is correct, perhaps we should describe this
>  special case in the doc string. This function must end up at the
>  beginning of a visual line, and in your case there's only one such
>  place. Note that (beginning-of-visual-line 1) and
>  (beginning-of-visual-line) all end up in the same place in this case.
> 
> That's fine with me. While we're at it, move-beginning-of-line has the same inconsistency in the docstring.

I've fixed the doc strings of both these functions, thanks.

> Also, it would be nice to specify what the return value of these functions is if the docstring will change. 

The return value is unspecified, and I don't think I see why we should
specify it.

Re: Patch for beginning-of-visual-line and end-of-visual-line

[Justin Burkett]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

> The return value is unspecified, and I don't think I see why we should
specify it.

No problem. Thanks again

On Sat, Aug 12, 2017 at 8:13 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Justin Burkett <justin@burkett.cc>
> > Date: Sat, 12 Aug 2017 07:54:16 -0400
> > Cc: emacs-devel@gnu.org
> >
> >  So I think this behavior is correct, perhaps we should describe this
> >  special case in the doc string. This function must end up at the
> >  beginning of a visual line, and in your case there's only one such
> >  place. Note that (beginning-of-visual-line 1) and
> >  (beginning-of-visual-line) all end up in the same place in this case.
> >
> > That's fine with me. While we're at it, move-beginning-of-line has the
> same inconsistency in the docstring.
>
> I've fixed the doc strings of both these functions, thanks.
>
> > Also, it would be nice to specify what the return value of these
> functions is if the docstring will change.
>
> The return value is unspecified, and I don't think I see why we should
> specify it.
>

[-- Attachment #2: Type: text/html, Size: 1738 bytes --]

Re: Patch for beginning-of-visual-line and end-of-visual-line

[Eli Zaretskii]

> From: Justin Burkett <justin@burkett.cc>
> Date: Sat, 12 Aug 2017 09:14:47 -0400
> Cc: emacs-devel@gnu.org
> 
> > The return value is unspecified, and I don't think I see why we should
> specify it.
> 
> No problem. Thanks again

And thank you for bringing this to our attention in the first place.