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: Sun, 19 Sep 2021 21:55:07 +0800
> 
> Ok, I spent some time doing that.  I have attached a patch which
> resolves this problem on my side, but could you please look at it and
> see if I'm doing anything wrong?  Thanks.
> 
> I have tried to provide a detailed explanation of the changes in the
> patch message and the code itself.

Thanks.

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?

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.

Next, I don't think I follow this part:

> Unfortunately, previously, this information would not be updated when
> the the glyphs become displayed under mouse face

What information was not updated, exactly?  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.  And the only glyphs whose pixel_width is affected in this
situation are the glyphs where the box face begins or ends.  What was
missing in handling this situation?

When you explain these things, please refer to the code which you
describe, so that I could follow your line of thought better.

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.)

Some minor comments and questions below.

> @@ -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.

> +  /* Basically, when have_glyph_with_box_p is true,
> +     we know we are dealing with a row that has at least one
> +     glyph with a box line.
> +
> +     As such, for each glyph within the highlighted region that has
> +     box lines and is the start of a box, we subtract the width of the
> +     lines from its pixel_width.  */

I don't understand why we need to do this adjustment of glyphs' width.

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

> +       /* 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)?

> +             struct glyph *start = row->glyphs[TEXT_AREA];
> +             struct glyph *last = start + row->used[TEXT_AREA] - (intptr_t) 
> 1;
> +
> +             while (last > start && last->charpos < 0)
> +               --last;

Here you assume that only glyphs at end of the row could have negative
charpos, but that's not true.  Glyphs at the start could have that as
well.

Thanks again for working on this.