problem of display property [Re: list-charset-chars and unicode-bmp]

problem of display property [Re: list-charset-chars and unicode-bmp]

[Kenichi Handa]

Being a little bit tired of the discussion in emacs-devel, I
tried to fix this bug.  At first sight, it seems easy ...
but it's not ... of course.  :-(

In article <rzqsmvjtrnx.fsf@albion.dl.ac.uk>, Dave Love <d.love@dl.ac.uk> writes:
> I think there are comments in the code about my attempt to do that
> some time ago.  `display' properties behave in strange ways and I seem
> to remember that composition doesn't work as you'd hope either.  This
> isn't specific to the emacs-unicode branch whose redisplay is
> seriously buggy anyway.  If you want to work on it, I suggest using
> the mainline source, but it doesn't seems important to me.

That bug can be reproduced as below.

(insert "abc" (propertize "." 'display "<dot>") "def")

When you put cursor on `c' and type C-f, the cursor is
placed on `d' instead of `<' of "<dot>", but C-x = shows
that point is correctly on "."

The problem is in this code of set_cursor_from_row.

  while (glyph < end
	 && !INTEGERP (glyph->object)
	 && (!BUFFERP (glyph->object)
	     || glyph->charpos < pt_old))
    {
      x += glyph->pixel_width;
      ++glyph;
    }

It skips all glyphs that don't come directy from a buffer.
So when point is on ".", at the glyphs comes from display
property, instead of stopping at there, it stops at "d".

The current members of the struct glyph is not enough to
determine the correct cursor position here.  We need a new
member `buf_charpos' that always holds a buffer position
corresponding to each glyph.

I'll attach a patch I tried.  The strategy is this:

At first, record IT_CHARPOS (*IT) in IT->buf_charpos in
next_element_from_buffer.  Don't change it in the other
places (except for reseat_1 and reseat_to_string, they
initialize it).

Next, in functions that generate GLYPH from IT (e.g
append_glyph), copy IT->buf_charpos to GLYPH->buf_charpos.

At last, in set_cursor_from_row, check (GLYPH->buf_charpos <
pt_old) instead of (GLYPH->charpos < pt_old).

It seems that it is working well.  But, the comment of the
struct glyph says this:

/* Glyphs.

   Be extra careful when changing this structure!  Esp. make sure that
   functions producing glyphs, like x_append_glyph, fill ALL of the
   glyph structure, and that GLYPH_EQUAL_P compares all
   display-relevant members of glyphs (not to imply that these are the
   only things to check when you add a member).  */

This makes me nervous about the change.  So, I included Gerd
in CC:.  Gerd, could you help me?  What do you think about
it?  The relevant codes are not changed from 21.1, so I
think any new codes added later doesn't affect it.  It also
means that this bug has been there from 21.1.

---
Ken'ichi HANDA
handa@m17n.org

Index: src/dispextern.h
===================================================================
RCS file: /cvsroot/emacs/emacs/src/dispextern.h,v
retrieving revision 1.141
diff -u -r1.141 dispextern.h
--- src/dispextern.h	17 Nov 2002 23:46:26 -0000	1.141
+++ src/dispextern.h	28 Jan 2003 11:15:11 -0000
@@ -254,6 +254,12 @@
      the start of a row.  */
   int charpos;
 
+  /* Buffer position corresponding to this glyph if this glyph is for
+     displaying a buffer contents.  It is used to determine the cursor
+     position in set_cursor_from_row.  The value is simply copied from
+     it->buf_charpos.  */
+  int buf_charpos;
+
   /* Lisp object source of this glyph.  Currently either a buffer or
      a string, if the glyph was produced from characters which came from 
      a buffer or a string; or 0 if the glyph was inserted by redisplay
@@ -1586,6 +1592,11 @@
   /* Function to call to load this structure with the next display
      element.  */
   int (* method) P_ ((struct it *it));
+
+  /* Buffer position if we are iterating over current_buffer,
+     otherwise -1.  The value doesn't change while we are processing
+     overlay strings, etc.  */
+  int buf_charpos;
 
   /* The next position at which to check for face changes, invisible
      text, overlay strings, end of text etc., which see.  */
Index: src/term.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/term.c,v
retrieving revision 1.143
diff -u -r1.143 term.c
--- src/term.c	19 Jul 2002 14:37:18 -0000	1.143
+++ src/term.c	28 Jan 2003 11:15:11 -0000
@@ -1630,6 +1630,7 @@
       glyph->face_id = it->face_id;
       glyph->padding_p = i > 0;
       glyph->charpos = CHARPOS (it->position);
+      glyph->buf_charpos = it->buf_charpos;
       glyph->object = it->object;
       
       ++it->glyph_row->used[it->area];
Index: src/w32term.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/w32term.c,v
retrieving revision 1.177
diff -u -r1.177 w32term.c
--- src/w32term.c	22 Jan 2003 23:04:05 -0000	1.177
+++ src/w32term.c	28 Jan 2003 11:15:13 -0000
@@ -1653,6 +1653,7 @@
   if (glyph < it->glyph_row->glyphs[area + 1])
     {
       glyph->charpos = CHARPOS (it->position);
+      glyph->buf_charpos = it->buf_charpos;
       glyph->object = it->object;
       glyph->pixel_width = it->pixel_width;
       glyph->voffset = it->voffset;
@@ -1687,6 +1688,7 @@
   if (glyph < it->glyph_row->glyphs[area + 1])
     {
       glyph->charpos = CHARPOS (it->position);
+      glyph->buf_charpos = it->buf_charpos;
       glyph->object = it->object;
       glyph->pixel_width = it->pixel_width;
       glyph->voffset = it->voffset;
@@ -1779,6 +1781,7 @@
       if (glyph < it->glyph_row->glyphs[area + 1])
 	{
 	  glyph->charpos = CHARPOS (it->position);
+	  glyph->buf_charpos = it->buf_charpos;
 	  glyph->object = it->object;
 	  glyph->pixel_width = it->pixel_width;
 	  glyph->voffset = it->voffset;
@@ -1819,6 +1822,7 @@
   if (glyph < it->glyph_row->glyphs[area + 1])
     {
       glyph->charpos = CHARPOS (it->position);
+      glyph->buf_charpos = it->buf_charpos;
       glyph->object = object;
       glyph->pixel_width = width;
       glyph->voffset = it->voffset;
Index: src/xdisp.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xdisp.c,v
retrieving revision 1.802
diff -u -r1.802 xdisp.c
--- src/xdisp.c	20 Jan 2003 08:54:46 -0000	1.802
+++ src/xdisp.c	28 Jan 2003 11:15:16 -0000
@@ -4105,6 +4105,7 @@
   xassert (CHARPOS (pos) >= BEGV && CHARPOS (pos) <= ZV);
 
   it->current.pos = it->position = pos;
+  it->buf_charpos = IT_CHARPOS (*it);
   XSETBUFFER (it->object, current_buffer);
   it->end_charpos = ZV;
   it->dpvec = NULL;
@@ -4161,6 +4162,8 @@
   it->current.dpvec_index = -1;
   xassert (charpos >= 0);
 
+  it->buf_charpos = -1;
+
   /* If STRING is specified, use its multibyteness, otherwise use the
      setting of MULTIBYTE, if specified.  */
   if (multibyte >= 0)
@@ -4858,6 +4861,8 @@
   xassert (IT_CHARPOS (*it) >= BEGV
 	   && IT_CHARPOS (*it) <= it->stop_charpos);
 
+  it->buf_charpos = IT_CHARPOS (*it);
+
   if (IT_CHARPOS (*it) >= it->stop_charpos)
     {
       if (IT_CHARPOS (*it) >= it->end_charpos)
@@ -9477,11 +9482,19 @@
 
   while (glyph < end
 	 && !INTEGERP (glyph->object)
-	 && (!BUFFERP (glyph->object)
-	     || glyph->charpos < pt_old))
+	 && glyph->buf_charpos < pt_old)
     {
       x += glyph->pixel_width;
       ++glyph;
+    }
+
+  if (glyph != row->glyphs[TEXT_AREA]
+      && glyph->buf_charpos > pt_old)
+    {
+      /* The last skipped glyph strides over point.  Go back to that
+	 glyph to display a cursor on it.  */
+      --glyph;
+      x -= glyph->pixel_width;
     }
 
   w->cursor.hpos = glyph - row->glyphs[TEXT_AREA];
Index: src/xterm.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xterm.c,v
retrieving revision 1.769
diff -u -r1.769 xterm.c
--- src/xterm.c	25 Jan 2003 16:25:38 -0000	1.769
+++ src/xterm.c	28 Jan 2003 11:15:19 -0000
@@ -1455,6 +1455,7 @@
   if (glyph < it->glyph_row->glyphs[area + 1])
     {
       glyph->charpos = CHARPOS (it->position);
+      glyph->buf_charpos = it->buf_charpos;
       glyph->object = it->object;
       glyph->pixel_width = it->pixel_width;
       glyph->voffset = it->voffset;
@@ -1488,6 +1489,7 @@
   if (glyph < it->glyph_row->glyphs[area + 1])
     {
       glyph->charpos = CHARPOS (it->position);
+      glyph->buf_charpos = it->buf_charpos;
       glyph->object = it->object;
       glyph->pixel_width = it->pixel_width;
       glyph->voffset = it->voffset;
@@ -1579,6 +1581,7 @@
       if (glyph < it->glyph_row->glyphs[area + 1])
 	{
 	  glyph->charpos = CHARPOS (it->position);
+	  glyph->buf_charpos = it->buf_charpos;
 	  glyph->object = it->object;
 	  glyph->pixel_width = it->pixel_width;
 	  glyph->voffset = it->voffset;
@@ -1618,6 +1621,7 @@
   if (glyph < it->glyph_row->glyphs[area + 1])
     {
       glyph->charpos = CHARPOS (it->position);
+      glyph->buf_charpos = it->buf_charpos;
       glyph->object = object;
       glyph->pixel_width = width;
       glyph->voffset = it->voffset;

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Gerd Moellmann]

Kenichi Handa <handa@m17n.org> writes:

> Gerd, could you help me?  What do you think about it?  The relevant
> codes are not changed from 21.1, so I think any new codes added
> later doesn't affect it.  It also means that this bug has been there
> from 21.1.

Hi Kenichi!

I admit I haven't checked your patch in depth; please bear with me if
I've overlooked something.

When current glyphs are reused, their positions are adjusted for the
changes in the buffer since the glyphs were made current.  The
function try_window_id is one such example.  I didn't see this being
done for the new slot you added to struct glyph.  (Insofar, the
comment of struct glyph doesn't appear to be that devious :).)

Be it as it may, I think I'd rather try to avoid enlarging struct
glyph.  Instead, I'd try using the function string_buffer_position in
the cursor position computation.  Strings aren't displayed frequently,
so the additional function calls shouldn't matter, and anyway,
enlarging glyphs costs performance too, and that in all cases.

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Kenichi Handa]

In article <868yx5jvuf.fsf@gerd.free-bsd.org>, gerd.moellmann@t-online.de (Gerd Moellmann) writes:
> When current glyphs are reused, their positions are adjusted for the
> changes in the buffer since the glyphs were made current.  The
> function try_window_id is one such example.  I didn't see this being
> done for the new slot you added to struct glyph.  (Insofar, the
> comment of struct glyph doesn't appear to be that devious :).)

Ah, thank you!  That explains the strange behaviour I
noticed after I applied my patch, and is fixed by adjusting
buf_charpos in increment_row_positions.  But...

> Be it as it may, I think I'd rather try to avoid enlarging struct
> glyph.  Instead, I'd try using the function string_buffer_position in
> the cursor position computation.  Strings aren't displayed frequently,
> so the additional function calls shouldn't matter, and anyway,
> enlarging glyphs costs performance too, and that in all cases.

Oops, I didn't know about string_buffer_position.  You are
right.  By using it, we can confine the change to
set_cursor_from_row as attached.  The change was not that
straight, but I think it is better than the previous one.

The reason why the change looks complicated is to correctly
handle this kind of case where string_buffer_position is
confused by the same string used in display properties.

(setq s1 "<dot>")
(setq s2 "<comma>")
(setq c1 (propertize "." 'display s1))
(setq c2 (propertize "," 'display s2))
(insert c1 c2 c1 c2 c1 c2)

Could you check the change and comment again?

---
Ken'ichi HANDA
handa@m17n.org

*** xdisp.c.~1.729.2.12.~	Thu Nov  7 15:24:40 2002
--- xdisp.c	Wed Jan 29 15:54:27 2003
***************
*** 9369,9374 ****
--- 9369,9382 ----
  {
    struct glyph *glyph = row->glyphs[TEXT_AREA];
    struct glyph *end = glyph + row->used[TEXT_AREA];
+   /* The first glyph that starts a sequence of glyphs from string.  */
+   struct glyph *string_start;
+   /* The X coordinate of string_start.  */
+   int string_start_x;
+   /* The last known character position.  */
+   int last_pos = MATRIX_ROW_START_CHARPOS (row) + delta;
+   /* The last known character position before string_start.  */
+   int string_before_pos;
    int x = row->x;
    int pt_old = PT - delta;
  
***************
*** 9384,9396 ****
  	++glyph;
        }
  
    while (glyph < end
  	 && !INTEGERP (glyph->object)
  	 && (!BUFFERP (glyph->object)
! 	     || glyph->charpos < pt_old))
      {
!       x += glyph->pixel_width;
!       ++glyph;
      }
  
    w->cursor.hpos = glyph - row->glyphs[TEXT_AREA];
--- 9392,9454 ----
  	++glyph;
        }
  
+   string_start = NULL;
    while (glyph < end
  	 && !INTEGERP (glyph->object)
  	 && (!BUFFERP (glyph->object)
! 	     || (last_pos = glyph->charpos) < pt_old))
      {
!       if (! STRINGP (glyph->object))
! 	{
! 	  string_start = NULL;
! 	  x += glyph->pixel_width;
! 	  ++glyph;
! 	}
!       else
! 	{
! 	  string_before_pos = last_pos;
! 	  string_start = glyph;
! 	  string_start_x = x;
! 	  /* Skip all glyphs from string.  */
! 	  do
! 	    {
! 	      x += glyph->pixel_width;
! 	      ++glyph;
! 	    }
! 	  while (glyph < end && STRINGP (glyph->object));
! 	}
!     }
! 
!   if (string_start
!       && (glyph == end || !BUFFERP (glyph->object) || last_pos > pt_old))
!     {
!       /* We may have skipped over point because the previous glyphs
! 	 are from string.  As there's no easy way to know the
! 	 character position of the current glyph, find the correct
! 	 glyph on point by scanning from string_start again.  */
!       Lisp_Object pos, limit;
! 
!       limit = make_number (MATRIX_ROW_END_CHARPOS (row) + delta);
!       glyph = string_start;
!       x = string_start_x;
!       pos = make_number (string_buffer_position (w, glyph->object,
! 						 string_before_pos));
!       pos = Fnext_single_char_property_change (pos, Qdisplay, Qnil, limit);
!       while (XINT (pos) <= pt_old)
! 	{
! 	  /* Skip glyphs from the same string.  */
! 	  do
! 	    {
! 	      x += glyph->pixel_width;
! 	      ++glyph;
! 	    }
! 	  while (glyph < end
! 		 && EQ (glyph->object, string_start->object));
! 	  if (glyph == end || !STRINGP (glyph->object))
! 	    break;
! 	  string_start = glyph;
! 	  pos = Fnext_single_char_property_change (pos, Qdisplay, Qnil, limit);
! 	}
      }
  
    w->cursor.hpos = glyph - row->glyphs[TEXT_AREA];

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Gerd Moellmann]

Kenichi Handa <handa@m17n.org> writes:

> The reason why the change looks complicated is to correctly
> handle this kind of case where string_buffer_position is
> confused by the same string used in display properties.
> 
> (setq s1 "<dot>")
> (setq s2 "<comma>")
> (setq c1 (propertize "." 'display s1))
> (setq c2 (propertize "," 'display s2))
> (insert c1 c2 c1 c2 c1 c2)

Oh, hm, yes, that's a problem.

> Could you check the change and comment again?

I can't see anything wrong.

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Kenichi Handa]

In article <86el6wunsr.fsf@gerd.free-bsd.org>, gerd.moellmann@t-online.de (Gerd Moellmann) writes:
>>  Could you check the change and comment again?

> I can't see anything wrong.

Thank you.  If you can't, perhaps no one can.  :-)

Ok, I've just installed that change.  Let's see the effect.

By the way, I found that current-column, move-to-column,
etc. don't take display properties into acount.  I vaguely
remember that once there was a discussion about using
display engine to calculte column numbers.  Are there anyone
working on it?

---
Ken'ichi HANDA
handa@m17n.org

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Eli Zaretskii]

On Wed, 29 Jan 2003, Kenichi Handa wrote:

> By the way, I found that current-column, move-to-column,
> etc. don't take display properties into acount.  I vaguely
> remember that once there was a discussion about using
> display engine to calculte column numbers.

That discussion revealed a problem with this approach: sometimes you need 
to compute columns in a portion of a buffer that isn't displayed, so it 
doesn't appear in the glyph matrices.

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Kenichi Handa]

In article <Pine.SUN.3.91.1030129162559.20453D-100000@is>, Eli Zaretskii <eliz@is.elta.co.il> writes:
> On Wed, 29 Jan 2003, Kenichi Handa wrote:

>>  By the way, I found that current-column, move-to-column,
>>  etc. don't take display properties into acount.  I vaguely
>>  remember that once there was a discussion about using
>>  display engine to calculte column numbers.

> That discussion revealed a problem with this approach: sometimes you need 
> to compute columns in a portion of a buffer that isn't displayed, so it 
> doesn't appear in the glyph matrices.

Of course we can't reuse the existing glyph matrics, but,
was it the conclusion that we can't use `struct it' without
a glyph matrics?

For instance, it seems to me that doing this is possible.

(1) call init_iterator with ROW == NULL
(2) call reseat with POS == (line-beginning-position)
(3) call move_it_to with TO_CHARPOS == (point)
(4) it->hpos is the current column.

---
Ken'ichi HANDA
handa@m17n.org

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Eli Zaretskii]

> Date: Thu, 30 Jan 2003 13:26:43 +0900 (JST)
> From: Kenichi Handa <handa@m17n.org>
> 
> was it the conclusion that we can't use `struct it' without
> a glyph matrics?

I don't think so.

> For instance, it seems to me that doing this is possible.
> 
> (1) call init_iterator with ROW == NULL
> (2) call reseat with POS == (line-beginning-position)
> (3) call move_it_to with TO_CHARPOS == (point)
> (4) it->hpos is the current column.

I'm not sure; does this really work?  Including some unusual
situations like display tables, overlays/text properties, and other
calamities that affect how text is displayed?

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Stefan Monnier]

> > Date: Thu, 30 Jan 2003 13:26:43 +0900 (JST)
> > From: Kenichi Handa <handa@m17n.org>
> > 
> > was it the conclusion that we can't use `struct it' without
> > a glyph matrics?
> 
> I don't think so.
> 
> > For instance, it seems to me that doing this is possible.
> > 
> > (1) call init_iterator with ROW == NULL
> > (2) call reseat with POS == (line-beginning-position)
> > (3) call move_it_to with TO_CHARPOS == (point)
> > (4) it->hpos is the current column.
> 
> I'm not sure; does this really work?  Including some unusual
> situations like display tables, overlays/text properties, and other
> calamities that affect how text is displayed?

I don't think it's too bad.  The one concrete case where I know that
current-column purposefully does not try to reproduce the behavior
of the display is that current-column does *not* ignore text that's
invisible but represented by an ellipsis.  

We'll probably need some tweaking to get the iterator to do that
correctly.  AFAIK, this is only done so as to get meaningful results
when current-column (and move-to-column) is called from within such
"ellipsisified" text, so maybe we can take care of it without
doing anything special in the iterator itself.

Anybody knows of another such issue ?


	Stefan

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Gerd Moellmann]

"Eli Zaretskii" <eliz@is.elta.co.il> writes:

> > For instance, it seems to me that doing this is possible.
> > 
> > (1) call init_iterator with ROW == NULL
> > (2) call reseat with POS == (line-beginning-position)
> > (3) call move_it_to with TO_CHARPOS == (point)
> > (4) it->hpos is the current column.
> 
> I'm not sure; does this really work?  Including some unusual
> situations like display tables, overlays/text properties, and other
> calamities that affect how text is displayed?

The only thing the glyph_row pointer in struct it is used for
is for filling out glyhs.  Everything else should be the same with
or without filling glyhs.

Re: problem of display property [Re: list-charset-chars and unicode-bmp]

[Dave Love]

Kenichi Handa <handa@m17n.org> writes:

> That bug can be reproduced as below.
> 
> (insert "abc" (propertize "." 'display "<dot>") "def")
> 
> When you put cursor on `c' and type C-f, the cursor is
> placed on `d' instead of `<' of "<dot>", but C-x = shows
> that point is correctly on "."

That's actually not the only issue.  Another one in that context was
the behaviour if you put a display property on a newline to avoid the
table of characters being broken.

I haven't followed this, but I think that at least display properties
which are strings should work in pretty much the same way as
compositions, which seem to be better behaved, and images should
probably be likewise.  

Apart from issues of cursor movement, I think display text properties
should be uniquified on insertions the same way as compositions
currently are.  Thus if you cut and paste an image to duplicate it,
you should end up with two copies, not one.  This seems much more
likely to be what you want than merging the images due to text
properties of the adjacent text being the same.  (There's currently
code in image.el to do that sort of thing when you insert images
explicitly with the functions in image.el, but it's not a general
mechanism.)