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: Wed, 29 Sep 2021 09:35:24 +0800
> 
> Thanks, but I think I've already solved the problem.  Can you try the
> attached patch and see if there are any problems with it?  TIA

Thanks, this looks almost right to me, see the minor stylistic
comments and questions below.  (I didn't yet have time to try the
patch, but if you did try it in all the relevant combinations,
including R2L text, that's good enough for me.)

> +      if (cursor_in_mouse_face_p (w)
> +       && cursor_on_p)

This could be on a single line.

> +#ifdef HAVE_WINDOW_SYSTEM
> +  if (mouse_face_here_p)
> +    {
> +      get_cursor_offset_for_mouse_face (w, cursor_row, &mouse_delta);
> +      w->phys_cursor.x += mouse_delta;
> +    }
> +#endif

Please add a comment here explaining the problem and the general idea
of the solution.

> +#ifdef HAVE_WINDOW_SYSTEM
> +       if (MATRIX_ROW_VPOS (row, w->current_matrix)
> +           == w->phys_cursor.vpos && !w->pseudo_window_p
> +           && draw == DRAW_MOUSE_FACE)

Style: we line up the logical && and || operators, like this:

    if ((MATRIX_ROW_VPOS (row, w->current_matrix)
         == w->phys_cursor.vpos)
        && !w->pseudo_window_p
        && draw == DRAW_MOUSE_FACE)

> +#ifdef HAVE_WINDOW_SYSTEM
> +/* Get the offset to apply before drawing phys_cursor, and return it
> +   in OFFSET, if ROW has something currently under mouse face. */

This comment doesn't tell the main part of the function's job, which
is related to the box face.  Please include that, and please explain
in the comment why the box face requires the offset for the cursor.

> +  if (row->mode_line_p)
> +    return;

This warrants a comment regarding the reason why the function returns
in that case.

> +  for (; start != end; row->reversed_p ?
> +      --start : ++start)

Style: this should not break the last part of the 'for'.  Like this:

     for (; start != end; row->reversed_p ? --start : ++start)

> +      if (row->reversed_p && g->type == IMAGE_GLYPH)
> +     {
> +       struct image *img = IMAGE_FROM_ID (WINDOW_XFRAME (w),
> +                                          g->u.img_id);
> +       do_left_box_p = g->left_box_line_p &&
> +         g->slice.img.x + g->slice.img.width == img->width;
> +       do_right_box_p = g->right_box_line_p &&
> +         g->slice.img.x == 0;
> +     }
> +      else if (g->type == IMAGE_GLYPH)
> +     {
> +       struct image *img = IMAGE_FROM_ID (WINDOW_XFRAME (w),
> +                                          g->u.img_id);
> +       do_left_box_p = g->left_box_line_p &&
> +         g->slice.img.x + g->slice.img.width == img->width;
> +       do_right_box_p = g->right_box_line_p &&
> +         g->slice.img.x == 0;
> +     }

It is better to have a two-level if here:

    if (g->type == IMAGE_GLYPH)
      {
        if (row->reversed_p)
          {
            ...
          }
        else
          {
            ...
          }

But it looks like the code in both conditions is the same?  More
generally, what kind of problem specific to images does this part try
to handle?

> +      if (do_left_box_p)
> +        *offset -= max (0, regular_face->box_vertical_line_width);
> +      /* Likewise with the right box line, as there may be a
> +      box there as well. */
> +      if (do_right_box_p)
> +        *offset -= max (0, regular_face->box_vertical_line_width);

There's no reason to use -= and += here.  The callers never initialize
the argument to anything but zero, nor should they.  This function
_calculates_ the offset, it doesn't _correct_ it.  So a simple
assignment should do better, because using the above begs the
question: what could the initial value be?  The callers should add or
subtract the corrections as they see fit.

Thanks.