bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Teemu Likonen]

On 2009-07-11 12:03 (-0400), Chong Yidong wrote:

> Does the following patch fix the problem?
>
> diff -c /home/cyd/emacs/lisp/simple.el.\~1.988.\~ /home/cyd/emacs/lisp/simple.el
> *** emacs/lisp/simple.el.~1.988.~	2009-06-21 00:37:46.000000000 -0400
> --- emacs/lisp/simple.el	2009-07-11 11:58:55.000000000 -0400

Not quite, I'm afraid. Setting goal column still messes it up badly. To
reproduce:

 1. emacs -Q -f toggle-truncate-lines

 2. Don't change the size of the window. Create test buffer with
    commands:

        C-u 85 a RET
        C-u 100 b RET

 3. Move the cursor to the end of the first (the line with a's).

 4. Enable goal column: C-x C-n

 5. Repeat C-n and C-p. The column changes quite wildly.

The problem goes away if I set line-move-visual to nil.

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Chong Yidong]

Teemu Likonen <tlikonen@iki.fi> writes:

>  1. emacs -Q -f toggle-truncate-lines
>
>  2. Don't change the size of the window. Create test buffer with
>     commands:
>
>         C-u 85 a RET
>         C-u 100 b RET
>
>  3. Move the cursor to the end of the first (the line with a's).
>
>  4. Enable goal column: C-x C-n
>
>  5. Repeat C-n and C-p. The column changes quite wildly.
>
> The problem goes away if I set line-move-visual to nil.

I can't reproduce this.  Could you try with the latest patch in the
trunk?

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Teemu Likonen]

On 2009-07-11 13:25 (-0400), Chong Yidong wrote:

> Teemu Likonen <tlikonen@iki.fi> writes:
>
>>  1. emacs -Q -f toggle-truncate-lines
>>
>>  2. Don't change the size of the window. Create test buffer with
>>     commands:
>>
>>         C-u 85 a RET
>>         C-u 100 b RET
>>
>>  3. Move the cursor to the end of the first (the line with a's).
>>
>>  4. Enable goal column: C-x C-n
>>
>>  5. Repeat C-n and C-p. The column changes quite wildly.
>>
>> The problem goes away if I set line-move-visual to nil.
>
> I can't reproduce this.  Could you try with the latest patch in the
> trunk?

The above is 100% reproducible in my Debian GNU/Linux system. I use the
GTK version

    GNU Emacs 23.1.50.1 (i686-pc-linux-gnu, GTK+ Version 2.12.12) of
    2009-07-11 on mithlond

This is the revision I compiled (with make bootstrap):

    commit 9bcb78f2eeba70b10d6d1d74dd98c1b36dfc1ca6
    Author: Chong Yidong <cyd@stupidchicken.com>
    Date:   2009-07-11 16:36:05 +0000

        * simple.el (temporary-goal-column): Change the value for
        line-move-visual to a cons cell.
        (line-move-visual): Record or set the window hscroll, if
        necessary (Bug#3494).
        (line-move-1): Handle cons value of temporary-goal-column.

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Chong Yidong]

Teemu Likonen <tlikonen@iki.fi> writes:

>>>  1. emacs -Q -f toggle-truncate-lines
>>>  2. Don't change the size of the window. Create test buffer with
>>>     commands:
>>>         C-u 85 a RET
>>>         C-u 100 b RET
>>>  3. Move the cursor to the end of the first (the line with a's).
>>>  4. Enable goal column: C-x C-n
>>>  5. Repeat C-n and C-p. The column changes quite wildly.
>>>
>>> The problem goes away if I set line-move-visual to nil.
>>
>> I can't reproduce this.  Could you try with the latest patch in the
>> trunk?
>
> The above is 100% reproducible in my Debian GNU/Linux system.
>
>     GNU Emacs 23.1.50.1 (i686-pc-linux-gnu, GTK+ Version 2.12.12) of
>     2009-07-11 on mithlond

I can't reproduce it.  Your recipe is missing one step: C-x C-n is a
disabled command, so you need to type "y" to enable it, as I presume you
intended.  I don't observe any column change, though.

Did you do xrdb -remove beforehand?  (I can't reproduce the problem with
or without Xresource settings.)  What's the size of your Emacs frame?

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Teemu Likonen]

On 2009-07-11 18:59 (-0400), Chong Yidong wrote:

> I can't reproduce it. Your recipe is missing one step: C-x C-n is a
> disabled command, so you need to type "y" to enable it, as I presume
> you intended. I don't observe any column change, though.
>
> Did you do xrdb -remove beforehand? (I can't reproduce the problem
> with or without Xresource settings.) What's the size of your Emacs
> frame?

Yes, I enabled the disabled set-goal-column command with "y". The bug
appears on terminal too. Here's another, even more accurate recipe what
I do. I uploaded screenshots too (see the link below). This time I use
Emacs in terminal:


 1. xrdb -remove; xterm -geometry 80x25

 2. emacs -Q -nw -f toggle-truncate-lines -f column-number-mode

 3. Create content exactly like this:

        C-u 85 a RET
        C-u 100 b RET

 4. Press C-p two times to move the the beginning of "a" line.

 5. Press C-e to get to the end of "a" line. (See screenshot 1.)

 6. Press C-x C-n and "y" to set goal column. This message is displayed:

        Goal column 85 (use C-x C-n with an arg to unset it)

 7. Press C-n. The screen is scrolled horizontally and the cursor at the
    column 94. (See screenshot 2.)


Let's continue to reproduce the bug which appears without goal column.

 8. Disable the goal column: C-u C-x C-n

 9. Move to the beginning of the "a" line.

10. Move to the end of the "a" line (C-e). Again the screen looks like
    in my first screenshot, see the item 5 above.

11. Press C-n to go the next line. The screen is scrolled horizontally
    and the cursor column is correct: 85. (See screenshot 3.)

12. Press C-p to go the previous line. The cursor stays on the "b" line
    but the cursor column is now 79. (See screenshot 4.)


None of these bugs appear when line-move-visual=nil.


Screenshots here:
http://dtw.silverentertainment.fi/pics/emacs/emacs-bug-3494.png

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Chong Yidong]

>  1. xrdb -remove; xterm -geometry 80x25
>  2. emacs -Q -nw -f toggle-truncate-lines -f column-number-mode
>  3. C-u 85 a RET
>     C-u 100 b RET
>  4. Press C-p two times to move the the beginning of "a" line.
>  5. Press C-e to get to the end of "a" line. (See screenshot 1.)
>  6. Press C-x C-n and "y" to set goal column.
>  7. Press C-n. The screen is scrolled horizontally and the cursor at the
>     column 94. (See screenshot 2.)

I see the problem.  It's not trivial to fix; maybe it's preferable for
`line-move' to call `line-move-1' instead of `line-move-visual' when the
window is hscrolled.

Stefan, WDYT?

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Lennart Borgman]

On Sun, Jul 12, 2009 at 6:04 PM, Chong Yidong<cyd@stupidchicken.com> wrote:
>>  1. xrdb -remove; xterm -geometry 80x25
>>  2. emacs -Q -nw -f toggle-truncate-lines -f column-number-mode
>>  3. C-u 85 a RET
>>     C-u 100 b RET
>>  4. Press C-p two times to move the the beginning of "a" line.
>>  5. Press C-e to get to the end of "a" line. (See screenshot 1.)
>>  6. Press C-x C-n and "y" to set goal column.
>>  7. Press C-n. The screen is scrolled horizontally and the cursor at the
>>     column 94. (See screenshot 2.)
>
> I see the problem.  It's not trivial to fix; maybe it's preferable for
> `line-move' to call `line-move-1' instead of `line-move-visual' when the
> window is hscrolled.

I don't understand what is wrong here, but is this perhaps related to
the problem with posn-at-point that I reported for example here:

http://www.opensubscriber.com/message/emacs-devel@gnu.org/10283709.html

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Chong Yidong]

Please test this patch instead of the one I previously sent (I'll
install it on the trunk as well, for additional testing).

*** emacs/lisp/simple.el.~1.988.~	2009-06-21 00:37:46.000000000 -0400
--- emacs/lisp/simple.el	2009-07-11 12:30:05.000000000 -0400
***************
*** 3956,3965 ****
  (defvar temporary-goal-column 0
    "Current goal column for vertical motion.
  It is the column where point was at the start of the current run
! of vertical motion commands.  It is a floating point number when
! moving by visual lines via `line-move-visual'; this is the
! x-position, in pixels, divided by the default column width.  When
! the `track-eol' feature is doing its job, the value is
  `most-positive-fixnum'.")
  
  (defcustom line-move-ignore-invisible t
--- 3956,3969 ----
  (defvar temporary-goal-column 0
    "Current goal column for vertical motion.
  It is the column where point was at the start of the current run
! of vertical motion commands.
! 
! When moving by visual lines via `line-move-visual', it is a cons
! cell (COL . HSCROLL), where COL is the x-position, in pixels,
! divided by the default column width, and HSCROLL is the number of
! columns by which window is scrolled from left margin.
! 
! When the `track-eol' feature is doing its job, the value is
  `most-positive-fixnum'.")
  
  (defcustom line-move-ignore-invisible t
***************
*** 4059,4075 ****
    (let ((posn (posn-at-point))
  	(opoint (point))
  	x)
!     ;; Reset temporary-goal-column, unless the previous command was a
!     ;; line-motion command or we were called from some other command.
!     (unless (and (floatp temporary-goal-column)
! 		 (memq last-command `(next-line previous-line ,this-command)))
!       (cond ((eq (nth 1 posn) 'right-fringe) ; overflow-newline-into-fringe
! 	     (setq temporary-goal-column (- (window-width) 1)))
! 	    ((setq x (car (posn-x-y posn)))
! 	     (setq temporary-goal-column (/ (float x) (frame-char-width))))))
      ;; Move using `vertical-motion'.
      (or (and (= (vertical-motion
! 		 (cons (or goal-column (truncate temporary-goal-column)) arg))
  		arg)
  	     (or (>= arg 0)
  		 (/= (point) opoint)
--- 4063,4095 ----
    (let ((posn (posn-at-point))
  	(opoint (point))
  	x)
!     ;; Check if the previous command was a line-motion command or we
!     ;; were called from some other command.
!     (cond ((and (consp temporary-goal-column)
! 		(memq last-command `(next-line previous-line ,this-command)))
! 	   ;; If so, there's no need to reset `temporary-goal-column',
! 	   ;; unless the window hscroll has changed.
! 	   (when (/= (window-hscroll) (cdr temporary-goal-column))
! 	     (set-window-hscroll nil 0)
! 	     (setq temporary-goal-column
! 		   (cons (+ (car temporary-goal-column)
! 			    (cdr temporary-goal-column)) 0))))
! 	  ;; Otherwise, we should reset `temporary-goal-column'.
! 	  ;; Handle the `overflow-newline-into-fringe' case:
! 	  ((eq (nth 1 posn) 'right-fringe)
! 	   (setq temporary-goal-column (cons (- (window-width) 1)
! 					     (window-hscroll))))
! 	  ((setq x (car (posn-x-y posn)))
! 	   (setq temporary-goal-column
! 		 (cons (/ (float x) (frame-char-width))
! 		       (window-hscroll)))))
      ;; Move using `vertical-motion'.
      (or (and (= (vertical-motion
! 		 (cons (or goal-column
! 			   (if (consp temporary-goal-column)
! 			       (truncate (car temporary-goal-column))
! 			     temporary-goal-column))
! 		       arg))
  		arg)
  	     (or (>= arg 0)
  		 (/= (point) opoint)
***************
*** 4091,4098 ****
    (let ((inhibit-point-motion-hooks t)
  	(opoint (point))
  	(orig-arg arg))
!     (if (floatp temporary-goal-column)
! 	(setq temporary-goal-column (truncate temporary-goal-column)))
      (unwind-protect
  	(progn
  	  (if (not (memq last-command '(next-line previous-line)))
--- 4111,4119 ----
    (let ((inhibit-point-motion-hooks t)
  	(opoint (point))
  	(orig-arg arg))
!     (if (consp temporary-goal-column)
! 	(setq temporary-goal-column (+ (car temporary-goal-column)
! 				       (cdr temporary-goal-column))))
      (unwind-protect
  	(progn
  	  (if (not (memq last-command '(next-line previous-line)))

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Teemu Likonen]

On 2009-07-11 12:31 (-0400), Chong Yidong wrote:

> Please test this patch instead of the one I previously sent (I'll
> install it on the trunk as well, for additional testing).

Still no. There are still weird problems:

 1. emacs -Q -f toggle-truncate-lines

 2. Don't touch the frame layout. Create test content:

        C-u 85 a RET
        C-u 100 b RET

 3. Move to the _start_ of the "a" line, then to the _end_ of the line
    (C-e).

 4. First C-n once then C-p once.

 5. You are at the column 80 of the "b" line.

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Chong Yidong]

Teemu Likonen <tlikonen@iki.fi> writes:

>  1. emacs -Q -f toggle-truncate-lines
>
>  2. Don't touch the frame layout. Create test content:
>
>         C-u 85 a RET
>         C-u 100 b RET
>
>  3. Move to the _start_ of the "a" line, then to the _end_ of the line
>     (C-e).
>
>  4. First C-n once then C-p once.
>
>  5. You are at the column 80 of the "b" line.

I can't reproduce this problem.  Doing C-n, then C-p, I end up on column
85.

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Chong Yidong]

Does the following patch fix the problem?

diff -c /home/cyd/emacs/lisp/simple.el.\~1.988.\~ /home/cyd/emacs/lisp/simple.el
*** emacs/lisp/simple.el.~1.988.~	2009-06-21 00:37:46.000000000 -0400
--- emacs/lisp/simple.el	2009-07-11 11:58:55.000000000 -0400
***************
*** 3956,3966 ****
  (defvar temporary-goal-column 0
    "Current goal column for vertical motion.
  It is the column where point was at the start of the current run
! of vertical motion commands.  It is a floating point number when
! moving by visual lines via `line-move-visual'; this is the
! x-position, in pixels, divided by the default column width.  When
! the `track-eol' feature is doing its job, the value is
! `most-positive-fixnum'.")
  
  (defcustom line-move-ignore-invisible t
    "Non-nil means \\[next-line] and \\[previous-line] ignore invisible lines.
--- 3956,3967 ----
  (defvar temporary-goal-column 0
    "Current goal column for vertical motion.
  It is the column where point was at the start of the current run
! of vertical motion commands.  When moving by visual lines via
! `line-move-visual', it is a cons cell (COL . HSCROLL), where COL
! is the x-position, in pixels, divided by the default column
! width, and HSCROLL is the number of columns by which window is
! scrolled from left margin.  When the `track-eol' feature is doing
! its job, the value is `most-positive-fixnum'.")
  
  (defcustom line-move-ignore-invisible t
    "Non-nil means \\[next-line] and \\[previous-line] ignore invisible lines.
***************
*** 4061,4075 ****
  	x)
      ;; Reset temporary-goal-column, unless the previous command was a
      ;; line-motion command or we were called from some other command.
!     (unless (and (floatp temporary-goal-column)
! 		 (memq last-command `(next-line previous-line ,this-command)))
        (cond ((eq (nth 1 posn) 'right-fringe) ; overflow-newline-into-fringe
! 	     (setq temporary-goal-column (- (window-width) 1)))
  	    ((setq x (car (posn-x-y posn)))
! 	     (setq temporary-goal-column (/ (float x) (frame-char-width))))))
      ;; Move using `vertical-motion'.
      (or (and (= (vertical-motion
! 		 (cons (or goal-column (truncate temporary-goal-column)) arg))
  		arg)
  	     (or (>= arg 0)
  		 (/= (point) opoint)
--- 4062,4084 ----
  	x)
      ;; Reset temporary-goal-column, unless the previous command was a
      ;; line-motion command or we were called from some other command.
!     (if (and (consp temporary-goal-column)
! 	     (memq last-command `(next-line previous-line ,this-command)))
! 	;; Check window-hscroll
! 	(if (/= (window-hscroll) (cdr temporary-goal-column))
! 	    (set-window-hscroll nil (cdr temporary-goal-column)))
        (cond ((eq (nth 1 posn) 'right-fringe) ; overflow-newline-into-fringe
! 	     (setq temporary-goal-column
! 		   (cons (- (window-width) 1) (window-hscroll))))
  	    ((setq x (car (posn-x-y posn)))
! 	     (setq temporary-goal-column
! 		   (cons (/ (float x) (frame-char-width)) (window-hscroll))))))
      ;; Move using `vertical-motion'.
      (or (and (= (vertical-motion
! 		 (cons (or goal-column
! 			   (if (consp temporary-goal-column)
! 			       (truncate (car temporary-goal-column))
! 			     temporary-goal-column)) arg))
  		arg)
  	     (or (>= arg 0)
  		 (/= (point) opoint)
***************
*** 4091,4098 ****
    (let ((inhibit-point-motion-hooks t)
  	(opoint (point))
  	(orig-arg arg))
!     (if (floatp temporary-goal-column)
! 	(setq temporary-goal-column (truncate temporary-goal-column)))
      (unwind-protect
  	(progn
  	  (if (not (memq last-command '(next-line previous-line)))
--- 4100,4108 ----
    (let ((inhibit-point-motion-hooks t)
  	(opoint (point))
  	(orig-arg arg))
!     (if (consp temporary-goal-column)
! 	(setq temporary-goal-column (+ (car temporary-goal-column)
! 				       (cdr temporary-goal-column))))
      (unwind-protect
  	(progn
  	  (if (not (memq last-command '(next-line previous-line)))

Diff finished.  Sat Jul 11 11:58:57 2009

bug#3494: 23.0.94; line-move-visual: temporary goal column confused by hscrolling

[Johan =?UTF-8?Q?Bockg=C3=A5rd]

;; Text area is standard 80 columns
$ emacs -Q -f toggle-truncate-lines

C-u 85 a
RET
C-u 75 b
C-p C-n C-p

Point is now at the end of line "a".