bug#50660: 28.0.50; Text artifacting when the cursor moves over text under mouse face that originally displayed a box

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: larsi@gnus.org,  50660@debbugs.gnu.org
> Date: Mon, 20 Sep 2021 09:00:57 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I admit that I'm confused.  I don't think I understand what did you
> > find was the problem, and how it came into existence.  Can you explain
> > it in detail, step by step, with references to the current code on
> > master?
> 
> In set_cursor_from_row, the cursor's X position is calculated by summing
> up the pixel_width of each glyph, from the start of the row (if not
> reversed), or from the end (if reversed).
> 
> Inside the produce_XXX_glyph series of functions, the widths of each box
> line is added to the iterator's pixel_width (take this snippet from
> produce_image_glyph)
> 
>   if (face->box != FACE_NO_BOX)
>     {
>       if (face->box_horizontal_line_width > 0)
>       {
>         if (slice.y == 0)
>           it->ascent += face->box_horizontal_line_width;
>         if (slice.y + slice.height == img->height)
>           it->descent += face->box_horizontal_line_width;
>       }
> 
>       if (face->box_vertical_line_width > 0)
>       {
>         if (it->start_of_box_run_p && slice.x == 0)
>           it->pixel_width += face->box_vertical_line_width;
>         if (it->end_of_box_run_p && slice.x + slice.width == img->width)
>           it->pixel_width += face->box_vertical_line_width;
>       }
>     }
> 
> But when part of the row gets highlighted, it becomes drawn in the mouse
> face, which might have greatly different box widths (or none at all)
> compared to the face in which it was originally drawn.  This causes
> issues when moving the point (and thus the cursor) over the highlighted
> area, because the glyph_width of the glyphs reflect the state before the
> area was highlighted, and not after, and the cursor will be offset by
> the original width of the box lines, which may be incorrect under mouse
> face.
> 
> For example, the face `custom-button' has a vertical line 2 pixels wide,
> but `highlight' has no vertical line.  If an area under the face
> `custom-button' is becomes highlighted and under the face `highlight',
> and the cursor is moved into the area, the cursor will be 2 pixels too
> far towards the end of the row (assuming it is not reversed), and
> potentially more, if there are more glyphs between the cursor and the
> start/end of the highlighted area with left and/or right box lines!
> 
> > You said the problem happens when one moves the mouse pointer over
> > text whose face has a box.  You said explicitly that clicking the
> > mouse or dragging it over the text was not required.  Did I understand
> > you correctly?  If I understood you correctly, then I don't think I
> > see how drawing the cursor could be involved, because moving the mouse
> > pointer without clicking doesn't move point, and thus the cursor
> > doesn't need to be moved.
> 
> Yes, but if you move the point into the highlighted area with other
> editing commands, such as the arrow keys, the problem will manifest.
> 
> >> Unfortunately, previously, this information would not be updated when
> >> the the glyphs become displayed under mouse face
> 
> > What information was not updated, exactly?
> 
> The pixel_width of each glyph in the row that was highlighted and has
> left_box_line_p, or right_box_line_p.

Thanks, this becomes clearer now.

However, it is IMO wrong to "fix" the glyphs' pixel_width to account
for the box face.  The glyphs didn't change, only their X-coordinate
did.  By changing the width, we are in effect lying to any code that
accesses the glyphs.  We should find another solution, perhaps similar
to what the iterator does during the layout phase.

> > When the text becomes highlighted, the face in effect is mouse-face,
> > which usually doesn't have the box attribute, and so the text moves
> > horizontally, which is expected.
> 
> Yes, that isn't the problem I'm talking about.

I still don't think I understand completely the problem you are
talking about.  Is the problematic recipe as below?

  . move the mouse pointer above text with box-face, so it is highlighted
  . move the text cursor into the highlighted text

Are there any other problems, or is the above the only problematic
situation you saw and intended to fix?

> > In any case, I'm surprised that fixing such a minor issue needed so
> > much code, including changes to data structures.  Are you sure you
> > used all the information we store in glyph_row and in each glyph?
> > (Once I understand the problem better, I will be able to answer this
> > question myself, but I'm not there yet.)
> 
> I hope I have, but please tell me if I haven't.

Why do you need the two new flags?  If it's so you could avoid
accounting for the box face too many times, isn't that a case of
premature optimization?  A loop through a glyph-row's glyphs is
straightforward and runs very fast.  The face of each glyph is stored
in glyph->face_id, so you can easily see if its face includes the box
attribute and get the box line thickness from that, and there are
flags that tell you whether the box line is drawn on the left and on
the right of the glyph.  What else is missing?

> >> @@ -17131,6 +17131,8 @@ set_cursor_from_row (struct window *w, struct 
> >> glyph_row *row,
> >>      x = -1;
> >>    }
> >>      }
> >> +  if (row->have_glyph_with_box_p)
> >> +    x = -1;
> >
> > Here, I don't understand why the cursor position is affected, and I
> > don't understand why you subtract the fixed value of 1 pixel.  The box
> > thickness doesn't have to be 1, and it could be positive or negative.
> 
> If I understand correctly, setting x to -1 should force it to be
> re-computed later, when the code enters compute_x.  Is that not correct?

Why did you need to recompute X in that case? why not fix the original
computation instead?

> >> +      struct face *mouse_face =
> >> +  FACE_FROM_ID_OR_NULL (f, MOUSE_HL_INFO (f)->mouse_face_face_id);
> >> +
> >> +      if (mouse_face == NULL)
> >> +  mouse_face = FACE_FROM_ID (f, MOUSE_FACE_ID);
> 
> > When is this last NULL check actually needed?
> 
> I'm not exactly sure, but a lot of code seems to do that.

I see only a couple of places, and they are all on the level of
xterm.c/w32term.c, which is in an entirely different layer of the
display code.  On the level of xdisp.c we only use mouse_face_face_id,
AFAICT.

> >> +    /* If there's a row with a box somewhere, by all likelyhood
> >> +       the dimensions of the row has been changed.  If that is
> >> +       the case, and we also find a row where the phys cursor
> >> +       is, recalculate the dimensions of the phys cursor. */
> 
> > I also don't understand this part.  When is it needed and why?  And
> > why not handle it in display_and_set_cursor (if it isn't handled
> > already)?
> 
> I think display_and_set_cursor doesn't do any calculating.  Here, what's
> happening is that the X-offset of the phys cursor is being re-computed,
> to compensate for any changes that might have happened to the
> pixel_width of the glyphs.

Doesn't it logically belong to the job of display_and_set_cursor?

Thanks.