Bidi reordering engine upgraded

Bidi reordering engine upgraded

[Eli Zaretskii]

With revision 118121, the upgraded bidi reordering engine landed on
the trunk.  Barring bugs, we are now fully compliant with The Unicode
Bidirectional Algorithm defined by the latest Unicode Standard v7.0,
and even with a few suggestions that will only make it into Unicode
8.0.

The changes in the UBA as defined by the latest Unicode Standard, wrt
to the original UBA we had until and including Unicode 6.2, are
significant.  They required serious refactoring and reimplementation
of several core parts in bidi.c, something that wasn't attempted for
quite a few years.

In addition, one of the new UBA features, the so-called Bidirectional
Parentheses Algorithm (BPA), affects pure-ASCII text as well, and
specifically editing of program sources (which widely use parentheses
and brackets of several kinds).

The result is some small slowdown -- a few percents in my testing --
in redisplay operations.  If more significant slowdown will be
reported in some special cases, I will try to find optimizations to
countermand that.

No matter how much testing I've put into the new code, there will
probably be bugs.  Please be attentive to any display glitches in the
trunk code, let alone crashes etc., and report them as bugs.

Thanks, and enjoy.

Re: Bidi reordering engine upgraded

[Dmitry Antipov]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On 10/15/2014 06:51 PM, Eli Zaretskii wrote:

> No matter how much testing I've put into the new code, there will
> probably be bugs.  Please be attentive to any display glitches in the
> trunk code, let alone crashes etc., and report them as bugs.

1) s/lnger/longer in ChangeLog

2) a) ./src/emacs -Q
    b) in *scratch*, M-x insert-char 061c
    c) on the same line, 'aaaa'
    d) C-a
    e) M-x
    ==> subtle redisplay glitch (attached)

Dmitry



[-- Attachment #2: glitch.png --]
[-- Type: image/png, Size: 27369 bytes --]

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Wed, 15 Oct 2014 19:22:10 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> 1) s/lnger/longer in ChangeLog

Thanks, fixed.

> 2) a) ./src/emacs -Q
>     b) in *scratch*, M-x insert-char 061c
>     c) on the same line, 'aaaa'
>     d) C-a
>     e) M-x
>     ==> subtle redisplay glitch (attached)

What glitch?  I don't see any problem on my machine (and don't
understand what is wrong with the snapshot you sent).

Re: Bidi reordering engine upgraded

[Dmitry Antipov]

On 10/15/2014 07:47 PM, Eli Zaretskii wrote:

> What glitch?  I don't see any problem on my machine (and don't
> understand what is wrong with the snapshot you sent).

Why the black unclosed rectangle on 'aaaa' line?

Dmitry

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Wed, 15 Oct 2014 20:00:28 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> On 10/15/2014 07:47 PM, Eli Zaretskii wrote:
> 
> > What glitch?  I don't see any problem on my machine (and don't
> > understand what is wrong with the snapshot you sent).
> 
> Why the black unclosed rectangle on 'aaaa' line?

I don't see it here.  And the changes didn't touch terminal specific
code, which is where the cursor is drawn.

Can you look at the cursor glyph where hollow cursor is drawn on X,
and tell which parameters of the glyph have bad values?

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Wed, 15 Oct 2014 19:31:34 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > Date: Wed, 15 Oct 2014 20:00:28 +0400
> > From: Dmitry Antipov <dmantipov@yandex.ru>
> > CC: emacs-devel@gnu.org
> > 
> > On 10/15/2014 07:47 PM, Eli Zaretskii wrote:
> > 
> > > What glitch?  I don't see any problem on my machine (and don't
> > > understand what is wrong with the snapshot you sent).
> > 
> > Why the black unclosed rectangle on 'aaaa' line?
> 
> I don't see it here.  And the changes didn't touch terminal specific
> code, which is where the cursor is drawn.

Actually, I see some incorrect visual order here (try inserting a few
more 0x061c characters: they are displayed in reverse order, at least
for me).  So I guess I have something to work here, thanks.

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Wed, 15 Oct 2014 20:28:00 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > Date: Wed, 15 Oct 2014 19:31:34 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: emacs-devel@gnu.org
> > 
> > > Date: Wed, 15 Oct 2014 20:00:28 +0400
> > > From: Dmitry Antipov <dmantipov@yandex.ru>
> > > CC: emacs-devel@gnu.org
> > > 
> > > On 10/15/2014 07:47 PM, Eli Zaretskii wrote:
> > > 
> > > > What glitch?  I don't see any problem on my machine (and don't
> > > > understand what is wrong with the snapshot you sent).
> > > 
> > > Why the black unclosed rectangle on 'aaaa' line?
> > 
> > I don't see it here.  And the changes didn't touch terminal specific
> > code, which is where the cursor is drawn.
> 
> Actually, I see some incorrect visual order here (try inserting a few
> more 0x061c characters: they are displayed in reverse order, at least
> for me).  So I guess I have something to work here, thanks.

Oops, this is an Arabic letter, so the reordering is expected.

So once again, I need to ask you to step through x_draw_hollow_cursor,
and see what goes wrong there in this case.

TIA

Re: Bidi reordering engine upgraded

[Dmitry Antipov]

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.

Dmitry

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Thu, 16 Oct 2014 07:55:53 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> 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?

Re: Bidi reordering engine upgraded

[Thien-Thi Nguyen]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

() Eli Zaretskii <eliz@gnu.org>
() Thu, 16 Oct 2014 10:21:52 +0300

   why we subtract 1 pixel from the cursor glyph's pixel_width

I see in XDrawRectangle(3):

 The XDrawRectangle and XDrawRectangles functions draw the
 outlines of the specified rectangle or rectangles as if a
 five-point PolyLine protocol request were specified for
 each rectangle:

   [x,y] [x+width,y] [x+width,y+height] [x,y+height] [x,y]

So if you use the glyph width directly, the resulting rectangle
drawn will invade the next character cell by one pixel.  E.g.:
Given a glyph width of ten, this is wrong:

 | 0         1         2
 | 012345678901234567890123456
 |           XXXXXXXXXXX
 |           X         X

but this is right:

 | 0         1         2
 | 012345678901234567890123456
 |           XXXXXXXXXX
 |           X        X

Probably (i don't have docs handy) the other toolkits'
rectangle-drawing primitives draw from X,Y to X+W-1,Y+H-1
(NB the -1) so there is no need to manully decrement for them.

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> From: Thien-Thi Nguyen <ttn@gnu.org>
> Date: Thu, 16 Oct 2014 11:42:34 +0200
> 
>    why we subtract 1 pixel from the cursor glyph's pixel_width
> 
> I see in XDrawRectangle(3):
> 
>  The XDrawRectangle and XDrawRectangles functions draw the
>  outlines of the specified rectangle or rectangles as if a
>  five-point PolyLine protocol request were specified for
>  each rectangle:
> 
>    [x,y] [x+width,y] [x+width,y+height] [x,y+height] [x,y]
> 
> So if you use the glyph width directly, the resulting rectangle
> drawn will invade the next character cell by one pixel.

The last sentence is your interpretation, because the documentation
you cite does not explain what exactly it means by "the outlines of
the rectangle".  That is, it doesn't say whether the 1-pixel border of
the outline exceeds the x+width coordinate, or ends at it.  And that's
the crucial point here.

> Probably (i don't have docs handy) the other toolkits'
> rectangle-drawing primitives draw from X,Y to X+W-1,Y+H-1

I don't think so, at least not the w32 toolkit, whose documentation
(http://msdn.microsoft.com/en-us/library/dd144838%28v=vs.85%29.aspx)
says:

  The FrameRect function draws a border around the specified rectangle
  by using the specified brush. The width and height of the border are
  always one logical unit.

This is equivalent to what you cited from the X docs, AFAIU.

Re: Bidi reordering engine upgraded

[Thien-Thi Nguyen]

[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]

() Eli Zaretskii <eliz@gnu.org>
() Thu, 16 Oct 2014 13:15:24 +0300

   >  The XDrawRectangle and XDrawRectangles functions draw the
   >  outlines of the specified rectangle or rectangles as if a
   >  five-point PolyLine protocol request were specified for
   >  each rectangle:
   > 
   >    [x,y] [x+width,y] [x+width,y+height] [x,y+height] [x,y]
   > 
   > So if you use the glyph width directly, the resulting rectangle
   > drawn will invade the next character cell by one pixel.

   The last sentence is your interpretation, because the
   documentation you cite does not explain what exactly it means by
   "the outlines of the rectangle".  That is, it doesn't say whether
   the 1-pixel border of the outline exceeds the x+width coordinate,
   or ends at it.  And that's the crucial point here.

My interpretation is informed by the word "PolyLine" (and general
experience playing w/ the X protocol[0], but that is tangential); i
ignore the word "outline" as noise.  XDrawline(3) explains things
in gory detail, but to summarize, pixels "around" each point in a
"polyline" are "rendered", so the upshot (of my example) is if ‘x’ is
20, ‘w’ is 10, and the "line width" is 1, then the pixel at 30,y will
be rendered (undesirable) instead of the one at 29,y (desirable).

   > Probably (i don't have docs handy) the other toolkits'
   > rectangle-drawing primitives draw from X,Y to X+W-1,Y+H-1

   I don't think so, at least not the w32 toolkit, whose documentation
   (http://msdn.microsoft.com/en-us/library/dd144838%28v=vs.85%29.aspx)
   says:

     The FrameRect function draws a border around the specified
     rectangle by using the specified brush.  The width and height of
     the border are always one logical unit.

   This is equivalent to what you cited from the X docs, AFAIU.

I think it hinges on what how "the specified rectangle" is specified.
Also, here the "around" does indeed cast ambiguity in my mind.

___________________________________________________
[0] that is, directly scheming on a socket
    (http://www.gnuvola.org/software/ttn-do/).
    See modules ‘(ttn-do zzz x-protocol)’ et al
    and program xout (xout.scm), proc ‘xout frame!’,
    specifically line 127 where the -1 occurs for the
    "manual XDrawRectangle", i.e., 4-segment (5 points)
    PolyLine protocol, call.

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> From: Thien-Thi Nguyen <ttn@gnu.org>
> Date: Thu, 16 Oct 2014 15:27:17 +0200
> 
> 
> [1:text/plain Hide]
> 
> () Eli Zaretskii <eliz@gnu.org>
> () Thu, 16 Oct 2014 13:15:24 +0300
> 
>    >  The XDrawRectangle and XDrawRectangles functions draw the
>    >  outlines of the specified rectangle or rectangles as if a
>    >  five-point PolyLine protocol request were specified for
>    >  each rectangle:
>    > 
>    >    [x,y] [x+width,y] [x+width,y+height] [x,y+height] [x,y]
>    > 
>    > So if you use the glyph width directly, the resulting rectangle
>    > drawn will invade the next character cell by one pixel.
> 
>    The last sentence is your interpretation, because the
>    documentation you cite does not explain what exactly it means by
>    "the outlines of the rectangle".  That is, it doesn't say whether
>    the 1-pixel border of the outline exceeds the x+width coordinate,
>    or ends at it.  And that's the crucial point here.
> 
> My interpretation is informed by the word "PolyLine" (and general
> experience playing w/ the X protocol[0], but that is tangential); i
> ignore the word "outline" as noise.  XDrawline(3) explains things
> in gory detail, but to summarize, pixels "around" each point in a
> "polyline" are "rendered", so the upshot (of my example) is if ‘x’ is
> 20, ‘w’ is 10, and the "line width" is 1, then the pixel at 30,y will
> be rendered (undesirable) instead of the one at 29,y (desirable).

I cannot argue with experience and facts, but please be precise in
your description: when you say "the pixel at 30,y will be rendered",
do you mean between 30 and 31, or do you mean between 29 and 30?

More importantly, if you remove the "- 1" part, per my suggested
patch, do you see a change in the cursor dimensions when you toggle a
window between selected (filled block cursor) and non-selected (hollow
block cursor) status?

>      The FrameRect function draws a border around the specified
>      rectangle by using the specified brush.  The width and height of
>      the border are always one logical unit.
> 
>    This is equivalent to what you cited from the X docs, AFAIU.
> 
> I think it hinges on what how "the specified rectangle" is specified.

This sentence didn't parse.  How the rectangle is specified is
described on the same page, so you've probably read that already.

Re: Bidi reordering engine upgraded

[Thien-Thi Nguyen]

[-- Attachment #1: Type: text/plain, Size: 1663 bytes --]

() Eli Zaretskii <eliz@gnu.org>
() Thu, 16 Oct 2014 16:51:43 +0300

   when you say "the pixel at 30,y will be rendered", do you
   mean between 30 and 31, or do you mean between 29 and 30?

I don't mean "between" anything.  My mental model of the
mapping of a coordinate component to a screen pixel (in X) is
direct.  It doesn't admit subpixel (fractional) specification.
Left-most pixel is numbered 0, and "at 0" refers to that pixel.

So in my description "at" is actually superfluous.  Thanks for
prompting this realization!  Please (re-)read as "pixel 30,y".

   More importantly, if you remove the "- 1" part, per my
   suggested patch, do you see a change in the cursor
   dimensions when you toggle a window between selected
   (filled block cursor) and non-selected (hollow block
   cursor) status?

I confess i haven't yet run the test case, but just jumped
into the discussion pedantically.

   > I think it hinges on what how "the specified rectangle"
   > is specified.

   This sentence didn't parse.  How the rectangle is specified
   is described on the same page, so you've probably read that
   already.

Likewise, i have not visited that page.  All i know is that
there are different models for describing a rectangle (even
w/in the same system -- e.g., SDL "rectangle" and SDL_gfx
"draw rectangle") and that surfing among them one finds many
OBOEs sounding right and left...  :-D

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> From: Thien-Thi Nguyen <ttn@gnu.org>
> Cc: emacs-devel@gnu.org
> Date: Fri, 17 Oct 2014 07:42:16 +0200
> 
> () Eli Zaretskii <eliz@gnu.org>
> () Thu, 16 Oct 2014 16:51:43 +0300
> 
>    when you say "the pixel at 30,y will be rendered", do you
>    mean between 30 and 31, or do you mean between 29 and 30?
> 
> I don't mean "between" anything.  My mental model of the
> mapping of a coordinate component to a screen pixel (in X) is
> direct.  It doesn't admit subpixel (fractional) specification.
> Left-most pixel is numbered 0, and "at 0" refers to that pixel.

But with that model, a 1-pixel line "at 0" does NOT "invade" on pixel
1.

Anyway, this is all pointless, because we need to decide first what we
would like to see as the hollow cursor for a glyph that is 1-pixel
wide.

Re: Bidi reordering engine upgraded

[Thien-Thi Nguyen]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

() Eli Zaretskii <eliz@gnu.org>
() Fri, 17 Oct 2014 09:16:05 +0300

   > Left-most pixel is numbered 0, and "at 0"
   > refers to that pixel.

   But with that model, a 1-pixel line "at 0"
   does NOT "invade" on pixel 1.

Granted (if i take "1-pixel" to mean the length of
the line, and not its "line width" (a measure
orthogonal to the line's orientation), which is
what i normally understand "1-pixel line" to mean),
but a foo-pixel line "from X to X+W"[0] renders X,
X+1, X+2 ... X+W, for a total count of W+1 pixels.
OBOE!  The invading pixel is not X+1, but X+W.

 | 1         2         3
 | 012345678901234567890123456789
 |           ***********
 |                     ^ out out damned dot!

(11 *s; X = 20, W = 10.)  But anyway...

   decide first what we would like to see as the
   hollow cursor for a glyph that is 1-pixel wide.

Perhaps "hollow" in the vertical sense only could be
expressed as two pixels, one at top, one at bottom.
That's almost invisible, though; maybe two or three
at top and two at bottom is better.

___________________________________
[0] (under X Windows semantics)
    In src/xdisp.c ‘get_phys_cursor_geometry’, the
    comment /* Why? */ alludes to (confusion /
    frustration probably felt due to) the differences
    i mentioned previously.  Probably Someone needs
    to define the "Emacs native coordinate and pixel
    reference system" and add a centrally maintained
    abstraction layer for X, NTGUI, NS, and so on.
    This layer is where X-specific -1/+1 futzing
    should live.  Standard slog...

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> From: Thien-Thi Nguyen <ttn@gnu.org>
> Date: Fri, 17 Oct 2014 09:50:30 +0200
> 
> 
> [1:text/plain Hide]
> 
> () Eli Zaretskii <eliz@gnu.org>
> () Fri, 17 Oct 2014 09:16:05 +0300
> 
>    > Left-most pixel is numbered 0, and "at 0"
>    > refers to that pixel.
> 
>    But with that model, a 1-pixel line "at 0"
>    does NOT "invade" on pixel 1.
> 
> Granted (if i take "1-pixel" to mean the length of
> the line, and not its "line width" (a measure
> orthogonal to the line's orientation), which is
> what i normally understand "1-pixel line" to mean),
> but a foo-pixel line "from X to X+W"[0] renders X,
> X+1, X+2 ... X+W, for a total count of W+1 pixels.
> OBOE!  The invading pixel is not X+1, but X+W.

But there are W+1 pixels in a line that starts at X and ends at X+W.
W+1, not W.

> 
>  | 1         2         3
>  | 012345678901234567890123456789
>  |           ***********
>  |                     ^ out out damned dot!

Count the stars, and you will see there are 11 of them, not 10.  Where
did the 11th one come from, when we requested a width of 10?

>    decide first what we would like to see as the
>    hollow cursor for a glyph that is 1-pixel wide.
> 
> Perhaps "hollow" in the vertical sense only could be
> expressed as two pixels, one at top, one at bottom.
> That's almost invisible, though; maybe two or three
> at top and two at bottom is better.

Why not just a single-pixel vertical line instead?

Re: Bidi reordering engine upgraded

[Thien-Thi Nguyen]

[-- Attachment #1: Type: text/plain, Size: 1774 bytes --]

() Eli Zaretskii <eliz@gnu.org>
() Fri, 17 Oct 2014 11:25:54 +0300

   But there are W+1 pixels in a line that
   starts at X and ends at X+W. W+1, not W.

Exactly!  We have achieved identical conclusions from
reproducible observations!  Cool.

   >  | 1         2         3
   >  | 012345678901234567890123456789
   >  |           ***********
   >  |                     ^ out out damned dot!

   Count the stars, and you will see there are 11 of them,
   not 10.

Right -- that's what "11 *s" my message was meant to convey.
(No argument here from this pedant. :-D)

   Where did the 11th one come from, when we requested a
   width of 10?

It comes from the (deeply-reviled, IMNSHO) misdesign of
XDrawRectangle(3), which uses the word "width" in the
parameter name (and description), but is actually described
later to draw a rectangle whose width, as measured by pixels
rendered, is ‘width + 1’.  Yuck!  And that description is not
straightforward, but instead exposes, and thus requires the
reader to understand, the ‘PolyLine’ X protocol request, an
implementation detail.  Ugh!Ly!  [Insert more ranting here.]

Well, i'm starting to foam at the mouth re Xlib, which was
one of the motivations for scheming on the socket directly
(inducing other foamings, of course...), so i'll stop now.

   Why not just a single-pixel vertical line instead?

Wouldn't that lose the "hollow" look?  Also, depending on
choice of foreground (text) and cursor colors, that could
obscure the glyph completely.

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> From: Thien-Thi Nguyen <ttn@gnu.org>
> Date: Fri, 17 Oct 2014 12:27:24 +0200
> 
> 
>    Where did the 11th one come from, when we requested a
>    width of 10?
> 
> It comes from the (deeply-reviled, IMNSHO) misdesign of
> XDrawRectangle(3), which uses the word "width" in the
> parameter name (and description), but is actually described
> later to draw a rectangle whose width, as measured by pixels
> rendered, is ‘width + 1’.  Yuck!  And that description is not
> straightforward, but instead exposes, and thus requires the
> reader to understand, the ‘PolyLine’ X protocol request, an
> implementation detail.  Ugh!Ly!  [Insert more ranting here.]

OK, we are in violent agreement here.

>    Why not just a single-pixel vertical line instead?
> 
> Wouldn't that lose the "hollow" look?

Yes, but I think the alternatives are worse.

Re: Bidi reordering engine upgraded

[Dmitry Antipov]

On 10/16/2014 11:21 AM, Eli Zaretskii wrote:

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

Yes.

> 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?

AFAICS it draws nothing.

Dmitry

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Thu, 16 Oct 2014 13:51:53 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Jan Djärv <jan.h.d@swipnet.se>, 
>  emacs-devel@gnu.org
> 
> > 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?
> 
> AFAICS it draws nothing.

Is that what we want for 1-pixel "thin space" characters?  It means
the cursor will only appear when the window is the selected one, which
sounds wrong to me.

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Thu, 16 Oct 2014 13:51:53 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Jan Djärv <jan.h.d@swipnet.se>, 
>  emacs-devel@gnu.org
> 
> On 10/16/2014 11:21 AM, Eli Zaretskii wrote:
> 
> > 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?
> 
> AFAICS it draws nothing.

Btw, do you see the same situation (i.e. no cursor in inactive window
on L2R thin-space character, and some "glitchy" display on R2L
thin-space characters) if you use a horizontal-bar cursor, i.e.

  M-: (setq cursor-type 'hbar) RET

?

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

I installed a safer fix on the emacs-24 branch, only in xterm.c: it
decrements 'wd' only if it is positive.  (Sorry, Glenn.)

Please try that, you should now see no hollow cursor in that case,
exactly like when the 1-pixel character is L2R.

Re: Bidi reordering engine upgraded

[Jan Djärv]

Hello.

16 okt 2014 kl. 09:21 skrev Eli Zaretskii <eliz@gnu.org>:

> 
> 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?

As you figured out, XDrawRectangle with width 0 and non-zero height draws a 1-pixel width line.

	Jan D.

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> From: Jan Djärv <jan.h.d@swipnet.se>
> Date: Fri, 17 Oct 2014 19:45:34 +0200
> Cc: Dmitry Antipov <dmantipov@yandex.ru>,
>  emacs-devel@gnu.org
> 
> > === 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?
> 
> As you figured out, XDrawRectangle with width 0 and non-zero height draws a 1-pixel width line.

Thanks.  Since this appears to be specific to X, I propose the
change below for the trunk.  Does it look reasonable?

The only other question is whether we want the hollow cursor on X to
disappear when the underlying glyph is displayed as a thin space of 1
pixel.  It doesn't disappear on w32 (and possibly also on NS, although
I cannot test that).  Opinions, anyone?

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2014-10-14 18:10:37 +0000
+++ src/xdisp.c	2014-10-17 18:39:20 +0000
@@ -2302,10 +2302,7 @@ get_phys_cursor_geometry (struct window 
      glyph, and `x-stretch-block-cursor' is nil, don't draw a
      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
+  wd = glyph->pixel_width;
 
   x = w->phys_cursor.x;
   if (x < 0)

=== modified file 'src/xterm.c'
--- src/xterm.c	2014-10-17 16:14:37 +0000
+++ src/xterm.c	2014-10-17 18:42:08 +0000
@@ -7965,7 +7965,7 @@ x_draw_hollow_cursor (struct window *w, 
 
   /* Compute frame-relative coordinates for phys cursor.  */
   get_phys_cursor_geometry (w, row, cursor_glyph, &x, &y, &h);
-  wd = w->phys_cursor_width;
+  wd = w->phys_cursor_width - 1;
 
   /* The foreground of cursor_gc is typically the same as the normal
      background color, which can cause the cursor box to be invisible.  */
@@ -7981,9 +7981,9 @@ x_draw_hollow_cursor (struct window *w, 
      glyph, unless the cursor box is as wide as the glyph or wider
      (the latter happens when x-stretch-cursor is non-nil).  */
   if ((cursor_glyph->resolved_level & 1) != 0
-      && cursor_glyph->pixel_width > w->phys_cursor_width)
+      && cursor_glyph->pixel_width > wd)
     {
-      x += cursor_glyph->pixel_width - w->phys_cursor_width;
+      x += cursor_glyph->pixel_width - wd;
       if (wd > 0)
 	wd -= 1;
     }
@@ -8086,12 +8086,12 @@ x_draw_bar_cursor (struct window *w, str
 				    &dummy_y, &dummy_h);
 
 	  if ((cursor_glyph->resolved_level & 1) != 0
-	      && cursor_glyph->pixel_width > w->phys_cursor_width)
-	    x += cursor_glyph->pixel_width - w->phys_cursor_width;
+	      && cursor_glyph->pixel_width > w->phys_cursor_width - 1)
+	    x += cursor_glyph->pixel_width - w->phys_cursor_width - 1;
 	  XFillRectangle (dpy, window, gc, x,
 			  WINDOW_TO_FRAME_PIXEL_Y (w, w->phys_cursor.y +
 						   row->height - width),
-			  w->phys_cursor_width, width);
+			  w->phys_cursor_width - 1, width);
 	}
 
       XSetClipMask (dpy, gc, None);

Re: Bidi reordering engine upgraded

[Stefan Monnier]

> The only other question is whether we want the hollow cursor on X to
> disappear when the underlying glyph is displayed as a thin space of 1
> pixel.  It doesn't disappear on w32 (and possibly also on NS, although
> I cannot test that).  Opinions, anyone?

"hollow" sounds to me like it should still be visible, so I think we
should draw it as a 1-pixel line.  Maybe in the case of a 2-pixel
cursor, the hollow version should be reduced to 1 pixel.


        Stefan

Re: Bidi reordering engine upgraded

[Jan Djärv]

Hi.

17 okt 2014 kl. 20:45 skrev Eli Zaretskii <eliz@gnu.org>:

> 
>   /* Compute frame-relative coordinates for phys cursor.  */
>   get_phys_cursor_geometry (w, row, cursor_glyph, &x, &y, &h);
> -  wd = w->phys_cursor_width;
> +  wd = w->phys_cursor_width - 1;

Are we sure this never becomes negative?
I suggest you check it in so we can test it.  Do we have a test case?

	Jan D.

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> From: Jan Djärv <jan.h.d@swipnet.se>
> Date: Sat, 18 Oct 2014 14:34:06 +0200
> Cc: dmantipov@yandex.ru,
>  emacs-devel@gnu.org
> 
> >   /* Compute frame-relative coordinates for phys cursor.  */
> >   get_phys_cursor_geometry (w, row, cursor_glyph, &x, &y, &h);
> > -  wd = w->phys_cursor_width;
> > +  wd = w->phys_cursor_width - 1;
> 
> Are we sure this never becomes negative?

If it does, we already have that problem, since previously, the
subtraction of 1 was done in get_phys_cursor_geometry.

> I suggest you check it in so we can test it.

OK, will do.

> Do we have a test case?

Yes, the one posted by Dmitry here:

  http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00518.html

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Sat, 18 Oct 2014 15:54:15 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: dmantipov@yandex.ru, emacs-devel@gnu.org
> 
> > I suggest you check it in so we can test it.
> 
> OK, will do.
> 
> > Do we have a test case?
> 
> Yes, the one posted by Dmitry here:
> 
>   http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00518.html

OK, committed as trunk revision 118149.

Note that I also asked about similar problems with hbar cursor, see
http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00555.html.

Re: Bidi reordering engine upgraded

[Jan Djärv]

Den 2014-10-18 15:13, Eli Zaretskii skrev:
>> Date: Sat, 18 Oct 2014 15:54:15 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: dmantipov@yandex.ru, emacs-devel@gnu.org
>>
>>> I suggest you check it in so we can test it.
>>
>> OK, will do.
>>
>>> Do we have a test case?
>>
>> Yes, the one posted by Dmitry here:
>>
>>    http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00518.html
>
> OK, committed as trunk revision 118149.

Looks fine to me, no lines except the cursor.

	Jan D.

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Sun, 19 Oct 2014 13:45:40 +0200
> From: Jan Djärv <jan.h.d@swipnet.se>
> CC: dmantipov@yandex.ru, emacs-devel@gnu.org
> 
> Den 2014-10-18 15:13, Eli Zaretskii skrev:
> >> Date: Sat, 18 Oct 2014 15:54:15 +0300
> >> From: Eli Zaretskii <eliz@gnu.org>
> >> Cc: dmantipov@yandex.ru, emacs-devel@gnu.org
> >>
> >>> I suggest you check it in so we can test it.
> >>
> >> OK, will do.
> >>
> >>> Do we have a test case?
> >>
> >> Yes, the one posted by Dmitry here:
> >>
> >>    http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00518.html
> >
> > OK, committed as trunk revision 118149.
> 
> Looks fine to me, no lines except the cursor.

Great, thanks for testing.

Re: Bidi reordering engine upgraded

[Jan Djärv]

Den 2014-10-18 15:13, Eli Zaretskii skrev:
>> Date: Sat, 18 Oct 2014 15:54:15 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: dmantipov@yandex.ru, emacs-devel@gnu.org
>>
>>> I suggest you check it in so we can test it.
>>
>> OK, will do.
>>
>>> Do we have a test case?
>>
>> Yes, the one posted by Dmitry here:
>>
>>    http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00518.html
>
> OK, committed as trunk revision 118149.
>
> Note that I also asked about similar problems with hbar cursor, see
> http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00555.html.
>

Missed this.  In this case, the cusor is not seen.  Both on trunk and 24.4. 
No display artifacts on 24.4 as with normal cursor.

	Jan D.

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Sun, 19 Oct 2014 13:49:28 +0200
> From: Jan Djärv <jan.h.d@swipnet.se>
> CC: dmantipov@yandex.ru, emacs-devel@gnu.org
> 
> > Note that I also asked about similar problems with hbar cursor, see
> > http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00555.html.
> >
> 
> Missed this.  In this case, the cusor is not seen.  Both on trunk and 24.4. 
> No display artifacts on 24.4 as with normal cursor.

That's what I'd expect.  Thanks for testing.

Re: Bidi reordering engine upgraded

[grischka]

> But there are W+1 pixels in a line that starts at X and ends at X+W.
> W+1, not W.
> 
>> 
>>  | 1         2         3
>>  | 012345678901234567890123456789
>>  |           ***********
>>  |                     ^ out out damned dot!
> 
> Count the stars, and you will see there are 11 of them, not 10.  Where
> did the 11th one come from, when we requested a width of 10?

In general, drawing an horizontal or vertical line of length 10
needs 11 pixels to be set.

This is because to the human eye pixels are similar to points in math
(i.e.has location, does not have extent).  Therefor, a mental "magnifying
glass" (in order to be useful) needs to preserve the point-like property
of pixels unaffected from the "zoom factor".

Which means that what counts for line length is not number of pixels
but the spaces between.  See line of length 10, magnified:

     0   1   2   3   4   5   6   7   8   9   0
     *   *   *   *   *   *   *   *   *   *   *

(Disclaimer:  The above is not meant to necessarily support
conclusions about the behavior of XDrawRectangle or FrameRect,
or specific details thereof. :P)

--- grischka

Re: Bidi reordering engine upgraded

[martin rudalics]

 > This is because to the human eye pixels are similar to points in math
 > (i.e.has location, does not have extent).  Therefor, a mental "magnifying
 > glass" (in order to be useful) needs to preserve the point-like property
 > of pixels unaffected from the "zoom factor".
 >
 > Which means that what counts for line length is not number of pixels
 > but the spaces between.  See line of length 10, magnified:
 >
 >      0   1   2   3   4   5   6   7   8   9   0
 >      *   *   *   *   *   *   *   *   *   *   *

I doubt my eye (and whatever there's left behind it) would consider

   0
   *

having zero extent.

martin

Re: Bidi reordering engine upgraded

[David Kastrup]

martin rudalics <rudalics@gmx.at> writes:

>> This is because to the human eye pixels are similar to points in math
>> (i.e.has location, does not have extent).  Therefor, a mental "magnifying
>> glass" (in order to be useful) needs to preserve the point-like property
>> of pixels unaffected from the "zoom factor".
>>
>> Which means that what counts for line length is not number of pixels
>> but the spaces between.  See line of length 10, magnified:
>>
>>      0   1   2   3   4   5   6   7   8   9   0
>>      *   *   *   *   *   *   *   *   *   *   *
>
> I doubt my eye (and whatever there's left behind it) would consider
>
>   0
>   *
>
> having zero extent.

So which direction is the non-zero extent in?  Left-right or up-down?
I think the argument is that the 1-pixel undirectional extent you see is
a property of the pen rather than of the line.

-- 
David Kastrup

Re: Bidi reordering engine upgraded

[grischka]

martin rudalics wrote:
>  > This is because to the human eye pixels are similar to points in math
>  > (i.e.has location, does not have extent).  Therefor, a mental 
> "magnifying
>  > glass" (in order to be useful) needs to preserve the point-like property
>  > of pixels unaffected from the "zoom factor".
>  >
>  > Which means that what counts for line length is not number of pixels
>  > but the spaces between.  See line of length 10, magnified:
>  >
>  >      0   1   2   3   4   5   6   7   8   9   0
>  >      *   *   *   *   *   *   *   *   *   *   *
> 
> I doubt my eye (and whatever there's left behind it) would consider
> 
>   0
>   *
> 
> having zero extent.

Nobody would consider a single pixel to mean anything in particular.
For that to happen it needs to be placed in relation to other pixels.

For example two pixels can make a line already.

--- grischka

> martin
> 

Re: Bidi reordering engine upgraded

[Eli Zaretskii]

> Date: Fri, 17 Oct 2014 15:21:39 +0200
> From: grischka <grishka@gmx.de>
> Cc: emacs-devel@gnu.org
> 
> martin rudalics wrote:
> >  > This is because to the human eye pixels are similar to points in math
> >  > (i.e.has location, does not have extent).  Therefor, a mental 
> > "magnifying
> >  > glass" (in order to be useful) needs to preserve the point-like property
> >  > of pixels unaffected from the "zoom factor".
> >  >
> >  > Which means that what counts for line length is not number of pixels
> >  > but the spaces between.  See line of length 10, magnified:
> >  >
> >  >      0   1   2   3   4   5   6   7   8   9   0
> >  >      *   *   *   *   *   *   *   *   *   *   *
> > 
> > I doubt my eye (and whatever there's left behind it) would consider
> > 
> >   0
> >   *
> > 
> > having zero extent.
> 
> Nobody would consider a single pixel to mean anything in particular.
> For that to happen it needs to be placed in relation to other pixels.
> 
> For example two pixels can make a line already.

We are talking about a single-pixel vertical line (whose height is
more than 1 pixel).  Such a line definitely means something.