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

[Po Lu]

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.

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

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

Basically, the pixel_width of these glyphs at the beginning or the end
of the box face have to be updated.  Otherwise, if the cursor moves over
the box it is highlighted with the mouse-face, it might appear at the
wrong position, as explained above.

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

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

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?

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

Hopefully my explanation above should have made this clear.

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

>> +      /* 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.

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