Bug in vertical-motion vs overlay with display property

Bug in vertical-motion vs overlay with display property

[Karl Chen]

Hi Eli/Emacs-devel,

I believe I've discovered a small bug in `vertical-motion' in
indent.c.  It affects movement past blank lines when there is an
overlay with a display/cursor property.

When using `fci-mode' from fill-column-indicator.el,
`previous-line' sometimes goes past two blank lines instead of
one.

Affected Emacs versions:
 - Good: 24.2
 - BAD:  24.3
 - BAD:  24.3.50 as of 2013-04-08

1. One can reproduce the problem using fill-column-indicator.el:

   $ wget https://raw.github.com/alpaker/Fill-Column-Indicator/master/fill-column-indicator.el
   $ emacs -Q

   M-x load-file fill-column-indicator.el
   M-x fci-mode
   <enter> <enter> <enter> <enter> <enter>
   <up>
   <up>  ;; BAD: this moves up 2 lines instead of 1 line


2. I minimized the above down to the following self-contained test
   case:

   $ cat > aa.el <<EOF
     (defun fci-redraw-region (start end _ignored)
       (save-match-data
         (save-excursion
           (goto-char start)
           (while
               (search-forward "\n" end t)
             (setq eol-str "X")
             (setq o (make-overlay (match-beginning 0) (match-beginning 0)))
             (overlay-put o 'after-string
                          (propertize eol-str 'display (propertize eol-str 'cursor t)))
             ))))
     (add-hook 'after-change-functions 'fci-redraw-region)
     (insert "\n\n\n\n")
     (vertical-motion -1)
     (vertical-motion -1)
     (kill-emacs (+ 100 (line-number-at-pos)))
   EOF

   $ emacs-24.2 -nw -Q --load aa.el ; echo $?
   103  # good

   $ emacs-24.3 -nw -Q --load aa.el ; echo $?
   102  # BAD

   Note that `emacs --batch' doesn't reproduce the problem.


3. Using the above I bisected the changes between 24.2 and 24.3
   down to the following commit:

   commit b65a46be5055c338a9f8e7640ad97b8e592d5977
   Refs: HEAD, refs/bisect/bad
   Author:     Eli Zaretskii <eliz@gnu.org>
   AuthorDate: 2012-11-21 21:28:14 +0200
   Commit:     Eli Zaretskii <eliz@gnu.org>
   CommitDate: 2012-11-21 21:28:14 +0200

       Fix bug #12930 with vertical-motion through a display string.

        src/indent.c (Fvertical_motion): If the starting position is covered
        by a display string, return to one position before that, to avoid
        overshooting it inside move_it_to.
   ---
    src/ChangeLog | 6 ++++++
    src/indent.c  | 8 +++++++-
    2 files changed, 13 insertions(+), 1 deletion(-)

   diff --git a/src/ChangeLog b/src/ChangeLog
   index 020948e..c51f58a 100644
   --- a/src/ChangeLog
   +++ b/src/ChangeLog
   @@ -1,3 +1,9 @@
   +2012-11-21  Eli Zaretskii  <eliz@gnu.org>
   +
   +       * indent.c (Fvertical_motion): If the starting position is covered
   +       by a display string, return to one position before that, to avoid
   +       overshooting it inside move_it_to.  (Bug#12930)
   +
    2012-11-20  Daniel Colascione  <dancol@dancol.org>

           * w32fns.c (Fx_file_dialog):
   diff --git a/src/indent.c b/src/indent.c
   index bbc944d..3332228 100644
   --- a/src/indent.c
   +++ b/src/indent.c
   @@ -2057,7 +2057,13 @@ whether or not it is currently displayed in some window.  */)
              comment said this is "so we don't move too far" (2005-01-19
              checkin by kfs).  But this does nothing useful that I can
              tell, and it causes Bug#2694 .  -- cyd */
   -       move_it_to (&it, PT, -1, -1, -1, MOVE_TO_POS);
   +       /* When the position we started from is covered by a display
   +          string, move_it_to will overshoot it, while vertical-motion
   +          wants to put the cursor _before_ the display string.  So in
   +          that case, we move to buffer position before the display
   +          string, and avoid overshooting.  */
   +       move_it_to (&it, disp_string_at_start_p ? PT - 1 : PT,
   +                   -1, -1, -1, MOVE_TO_POS);

          /* IT may move too far if truncate-lines is on and PT lies
            beyond the right margin.  IT may also move too far if the

Thoughts?

Thanks,
Karl

Re: Bug in vertical-motion vs overlay with display property

[Eli Zaretskii]

> From: Karl Chen <Karl.Chen@quarl.org>
> CC: alp.tekin.aker@gmail.com,
>     Emacs Developement List <emacs-devel@gnu.org>
> Date: Fri, 12 Apr 2013 19:34:24 -0400
> 
> Hi Eli/Emacs-devel,
> 
> I believe I've discovered a small bug in `vertical-motion' in
> indent.c.  It affects movement past blank lines when there is an
> overlay with a display/cursor property.

Thanks.  But such reports should go to bug-gnu-emacs@gnu.org.

> When using `fci-mode' from fill-column-indicator.el,
> `previous-line' sometimes goes past two blank lines instead of
> one.
> 
> Affected Emacs versions:
>  - Good: 24.2
>  - BAD:  24.3
>  - BAD:  24.3.50 as of 2013-04-08
> 
> 1. One can reproduce the problem using fill-column-indicator.el:
> 
>    $ wget https://raw.github.com/alpaker/Fill-Column-Indicator/master/fill-column-indicator.el
>    $ emacs -Q
> 
>    M-x load-file fill-column-indicator.el
>    M-x fci-mode
>    <enter> <enter> <enter> <enter> <enter>
>    <up>
>    <up>  ;; BAD: this moves up 2 lines instead of 1 line
> 
> 
> 2. I minimized the above down to the following self-contained test
>    case:
> 
>    $ cat > aa.el <<EOF
>      (defun fci-redraw-region (start end _ignored)
>        (save-match-data
>          (save-excursion
>            (goto-char start)
>            (while
>                (search-forward "\n" end t)
>              (setq eol-str "X")
>              (setq o (make-overlay (match-beginning 0) (match-beginning 0)))
>              (overlay-put o 'after-string
>                           (propertize eol-str 'display (propertize eol-str 'cursor t)))
>              ))))
>      (add-hook 'after-change-functions 'fci-redraw-region)
>      (insert "\n\n\n\n")
>      (vertical-motion -1)
>      (vertical-motion -1)
>      (kill-emacs (+ 100 (line-number-at-pos)))
>    EOF
> 
>    $ emacs-24.2 -nw -Q --load aa.el ; echo $?
>    103  # good
> 
>    $ emacs-24.3 -nw -Q --load aa.el ; echo $?
>    102  # BAD

Thanks for the test case.  This is fixed in trunk revision 112274.
The fixed code works correctly both with fill-column-indicator and
with the simplified recipe above.

> 3. Using the above I bisected the changes between 24.2 and 24.3
>    down to the following commit:
> 
>    commit b65a46be5055c338a9f8e7640ad97b8e592d5977
>    Refs: HEAD, refs/bisect/bad
>    Author:     Eli Zaretskii <eliz@gnu.org>
>    AuthorDate: 2012-11-21 21:28:14 +0200
>    Commit:     Eli Zaretskii <eliz@gnu.org>
>    CommitDate: 2012-11-21 21:28:14 +0200

Thanks, but in the future please try to identify the bzr revision
number or revision id of the offending commit.

For the record:

 revision-id: eliz@gnu.org-20121121192814-1p9f510g94zc10e1
 revision number on the trunk: 110764.1.170

> Thoughts?

It's a bug.  The code I committed back then to fix another bug
considered a display string to be always set on buffer text.  But in
this case, the display string was set on an after-string, a
possibility that the code I wrote back then didn't take into
consideration.

Re: Bug in vertical-motion vs overlay with display property

[Karl Chen]

>>>>> On 2013-04-13 05:03 EDT, Eli Zaretskii writes:

    Eli> This is fixed in trunk revision 112274.  The fixed code
    Eli> works correctly both with fill-column-indicator and with
    Eli> the simplified recipe above.

Confirmed.

I'll keep the bzr and bug reporting guidelines in mind.

Thanks!
Karl

Re: Bug in vertical-motion vs overlay with display property

[Eli Zaretskii]

> From: Karl Chen <Karl.Chen@quarl.org>
> Date: Sat, 13 Apr 2013 11:13:54 -0400
> Cc: alp.tekin.aker@gmail.com, emacs-devel@gnu.org
> 
> >>>>> On 2013-04-13 05:03 EDT, Eli Zaretskii writes:
> 
>     Eli> This is fixed in trunk revision 112274.  The fixed code
>     Eli> works correctly both with fill-column-indicator and with
>     Eli> the simplified recipe above.
> 
> Confirmed.

Thanks.

> I'll keep the bzr and bug reporting guidelines in mind.

Thank you.