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 <address@hidden>, Dave Love <address@hidden> 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
address@hidden

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;