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

[Po Lu via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Feel free to dig in the display code involved in this, I don't intend
> doing that any time soon.

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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix --]
[-- Type: text/x-patch, Size: 11275 bytes --]

From 04ad1a5eb79a8a427ba01c1a107692cf66fcec95 Mon Sep 17 00:00:00 2001
Date: Sun, 19 Sep 2021 21:41:36 +0800
Subject: [PATCH] Fix cursor showing up in incorrect position highlighting box

When computing the pixel width of glyphs, the produce_XXX_glyph series
of functions append the width of box lines to the glyph's pixel width;
This information is used for several tasks, such as calculating the
X-offset of the cursor.

Unfortunately, previously, this information would not be updated when
the the glyphs become displayed under mouse face, which caused issues
when the cursor was drawn over a section of text that was highlighted
while previously having a box.

* src/dispextern.h (have_glyph_with_box_p): New variable.
* src/dispextern.h (mouse_face_glyphs_processed_p): Likewise.
* src/xdisp.c (set_cursor_from_box): Force X-offset computation if the
row has at least 1 glyph with some kind of left or right box.
* src/xdisp.c (produce_image_glyph): Mark the row as having a box
if the vertical line width is more than 0.
* src/xdisp.c (produce_xwidget_glyph): Likewise.
* src/xdisp.c (IT_APPLY_FACE_BOX): Mark the iterator's row as having a
box if the vertical line width is more than 0.
* src/xdisp.c (draw_row_with_mouse_face): Modify glyphs in the row to
take into account differing vertical box line widths between the mouse
face and the original face.
* src/xdisp.c (clear_mouse_face): Recompute cursor position after
clearing mouse face.
---
 src/dispextern.h |  10 +++
 src/xdisp.c      | 181 +++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 169 insertions(+), 22 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 6aefe43e19..dfaf271639 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1065,6 +1065,16 @@ #define CHECK_MATRIX(MATRIX) ((void) 0)
      right-to-left paragraph.  */
   bool_bf reversed_p : 1;
 
+  /* True means there is at least one glyph in this row with a left
+     box line.  See the commentary inside `draw_row_with_mouse_face'
+     in xdisp.c for more details. */
+  bool_bf have_glyph_with_box_p : 1;
+
+  /* True means we have already processed the box glyphs on this
+     row for display under mouse face.  This can only be set if
+     have_glyph_with_box_p is true. */
+  bool_bf mouse_face_glyphs_processed_p : 1;
+
   /* Continuation lines width at the start of the row.  */
   int continuation_lines_width;
 
diff --git a/src/xdisp.c b/src/xdisp.c
index 2e72f6b591..a90c52644f 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -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;
 
  compute_x:
   if (cursor != NULL)
@@ -29519,6 +29521,8 @@ produce_image_glyph (struct it *it)
 
       if (face->box_vertical_line_width > 0)
 	{
+	  if (it->glyph_row)
+	    it->glyph_row->have_glyph_with_box_p = 1;
 	  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)
@@ -29629,6 +29633,8 @@ produce_xwidget_glyph (struct it *it)
 
       if (face->box_vertical_line_width > 0)
 	{
+	  if (it->glyph_row)
+	    it->glyph_row->have_glyph_with_box_p = 1;
 	  if (it->start_of_box_run_p)
 	    it->pixel_width += face->box_vertical_line_width;
 	  it->pixel_width += face->box_vertical_line_width;
@@ -30393,27 +30399,29 @@ produce_glyphless_glyph (struct it *it, bool for_no_font, Lisp_Object acronym)
 /* If face has a box, add the box thickness to the character
    height.  If character has a box line to the left and/or
    right, add the box line width to the character's width.  */
-#define IT_APPLY_FACE_BOX(it, face)				\
-  do {								\
-    if (face->box != FACE_NO_BOX)				\
-      {								\
-	int thick = face->box_horizontal_line_width;		\
-	if (thick > 0)						\
-	  {							\
-	    it->ascent += thick;				\
-	    it->descent += thick;				\
-	  }							\
-								\
-	thick = face->box_vertical_line_width;			\
-	if (thick > 0)						\
-	  {							\
-	    if (it->start_of_box_run_p)				\
-	      it->pixel_width += thick;				\
-	    if (it->end_of_box_run_p)				\
-	      it->pixel_width += thick;				\
-	  }							\
-      }								\
-    } while (false)
+#define IT_APPLY_FACE_BOX(it, face)					\
+  do {									\
+    if (face->box != FACE_NO_BOX)					\
+      {									\
+	int thick = face->box_horizontal_line_width;			\
+	if (thick > 0)							\
+	  {								\
+	    it->ascent += thick;					\
+	    it->descent += thick;					\
+	  }								\
+									\
+	thick = face->box_vertical_line_width;				\
+	if (thick > 0)							\
+	  {								\
+	    if (it->glyph_row)						\
+	      it->glyph_row->have_glyph_with_box_p = 1;	\
+	    if (it->start_of_box_run_p)					\
+	      it->pixel_width += thick;					\
+	    if (it->end_of_box_run_p)					\
+	      it->pixel_width += thick;					\
+	  }								\
+      }									\
+  } while (false)
 
 /* RIF:
    Produce glyphs/get display metrics for the display element IT is
@@ -32044,7 +32052,75 @@ draw_row_with_mouse_face (struct window *w, int start_x, struct glyph_row *row,
 			  enum draw_glyphs_face draw)
 {
 #ifdef HAVE_WINDOW_SYSTEM
-  if (FRAME_WINDOW_P (XFRAME (w->frame)))
+  /* 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.  */
+  int remove_p = draw != DRAW_MOUSE_FACE;
+
+  if (row->have_glyph_with_box_p &&
+      FRAME_WINDOW_P (XFRAME (w->frame)) &&
+      remove_p == row->mouse_face_glyphs_processed_p)
+    {
+      struct frame *f = WINDOW_XFRAME (w);
+      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);
+
+      int end_of_modified_glyphs = start_x;
+      struct glyph *g = NULL;
+
+      for (int i = start_hpos; i <= end_hpos; ++i)
+	{
+	  g = &row->glyphs[TEXT_AREA][i];
+	  struct face *mouse = mouse_face;
+	  struct face *regular_face = FACE_FROM_ID (f, g->face_id);
+
+	  if (remove_p)
+	    {
+	      if (g->type == CHAR_GLYPH)
+		mouse = FACE_FROM_ID (f, FACE_FOR_CHAR
+				      (f, mouse_face, g->u.ch, -1, Qnil));
+
+	      struct face *temp = regular_face;
+	      regular_face = mouse;
+	      mouse = temp;
+	    }
+
+	  /* If the glyph has a left box line, subtract from it the
+	     original width of the line. */
+	  if (g->left_box_line_p)
+	    g->pixel_width -= max (0, regular_face->box_vertical_line_width);
+	  /* Likewise with the right box line, as there may be a box
+	     there as well. */
+	  if (g->right_box_line_p)
+	    g->pixel_width -= max (0, regular_face->box_vertical_line_width);
+	  /* Now we add the line widths from the new face. */
+	  if (g->left_box_line_p)
+	    g->pixel_width += max (0, mouse->box_vertical_line_width);
+	  if (g->right_box_line_p)
+	    g->pixel_width += max (0, mouse->box_vertical_line_width);
+
+	  end_of_modified_glyphs += g->pixel_width;
+	}
+      row->mouse_face_glyphs_processed_p = !remove_p;
+
+      /* Redraw the entire row so changes are taken into effect. */
+      draw_glyphs (w, row->x, row, TEXT_AREA,
+		   0, row->used[TEXT_AREA],
+		   DRAW_NORMAL_TEXT, 0);
+
+      /* Now draw the mouse face area. */
+      if (draw != DRAW_NORMAL_TEXT)
+	draw_glyphs (w, start_x, row, TEXT_AREA, start_hpos, end_hpos, draw, 0);
+      return;
+    }
+  else if (FRAME_WINDOW_P (XFRAME (w->frame)))
     {
       draw_glyphs (w, start_x, row, TEXT_AREA, start_hpos, end_hpos, draw, 0);
       return;
@@ -32067,6 +32143,8 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
   struct window *w = XWINDOW (hlinfo->mouse_face_window);
   struct frame *f = XFRAME (WINDOW_FRAME (w));
 
+  int unblock_flipping = 0;
+
   /* Don't bother doing anything if we are on a wrong frame.  */
   if (f != hlinfo->mouse_face_mouse_frame)
     return;
@@ -32148,6 +32226,19 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
 
 	  if (end_hpos > start_hpos)
 	    {
+#ifdef HAVE_WINDOW_SYSTEM
+	      if (FRAME_WINDOW_P (f) &&
+		  w->phys_cursor_on_p && MATRIX_ROW (w->current_matrix,
+						     w->phys_cursor.vpos) == row)
+		{
+		  /* Helps reduce flicker. */
+		  unblock_flipping = true;
+		  block_buffer_flips ();
+		  /* The cursor's position will be changed later, and if we don't clear it now,
+		     artifacting can result. */
+		  gui_clear_cursor (w);
+		}
+#endif
 	      draw_row_with_mouse_face (w, start_x, row,
 					start_hpos, end_hpos, draw);
 
@@ -32173,6 +32264,38 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
 	    hpos = row->used[TEXT_AREA] - 1;
 
 	  block_input ();
+	  /* 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. */
+	  for (row = first; row <= last && row->enabled_p; ++row)
+	    if (row->have_glyph_with_box_p &&
+		MATRIX_ROW (w->current_matrix, w->phys_cursor.vpos) == row)
+	      {
+		int cx = 0, hpos = 0;
+		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;
+
+		for (struct glyph *glyph = start; glyph < last; glyph++)
+		  {
+		    cx += glyph->pixel_width;
+		    ++hpos;
+
+		    if (hpos == w->phys_cursor.hpos)
+		      {
+			w->cursor.x = cx;
+			w->phys_cursor.x = cx;
+			goto set_cursor;
+		      }
+		  }
+		/* Why was the phys cursor glyph not found, even
+		   though the phys cursor is on this row? */
+		emacs_abort ();
+	      }
+	set_cursor:
 	  display_and_set_cursor (w, true, hpos, w->phys_cursor.vpos,
 				  w->phys_cursor.x, w->phys_cursor.y);
 	  unblock_input ();
@@ -32197,6 +32320,9 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
 	FRAME_RIF (f)->define_frame_cursor (f, FRAME_OUTPUT_DATA (f)->nontext_cursor);
     }
 #endif	/* HAVE_WINDOW_SYSTEM */
+
+  if (unblock_flipping)
+    unblock_buffer_flips ();
 }
 
 /* EXPORT:
@@ -32209,12 +32335,23 @@ clear_mouse_face (Mouse_HLInfo *hlinfo)
 {
   bool cleared
     = !hlinfo->mouse_face_hidden && !NILP (hlinfo->mouse_face_window);
+#ifdef HAVE_WINDOW_SYSTEM
+  bool cursor_was_in_mouse_face_p =
+    cleared && cursor_in_mouse_face_p (XWINDOW (hlinfo->mouse_face_window));
+  struct window *w = cleared ? XWINDOW (hlinfo->mouse_face_window) : NULL;
+#endif /* HAVE_WINDOW_SYSTEM */
   if (cleared)
     show_mouse_face (hlinfo, DRAW_NORMAL_TEXT);
   hlinfo->mouse_face_beg_row = hlinfo->mouse_face_beg_col = -1;
   hlinfo->mouse_face_end_row = hlinfo->mouse_face_end_col = -1;
   hlinfo->mouse_face_window = Qnil;
   hlinfo->mouse_face_overlay = Qnil;
+#ifdef HAVE_WINDOW_SYSTEM
+  if (cursor_was_in_mouse_face_p)
+    set_cursor_from_row (w, MATRIX_ROW (w->current_matrix,
+					w->phys_cursor.vpos),
+			 w->current_matrix, 0, 0, 0, 0);
+#endif /* HAVE_WINDOW_SYSTEM */
   return cleared;
 }
 
-- 
2.33.0