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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eli Zaretskii
> From: Justin Burkett <[hidden email]>
> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <[hidden email]> wrote:

>> From: Justin Burkett <[hidden email]>
>> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eli Zaretskii
> From: Justin Burkett <[hidden email]>
> Date: Fri, 11 Aug 2017 17:10:23 -0400
> Cc: [hidden email]
>
> $ 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Justin Burkett

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 <[hidden email]> wrote:
> From: Justin Burkett <[hidden email]>
> Date: Fri, 11 Aug 2017 17:10:23 -0400
> Cc: [hidden email]
>
> $ 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eli Zaretskii
> From: Justin Burkett <[hidden email]>
> Date: Sat, 12 Aug 2017 07:54:16 -0400
> Cc: [hidden email]
>
>  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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Justin Burkett
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 <[hidden email]> wrote:
> From: Justin Burkett <[hidden email]>
> Date: Sat, 12 Aug 2017 07:54:16 -0400
> Cc: [hidden email]
>
>  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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eli Zaretskii
> From: Justin Burkett <[hidden email]>
> Date: Sat, 12 Aug 2017 09:14:47 -0400
> Cc: [hidden email]
>
> > 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.

Loading...