vertical-motion bug

vertical-motion bug

[Chong Yidong]

In a scratch buffer:

  M-<
  M-: (insert (propertize "a" 'display "a\nb\nc\n")) RET
  M-<
  C-n (or M-: (vertical-motion 1))

Result: point moves down three lines.  The expected behavior, based on
the `vertical-motion' docstring, is to move just one line.  This
behavior was broken by something in the following change:

2006-06-14  Kim F. Storm  <storm@cua.dk>

	* dispextern.h (IT_STACK_SIZE): New macro specifying size of
	iterator stack (instead of hardcoded number).  Increase from 2 to
	4 to make room for propertized overlay strings before and after a
	display string, image or composition.
	(struct it): Add image_id and method members to iterator stack.

	* xdisp.c (init_from_display_pos): Don't set it->method and
	overlay_string_index after pop_it.  Add asserts.
	(handle_stop): Look for overlay strings around a display string,
	image, or composition.  Handle properties on those strings.
	(next_overlay_string): Don't set string, pos or method after pop_it.
	(get_overlay_strings_1): Split from get_overlay_strings; don't
	modify it if no overlay strings are found.
	(get_overlay_strings): Use get_overlay_strings_1.  Always set
	it->string and it->method.
	(push_it): Push it->image_id and it->method.  Push it->object
	instead of it->string if method is GET_FROM_IMAGE.
	(pop_it): Pop it->image_id and it->method.  Ppo it->object
	instead of it->string if method is GET_FROM_IMAGE.
	Reset it->current.string_pos if popped it->string is nil.
	(reseat_1): Remove comment dated 19 May 2003.  It expressed doubt
	whether a given change was correct; but the change is correct.
	Clear it->string_from_display_prop_p.
	(set_iterator_to_next): Rely on it->method and it->image_id from
	iterator stack, instead of setting them explicitly after pop_it.

Re: vertical-motion bug

[Chong Yidong]

For what it's worth, reverting the following line in the 2006-06-14
change makes things work again.  I don't know if this is the correct
fix, since I don't know why that line was introduced in the first
place.

*** emacs/src/xdisp.c.~1.1109.~	2006-07-15 11:38:46.000000000 -0400
--- emacs/src/xdisp.c	2006-07-15 11:46:48.000000000 -0400
***************
*** 5315,5321 ****
    it->area = TEXT_AREA;
    it->multibyte_p = !NILP (current_buffer->enable_multibyte_characters);
    it->sp = 0;
-   it->string_from_display_prop_p = 0;
    it->face_before_selective_p = 0;
  
    if (set_stop_p)
--- 5315,5320 ----

>   From: Chong Yidong <cyd@stupidchicken.com>
>
>   In a scratch buffer:
>
>     M-<
>     M-: (insert (propertize "a" 'display "a\nb\nc\n")) RET
>     M-<
>     C-n (or M-: (vertical-motion 1))
>
>   Result: point moves down three lines.  The expected behavior, based on
>   the `vertical-motion' docstring, is to move just one line.  This
>   behavior was broken by something in the following change:
>
>   2006-06-14  Kim F. Storm  <storm@cua.dk>
>
>   * dispextern.h (IT_STACK_SIZE): New macro specifying size of
>   iterator stack (instead of hardcoded number).  Increase from 2 to
>   4 to make room for propertized overlay strings before and after a
>   display string, image or composition.
>   (struct it): Add image_id and method members to iterator stack.
>
>   * xdisp.c (init_from_display_pos): Don't set it->method and
>   overlay_string_index after pop_it.  Add asserts.
>   (handle_stop): Look for overlay strings around a display string,
>   image, or composition.  Handle properties on those strings.
>   (next_overlay_string): Don't set string, pos or method after pop_it.
>   (get_overlay_strings_1): Split from get_overlay_strings; don't
>   modify it if no overlay strings are found.
>   (get_overlay_strings): Use get_overlay_strings_1.  Always set
>   it->string and it->method.
>   (push_it): Push it->image_id and it->method.  Push it->object
>   instead of it->string if method is GET_FROM_IMAGE.
>   (pop_it): Pop it->image_id and it->method.  Ppo it->object
>   instead of it->string if method is GET_FROM_IMAGE.
>   Reset it->current.string_pos if popped it->string is nil.
>   (reseat_1): Remove comment dated 19 May 2003.  It expressed doubt
>   whether a given change was correct; but the change is correct.
>   Clear it->string_from_display_prop_p.
>   (set_iterator_to_next): Rely on it->method and it->image_id from
>   iterator stack, instead of setting them explicitly after pop_it.

Re: vertical-motion bug

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

> For what it's worth, reverting the following line in the 2006-06-14
> change makes things work again.  I don't know if this is the correct
> fix, since I don't know why that line was introduced in the first
> place.

I don't remember the exact reason, but it was related to one of the
(buggy) uses of propertized before or after strings for an overlay with
something non-trivial in those strings (a composition, IIRC).

So I fear that reverting that change will re-introduce the other bug.


FWIW, here are the examples that I have on file -- there may be more
in the mail archive...

; emacs -Q -D
(setq overlay (make-overlay 1 3))
(overlay-put overlay 'before-string (compose-string "BE"))
-> before-string is doubled.

(overlay-put overlay 'after-string (compose-string "AF"))
-> after-string is not displayed.

(overlay-put overlay 'before-string nil)
-> after-string is displayed but doubled.


 * Case 1: The before-string is shown twice. ssssssssssss
; emacs -Q -D
(progn
(setq overlay (make-overlay 1 3))
(overlay-put overlay 'before-string (propertize "BE" 'face 'bold))
(overlay-put overlay 'after-string (propertize "AF" 'display
	     		 (propertize "XY" 'face 'underline)))
(put-text-property 1 3 'display (compose-string "DISP"))
)

* Case 2: Cursor at a wrong position.
; emacs -q -D  ;; not `-Q' in order to show a message in *scratch*
(progn
(setq overlay (make-overlay 1 3))
(overlay-put overlay 'before-string (propertize "BEFORE_STRING" 'face 'bold))
(overlay-put overlay 'after-string (propertize "AF" 'display
	     		 (propertize "XY" 'face 'underline)))
(put-text-property 1 3 'invisible t)
)
;; make sure that the first line is wrapped
M-<


; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
; emacs -q -D  ;; not `-Q' in order to show a message in *scratch*
(progn
(setq overlay (make-overlay 1 3))
(overlay-put overlay 'before-string (propertize "BE" 'face 'bold))
(overlay-put overlay 'after-string (propertize "AF" 'display
	     		 (propertize "XY" 'face 'underline)))
(put-text-property 1 3 'display "DISP")
)
;; make sure that the first line is wrapped
M-<

Then the cursor is displayed at a wrong position.

Another assertion violation can be observed by replacing the last
expression with (put-text-property 1 3 'display '(space :width 2))

; emacs -Q -D
(progn
(setq overlay (make-overlay 1 3))
(overlay-put overlay 'before-string (propertize "BE" 'face 'bold))
(overlay-put overlay 'after-string (propertize "AF" 'display
	     		 (propertize "XY" 'face 'underline)))
(compose-region 1 3)
)

; emacs -Q -D
(progn
(defconst breakpoint-xpm-data
  "/* XPM */
static char *magick[] = {
/* columns rows colors chars-per-pixel */
\"10 10 2 1\",
\"  c red\",
\"+ c None\",
/* pixels */
\"+++    +++\",
\"++      ++\",
\"+        +\",
\"          \",
\"          \",
\"          \",
\"          \",
\"+        +\",
\"++      ++\",
\"+++    +++\",
};"
  "XPM data used for breakpoint icon.")

(setq im (find-image `((:type xpm :data ,breakpoint-xpm-data))))
)

(progn ;; IMAGE
(setq overlay (make-overlay 1 3))
(overlay-put overlay 'before-string (propertize "BE" 'display im))
(overlay-put overlay 'after-string
	     (propertize "AF" 'display
	     		 (propertize "XY" 'face 'underline)))
)

(progn ;; STRETCH
(setq overlay (make-overlay 1 3))
(overlay-put overlay 'before-string (propertize "BE" 'display '(space . (:width 5))))
(overlay-put overlay 'after-string
	     (propertize "AF" 'display
	     		 (propertize "XY" 'face 'underline)))
(overlay-put overlay 'priority 100)
)

(progn
(put-text-property 1 3 'display "DISP")
(put-text-property 2 3 'display "disp")
(put-text-property 3 4 'display "123")
)

(progn
 (goto-char (point-min))
 (insert-image im)
)

>
> *** emacs/src/xdisp.c.~1.1109.~	2006-07-15 11:38:46.000000000 -0400
> --- emacs/src/xdisp.c	2006-07-15 11:46:48.000000000 -0400
> ***************
> *** 5315,5321 ****
>     it->area = TEXT_AREA;
>     it->multibyte_p = !NILP (current_buffer->enable_multibyte_characters);
>     it->sp = 0;
> -   it->string_from_display_prop_p = 0;
>     it->face_before_selective_p = 0;
>   
>     if (set_stop_p)
> --- 5315,5320 ----
>
>>   From: Chong Yidong <cyd@stupidchicken.com>
>>
>>   In a scratch buffer:
>>
>>     M-<
>>     M-: (insert (propertize "a" 'display "a\nb\nc\n")) RET
>>     M-<
>>     C-n (or M-: (vertical-motion 1))
>>
>>   Result: point moves down three lines.  The expected behavior, based on
>>   the `vertical-motion' docstring, is to move just one line.  This
>>   behavior was broken by something in the following change:
>>
>>   2006-06-14  Kim F. Storm  <storm@cua.dk>
>>
>>   * dispextern.h (IT_STACK_SIZE): New macro specifying size of
>>   iterator stack (instead of hardcoded number).  Increase from 2 to
>>   4 to make room for propertized overlay strings before and after a
>>   display string, image or composition.
>>   (struct it): Add image_id and method members to iterator stack.
>>
>>   * xdisp.c (init_from_display_pos): Don't set it->method and
>>   overlay_string_index after pop_it.  Add asserts.
>>   (handle_stop): Look for overlay strings around a display string,
>>   image, or composition.  Handle properties on those strings.
>>   (next_overlay_string): Don't set string, pos or method after pop_it.
>>   (get_overlay_strings_1): Split from get_overlay_strings; don't
>>   modify it if no overlay strings are found.
>>   (get_overlay_strings): Use get_overlay_strings_1.  Always set
>>   it->string and it->method.
>>   (push_it): Push it->image_id and it->method.  Push it->object
>>   instead of it->string if method is GET_FROM_IMAGE.
>>   (pop_it): Pop it->image_id and it->method.  Ppo it->object
>>   instead of it->string if method is GET_FROM_IMAGE.
>>   Reset it->current.string_pos if popped it->string is nil.
>>   (reseat_1): Remove comment dated 19 May 2003.  It expressed doubt
>>   whether a given change was correct; but the change is correct.
>>   Clear it->string_from_display_prop_p.
>>   (set_iterator_to_next): Rely on it->method and it->image_id from
>>   iterator stack, instead of setting them explicitly after pop_it.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: vertical-motion bug

[Richard Stallman]

      M-<
      M-: (insert (propertize "a" 'display "a\nb\nc\n")) RET
      M-<
      C-n (or M-: (vertical-motion 1))

    Result: point moves down three lines.  The expected behavior, based on
    the `vertical-motion' docstring, is to move just one line.

There is no way to move down just one line, since no buffer
position corresponds to that screen position.  Emacs can either
move three lines or not move.  So I think its actual behavior
is the best possible thing it could do.

Re: vertical-motion bug

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

>       M-<
>       M-: (insert (propertize "a" 'display "a\nb\nc\n")) RET
>       M-<
>       C-n (or M-: (vertical-motion 1))
>
>     Result: point moves down three lines.  The expected behavior, based on
>     the `vertical-motion' docstring, is to move just one line.
>
> There is no way to move down just one line, since no buffer
> position corresponds to that screen position.  Emacs can either
> move three lines or not move.  So I think its actual behavior
> is the best possible thing it could do.

Sorry, I gave a bad test case.  Try this:

  M-:  (let ((pos (point-min)))
         (dotimes (i 10) (insert "a"))
         (while (< pos (point-max))
           (put-text-property pos (1+ pos) 'display (propertize "a\n"))
           (setq pos (1+ pos))))

This puts a display property on each of the characters in the buffer,
so there is a valid buffer position.  C-n and C-p skip past all of
them.