Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Thu, 16 Oct 2014 07:55:53 +0400
> From: Dmitry Antipov <address@hidden>
> CC: address@hidden
> 
> On 10/15/2014 09:50 PM, Eli Zaretskii wrote:
> 
> > So once again, I need to ask you to step through x_draw_hollow_cursor,
> > and see what goes wrong there in this case.
> 
>    7950  x_draw_hollow_cursor (struct window *w, struct glyph_row *row)
>    7951  {
>    7952    struct frame *f = XFRAME (WINDOW_FRAME (w));
>    7953    struct x_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
>    7954    Display *dpy = FRAME_X_DISPLAY (f);
>    7955    int x, y, wd, h;
>    7956    XGCValues xgcv;
>    7957    struct glyph *cursor_glyph;
>    7958    GC gc;
>    7959
>    7960    /* Get the glyph the cursor is on.  If we can't tell because
>    7961       the current matrix is invalid or such, give up.  */
> 1 7962    cursor_glyph = get_phys_cursor_glyph (w);
>    7963    if (cursor_glyph == NULL)
>    7964      return;
>    7965
>    7966    /* Compute frame-relative coordinates for phys cursor.  */
>    7967    get_phys_cursor_geometry (w, row, cursor_glyph, &x, &y, &h);
> 2 7968    wd = w->phys_cursor_width;
>    7969
>    7970    /* The foreground of cursor_gc is typically the same as the normal
>    7971       background color, which can cause the cursor box to be 
> invisible.  */
>    7972    xgcv.foreground = f->output_data.x->cursor_pixel;
>    7973    if (dpyinfo->scratch_cursor_gc)
>    7974      XChangeGC (dpy, dpyinfo->scratch_cursor_gc, GCForeground, &xgcv);
>    7975    else
>    7976      dpyinfo->scratch_cursor_gc = XCreateGC (dpy, FRAME_X_WINDOW (f),
>    7977                                              GCForeground, &xgcv);
>    7978    gc = dpyinfo->scratch_cursor_gc;
>    7979
>    7980    /* When on R2L character, show cursor at the right edge of the
>    7981       glyph, unless the cursor box is as wide as the glyph or wider
>    7982       (the latter happens when x-stretch-cursor is non-nil).  */
>    7983    if ((cursor_glyph->resolved_level & 1) != 0
>    7984        && cursor_glyph->pixel_width > w->phys_cursor_width)
>    7985      {
>    7986        x += cursor_glyph->pixel_width - w->phys_cursor_width;
> 3 7987        wd -= 1;
>    7988      }
>    7989    /* Set clipping, draw the rectangle, and reset clipping again.  */
>    7990    x_clip_to_row (w, row, TEXT_AREA, gc);
> 4 7991    XDrawRectangle (dpy, FRAME_X_WINDOW (f), gc, x, y, wd, h - 1);
>    7992    XSetClipMask (dpy, gc, None);
>    7993  }
> 
> At 1), cursor_glyph is GLYPHLESS_GLYPH with pixel_width 1;
> at 2), wd is 0;
> at 3), wd is -1;
> at 4), there is an integer overflow because XDrawRectangle accepts
>         width and height as unsigned.

Thanks.  So this is not really a result of my merge-commit in r118121,
this problem should have existed before, since r117923, right?

The other question I have is: what does XDrawRectangle do when its 6th
argument 'width' is zero?  That's what it was getting before r117923
on this character, AFAIU.  Does it enlarge the rectangle to 1 pixel,
or does it not show the hollow cursor at all?  (You could probably
simulate what was happening before r117923 by setting
bidi-display-reordering to nil.)

Anyway, it looks like the right fix for this is as follows:

=== modified file 'src/xdisp.c'
--- src/xdisp.c 2014-10-14 18:10:37 +0000
+++ src/xdisp.c 2014-10-16 07:16:49 +0000
@@ -2303,9 +2303,6 @@ get_phys_cursor_geometry (struct window 
      rectangle as wide as the glyph, but use a canonical character
      width instead.  */
   wd = glyph->pixel_width - 1;
-#if defined (HAVE_NTGUI) || defined (HAVE_NS)
-  wd++; /* Why? */
-#endif
 
   x = w->phys_cursor.x;
   if (x < 0)


I never understood why we subtract 1 pixel from the cursor glyph's
pixel_width, anyway, and w32 and ns countermanded that, as you see.
Maybe we should also limit 'wd' from below, so it is at least 1.

Jan, can you comment on these issues and on the proposed patch,
please?