Display problems with `before-string' in overlay

Display problems with `before-string' in overlay

[Richard Stallman]

You proposed a change in the code at 2010 in keyboard.c.
(I've copied it below.)  I don't see why it is correct,
so here are some questions.

    ! 	  && (OVERLAYP (overlay)
    ! 	      ? (beg = OVERLAY_POSITION (OVERLAY_START (overlay)),
    ! 		 end = OVERLAY_POSITION (OVERLAY_END (overlay)),
    ! 		 beg <= PT)

How can it ever happen that BEG <= PT is false?
If that were false, the overlay would not cover PT,
so it should not have been returned in the first place, right?

Anyway, your change is to replace 

             (beg < PT /* && end > PT   <- It's always the case.  */
 	      || (beg <= PT && STRINGP (val) && SCHARS (val) == 0))

with a condition that is always true.  Could you please explain the
motive for that change?  Why is it correct to adjust point when it
starts out at the beginning of an overlay with a `display' property?

Also, why is the condition

 	      || (beg <= PT && STRINGP (val) && SCHARS (val) == 0)

used at all?  What usage case is that meant to cover, and what is the
right behavior for that case?  Even if we don't change the code here
now, we need to put in a comment about that.



*** emacs/src/keyboard.c.~1.899.~	2007-04-04 13:05:31.000000000 -0400
--- emacs/src/keyboard.c	2007-04-13 14:04:15.000000000 -0400
***************
*** 2010,2021 ****
  	  && !NILP (val = get_char_property_and_overlay
  		              (make_number (PT), Qdisplay, Qnil, &overlay))
  	  && display_prop_intangible_p (val)
! 	  && (!OVERLAYP (overlay)
! 	      ? get_property_and_range (PT, Qdisplay, &val, &beg, &end, Qnil)
! 	      : (beg = OVERLAY_POSITION (OVERLAY_START (overlay)),
! 		 end = OVERLAY_POSITION (OVERLAY_END (overlay))))
! 	  && (beg < PT /* && end > PT   <- It's always the case.  */
! 	      || (beg <= PT && STRINGP (val) && SCHARS (val) == 0)))
  	{
  	  xassert (end > PT);
  	  SET_PT (PT < last_pt
--- 2010,2022 ----
  	  && !NILP (val = get_char_property_and_overlay
  		              (make_number (PT), Qdisplay, Qnil, &overlay))
  	  && display_prop_intangible_p (val)
! 	  && (OVERLAYP (overlay)
! 	      ? (beg = OVERLAY_POSITION (OVERLAY_START (overlay)),
! 		 end = OVERLAY_POSITION (OVERLAY_END (overlay)),
! 		 beg <= PT)
! 	      : (get_property_and_range (PT, Qdisplay, &val, &beg, &end, Qnil),
! 		 (beg < PT
! 		  || (beg <= PT && STRINGP (val) && SCHARS (val) == 0)))))
  	{
  	  xassert (end > PT);
  	  SET_PT (PT < last_pt

Re: Display problems with `before-string' in overlay

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

> your change is to replace 
>
>              (beg < PT /* && end > PT   <- It's always the case.  */
>  	      || (beg <= PT && STRINGP (val) && SCHARS (val) == 0))
>
> with a condition that is always true.  Could you please explain the
> motive for that change?  Why is it correct to adjust point when it
> starts out at the beginning of an overlay with a `display' property?

Let me try to explain (again) what the problem is.  It boils down to
where to display the cursor when point is at the beginning/end of an
overlay with a `display' property.  Currently, the cursor can show up
at the following positions (where OOO denotes the overlay, and ABC
denotes the surrounding characters):

 A B|O O O C   (point = 3)
 A B O O O|C   (point = 6)

The problem arises when you have multi-line overlays.  In Emacs 21,
redisplay used a simple rule (see cursor_row_p): "if PT is at the end
of a row, and the row ends with a newline from a string, don't put
cursor on that row."  This rule is desirable for multi-line
before-strings:

 A B X X
 X|O C
 A B X X
 X O|C

But this rule is bad for multi-line display strings, because the
cursor gets pushed down to the next line:

 A B O O
|O C           (point = 3)
 A B O O
 O|C           (point = 6)

The 2005-07-13 change by KFS revoked this rule.  Now, that cursor can
live on a row ending in a newline from a string.  For multi-line
display strings, we have

 A B|O O
 O C           (point = 3)
 A B O O
 O|C           (point = 6)

Note that we MUST revoke the rule if we want multi-line display
strings to behave similar to single-line display strings.

So far so good.  But revoking the rule screws up multi-line
before-strings, because redisplay now tries to draw the cursor on the
first line where the string occurs:

 A B X X|
 X C
 A B X X
 X C|

The naive approach to solving this problem is to change the rule to
"if a row ends with a newline from a before-string, don't put cursor
on that row."  Unfortunately, this is very difficult to implement,
because the glyph matrix does not store information about whether a
string came from a display string or some other source (this is a
deliberate decision for reducing the size of the glyph structure).
Various approaches to extract this information (such as using
get_char_property) don't work for complicated reasons; if someone
comes up with a fast workable solution for this, I'm all ears.

The only other possibility I'm aware of is to sidestep the issue
entirely, by changing adjust_point_for_property so that point can't
land on the start of the overlay.  For instance:

 A|B O O O C   (point = 2)   [point can't be at position 3]
 A B O O O|C   (point = 6)

 A|B O O
 O C           (point = 2)
 A B O O
 O|C           (point = 6)

The trouble with this is that the current adjust_point_for_property
behavior has been in place for a very long time now, and some packages
may have come to rely on it.  So changing this code is not to be done
lightly.


Thus, the current situation is that the cursor is drawn in the wrong
position for mutli-line before-strings.  However,

 (1) this is a seldom-used feature; note that it took 2 years after
     KFS's change for someone to notice this

 (2) There are many redisplay options for working around this bug.

 (3) Even though the cursor position is drawn incorrectly, there is no
     corruption of point or buffer contents, so it's at most a glitch
     that people can live with.

So unless you can come up with an explanation for why changing the
code has no danger of causing instability, I think we should leave
things as they currently are, and adopt my proposed fix for 22.2.

Re: Display problems with `before-string' in overlay

[Richard Stallman]

    Let me try to explain (again) what the problem is.  It boils down to
    where to display the cursor when point is at the beginning/end of an
    overlay with a `display' property.  Currently, the cursor can show up
    at the following positions (where OOO denotes the overlay, and ABC
    denotes the surrounding characters):

     A B|O O O C   (point = 3)
     A B O O O|C   (point = 6)

I am not sure what you mean when you say "the cursor can show up".
Are you talking about where point is, or where the cursor appears?

    The problem arises when you have multi-line overlays.  In Emacs 21,
    redisplay used a simple rule (see cursor_row_p): "if PT is at the end
    of a row, and the row ends with a newline from a string, don't put
    cursor on that row."  This rule is desirable for multi-line
    before-strings:

     A B X X
     X|O C
     A B X X
     X O|C

I don't understand those examples.  What does X stand for?  Where is
the newline from a string?  And why would PT be at the end of that
row?  If that is two example, what is the relationship between them?
I'm completely lost here.

    But this rule is bad for multi-line display strings, because the
    cursor gets pushed down to the next line:

     A B O O
    |O C           (point = 3)
     A B O O
     O|C           (point = 6)

Once again, what is the relationshp between these two examples?

This function moves point, it doesn't do anything to _the cursor_
as such.  And when it moves point due to a property on an overlay,
it moves point to the end of the overlay.  So how could that ever
put point like this?

     A B O O
    |O C           (point = 3)

Re: Display problems with `before-string' in overlay

[Richard Stallman]

    The only other possibility I'm aware of is to sidestep the issue
    entirely, by changing adjust_point_for_property so that point can't
    land on the start of the overlay.

Is that what your change in adjust_point_for_property tries to do?

I don't think it is correct, on principle.  It SHOULD be possible
to put point on either side of this overlay.

Display problems with `before-string' in overlay

[Richard Stallman]

Most of your proposed change in xdisp.c is the addition of the OVERLAY
argument to `string_buffer_position'.  That is harmless.

It includes two substantial changes which could really alter Emacs's
behavior.  I've included them below.

Could you explain them more clearly, in a way that demonstrates
they are correct?



***************
*** 11933,11952 ****
        Lisp_Object string;
        struct glyph *stop = glyph;
        int pos;
  
        limit = make_number (pt_old + 1);
        glyph = string_start;
        x = string_start_x;
        string = glyph->object;
!       pos = string_buffer_position (w, string, string_before_pos);
!       /* If STRING is from overlay, LAST_POS == 0.  We skip such glyphs
! 	 because we always put cursor after overlay strings.  */
!       while (pos == 0 && glyph < stop)
  	{
  	  string = glyph->object;
  	  SKIP_GLYPHS (glyph, stop, x, EQ (glyph->object, string));
  	  if (glyph < stop)
! 	    pos = string_buffer_position (w, glyph->object, string_before_pos);
  	}
  
        while (glyph < stop)
--- 11934,11955 ----
        Lisp_Object string;
        struct glyph *stop = glyph;
        int pos;
+       Lisp_Object overlay = Qnil;
  
        limit = make_number (pt_old + 1);
        glyph = string_start;
        x = string_start_x;
        string = glyph->object;
!       pos = string_buffer_position (w, string, string_before_pos, &overlay);
!       /* If STRING is from overlay, skip its glyphs because we always
! 	 put cursor after overlay strings.  */
!       while ((pos == 0 || !NILP (overlay)) && glyph < stop)
  	{
  	  string = glyph->object;
  	  SKIP_GLYPHS (glyph, stop, x, EQ (glyph->object, string));
  	  if (glyph < stop)
! 	    pos = string_buffer_position (w, glyph->object,
! 					  string_before_pos, overlay);
  	}
  
        while (glyph < stop)

***************
*** 15854,15865 ****
    if (PT == MATRIX_ROW_END_CHARPOS (row))
      {
        /* If the row ends with a newline from a string, we don't want
! 	 the cursor there, but we still want it at the start of the
! 	 string if the string starts in this row.
  	 If the row is continued it doesn't end in a newline.  */
        if (CHARPOS (row->end.string_pos) >= 0)
! 	cursor_row_p = (row->continued_p
! 			|| PT >= MATRIX_ROW_START_CHARPOS (row));
        else if (MATRIX_ROW_ENDS_IN_MIDDLE_OF_CHAR_P (row))
  	{
  	  /* If the row ends in middle of a real character,
--- 15857,15866 ----
    if (PT == MATRIX_ROW_END_CHARPOS (row))
      {
        /* If the row ends with a newline from a string, we don't want
! 	 the cursor there.
  	 If the row is continued it doesn't end in a newline.  */
        if (CHARPOS (row->end.string_pos) >= 0)
! 	cursor_row_p = row->continued_p;
        else if (MATRIX_ROW_ENDS_IN_MIDDLE_OF_CHAR_P (row))
  	{
  	  /* If the row ends in middle of a real character,

Re: Display problems with `before-string' in overlay

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

> Most of your proposed change in xdisp.c is the addition of the OVERLAY
> argument to `string_buffer_position'.  That is harmless.
>
> It includes two substantial changes which could really alter Emacs's
> behavior.  I've included them below.
>
> Could you explain them more clearly, in a way that demonstrates
> they are correct?

You're looking at the wrong patch.  I don't think
string_buffer_position should be changed right now.

Re: Display problems with `before-string' in overlay

[Richard Stallman]

    You're looking at the wrong patch.  I don't think
    string_buffer_position should be changed right now.

That was the only patch I had received at the time.
Now, however, I see another patch.  I will look at that one.