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

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]

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


Start Emacs with -Q, run `list-faces-display', move to a face with a box
defined (for example, mode-line-highlight), highlight the sample text
with the mouse, and move the cursor around inside the sample text.

If you're lucky, you will be able to observe artifacting, usually around
the characters "O", "Q", "R", "W", "X", "Y", and/or "Z".  If you're not,
you can usually at least observe the text being redrawn in a manner
inconsistent with its surroundings, where characters surrounding the
cursor appear to be spaced overly close to or overly far from their
surroundings.

I've attached a screenshot depicting the results of moving the cursor to
the end of the sample text for mode-line-highlight while under mouse
face, and then quickly moving the cursor to the next line.  Note the
artifacting to the left of the character "Z", and how "Z" appears to be
spaced further apart than its neighbors to the left.


[-- Attachment #2: artifacting.png --]
[-- Type: image/png, Size: 717 bytes --]

[-- Attachment #3: Type: text/plain, Size: 2889 bytes --]


In GNU Emacs 28.0.50 (build 1, i386-pc-solaris2.11, X toolkit, Xaw scroll bars)
 of 2021-09-18 built on blaster
Repository revision: 93731cdae0ff11d9f67e06ed3ea75b1320ce5034
Repository branch: HEAD
Windowing system distributor 'The X.Org Foundation', version 11.0.12101001
Configured using:
 'configure --with-gnutls=ifavailable --disable-acl'

Configured features:
GIF JPEG MODULES PDUMPER PNG THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE
XIM XPM LUCID ZLIB

Important settings:
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Help

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs text-property-search mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils time-date misearch
multi-isearch help-mode info sly-autoloads package browse-url url
url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap url-handlers url-parse auth-source cl-seq
eieio eieio-core cl-macs eieio-loaddefs password-cache json subr-x map
url-vars seq byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib
iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads
dynamic-setting x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 8 70437 15727)
 (symbols 24 7754 1)
 (strings 16 24397 1118)
 (string-bytes 1 758560)
 (vectors 8 16650)
 (vector-slots 4 258394 24378)
 (floats 8 38 22)
 (intervals 28 786 0)
 (buffers 564 13))

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

[Lars Ingebrigtsen]

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

Po Lu <luangruo@yahoo.com> writes:

> If you're lucky, you will be able to observe artifacting, usually around
> the characters "O", "Q", "R", "W", "X", "Y", and/or "Z".  If you're not,
> you can usually at least observe the text being redrawn in a manner
> inconsistent with its surroundings, where characters surrounding the
> cursor appear to be spaced overly close to or overly far from their
> surroundings.

I can't reproduce this -- perhaps it's dependent on which fonts are
used?

I do see one odd thing here.  When marking a region with the mouse, I
get this:


[-- Attachment #2: Type: image/png, Size: 2895 bytes --]

[-- Attachment #3: Type: text/plain, Size: 279 bytes --]


Note the little dots above to the right of the Zs.  I don't think those
should be there, so there might be a off-by-one error (or something)
when computing something here...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Note the little dots above to the right of the Zs.  I don't think those
> should be there, so there might be a off-by-one error (or something)
> when computing something here...

That is probably the same bug, though in a different form.  Thanks.

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]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I do see one odd thing here.  When marking a region with the mouse, I
> get this:

But wait a second... What do you mean by marking a region with the
mouse?  The recipe I provided is to hover your pointer over the sample
text, and then to move your (Emacs) cursor over the sample text.

Could you please try that, and see what happens?  Thanks.

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

[Eli Zaretskii]

> Cc: 50660@debbugs.gnu.org
> Date: Sun, 19 Sep 2021 08:33:37 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > Note the little dots above to the right of the Zs.  I don't think those
> > should be there, so there might be a off-by-one error (or something)
> > when computing something here...
> 
> That is probably the same bug, though in a different form.  Thanks.

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

TIA

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]

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

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

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> But wait a second... What do you mean by marking a region with the
> mouse?  The recipe I provided is to hover your pointer over the sample
> text, and then to move your (Emacs) cursor over the sample text.
>
> Could you please try that, and see what happens?  Thanks.

I tried that, and I didn't see any artefacts when doing so.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> 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 tried the patch, and it didn't fix the glitches I was seeing (when
mouse-selecting the region), so those seem to be unrelated things.  (I
haven't read the patch.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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.

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]

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.

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.

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

[Eli Zaretskii]

> Date: Mon, 20 Sep 2021 08:19:16 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: larsi@gnus.org, 50660@debbugs.gnu.org
> 
> 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

Actually, the only recipe in which I can see problems is this:

  . move the text cursor into text that has a face with the box attribute
  . move the mouse pointer into and out of that text

In this scenario, the characters of the text with box-face move
horizontally to account for the box border, as the mouse highlight is
turned on and off, but the character under cursor does NOT move, and
with large enough font this leaves small artifacts (because the
character is not erased correctly).

Is that the problem you are trying to solve, i.e. the problematic
display is limited to the character under cursor when text is
mouse-highlighted and unhighlighted?

Or maybe one needs specially-defined faces to see the problem?  If so,
please show the definitions of those faces, preferably as part of a
complete recipe starting from "emacs -Q".

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]

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

BTW, here's an updated version of the patch with some issues I noticed
rectified.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-cursor-position.patch --]
[-- Type: text/x-patch, Size: 11642 bytes --]

From e39d92b045b6d90c460874b5b3981c8fce69fab3 Mon Sep 17 00:00:00 2001
From: Your Name <you@example.com>
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 (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      | 199 +++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 187 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..aa352b59f1 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -29519,6 +29519,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 +29631,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 +30397,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 +32050,97 @@ 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;
+	    }
+
+	  bool do_left_box_p = g->left_box_line_p;
+	  bool do_right_box_p = g->right_box_line_p;
+
+	  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->right_box_line_p &&
+		g->slice.img.x + g->slice.img.width == img->width;
+	      do_right_box_p = g->left_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;
+	    }
+
+	  /* If the glyph has a left box line, subtract from it the
+	     original width of the line. */
+	  if (do_left_box_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 (do_right_box_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 +32163,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 +32246,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 +32284,36 @@ 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;
+
+		for (struct glyph *glyph = start; glyph <= last; glyph++)
+		  {
+
+		    if (hpos == w->phys_cursor.hpos)
+		      {
+			w->cursor.x = cx;
+			w->phys_cursor.x = cx;
+			goto set_cursor;
+		      }
+
+		    cx += glyph->pixel_width;
+		    ++hpos;
+		  }
+		/* 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 +32338,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 +32353,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

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

[Eli Zaretskii]

> Date: Mon, 20 Sep 2021 08:19:16 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: larsi@gnus.org, 50660@debbugs.gnu.org
> 
> However, it is IMO wrong to "fix" the glyphs' pixel_width to account
> for the box face.

Walking through the code, I now see that we already update the
pixel_width of the border glyphs due to the box's vertical line.  So
set_cursor_from_row does its job correctly, and I still don't
understand why any change would be needed in it.

I guess you are saying that in the problematic scenario the
pixel_width of the glyphs remains the same, even though the box
dimensions have changed or the box attribute disappeared?  If so, I
think the problem is not where the cursor is drawn or its position is
computed, the problem is where we redraw a portion of text due to
change of mouse highlight, i.e. inside draw_row_with_mouse_face and
friends.

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

[Eli Zaretskii]

> Date: Mon, 20 Sep 2021 10:07:27 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: larsi@gnus.org, 50660@debbugs.gnu.org
> 
> I guess you are saying that in the problematic scenario the
> pixel_width of the glyphs remains the same, even though the box
> dimensions have changed or the box attribute disappeared?  If so, I
> think the problem is not where the cursor is drawn or its position is
> computed, the problem is where we redraw a portion of text due to
> change of mouse highlight, i.e. inside draw_row_with_mouse_face and
> friends.

Actually, it looks like we already do everything we need to account
for the box border, when it exists, while drawing the glyphs (in
xterm.c/w32term.c).  The pixel_width of the glyphs is not used by the
back-end code which actually draws to the glass.  So the only place
which needs fixing is probably draw_phys_cursor_glyph and maybe also
erase_phys_cursor.  Assuming we are indeed talking about problems with
the glyph under the cursor.

Do you agree?

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]

Eli Zaretskii <eliz@gnu.org> writes:

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

Interesting.  I don't quite know what the iterator does during the
layout phase, could you please point me to the relevant part of the
code?  Thanks.

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

That is the only situation I saw and intended to fix.

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

I suppose that is a case of premature optimization, thanks.

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

Indeed, I have removed that change.

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

Hmm, it seems prudent to remove that then.  Thanks.

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

AFAIU, display_and_set_cursor only serves to set the position of the
cursor, and doesn't calculate or correct anything by itself.  Am I
missing something?

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]

Eli Zaretskii <eliz@gnu.org> writes:

> In this scenario, the characters of the text with box-face move
> horizontally to account for the box border, as the mouse highlight is
> turned on and off, but the character under cursor does NOT move, and
> with large enough font this leaves small artifacts (because the
> character is not erased correctly).

Yes, precisely.

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]

Eli Zaretskii <eliz@gnu.org> writes:

> Actually, it looks like we already do everything we need to account
> for the box border, when it exists, while drawing the glyphs (in
> xterm.c/w32term.c).  The pixel_width of the glyphs is not used by the
> back-end code which actually draws to the glass.  So the only place
> which needs fixing is probably draw_phys_cursor_glyph and maybe also
> erase_phys_cursor.  Assuming we are indeed talking about problems with
> the glyph under the cursor.

My understanding is that when the cursor is drawn, the string in
RIF->draw_glyph_string contains _only_ the glyph underneath the cursor,
while the terminals only compensate for the box if the first glyph in
the string has a left box line; when drawing a cursor on the area with a
mouse face, that is only true if the cursor lands on the start of the
box.

In addition to that, draw_phys_cursor_glyph uses the value
w->output_cursor.x as the X offset passed to draw_glyphs, so I still
don't think the problem lies in draw_phys_cursor_glyph, but either in
where the cursor position is calculated (by tallying up the
pixel_widths), or where the mouse face is drawn without updating the
contents of the glyph row to reflect the potentially changed dimensions.
Personally, I still think the problem lies in the latter area and not
the former, but I'll leave that up to your judgement.

BTW, in x_set_cursor_gc, I notice that s->face isn't being set to the
mouse face even when the cursor lies inside the mouse face.  Perhaps
checking for cursor_in_mouse_face_p (s->w), and setting s->face to the
mouse face when that is the case would be prudent?  AFAIK, something
similar is already being done in x_draw_stretch_glyph_string (see this
chunk of code):

	  if (s->row->mouse_face_p
	      && cursor_in_mouse_face_p (s->w))
	    {
	      x_set_mouse_face_gc (s);
	      gc = s->gc;
	    }

Thanks.

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 16:18:01 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Actually, it looks like we already do everything we need to account
> > for the box border, when it exists, while drawing the glyphs (in
> > xterm.c/w32term.c).  The pixel_width of the glyphs is not used by the
> > back-end code which actually draws to the glass.  So the only place
> > which needs fixing is probably draw_phys_cursor_glyph and maybe also
> > erase_phys_cursor.  Assuming we are indeed talking about problems with
> > the glyph under the cursor.
> 
> My understanding is that when the cursor is drawn, the string in
> RIF->draw_glyph_string contains _only_ the glyph underneath the cursor,

That is correct.

> while the terminals only compensate for the box if the first glyph in
> the string has a left box line; when drawing a cursor on the area with a
> mouse face, that is only true if the cursor lands on the start of the
> box.

Also correct.  But the cursor glyph that is not the first or last of
the box area doesn't need any compensation, it just needs to be drawn
at the correct X-coordinate.

So I think all that's needed is to adjust the value of
w->phys_cursor.x, when needed, in draw_phys_cursor_glyph, before
passing it to draw_glyphs, or perhaps in its callers.  The value of
w->phys_cursor.x should stay unaltered, but what we pass to
draw_glyphs should be offset if needed.

If you think this will not be enough, what else is needed, and why?

> In addition to that, draw_phys_cursor_glyph uses the value
> w->output_cursor.x as the X offset passed to draw_glyphs, so I still
> don't think the problem lies in draw_phys_cursor_glyph, but either in
> where the cursor position is calculated (by tallying up the
> pixel_widths), or where the mouse face is drawn without updating the
> contents of the glyph row to reflect the potentially changed dimensions.

The glyph row doesn't store any dimensions, it only stores the
pixel_width of each glyph and the starting X-coordinate of the row.
So where the mouse face is drawn, we don't need any updating.

> Personally, I still think the problem lies in the latter area and not
> the former, but I'll leave that up to your judgement.

The cursor position of a window, stored in w->phys_cursor, is
calculated when the cursor is moved.  It is never recalculated
thereafter, until point is moved again.  In particular, displaying
mouse-sensitive text in mouse-face just reuses the glyphs already
produced, it doesn't recalculate them.  But since we always redraw the
entire sequence of glyphs in the mouse face or in the box face, the
glyphs gets moved horizontally because we see the first glyph with the
box and handle that accordingly.  But when we then redraw the cursor,
we reuse the information in w->phys_cursor, which is slightly off when
the box attributes of the mouse face is different from that of the
"regular" face.  This is what causes the cursor glyph be drawn in the
wrong location.

The correct place to fix this is therefore somewhere under
note_mouse_highlight, which is where we handle redrawing of the
mouse-sensitive face, including the cursor.

> BTW, in x_set_cursor_gc, I notice that s->face isn't being set to the
> mouse face even when the cursor lies inside the mouse face.  Perhaps
> checking for cursor_in_mouse_face_p (s->w), and setting s->face to the
> mouse face when that is the case would be prudent?  AFAIK, something
> similar is already being done in x_draw_stretch_glyph_string (see this
> chunk of code):
> 
> 	  if (s->row->mouse_face_p
> 	      && cursor_in_mouse_face_p (s->w))
> 	    {
> 	      x_set_mouse_face_gc (s);
> 	      gc = s->gc;
> 	    }

x_draw_stretch_glyph_string does that only if needs to clear an area
that is wider than the width of the frame's default font, which cannot
happen in our case.

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]

Eli Zaretskii <eliz@gnu.org> writes:

> So I think all that's needed is to adjust the value of
> w->phys_cursor.x, when needed, in draw_phys_cursor_glyph, before
> passing it to draw_glyphs, or perhaps in its callers.  The value of
> w->phys_cursor.x should stay unaltered, but what we pass to
> draw_glyphs should be offset if needed.

Thanks, I'll work on that.

> The glyph row doesn't store any dimensions, it only stores the
> pixel_width of each glyph and the starting X-coordinate of the row.
> So where the mouse face is drawn, we don't need any updating.

> The cursor position of a window, stored in w->phys_cursor, is
> calculated when the cursor is moved.  It is never recalculated
> thereafter, until point is moved again.  In particular, displaying
> mouse-sensitive text in mouse-face just reuses the glyphs already
> produced, it doesn't recalculate them.  But since we always redraw the
> entire sequence of glyphs in the mouse face or in the box face, the
> glyphs gets moved horizontally because we see the first glyph with the
> box and handle that accordingly.  But when we then redraw the cursor,
> we reuse the information in w->phys_cursor, which is slightly off when
> the box attributes of the mouse face is different from that of the
> "regular" face.  This is what causes the cursor glyph be drawn in the
> wrong location.

> The correct place to fix this is therefore somewhere under
> note_mouse_highlight, which is where we handle redrawing of the
> mouse-sensitive face, including the cursor.

Thanks, I've learned a lot.  I hope I haven't been inconveniencing you
in any way.

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 18:27:02 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So I think all that's needed is to adjust the value of
> > w->phys_cursor.x, when needed, in draw_phys_cursor_glyph, before
> > passing it to draw_glyphs, or perhaps in its callers.  The value of
> > w->phys_cursor.x should stay unaltered, but what we pass to
> > draw_glyphs should be offset if needed.
> 
> Thanks, I'll work on that.

Specifically, I now think the adjustment should happen in this
fragment from show_mouse_face, before we call display_and_set_cursor:

	/* When we've written over the cursor, arrange for it to
	   be displayed again.  */
	if (FRAME_WINDOW_P (f)
	    && phys_cursor_on_p && !w->phys_cursor_on_p)
	  {
  #ifdef HAVE_WINDOW_SYSTEM
	    int hpos = w->phys_cursor.hpos;

	    /* When the window is hscrolled, cursor hpos can legitimately be
	       out of bounds, but we draw the cursor at the corresponding
	       window margin in that case.  */
	    if (!row->reversed_p && hpos < 0)
	      hpos = 0;
	    if (row->reversed_p && hpos >= row->used[TEXT_AREA])
	      hpos = row->used[TEXT_AREA] - 1;

	    block_input ();
	    display_and_set_cursor (w, true, hpos, w->phys_cursor.vpos,
				    w->phys_cursor.x, w->phys_cursor.y);
	    unblock_input ();
  #endif	/* HAVE_WINDOW_SYSTEM */
	  }

Here we know that if the DRAW argument is DRAW_MOUSE_FACE, we are
displaying the mouse-face, and if it's DRAW_NORMAL_TEXT, we are
removing the mouse face.  We can also know which glyphs are being
redisplayed with/without the mouse highlight, see the loop above that
calculates start_hpos and end_hpos (you can save aside the results
when it does that for the cursor row).  The glyphs in the glyph row
have their original face_id (Not the mouse-face ID!), so you can look
at their left_box_line_p and right_box_line_p attributes, the
face->box attribute, etc.  And the corresponding attributes of the
mouse-face can be accessed via mouse_face_face_id etc.

So you should be able to walk the glyphs from the beginning of the
redrawn area to the cursor glyph and calculate the offset for
w->phys_cursor.x you need.  I think you will need separate loops for
reversed_p rows, where you should loop from the end of the row, not
from the beginning.

A separate function to compute the offset will probably be best.

> > The correct place to fix this is therefore somewhere under
> > note_mouse_highlight, which is where we handle redrawing of the
> > mouse-sensitive face, including the cursor.
> 
> Thanks, I've learned a lot.  I hope I haven't been inconveniencing you
> in any way.

No, not at all.  Thank you for working on this.

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]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Specifically, I now think the adjustment should happen in this
> fragment from show_mouse_face, before we call display_and_set_cursor:

Thanks, but I already cooked something up.  The adjustment, in my case,
is done in draw_phys_cursor_glyph, conditional on
cursor_in_mouse_face_p.

Is there anything wrong with this approach?  Thanks.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-cursor-position.patch --]
[-- Type: text/x-patch, Size: 3056 bytes --]

diff --git a/src/xdisp.c b/src/xdisp.c
index 2e72f6b591..ca6b98155a 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -31696,7 +31696,69 @@ draw_phys_cursor_glyph (struct window *w, struct glyph_row *row,
       bool on_p = w->phys_cursor_on_p;
       int x1;
       int hpos = w->phys_cursor.hpos;
+      int mouse_off = 0;
+#ifdef HAVE_WINDOW_SYSTEM
+      if (cursor_in_mouse_face_p (w))
+	{
+	  struct frame *f = WINDOW_XFRAME (w);
+	  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+	  struct glyph *start, *end;
+	  struct face *mouse_face = FACE_FROM_ID (f, hlinfo->mouse_face_face_id);
+	  end = &row->glyphs[TEXT_AREA][hpos];
+
+	  if (MATRIX_ROW_VPOS (row, w->current_matrix) ==
+	      hlinfo->mouse_face_beg_row)
+	    start = &row->glyphs[TEXT_AREA][hlinfo->mouse_face_beg_col];
+	  else
+	    start = row->glyphs[TEXT_AREA];
+
+	  /* Calculate an offset to correct phys_cursor x if we are
+	     drawing the cursor in the mouse face. */
+
+	  for (; start <= end; ++start)
+	    {
+	      struct glyph *g = start;
+	      struct face *mouse = mouse_face;
+	      struct face *regular_face = FACE_FROM_ID (f, g->face_id);
+
+	      bool do_left_box_p = g->left_box_line_p;
+	      bool do_right_box_p = g->right_box_line_p;
 
+	      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->right_box_line_p &&
+		    g->slice.img.x + g->slice.img.width == img->width;
+		  do_right_box_p = g->left_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;
+		}
+
+	      /* If the glyph has a left box line, subtract it the
+		 offset. */
+	      if (do_left_box_p)
+		mouse_off -= 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)
+		mouse_off -= max (0, regular_face->box_vertical_line_width);
+	      /* Now we add the line widths from the new face. */
+	      if (g->left_box_line_p)
+		mouse_off += max (0, mouse->box_vertical_line_width);
+	      if (g->right_box_line_p)
+	        mouse_off += max (0, mouse->box_vertical_line_width);
+	    }
+	}
+#endif
       /* When the window is hscrolled, cursor hpos can legitimately be
 	 out of bounds, but we draw the cursor at the corresponding
 	 window margin in that case.  */
@@ -31705,7 +31767,8 @@ draw_phys_cursor_glyph (struct window *w, struct glyph_row *row,
       if (row->reversed_p && hpos >= row->used[TEXT_AREA])
 	hpos = row->used[TEXT_AREA] - 1;
 
-      x1 = draw_glyphs (w, w->phys_cursor.x, row, TEXT_AREA, hpos, hpos + 1,
+      x1 = draw_glyphs (w, w->phys_cursor.x + mouse_off,
+			row, TEXT_AREA, hpos, hpos + 1,
 			hl, 0);
       w->phys_cursor_on_p = on_p;
 

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]

Eli Zaretskii <eliz@gnu.org> writes:

> I think you will need separate loops for reversed_p rows, where you
> should loop from the end of the row, not from the beginning.

Oops... It seems to be the case that I missed this in the patch I just
sent you.  I'll correct it ASAP.

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 19:08:13 +0800
> 
> > Specifically, I now think the adjustment should happen in this
> > fragment from show_mouse_face, before we call display_and_set_cursor:
> 
> Thanks, but I already cooked something up.  The adjustment, in my case,
> is done in draw_phys_cursor_glyph, conditional on
> cursor_in_mouse_face_p.
> 
> Is there anything wrong with this approach?  Thanks.

My worry is that draw_phys_cursor_glyph is called from many other
places as well, none of which needs this adjustment.  So we are going
to do unnecessary work, especially if the cursor happens to be inside
mouse-highlight.

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 19:08:13 +0800
> 
> Thanks, but I already cooked something up.  The adjustment, in my case,
> is done in draw_phys_cursor_glyph, conditional on
> cursor_in_mouse_face_p.
> 
> Is there anything wrong with this approach?  Thanks.

One other problem with this implementation is that the additional code
will run when the mouse-highlight is removed as well, in which case we
know that no adjustment is needed for w->phys_cursor.x.

Also, I think it's incorrect to add mouse->box_vertical_line_width to
_every_ glyph that in the regular face has this attribute.  That's
because the entire mouse-highlighted area has just two glyphs which
could have a box line near it: the first and the last one.  So you
should only add it once.  Try this code on a screen line with several
different  stretches of characters with the box face, and arrange for
all of these stretches to be mouse-highlighted together.

Moreover, if the mouse-highlighted text takes more than one screen
line (a.k.a. "glyph row"), only the first screen line/glyph row need
to have the mouse->box_vertical_line_width added.

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]

Eli Zaretskii <eliz@gnu.org> writes:

> One other problem with this implementation is that the additional code
> will run when the mouse-highlight is removed as well, in which case we
> know that no adjustment is needed for w->phys_cursor.x.

Interesting.  What if the cursor is moved while it is inside mouse face
though, if the adjustment is done in show_mouse_face? Won't it only
apply to the cursor when the mouse face is first shown, and not
necessarily when changes happen to the position of the cursor later?

Thanks.

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: Tue, 21 Sep 2021 08:38:31 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > One other problem with this implementation is that the additional code
> > will run when the mouse-highlight is removed as well, in which case we
> > know that no adjustment is needed for w->phys_cursor.x.
> 
> Interesting.  What if the cursor is moved while it is inside mouse face
> though, if the adjustment is done in show_mouse_face? Won't it only
> apply to the cursor when the mouse face is first shown, and not
> necessarily when changes happen to the position of the cursor later?

When the cursor moves, w->phys_cursor.x is recomputed.  AFAIR, in this
case we perform the usual redisplay (which updates w->phys_cursor),
and then reapply mouse-face, so show_mouse_face should be called
again.  But you can easily establish if I'm right by running Emacs in
a debugger with a breakpoint in show_mouse_face.  Let me know if you
find something unexpected or need help in interpreting the findings.

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]

Eli Zaretskii <eliz@gnu.org> writes:

> When the cursor moves, w->phys_cursor.x is recomputed.  AFAIR, in this
> case we perform the usual redisplay (which updates w->phys_cursor),
> and then reapply mouse-face, so show_mouse_face should be called
> again.  But you can easily establish if I'm right by running Emacs in
> a debugger with a breakpoint in show_mouse_face.  Let me know if you
> find something unexpected or need help in interpreting the findings.

Interestingly enough, I don't see show_mouse_face being called when the
cursor moves within the highlighted area; I only see it called when the
highlight itself changes, such as when the mouse pointer moves into the
mouse face.

Let me know if you need any more information from me to debugging this;
I will probably be unable to debug this myself for the next day or two.

Thanks!

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: Tue, 21 Sep 2021 15:34:48 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > When the cursor moves, w->phys_cursor.x is recomputed.  AFAIR, in this
> > case we perform the usual redisplay (which updates w->phys_cursor),
> > and then reapply mouse-face, so show_mouse_face should be called
> > again.  But you can easily establish if I'm right by running Emacs in
> > a debugger with a breakpoint in show_mouse_face.  Let me know if you
> > find something unexpected or need help in interpreting the findings.
> 
> Interestingly enough, I don't see show_mouse_face being called when the
> cursor moves within the highlighted area; I only see it called when the
> highlight itself changes, such as when the mouse pointer moves into the
> mouse face.

If show_mouse_face is not called, then there's nothing to be adjusted.
Do you see any display problems with the glyph under the cursor if you
just move the cursor inside the mouse-highlighted region?  If so, can
you show the recipe for reproducing the problem?

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]

Eli Zaretskii <eliz@gnu.org> writes:

> Do you see any display problems with the glyph under the cursor if you
> just move the cursor inside the mouse-highlighted region?  If so, can
> you show the recipe for reproducing the problem?

Yes, starting from emacs -Q, create a buffer in fundamental-mode, and
evaluate:

  (insert (propertize "some sample text" 'face '(:box 10) 'mouse-face 'highlight))

Highlight the text that was inserted, and move the cursor around inside
the highlighted area.  The text will begin to jump all over the place.

Thanks.

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: Tue, 21 Sep 2021 17:20:31 +0800
> 
> Yes, starting from emacs -Q, create a buffer in fundamental-mode, and
> evaluate:
> 
>   (insert (propertize "some sample text" 'face '(:box 10) 'mouse-face 'highlight))
> 
> Highlight the text that was inserted, and move the cursor around inside
> the highlighted area.  The text will begin to jump all over the place.

Only the characters at the beginning and end of the box jump, so they
are the only ones that need an additional adjustment.  Right?

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]

Eli Zaretskii <eliz@gnu.org> writes:

> Only the characters at the beginning and end of the box jump, so they
> are the only ones that need an additional adjustment.  Right?

Yeah, they are the only characters causing the jump, but the effect of
that jump "ripples", in a way, throughout the entire box.  So even if I
move the cursor into the middle of the box, it will still be in the
wrong location, but when the cursor is moved, show_mouse_face is not
called, so I think putting the correction is show_mouse_face is not the
solution to this problem.

Thanks.

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: Tue, 21 Sep 2021 17:45:37 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Only the characters at the beginning and end of the box jump, so they
> > are the only ones that need an additional adjustment.  Right?
> 
> Yeah, they are the only characters causing the jump, but the effect of
> that jump "ripples", in a way, throughout the entire box.

It ripples only once, when the mouse-highlight is applied, and at that
time show_mouse_face will be called.  After the mouse-highlight is
applied, if you just move the cursor (NOT the mouse pointer), the only
part of the text that changes are the two edge characters.

> So even if I move the cursor into the middle of the box, it will
> still be in the wrong location, but when the cursor is moved,
> show_mouse_face is not called, so I think putting the correction is
> show_mouse_face is not the solution to this problem.

We could identify this situation and call show_mouse_face forcibly,
e.g. in gui_update_window_end or some such place.

It's also fine with me to do it where you wanted, as long as you can
resolve the problems with that which I mentioned earlier.

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

[Eli Zaretskii]

> Date: Tue, 21 Sep 2021 13:17:58 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: larsi@gnus.org, 50660@debbugs.gnu.org
> 
> > From: Po Lu <luangruo@yahoo.com>
> > Cc: larsi@gnus.org,  50660@debbugs.gnu.org
> > Date: Tue, 21 Sep 2021 17:45:37 +0800
> > 
> > So even if I move the cursor into the middle of the box, it will
> > still be in the wrong location, but when the cursor is moved,
> > show_mouse_face is not called, so I think putting the correction is
> > show_mouse_face is not the solution to this problem.
> 
> We could identify this situation and call show_mouse_face forcibly,
> e.g. in gui_update_window_end or some such place.

Here's a specific idea: add the same code which fixes the cursor
position into gui_update_window_end, before display_and_set_cursor is
called, here:

  if (!w->pseudo_window_p)
    {
      block_input ();

      if (cursor_on_p)
	display_and_set_cursor (w, true,
				w->output_cursor.hpos, w->output_cursor.vpos,
				w->output_cursor.x, w->output_cursor.y);

      if (draw_window_fringes (w, true))
	{
	  if (WINDOW_RIGHT_DIVIDER_WIDTH (w))
	    gui_draw_right_divider (w);
	  else
	    gui_draw_vertical_border (w);
	}
      unblock_input ();
    }

This is always called when the display is updated, even if we just
move the cursor.  By looking at the value of cursor_in_mouse_face_p,
we can determine whether the fix is needed, and apply it before
calling display_and_set_cursor.  (We will need to fix w->phys_cursor.x
after the call to display_and_set_cursor returns, because it records
there the value passed as the 5th argument.)

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

[Eli Zaretskii]

> Date: Tue, 21 Sep 2021 13:41:28 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: luangruo@yahoo.com, larsi@gnus.org, 50660@debbugs.gnu.org
> 
> Here's a specific idea: add the same code which fixes the cursor
> position into gui_update_window_end, before display_and_set_cursor is
> called, here:
> 
>   if (!w->pseudo_window_p)
>     {
>       block_input ();
> 
>       if (cursor_on_p)
> 	display_and_set_cursor (w, true,
> 				w->output_cursor.hpos, w->output_cursor.vpos,
> 				w->output_cursor.x, w->output_cursor.y);
> 
>       if (draw_window_fringes (w, true))
> 	{
> 	  if (WINDOW_RIGHT_DIVIDER_WIDTH (w))
> 	    gui_draw_right_divider (w);
> 	  else
> 	    gui_draw_vertical_border (w);
> 	}
>       unblock_input ();
>     }
> 
> This is always called when the display is updated, even if we just
> move the cursor.  By looking at the value of cursor_in_mouse_face_p,
> we can determine whether the fix is needed, and apply it before
> calling display_and_set_cursor.  (We will need to fix w->phys_cursor.x
> after the call to display_and_set_cursor returns, because it records
> there the value passed as the 5th argument.)

Actually, it looks like it would be enough to set
mouse_face_overwritten_p = true in gui_update_window_end, when the
cursor is inside mouse face highlighted text.  Then, as the comments
near the end of gui_update_window_end tell, this function already
arranges for the mouse-highlight to be redisplayed, and that will call
show_mouse_face.

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]

Eli Zaretskii <eliz@gnu.org> writes:

> Specifically, I now think the adjustment should happen in this
> fragment from show_mouse_face, before we call display_and_set_cursor:
>
> 	/* When we've written over the cursor, arrange for it to
> 	   be displayed again.  */
> 	if (FRAME_WINDOW_P (f)
> 	    && phys_cursor_on_p && !w->phys_cursor_on_p)
> 	  {
>   #ifdef HAVE_WINDOW_SYSTEM
> 	    int hpos = w->phys_cursor.hpos;
>
> 	    /* When the window is hscrolled, cursor hpos can legitimately be
> 	       out of bounds, but we draw the cursor at the corresponding
> 	       window margin in that case.  */
> 	    if (!row->reversed_p && hpos < 0)
> 	      hpos = 0;
> 	    if (row->reversed_p && hpos >= row->used[TEXT_AREA])
> 	      hpos = row->used[TEXT_AREA] - 1;
>
> 	    block_input ();
> 	    display_and_set_cursor (w, true, hpos, w->phys_cursor.vpos,
> 				    w->phys_cursor.x, w->phys_cursor.y);
> 	    unblock_input ();
>   #endif	/* HAVE_WINDOW_SYSTEM */
> 	  }

Thanks, one question though: within this block, w->phys_cursor.vpos is
0! How should I go about determining whether or not the cursor is inside
`row' ATM?

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: Tue, 21 Sep 2021 20:46:55 +0800
> 
> Thanks, one question though: within this block, w->phys_cursor.vpos is
> 0! How should I go about determining whether or not the cursor is inside
> `row' ATM?

Are you saying the vpos is _always_ zero?  I don't see how this could
be true, because display_and_set_cursor uses that to find the glyph
row that corresponds to the cursor position, and actually draw the
cursor glyph.  If vpos is incorrect, the cursor will appear in the
long place and will look incorrectly.

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]

Eli Zaretskii <eliz@gnu.org> writes:

> Are you saying the vpos is _always_ zero?  I don't see how this could
> be true, because display_and_set_cursor uses that to find the glyph
> row that corresponds to the cursor position, and actually draw the
> cursor glyph.  If vpos is incorrect, the cursor will appear in the
> long place and will look incorrectly.

Yes, if I start GDB, and step to this line:

-->	  block_input ();
	  display_and_set_cursor (w, true, hpos, w->phys_cursor.vpos,

and do

(gdb) p w->phys_cursor.vpos

I get 0.


Though it could have been a problem with the debugger, as I have had
problems with GDB on Solaris in the past.  I will try again tomorrow and
let you know what happens, thanks.

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: Tue, 21 Sep 2021 21:36:41 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Are you saying the vpos is _always_ zero?  I don't see how this could
> > be true, because display_and_set_cursor uses that to find the glyph
> > row that corresponds to the cursor position, and actually draw the
> > cursor glyph.  If vpos is incorrect, the cursor will appear in the
> > long place and will look incorrectly.
> 
> Yes, if I start GDB, and step to this line:
> 
> -->	  block_input ();
> 	  display_and_set_cursor (w, true, hpos, w->phys_cursor.vpos,
> 
> and do
> 
> (gdb) p w->phys_cursor.vpos
> 
> I get 0.

It's definitely not what I see here.  I see the value that is the
screen line number of the cursor.  If the cursor is on the first
screen line, the value is indeed zero (VPOS is zero-based), but not in
general.

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]

Eli Zaretskii <eliz@gnu.org> writes:

> It's definitely not what I see here.  I see the value that is the
> screen line number of the cursor.  If the cursor is on the first
> screen line, the value is indeed zero (VPOS is zero-based), but not in
> general.

I found the issue with that.  Hopefully, in a bit, I will have something
ready to work, but in the meantime I noticed another problem:

If the mouse face's box is set to something preposterous, like (:box
10000), the entire row is occluded by the box, and doesn't become
visible again when canceling mouse face.  Only redraw-display makes the
problem go away.

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: Fri, 24 Sep 2021 07:53:20 +0800
> 
> If the mouse face's box is set to something preposterous, like (:box
> 10000), the entire row is occluded by the box, and doesn't become
> visible again when canceling mouse face.  Only redraw-display makes the
> problem go away.

Emacs gives enough rope for Lisp programmers to hang themselves.  When
they try doing that, our only job is not to crash.  IOW, producing
preposterous display results for preposterous settings is justified;
we don't need to go out of our way to cater to ridiculous parameter
values like the one above, we only need to avoid crashing.

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]

Po Lu <luangruo@yahoo.com> writes:

> I found the issue with that.  Hopefully, in a bit, I will have something
> ready to work, but in the meantime I noticed another problem:

Thanks, another question: if I restore the original value of
phys_cursor->x after the call to display_and_set_cursor,
erase_phys_cursor gets very confused the next time it is called.

If I don't restore the original value, everything appears to work fine.

Should I replicate the cursor offset logic in erase_phys_cursor, or
should I keep the new value of phys_cursor->x?  Thanks.

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, 26 Sep 2021 14:46:09 +0800
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> > I found the issue with that.  Hopefully, in a bit, I will have something
> > ready to work, but in the meantime I noticed another problem:
> 
> Thanks, another question: if I restore the original value of
> phys_cursor->x after the call to display_and_set_cursor,
> erase_phys_cursor gets very confused the next time it is called.
> 
> If I don't restore the original value, everything appears to work fine.
> 
> Should I replicate the cursor offset logic in erase_phys_cursor, or
> should I keep the new value of phys_cursor->x?  Thanks.

I don't think I understand the situation well enough to answer these
questions.

First, which call to display_and_set_cursor are we talking about?
And what do you mean by "erase_phys_cursor gets very confused" --
confused how?

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]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think I understand the situation well enough to answer these
> questions.

Thanks, I'll try to explain.

> First, which call to display_and_set_cursor are we talking about?

The call inside show_mouse_face.

> And what do you mean by "erase_phys_cursor gets very confused" --
> confused how?

It calls draw_phys_cursor_glyph, which draws the glyph at the original X
position, and not the position with the mouse face offset applied.

What I was asking is whether or not it's okay to not restore
phys_cursor->x to its original value after the call to
display_and_set_cursor inside show_mouse_face, or if I should also
calculate and add the offset in erase_phys_cursor (if
cursor_in_mouse_face_p) instead.

TIA.

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, 26 Sep 2021 17:56:03 +0800
> 
> > First, which call to display_and_set_cursor are we talking about?
> 
> The call inside show_mouse_face.
> 
> > And what do you mean by "erase_phys_cursor gets very confused" --
> > confused how?
> 
> It calls draw_phys_cursor_glyph, which draws the glyph at the original X
> position, and not the position with the mouse face offset applied.
> 
> What I was asking is whether or not it's okay to not restore
> phys_cursor->x to its original value after the call to
> display_and_set_cursor inside show_mouse_face, or if I should also
> calculate and add the offset in erase_phys_cursor (if
> cursor_in_mouse_face_p) instead.

I think I'm still a bit confused.  Can you show the patch you have
where you see these problems with erase_phys_cursor?

P.S. And sorry for the delay in answering.

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]

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

Eli Zaretskii <eliz@gnu.org> writes:

> I think I'm still a bit confused.  Can you show the patch you have
> where you see these problems with erase_phys_cursor?

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-cursor-position.patch --]
[-- Type: text/x-patch, Size: 6669 bytes --]

diff --git a/src/dispnew.c b/src/dispnew.c
index 0c31319917..f58e1d28e4 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3848,6 +3848,10 @@ gui_update_window_end (struct window *w, bool cursor_on_p,
 				w->output_cursor.hpos, w->output_cursor.vpos,
 				w->output_cursor.x, w->output_cursor.y);
 
+      if (cursor_in_mouse_face_p (w)
+	  && cursor_on_p)
+	mouse_face_overwritten_p = 1;
+
       if (draw_window_fringes (w, true))
 	{
 	  if (WINDOW_RIGHT_DIVIDER_WIDTH (w))
diff --git a/src/xdisp.c b/src/xdisp.c
index 2e72f6b591..f542b3f526 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -1179,7 +1179,9 @@ #define face_after_it_pos(IT)  face_before_or_after_it_pos (IT, false)
 static Lisp_Object get_it_property (struct it *, Lisp_Object);
 static Lisp_Object calc_line_height_property (struct it *, Lisp_Object,
 					      struct font *, int, bool);
-
+static void get_cursor_offset_for_mouse_face (struct window *w,
+					      struct glyph_row *row,
+					      int *offset)
 #endif /* HAVE_WINDOW_SYSTEM */
 
 static void produce_special_glyphs (struct it *, enum display_element_type);
@@ -31741,6 +31743,10 @@ erase_phys_cursor (struct window *w)
   Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
   int hpos = w->phys_cursor.hpos;
   int vpos = w->phys_cursor.vpos;
+#ifdef HAVE_WINDOW_SYSTEM
+  int mouse_delta = 0;
+  int phys_x = w->phys_cursor.x;
+#endif
   bool mouse_face_here_p = false;
   struct glyph_matrix *active_glyphs = w->current_matrix;
   struct glyph_row *cursor_row;
@@ -31810,6 +31816,14 @@ erase_phys_cursor (struct window *w)
       && cursor_row->used[TEXT_AREA] > hpos && hpos >= 0)
     mouse_face_here_p = true;
 
+#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
+
   /* Maybe clear the display under the cursor.  */
   if (w->phys_cursor_type == HOLLOW_BOX_CURSOR)
     {
@@ -31845,6 +31859,9 @@ erase_phys_cursor (struct window *w)
   draw_phys_cursor_glyph (w, cursor_row, hl);
 
  mark_cursor_off:
+#ifdef HAVE_WINDOW_SYSTEM
+  w->phys_cursor.x = phys_x;
+#endif
   w->phys_cursor_on_p = false;
   w->phys_cursor_type = NO_CURSOR;
 }
@@ -32081,6 +32098,9 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
       && hlinfo->mouse_face_end_row < w->current_matrix->nrows)
     {
       bool phys_cursor_on_p = w->phys_cursor_on_p;
+#ifdef HAVE_WINDOW_SYSTEM
+      int mouse_off = 0;
+#endif
       struct glyph_row *row, *first, *last;
 
       first = MATRIX_ROW (w->current_matrix, hlinfo->mouse_face_beg_row);
@@ -32154,6 +32174,12 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
 	      row->mouse_face_p
 		= draw == DRAW_MOUSE_FACE || draw == DRAW_IMAGE_RAISED;
 	    }
+#ifdef HAVE_WINDOW_SYSTEM
+	  if (MATRIX_ROW_VPOS (row, w->current_matrix)
+	      == w->phys_cursor.vpos && !w->pseudo_window_p
+	      && draw == DRAW_MOUSE_FACE)
+	    get_cursor_offset_for_mouse_face (w, row, &mouse_off);
+#endif
 	}
 
       /* When we've written over the cursor, arrange for it to
@@ -32163,6 +32189,7 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
 	{
 #ifdef HAVE_WINDOW_SYSTEM
 	  int hpos = w->phys_cursor.hpos;
+	  int old_phys_cursor_x = w->phys_cursor.x;
 
 	  /* When the window is hscrolled, cursor hpos can legitimately be
 	     out of bounds, but we draw the cursor at the corresponding
@@ -32174,7 +32201,9 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
 
 	  block_input ();
 	  display_and_set_cursor (w, true, hpos, w->phys_cursor.vpos,
-				  w->phys_cursor.x, w->phys_cursor.y);
+				  w->phys_cursor.x + mouse_off,
+				  w->phys_cursor.y);
+	  w->phys_cursor.x = old_phys_cursor_x;
 	  unblock_input ();
 #endif	/* HAVE_WINDOW_SYSTEM */
 	}
@@ -35926,4 +35955,89 @@ cancel_hourglass (void)
     }
 }
 
+#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. */
+static void
+get_cursor_offset_for_mouse_face (struct window *w, struct glyph_row *row,
+				  int *offset)
+{
+  if (row->mode_line_p)
+    return;
+  block_input ();
+
+  struct frame *f = WINDOW_XFRAME (w);
+  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+  struct glyph *start, *end;
+  struct face *mouse_face = FACE_FROM_ID (f, hlinfo->mouse_face_face_id);
+  int hpos = w->phys_cursor.hpos;
+  end = &row->glyphs[TEXT_AREA][hpos];
+
+  if (!row->reversed_p)
+    {
+      if (MATRIX_ROW_VPOS (row, w->current_matrix) ==
+ 	  hlinfo->mouse_face_beg_row)
+	start = &row->glyphs[TEXT_AREA][hlinfo->mouse_face_beg_col];
+      else
+	start = row->glyphs[TEXT_AREA];
+    }
+  else
+    {
+      if (MATRIX_ROW_VPOS (row, w->current_matrix) ==
+	  hlinfo->mouse_face_end_row)
+	start = &row->glyphs[TEXT_AREA][hlinfo->mouse_face_end_col];
+      else
+	start = &row->glyphs[TEXT_AREA][row->used[TEXT_AREA] - 1];
+    }
+
+  /* Calculate an offset to correct phys_cursor x if we are
+     drawing the cursor in the mouse face. */
+
+  for (; start != end; row->reversed_p ?
+	 --start : ++start)
+    {
+      struct glyph *g = start;
+      struct face *mouse = mouse_face;
+      struct face *regular_face = FACE_FROM_ID (f, g->face_id);
+
+      bool do_left_box_p = g->left_box_line_p;
+      bool do_right_box_p = g->right_box_line_p;
+
+      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;
+	}
+
+      /* If the glyph has a left box line, subtract it the
+	 offset. */
+      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);
+      /* Now we add the line widths from the new face. */
+      if (g->left_box_line_p)
+	*offset += max (0, mouse->box_vertical_line_width);
+      if (g->right_box_line_p)
+        *offset += max (0, mouse->box_vertical_line_width);
+    }
+
+  unblock_input ();
+}
+#endif
 #endif /* HAVE_WINDOW_SYSTEM */

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.

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]

Eli Zaretskii <eliz@gnu.org> writes:

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

I tried to make it work with RTL text, but right now I don't have access
to the machine where I developed that patch, so I wasn't able to test
it.  (For me to move code between machines at work onto my personal
machine is difficult due to the policies of my organization.  But they
have no issue with making the changes public.  Oh well, can't convince
the suits.)

I will correct the issues ASAP.

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

Thanks, I think that's a problem.  The conditions should be different
depending on whether or not the row is reversed, because
produce_image_glyph uses start_of_box_run_p and end_of_box_run_p, which
AFAIU is unaffected by the row being reversed.

This part exists to take into account produce_image_glyph testing for
several conditions being met before appending the box width to the
iterator.

(See this part of produce_image_glyph, somewhere around line 29542:)

	  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;

I suppose it should be made a comment.  That will be done shortly,
thanks.


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

Yes, that one is specifically one of my vices.  Thanks for pointing it
out.

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]

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

Eli Zaretskii <eliz@gnu.org> writes:

Thanks for the comments, I'm attaching a rectified patch.

I think the rest of what you've brought up has been resolved by the new
patch, but I would like to clarify something here:

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

I changed the function to use an internal accumulator variable
initialized to 0.  The -= and += is still necessary, as we are inside a
loop which can potentially go through many glyphs that are relevant.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-cursor-position.patch --]
[-- Type: text/x-patch, Size: 7825 bytes --]

diff --git a/src/dispnew.c b/src/dispnew.c
index 0c31319917..62f7074dcd 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3848,6 +3848,9 @@ gui_update_window_end (struct window *w, bool cursor_on_p,
 				w->output_cursor.hpos, w->output_cursor.vpos,
 				w->output_cursor.x, w->output_cursor.y);
 
+      if (cursor_in_mouse_face_p (w) && cursor_on_p)
+	mouse_face_overwritten_p = 1;
+
       if (draw_window_fringes (w, true))
 	{
 	  if (WINDOW_RIGHT_DIVIDER_WIDTH (w))
diff --git a/src/xdisp.c b/src/xdisp.c
index 2e72f6b591..f2b4e43592 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -1179,7 +1179,9 @@ #define face_after_it_pos(IT)  face_before_or_after_it_pos (IT, false)
 static Lisp_Object get_it_property (struct it *, Lisp_Object);
 static Lisp_Object calc_line_height_property (struct it *, Lisp_Object,
 					      struct font *, int, bool);
-
+static void get_cursor_offset_for_mouse_face (struct window *w,
+					      struct glyph_row *row,
+					      int *offset);
 #endif /* HAVE_WINDOW_SYSTEM */
 
 static void produce_special_glyphs (struct it *, enum display_element_type);
@@ -29509,6 +29511,8 @@ produce_image_glyph (struct it *it)
 
   if (face->box != FACE_NO_BOX)
     {
+      /* If you change the logic here, please change it in
+	 get_cursor_offset_for_mouse_face as well. */
       if (face->box_horizontal_line_width > 0)
 	{
 	  if (slice.y == 0)
@@ -31741,6 +31745,10 @@ erase_phys_cursor (struct window *w)
   Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
   int hpos = w->phys_cursor.hpos;
   int vpos = w->phys_cursor.vpos;
+#ifdef HAVE_WINDOW_SYSTEM
+  int mouse_delta;
+  int phys_x = w->phys_cursor.x;
+#endif
   bool mouse_face_here_p = false;
   struct glyph_matrix *active_glyphs = w->current_matrix;
   struct glyph_row *cursor_row;
@@ -31810,6 +31818,16 @@ erase_phys_cursor (struct window *w)
       && cursor_row->used[TEXT_AREA] > hpos && hpos >= 0)
     mouse_face_here_p = true;
 
+#ifdef HAVE_WINDOW_SYSTEM
+  /* The problem solved by the code below is outlined
+     in the comment above get_cursor_offset_for_mouse_face. */
+  if (mouse_face_here_p)
+    {
+      get_cursor_offset_for_mouse_face (w, cursor_row, &mouse_delta);
+      w->phys_cursor.x += mouse_delta;
+    }
+#endif
+
   /* Maybe clear the display under the cursor.  */
   if (w->phys_cursor_type == HOLLOW_BOX_CURSOR)
     {
@@ -31845,6 +31863,9 @@ erase_phys_cursor (struct window *w)
   draw_phys_cursor_glyph (w, cursor_row, hl);
 
  mark_cursor_off:
+#ifdef HAVE_WINDOW_SYSTEM
+  w->phys_cursor.x = phys_x;
+#endif
   w->phys_cursor_on_p = false;
   w->phys_cursor_type = NO_CURSOR;
 }
@@ -32081,6 +32102,9 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
       && hlinfo->mouse_face_end_row < w->current_matrix->nrows)
     {
       bool phys_cursor_on_p = w->phys_cursor_on_p;
+#ifdef HAVE_WINDOW_SYSTEM
+      int mouse_off = 0;
+#endif
       struct glyph_row *row, *first, *last;
 
       first = MATRIX_ROW (w->current_matrix, hlinfo->mouse_face_beg_row);
@@ -32154,6 +32178,16 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
 	      row->mouse_face_p
 		= draw == DRAW_MOUSE_FACE || draw == DRAW_IMAGE_RAISED;
 	    }
+#ifdef HAVE_WINDOW_SYSTEM
+	  if ((MATRIX_ROW_VPOS (row, w->current_matrix)
+	       == w->phys_cursor.vpos)
+	      /* Otherwise this crashes when highlighting a pseudo
+		 window, such as the toolbar which can't have a cursor
+		 anyway. */
+	      && !w->pseudo_window_p
+	      && draw == DRAW_MOUSE_FACE)
+	    get_cursor_offset_for_mouse_face (w, row, &mouse_off);
+#endif
 	}
 
       /* When we've written over the cursor, arrange for it to
@@ -32163,6 +32197,7 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
 	{
 #ifdef HAVE_WINDOW_SYSTEM
 	  int hpos = w->phys_cursor.hpos;
+	  int old_phys_cursor_x = w->phys_cursor.x;
 
 	  /* When the window is hscrolled, cursor hpos can legitimately be
 	     out of bounds, but we draw the cursor at the corresponding
@@ -32174,7 +32209,9 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum draw_glyphs_face draw)
 
 	  block_input ();
 	  display_and_set_cursor (w, true, hpos, w->phys_cursor.vpos,
-				  w->phys_cursor.x, w->phys_cursor.y);
+				  w->phys_cursor.x + mouse_off,
+				  w->phys_cursor.y);
+	  w->phys_cursor.x = old_phys_cursor_x;
 	  unblock_input ();
 #endif	/* HAVE_WINDOW_SYSTEM */
 	}
@@ -35926,4 +35963,110 @@ cancel_hourglass (void)
     }
 }
 
+#ifdef HAVE_WINDOW_SYSTEM
+/* Get the offset to apply before drawing phys_cursor, and return it
+   in OFFSET.  ROW should be a row that is under mouse face and contains
+   the phys cursor.
+
+   This is required because the produce_XXX_glyph series of functions
+   add the width of the various vertical box lines to the total width
+   of the glyph, but isn't updated when the row is put under mouse
+   face, which can have different box dimensions. */
+static void
+get_cursor_offset_for_mouse_face (struct window *w, struct glyph_row *row,
+				  int *offset)
+{
+  int sum = 0;
+  /* Return because the mode line can't possibly have a cursor. */
+  if (row->mode_line_p)
+    return;
+
+  block_input ();
+
+  struct frame *f = WINDOW_XFRAME (w);
+  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+  struct glyph *start, *end;
+  struct face *mouse_face = FACE_FROM_ID (f, hlinfo->mouse_face_face_id);
+  int hpos = w->phys_cursor.hpos;
+  end = &row->glyphs[TEXT_AREA][hpos];
+
+  if (!row->reversed_p)
+    {
+      if (MATRIX_ROW_VPOS (row, w->current_matrix) ==
+ 	  hlinfo->mouse_face_beg_row)
+	start = &row->glyphs[TEXT_AREA][hlinfo->mouse_face_beg_col];
+      else
+	start = row->glyphs[TEXT_AREA];
+    }
+  else
+    {
+      if (MATRIX_ROW_VPOS (row, w->current_matrix) ==
+	  hlinfo->mouse_face_end_row)
+	start = &row->glyphs[TEXT_AREA][hlinfo->mouse_face_end_col];
+      else
+	start = &row->glyphs[TEXT_AREA][row->used[TEXT_AREA] - 1];
+    }
+
+  /* Calculate an offset to correct phys_cursor x if we are
+     drawing the cursor in the mouse face. */
+
+  for (; row->reversed_p ? start >= end : start <= end;
+       row->reversed_p ? --start : ++start)
+    {
+      struct glyph *g = start;
+      struct face *mouse = mouse_face;
+      struct face *regular_face = FACE_FROM_ID (f, g->face_id);
+
+      bool do_left_box_p = g->left_box_line_p;
+      bool do_right_box_p = g->right_box_line_p;
+
+      /* This is required because we test some parameters
+	 of the image slice before applying the box in
+	 produce_image_glyph. */
+
+      if (g->type == IMAGE_GLYPH)
+	{
+	  if (!row->reversed_p)
+	    {
+	      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 == 0;
+	      do_right_box_p = g->right_box_line_p &&
+		g->slice.img.x + g->slice.img.width == img->width;
+	    }
+	  else
+	    {
+	      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;
+	    }
+	}
+
+      /* If the glyph has a left box line, subtract it the
+	 offset. */
+      if (do_left_box_p)
+        sum -= 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)
+        sum -= max (0, regular_face->box_vertical_line_width);
+      /* Now we add the line widths from the new face. */
+      if (g->left_box_line_p)
+        sum += max (0, mouse->box_vertical_line_width);
+      if (g->right_box_line_p)
+        sum += max (0, mouse->box_vertical_line_width);
+    }
+
+  if (row->reversed_p)
+    sum = -sum;
+
+  *offset = sum;
+
+  unblock_input ();
+}
+#endif
 #endif /* HAVE_WINDOW_SYSTEM */

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: Sat, 02 Oct 2021 20:52:58 +0800
> 
> Thanks for the comments, I'm attaching a rectified patch.

Thanks, I installed this on the master branch with a few minor
stylistic fixes.  In the future, please try posting a patch formatted
with "git format-patch", or at least accompany the patch with a
ChangeLog-style commit log entry.  (I wrote the log message for you
this time.)

It looks like something is still amiss: the cursor blinking display is
incorrect in some cases.  For example, evaluate this in a buffer under
Fundamental mode:

  (insert (propertize "some sample text" 'face '(:box 10) 'mouse-face 'highlight))

and then put the mouse pointer above the text, so it's highlighted,
and move the text cursor to the first 's' or the last 't'.  As long as
the cursor blinks, you will see two characters drawn in the cursor
face, not one as expected.

Also, in your original recipe with list-faces-display, if the text
cursor is at the first character of the "abcdefg..." text of a line
with mode-line-highlight face, moving the mouse pointer to and from
the text, thus intermittently highlighting and de-highlighting it,
leaves artifacts of the 'a' character on display.

So I'm not closing this bug yet, as some work still needs to be
invested to clean up those minor remaining issues.

Thanks.

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]

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

Eli Zaretskii <eliz@gnu.org> writes:

> It looks like something is still amiss: the cursor blinking display is
> incorrect in some cases.  For example, evaluate this in a buffer under
> Fundamental mode:
>
>   (insert (propertize "some sample text" 'face '(:box 10) 'mouse-face 'highlight))
>
> and then put the mouse pointer above the text, so it's highlighted,
> and move the text cursor to the first 's' or the last 't'.  As long as
> the cursor blinks, you will see two characters drawn in the cursor
> face, not one as expected.

Thanks, here's a patch that should fix the issues, formatted with "git
format-patch", but with one caveat: in your example above, the
background of the character "s" at the beginning of the string "some
sample text" is drawn too wide, but I wasn't able to find the problem.

Could you please take a look at it?  Thanks.

I don't know how to apply the fixes to xterm.c to the other window
systems, so someone who can needs to apply them to the NS and MS-Windows
ports.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-minor-issues-with-text-display-when-cursor-is-in.patch --]
[-- Type: text/x-patch, Size: 7626 bytes --]

From 6d277be789bf30e0db5b27780bff86151e48f622 Mon Sep 17 00:00:00 2001
From: oldosfan <luangruo@yahoo.com>
Date: Thu, 14 Oct 2021 18:38:26 +0800
Subject: [PATCH] Fix minor issues with text display when cursor is in mouse
 face

 * src/xdisp.c (get_cursor_offset_for_mouse_face): Don't calculate
offsets for the glyph the cursor is on.

 * src/xterm.c (x_draw_glyph_string_foreground,
x_draw_composite_glyph_string_foreground,
x_draw_glyphless_glyph_string_foreground,
x_draw_image_foreground,
x_draw_image_foreground_1): Take mouse face into account when
offsetting X coordinate by the vertical line width.
---
 src/xdisp.c |   2 +-
 src/xterm.c | 107 ++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 93 insertions(+), 16 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 012c2ad8bf..0d964b1236 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -36042,7 +36042,7 @@ get_cursor_offset_for_mouse_face (struct window *w, struct glyph_row *row,
   /* Calculate the offset to correct phys_cursor x if we are
      drawing the cursor inside mouse-face highlighted text.  */
 
-  for (; row->reversed_p ? start >= end : start <= end;
+  for (; row->reversed_p ? start > end : start < end;
        row->reversed_p ? --start : ++start)
     {
       struct glyph *g = start;
diff --git a/src/xterm.c b/src/xterm.c
index 89885e0d88..d19f214019 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -1799,11 +1799,24 @@ x_draw_glyph_string_foreground (struct glyph_string *s)
 {
   int i, x;
 
+  struct face *face_for_box_line = s->face;
+
+  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
+    {
+      /* In this situation, the cursor is in the mouse face, but
+	 s->face hasn't been updated with the mouse face yet. */
+      face_for_box_line =
+	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
+
+      if (!face_for_box_line)
+	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
+
   /* If first glyph of S has a left box line, start drawing the text
      of S to the right of that box line.  */
-  if (s->face->box != FACE_NO_BOX
+  if (face_for_box_line->box != FACE_NO_BOX
       && s->first_glyph->left_box_line_p)
-    x = s->x + max (s->face->box_vertical_line_width, 0);
+    x = s->x + max (face_for_box_line->box_vertical_line_width, 0);
   else
     x = s->x;
 
@@ -1893,11 +1906,24 @@ x_draw_composite_glyph_string_foreground (struct glyph_string *s)
   int i, j, x;
   struct font *font = s->font;
 
+  struct face *face_for_box_line = s->face;
+
+  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
+    {
+      /* In this situation, the cursor is in the mouse face, but
+	 s->face hasn't been updated with the mouse face yet. */
+      face_for_box_line =
+	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
+
+      if (!face_for_box_line)
+	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
+
   /* If first glyph of S has a left box line, start drawing the text
      of S to the right of that box line.  */
-  if (s->face && s->face->box != FACE_NO_BOX
+  if (face_for_box_line->box != FACE_NO_BOX
       && s->first_glyph->left_box_line_p)
-    x = s->x + max (s->face->box_vertical_line_width, 0);
+    x = s->x + max (face_for_box_line->box_vertical_line_width, 0);
   else
     x = s->x;
 
@@ -2004,11 +2030,24 @@ x_draw_glyphless_glyph_string_foreground (struct glyph_string *s)
   unsigned char2b[8];
   int x, i, j;
 
+  struct face *face_for_box_line = s->face;
+
+  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
+    {
+      /* In this situation, the cursor is in the mouse face, but
+	 s->face hasn't been updated with the mouse face yet. */
+      face_for_box_line =
+	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
+
+      if (!face_for_box_line)
+	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
+
   /* If first glyph of S has a left box line, start drawing the text
      of S to the right of that box line.  */
-  if (s->face && s->face->box != FACE_NO_BOX
+  if (face_for_box_line->box != FACE_NO_BOX
       && s->first_glyph->left_box_line_p)
-    x = s->x + max (s->face->box_vertical_line_width, 0);
+    x = s->x + max (face_for_box_line->box_vertical_line_width, 0);
   else
     x = s->x;
 
@@ -3073,12 +3112,25 @@ x_draw_image_foreground (struct glyph_string *s)
   int x = s->x;
   int y = s->ybase - image_ascent (s->img, s->face, &s->slice);
 
+  struct face *face_for_box_line = s->face;
+
+  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
+    {
+      /* In this situation, the cursor is in the mouse face, but
+	 s->face hasn't been updated with the mouse face yet. */
+      face_for_box_line =
+	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
+
+      if (!face_for_box_line)
+	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
+
   /* If first glyph of S has a left box line, start drawing it to the
-     right of that line.  */
-  if (s->face->box != FACE_NO_BOX
+     right of that box line.  */
+  if (face_for_box_line->box != FACE_NO_BOX
       && s->first_glyph->left_box_line_p
       && s->slice.x == 0)
-    x += max (s->face->box_vertical_line_width, 0);
+    x += max (face_for_box_line->box_vertical_line_width, 0);
 
   /* If there is a margin around the image, adjust x- and y-position
      by that margin.  */
@@ -3191,13 +3243,25 @@ x_draw_image_relief (struct glyph_string *s)
   XRectangle r;
   int x = s->x;
   int y = s->ybase - image_ascent (s->img, s->face, &s->slice);
+  struct face *face_for_box_line = s->face;
+
+  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
+    {
+      /* In this situation, the cursor is in the mouse face, but
+	 s->face hasn't been updated with the mouse face yet. */
+      face_for_box_line =
+	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
+
+      if (!face_for_box_line)
+	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
 
   /* If first glyph of S has a left box line, start drawing it to the
-     right of that line.  */
-  if (s->face->box != FACE_NO_BOX
+     right of that box line.  */
+  if (face_for_box_line->box != FACE_NO_BOX
       && s->first_glyph->left_box_line_p
       && s->slice.x == 0)
-    x += max (s->face->box_vertical_line_width, 0);
+    x += max (face_for_box_line->box_vertical_line_width, 0);
 
   /* If there is a margin around the image, adjust x- and y-position
      by that margin.  */
@@ -3282,12 +3346,25 @@ x_draw_image_foreground_1 (struct glyph_string *s, Pixmap pixmap)
   int x = 0;
   int y = s->ybase - s->y - image_ascent (s->img, s->face, &s->slice);
 
+  struct face *face_for_box_line = s->face;
+
+  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
+    {
+      /* In this situation, the cursor is in the mouse face, but
+	 s->face hasn't been updated with the mouse face yet. */
+      face_for_box_line =
+	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
+
+      if (!face_for_box_line)
+	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
+
   /* If first glyph of S has a left box line, start drawing it to the
-     right of that line.  */
-  if (s->face->box != FACE_NO_BOX
+     right of that box line.  */
+  if (face_for_box_line->box != FACE_NO_BOX
       && s->first_glyph->left_box_line_p
       && s->slice.x == 0)
-    x += max (s->face->box_vertical_line_width, 0);
+    x += max (face_for_box_line->box_vertical_line_width, 0);
 
   /* If there is a margin around the image, adjust x- and y-position
      by that margin.  */
-- 
2.31.1

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

[Robert Pluim]

>>>>> On Thu, 14 Oct 2021 18:52:48 +0800, Po Lu via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    Po Lu> +  struct face *face_for_box_line = s->face;
    Po Lu> +
    Po Lu> +  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
    Po Lu> +    {
    Po Lu> +      /* In this situation, the cursor is in the mouse face, but
    Po Lu> +	 s->face hasn't been updated with the mouse face yet. */
    Po Lu> +      face_for_box_line =
    Po Lu> +	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
    Po Lu> +
    Po Lu> +      if (!face_for_box_line)
    Po Lu> +	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
    Po Lu> +    }

This chunk looks like itʼs repeated a lot, perhaps break it out into a
function?

Robert
-- 

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]

Robert Pluim <rpluim@gmail.com> writes:

>     Po Lu> +  struct face *face_for_box_line = s->face;
>     Po Lu> +
>     Po Lu> +  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
>     Po Lu> +    {
>     Po Lu> +      /* In this situation, the cursor is in the mouse face, but
>     Po Lu> +	 s->face hasn't been updated with the mouse face yet. */
>     Po Lu> +      face_for_box_line =
>     Po Lu> +	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
>     Po Lu> +
>     Po Lu> +      if (!face_for_box_line)
>     Po Lu> +	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
>     Po Lu> +    }
>
> This chunk looks like itʼs repeated a lot, perhaps break it out into a
> function?

Actually, on second thought, I think a better solution would be to set
s->face to the mouse face in x_set_cursor_gc if cursor_in_mouse_face_p.

Any thoughts?  TIA.

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: Thu, 14 Oct 2021 18:52:48 +0800
> 
> I don't know how to apply the fixes to xterm.c to the other window
> systems, so someone who can needs to apply them to the NS and MS-Windows
> ports.

You just make the same changes there, and ask people with access to
those other systems to test it.  But see below.

> @@ -1799,11 +1799,24 @@ x_draw_glyph_string_foreground (struct glyph_string *s)
>  {
>    int i, x;
>  
> +  struct face *face_for_box_line = s->face;
> +
> +  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
> +    {
> +      /* In this situation, the cursor is in the mouse face, but
> +	 s->face hasn't been updated with the mouse face yet. */
> +      face_for_box_line =
> +	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
> +
> +      if (!face_for_box_line)
> +	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
> +    }

Can't we "fix" this face in xdisp.c, before calling the
terminal-specific backend?  The bonus will be that we then do it only
in one place.

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]

Eli Zaretskii <eliz@gnu.org> writes:

>> @@ -1799,11 +1799,24 @@ x_draw_glyph_string_foreground (struct glyph_string *s)
>>  {
>>    int i, x;
>>  
>> +  struct face *face_for_box_line = s->face;
>> +
>> +  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
>> +    {
>> +      /* In this situation, the cursor is in the mouse face, but
>> +	 s->face hasn't been updated with the mouse face yet. */
>> +      face_for_box_line =
>> +	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
>> +
>> +      if (!face_for_box_line)
>> +	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
>> +    }

> Can't we "fix" this face in xdisp.c, before calling the
> terminal-specific backend?  The bonus will be that we then do it only
> in one place.

The only way to do that I can think of would be to offset the glyph
string's x position (but not the phys cursor) by the vertical box line
width, and I think it would be an ugly thing to do, because that would
imply lying to the window system backend.

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: Thu, 14 Oct 2021 19:54:38 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> @@ -1799,11 +1799,24 @@ x_draw_glyph_string_foreground (struct glyph_string *s)
> >>  {
> >>    int i, x;
> >>  
> >> +  struct face *face_for_box_line = s->face;
> >> +
> >> +  if (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w))
> >> +    {
> >> +      /* In this situation, the cursor is in the mouse face, but
> >> +	 s->face hasn't been updated with the mouse face yet. */
> >> +      face_for_box_line =
> >> +	FACE_FROM_ID_OR_NULL (s->f, MOUSE_HL_INFO (s->f)->mouse_face_face_id);
> >> +
> >> +      if (!face_for_box_line)
> >> +	face_for_box_line = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
> >> +    }
> 
> > Can't we "fix" this face in xdisp.c, before calling the
> > terminal-specific backend?  The bonus will be that we then do it only
> > in one place.
> 
> The only way to do that I can think of would be to offset the glyph
> string's x position (but not the phys cursor) by the vertical box line
> width, and I think it would be an ugly thing to do, because that would
> imply lying to the window system backend.

We may be miscommunicating.  My point is that the conditions on which
you base the face selection are known in draw_glyphs, so why delay
that to when xterm.c is called to actually draw the glyph string?  Why
not test this same condition in draw_glyphs (or some other suitable
place in xdisp.c) and fix the glyph string's face accordingly?  Am I
missing something?

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]

Eli Zaretskii <eliz@gnu.org> writes:

> We may be miscommunicating.  My point is that the conditions on which
> you base the face selection are known in draw_glyphs, so why delay
> that to when xterm.c is called to actually draw the glyph string?  Why
> not test this same condition in draw_glyphs (or some other suitable
> place in xdisp.c) and fix the glyph string's face accordingly?  Am I
> missing something?

OK, I think I understand what you mean now.  But is it really correct to
put that in draw_glyphs, and not say, fill_XXX_glyph_string?  And even
then, what about cases where a non-ASCII face is used?  Does the mouse
face in the Mouse_HLInfo take that into account?

Thanks.

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: Thu, 14 Oct 2021 20:16:23 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > We may be miscommunicating.  My point is that the conditions on which
> > you base the face selection are known in draw_glyphs, so why delay
> > that to when xterm.c is called to actually draw the glyph string?  Why
> > not test this same condition in draw_glyphs (or some other suitable
> > place in xdisp.c) and fix the glyph string's face accordingly?  Am I
> > missing something?
> 
> OK, I think I understand what you mean now.  But is it really correct to
> put that in draw_glyphs, and not say, fill_XXX_glyph_string?

If this affects the glyph string, then fill_XXX_glyph_string is a
better place, yes.

> And even then, what about cases where a non-ASCII face is used?
> Does the mouse face in the Mouse_HLInfo take that into account?

I'm not sure what issue you have in mind.  Why should it matter if the
glyph string's face is ASCII or non-ASCII?  Do you see any problems
related to the box face that happen when text is ASCII, but not when
it's non-ASCII, or vice versa?

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]

Eli Zaretskii <eliz@gnu.org> writes:

> If this affects the glyph string, then fill_XXX_glyph_string is a
> better place, yes.

Thanks, noted.

> I'm not sure what issue you have in mind.  Why should it matter if the
> glyph string's face is ASCII or non-ASCII?  Do you see any problems
> related to the box face that happen when text is ASCII, but not when
> it's non-ASCII, or vice versa?

IIUC, the face that is actually used in a glyph string is the one
returned by FACE_FOR_CHAR, which returns an adjusted face if the
character passed to it is multibyte.

What I'm asking is whether or not the adjustments made by FACE_FOR_CHAR
are also made to the mouse face (which I think they are not, because
there is only one mouse face at any given time, while the highlighted
area can span many faces that could have been adjusted for many
different characters).  Thanks.

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: Thu, 14 Oct 2021 20:27:34 +0800
> 
> > I'm not sure what issue you have in mind.  Why should it matter if the
> > glyph string's face is ASCII or non-ASCII?  Do you see any problems
> > related to the box face that happen when text is ASCII, but not when
> > it's non-ASCII, or vice versa?
> 
> IIUC, the face that is actually used in a glyph string is the one
> returned by FACE_FOR_CHAR, which returns an adjusted face if the
> character passed to it is multibyte.

Correct.  (More accurately, not if the character is multibyte, but if
the character cannot be displayed by the font of the ASCII face.)

> What I'm asking is whether or not the adjustments made by FACE_FOR_CHAR
> are also made to the mouse face (which I think they are not, because
> there is only one mouse face at any given time

That's not true.  See this part of x_set_mouse_face_gc:

  /* What face has to be used last for the mouse face?  */
  face_id = MOUSE_HL_INFO (s->f)->mouse_face_face_id;
  face = FACE_FROM_ID_OR_NULL (s->f, face_id);
  if (face == NULL)
    face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);

  if (s->first_glyph->type == CHAR_GLYPH)
    face_id = FACE_FOR_CHAR (s->f, face, s->first_glyph->u.ch, -1, Qnil);
  else
    face_id = FACE_FOR_CHAR (s->f, face, 0, -1, Qnil);
  s->face = FACE_FROM_ID (s->f, face_id);
  prepare_face_for_display (s->f, s->face);

IOW, we do call FACE_FOR_CHAR to create a mouse-face variant suitable
for non-ASCII characters.

> while the highlighted area can span many faces that could have been
> adjusted for many different characters).

If the original text includes characters that need different fonts,
then each run of characters that have the same font will produce a
separate glyph string.  A glyph string by definition has the same face
on all of its characters; when FACE_FOR_CHAR produces a new face
(because we found a character that needs a different font), we end the
glyph string and start a new one.  So such stretches of text will be
covered by several separate glyph strings with mouse-face, and each
one of them will have its own face from non-ASCII characters by virtue
of the above code.

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]

Thanks for the clarifications.  One last question though, if I make the
change to setting the face in draw_glyphs, it would be OK to remove
x_set_mouse_face_gc, because the face is already set correctly by
draw_glyphs, and no other adjustments to the GC or glyph string will be
required, right?

Thanks.

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: Thu, 14 Oct 2021 21:11:31 +0800
> 
> Thanks for the clarifications.  One last question though, if I make the
> change to setting the face in draw_glyphs, it would be OK to remove
> x_set_mouse_face_gc, because the face is already set correctly by
> draw_glyphs, and no other adjustments to the GC or glyph string will be
> required, right?

No, I think it's wrong to set the face GC in xdisp.c, because the GC
is fundamentally terminal-specific, so it belongs to xterm/w32term
etc.  I'm not even sure xdisp.c knows the exact type of GC, which
differs according to the platform.  The logic of selecting the face
should indeed be removed from x_set_mouse_face_gc, but the calls to
XChangeGC etc. should remain there.

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]

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

Eli Zaretskii <eliz@gnu.org> writes:

> No, I think it's wrong to set the face GC in xdisp.c, because the GC
> is fundamentally terminal-specific, so it belongs to xterm/w32term
> etc.  I'm not even sure xdisp.c knows the exact type of GC, which
> differs according to the platform.  The logic of selecting the face
> should indeed be removed from x_set_mouse_face_gc, but the calls to
> XChangeGC etc. should remain there.

Thanks.  Here's a patch that should remove the issues you pointed out:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-minor-issues-with-text-display-when-cursor-is-in.patch --]
[-- Type: text/x-patch, Size: 12644 bytes --]

From d9fa01cca632672efd20272131acd4e13b0b4106 Mon Sep 17 00:00:00 2001
From: Po Lu <luangruo@yahoo.com>
Date: Thu, 14 Oct 2021 18:38:26 +0800
Subject: [PATCH] Fix minor issues with text display when cursor is in mouse
 face

 * src/xdisp.c (get_cursor_offset_for_mouse_face): Don't calculate
offsets for the glyph the cursor is on.
(fill_composite_glyph_string)
(fill_gstring_glyph_string)
(fill_glyphless_glyph_string)
(fill_glyph_string)
(fill_image_glyph_string)
(fill_xwidget_glyph_string)
(fill_stretch_glyph_string): Set s->face to mouse face whenever
appropriate.
(set_glyph_string_background_width): Update background width and
s->width to take into account differing :box properties of the mouse
face, when producing strings for the cursor.
(erase_phys_cursor): Redraw mouse face when erasing a cursor on top of
the mouse face.
 * src/xterm.c (x_set_mouse_face_gc): Stop setting s->face when under
mouse face because redisplay now does that for us.
 * src/w32term.c (w32_set_mouse_face_gc): Likewise.
---
 src/w32term.c |  16 -----
 src/xdisp.c   | 158 +++++++++++++++++++++++++++++++++++++++++++-------
 src/xterm.c   |  16 -----
 3 files changed, 136 insertions(+), 54 deletions(-)

diff --git a/src/w32term.c b/src/w32term.c
index 9cf250cd73..07a5cd3564 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -954,22 +954,6 @@ w32_set_cursor_gc (struct glyph_string *s)
 static void
 w32_set_mouse_face_gc (struct glyph_string *s)
 {
-  int face_id;
-  struct face *face;
-
-  /* What face has to be used last for the mouse face?  */
-  face_id = MOUSE_HL_INFO (s->f)->mouse_face_face_id;
-  face = FACE_FROM_ID_OR_NULL (s->f, face_id);
-  if (face == NULL)
-    face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
-
-  if (s->first_glyph->type == CHAR_GLYPH)
-    face_id = FACE_FOR_CHAR (s->f, face, s->first_glyph->u.ch, -1, Qnil);
-  else
-    face_id = FACE_FOR_CHAR (s->f, face, 0, -1, Qnil);
-  s->face = FACE_FROM_ID (s->f, face_id);
-  prepare_face_for_display (s->f, s->face);
-
   /* If font in this face is same as S->font, use it.  */
   if (s->font == s->face->font)
     s->gc = s->face->gc;
diff --git a/src/xdisp.c b/src/xdisp.c
index 012c2ad8bf..c61d6a943b 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -28128,6 +28128,14 @@ fill_composite_glyph_string (struct glyph_string *s, struct face *base_face,
       s->font = s->face->font;
     }
 
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID (s->f, hlinfo->mouse_face_face_id);
+      s->font = s->face->font;
+    }
+
   /* All glyph strings for the same composition has the same width,
      i.e. the width set for the first component of the composition.  */
   s->width = s->first_glyph->pixel_width;
@@ -28164,7 +28172,14 @@ fill_gstring_glyph_string (struct glyph_string *s, int face_id,
   s->cmp_id = glyph->u.cmp.id;
   s->cmp_from = glyph->slice.cmp.from;
   s->cmp_to = glyph->slice.cmp.to + 1;
-  s->face = FACE_FROM_ID (s->f, face_id);
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID (s->f, hlinfo->mouse_face_face_id);
+    }
+  else
+    s->face = FACE_FROM_ID (s->f, face_id);
   lgstring = composition_gstring_from_id (s->cmp_id);
   s->font = XFONT_OBJECT (LGSTRING_FONT (lgstring));
   /* The width of a composition glyph string is the sum of the
@@ -28218,7 +28233,14 @@ fill_glyphless_glyph_string (struct glyph_string *s, int face_id,
   glyph = s->row->glyphs[s->area] + start;
   last = s->row->glyphs[s->area] + end;
   voffset = glyph->voffset;
-  s->face = FACE_FROM_ID (s->f, face_id);
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID (s->f, hlinfo->mouse_face_face_id);
+    }
+  else
+    s->face = FACE_FROM_ID (s->f, face_id);
   s->font = s->face->font ? s->face->font : FRAME_FONT (s->f);
   s->nchars = 1;
   s->width = glyph->pixel_width;
@@ -28281,6 +28303,16 @@ fill_glyph_string (struct glyph_string *s, int face_id,
 	break;
     }
 
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      struct face *face
+        = FACE_FROM_ID (s->f, hlinfo->mouse_face_face_id);
+      s->face
+        = FACE_FROM_ID (s->f, FACE_FOR_CHAR (s->f, face,
+					     s->first_glyph->u.ch, -1, Qnil));
+    }
   s->font = s->face->font;
 
   /* If the specified font could not be loaded, use the frame's font,
@@ -28310,7 +28342,15 @@ fill_image_glyph_string (struct glyph_string *s)
   s->img = IMAGE_FROM_ID (s->f, s->first_glyph->u.img_id);
   eassert (s->img);
   s->slice = s->first_glyph->slice.img;
-  s->face = FACE_FROM_ID (s->f, s->first_glyph->face_id);
+
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID (s->f, hlinfo->mouse_face_face_id);
+    }
+  else
+    s->face = FACE_FROM_ID (s->f, s->first_glyph->face_id);
   s->font = s->face->font;
   s->width = s->first_glyph->pixel_width;
 
@@ -28324,7 +28364,14 @@ fill_image_glyph_string (struct glyph_string *s)
 fill_xwidget_glyph_string (struct glyph_string *s)
 {
   eassert (s->first_glyph->type == XWIDGET_GLYPH);
-  s->face = FACE_FROM_ID (s->f, s->first_glyph->face_id);
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID (s->f, hlinfo->mouse_face_face_id);
+    }
+  else
+    s->face = FACE_FROM_ID (s->f, s->first_glyph->face_id);
   s->font = s->face->font;
   s->width = s->first_glyph->pixel_width;
   s->ybase += s->first_glyph->voffset;
@@ -28349,7 +28396,14 @@ fill_stretch_glyph_string (struct glyph_string *s, int start, int end)
   glyph = s->row->glyphs[s->area] + start;
   last = s->row->glyphs[s->area] + end;
   face_id = glyph->face_id;
-  s->face = FACE_FROM_ID (s->f, face_id);
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID (s->f, hlinfo->mouse_face_face_id);
+    }
+  else
+    s->face = FACE_FROM_ID (s->f, face_id);
   s->font = s->face->font;
   s->width = glyph->pixel_width;
   s->nchars = 1;
@@ -28598,7 +28652,12 @@ right_overwriting (struct glyph_string *s)
 
 /* Set background width of glyph string S.  START is the index of the
    first glyph following S.  LAST_X is the right-most x-position + 1
-   in the drawing area.  */
+   in the drawing area.
+
+   If S's hl is DRAW_CURSOR, S->f is a window system frame, and the
+   cursor in S's window is currently under mouse face, s->width will
+   also be updated to take into account differing :box properties
+   between the original face and the mouse face. */
 
 static void
 set_glyph_string_background_width (struct glyph_string *s, int start, int last_x)
@@ -28620,7 +28679,67 @@ set_glyph_string_background_width (struct glyph_string *s, int start, int last_x
   if (s->extends_to_end_of_line_p)
     s->background_width = last_x - s->x + 1;
   else
-    s->background_width = s->width;
+    {
+      s->background_width = s->width;
+#ifdef HAVE_WINDOW_SYSTEM
+      if (FRAME_WINDOW_P (s->f)
+	  && s->hl == DRAW_CURSOR
+	  && cursor_in_mouse_face_p (s->w))
+	{
+	  /* We will have to adjust the background width of the string
+	     in this situation, because the glyph's pixel_width might
+	     be inconsistent with the box of the mouse face, which
+	     leads to an ugly over-wide cursor. */
+
+	  struct glyph *g = s->first_glyph;
+	  struct face *regular_face = FACE_FROM_ID (s->f, g->face_id);
+
+          bool do_left_box_p = g->left_box_line_p;
+          bool do_right_box_p = g->right_box_line_p;
+
+          /* This is required because we test some parameters
+             of the image slice before applying the box in
+             produce_image_glyph. */
+
+          if (g->type == IMAGE_GLYPH)
+            {
+	      if (!s->row->reversed_p)
+		{
+		  struct image *img = IMAGE_FROM_ID (s->f, g->u.img_id);
+		  do_left_box_p = g->left_box_line_p &&
+		    g->slice.img.x == 0;
+		  do_right_box_p = g->right_box_line_p &&
+		    g->slice.img.x + g->slice.img.width == img->width;
+		}
+	      else
+		{
+		  struct image *img = IMAGE_FROM_ID (s->f, 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;
+		}
+            }
+
+          /* If the glyph has a left box line, subtract it from the
+	     offset.  */
+          if (do_left_box_p)
+            s->background_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 (do_right_box_p)
+            s->background_width -= max (0, regular_face->box_vertical_line_width);
+          /* Now add the line widths from the new face.  */
+          if (g->left_box_line_p)
+            s->background_width += max (0, s->face->box_vertical_line_width);
+          if (g->right_box_line_p)
+            s->background_width += max (0, s->face->box_vertical_line_width);
+
+	  /* s->width is probably worth adjusting here as well. */
+	  s->width = s->background_width;
+        }
+#endif
+    }
 }
 
 
@@ -31755,10 +31874,6 @@ erase_phys_cursor (struct window *w)
   Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
   int hpos = w->phys_cursor.hpos;
   int vpos = w->phys_cursor.vpos;
-#ifdef HAVE_WINDOW_SYSTEM
-  int mouse_delta;
-  int phys_x = w->phys_cursor.x;
-#endif
   bool mouse_face_here_p = false;
   struct glyph_matrix *active_glyphs = w->current_matrix;
   struct glyph_row *cursor_row;
@@ -31829,13 +31944,16 @@ erase_phys_cursor (struct window *w)
     mouse_face_here_p = true;
 
 #ifdef HAVE_WINDOW_SYSTEM
-  /* Adjust the physical cursor's X coordinate if needed.  The problem
-     solved by the code below is outlined in the comment above
-     'get_cursor_offset_for_mouse_face'.  */
-  if (mouse_face_here_p)
+  /* Since erasing the phys cursor will probably lead to corruption of
+     the mouse face display if the glyph's pixel_width is not kept up
+     to date with the :box property of the mouse face, just redraw the
+     mouse face. */
+  if (FRAME_WINDOW_P (WINDOW_XFRAME (w)) && mouse_face_here_p)
     {
-      get_cursor_offset_for_mouse_face (w, cursor_row, &mouse_delta);
-      w->phys_cursor.x += mouse_delta;
+      w->phys_cursor_on_p = false;
+      w->phys_cursor_type = NO_CURSOR;
+      show_mouse_face (MOUSE_HL_INFO (WINDOW_XFRAME (w)), DRAW_MOUSE_FACE);
+      return;
     }
 #endif
 
@@ -31874,10 +31992,6 @@ erase_phys_cursor (struct window *w)
   draw_phys_cursor_glyph (w, cursor_row, hl);
 
  mark_cursor_off:
-#ifdef HAVE_WINDOW_SYSTEM
-  /* Restore the original cursor position.  */
-  w->phys_cursor.x = phys_x;
-#endif
   w->phys_cursor_on_p = false;
   w->phys_cursor_type = NO_CURSOR;
 }
@@ -36042,7 +36156,7 @@ get_cursor_offset_for_mouse_face (struct window *w, struct glyph_row *row,
   /* Calculate the offset to correct phys_cursor x if we are
      drawing the cursor inside mouse-face highlighted text.  */
 
-  for (; row->reversed_p ? start >= end : start <= end;
+  for (; row->reversed_p ? start > end : start < end;
        row->reversed_p ? --start : ++start)
     {
       struct glyph *g = start;
diff --git a/src/xterm.c b/src/xterm.c
index 89885e0d88..961c61c245 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -1563,22 +1563,6 @@ x_set_cursor_gc (struct glyph_string *s)
 static void
 x_set_mouse_face_gc (struct glyph_string *s)
 {
-  int face_id;
-  struct face *face;
-
-  /* What face has to be used last for the mouse face?  */
-  face_id = MOUSE_HL_INFO (s->f)->mouse_face_face_id;
-  face = FACE_FROM_ID_OR_NULL (s->f, face_id);
-  if (face == NULL)
-    face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
-
-  if (s->first_glyph->type == CHAR_GLYPH)
-    face_id = FACE_FOR_CHAR (s->f, face, s->first_glyph->u.ch, -1, Qnil);
-  else
-    face_id = FACE_FOR_CHAR (s->f, face, 0, -1, Qnil);
-  s->face = FACE_FROM_ID (s->f, face_id);
-  prepare_face_for_display (s->f, s->face);
-
   if (s->font == s->face->font)
     s->gc = s->face->gc;
   else
-- 
2.31.1

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: Fri, 15 Oct 2021 09:28:17 +0800
> 
> @@ -28281,6 +28303,16 @@ fill_glyph_string (struct glyph_string *s, int face_id,
>  	break;
>      }
>  
> +  if (s->hl == DRAW_MOUSE_FACE
> +      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
> +    {
> +      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
> +      struct face *face
> +        = FACE_FROM_ID (s->f, hlinfo->mouse_face_face_id);
> +      s->face
> +        = FACE_FROM_ID (s->f, FACE_FOR_CHAR (s->f, face,
> +					     s->first_glyph->u.ch, -1, Qnil));
> +    }
>    s->font = s->face->font;

This part doesn't look right to me: FACE_FOR_CHAR could potentially
yield a face with a different font, but the glyph codes in the glyph
string will reference the previous font, because
get_glyph_face_and_encoding was called before the face was changed.

Also, why did you not follow the more cautious code of xterm.c:

> -  /* What face has to be used last for the mouse face?  */
> -  face_id = MOUSE_HL_INFO (s->f)->mouse_face_face_id;
> -  face = FACE_FROM_ID_OR_NULL (s->f, face_id);
> -  if (face == NULL)
> -    face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);

FACE_FROM_ID can abort if the face is not in the face cache.

> @@ -28620,7 +28679,67 @@ set_glyph_string_background_width (struct glyph_string *s, int start, int last_x
>    if (s->extends_to_end_of_line_p)
>      s->background_width = last_x - s->x + 1;
>    else
> -    s->background_width = s->width;
> +    {
> +      s->background_width = s->width;
> +#ifdef HAVE_WINDOW_SYSTEM
> +      if (FRAME_WINDOW_P (s->f)
> +	  && s->hl == DRAW_CURSOR
> +	  && cursor_in_mouse_face_p (s->w))
> +	{
> +	  /* We will have to adjust the background width of the string
> +	     in this situation, because the glyph's pixel_width might
> +	     be inconsistent with the box of the mouse face, which
> +	     leads to an ugly over-wide cursor. */
> +
> +	  struct glyph *g = s->first_glyph;
> +	  struct face *regular_face = FACE_FROM_ID (s->f, g->face_id);
> +
> +          bool do_left_box_p = g->left_box_line_p;
> +          bool do_right_box_p = g->right_box_line_p;
> +
> +          /* This is required because we test some parameters
> +             of the image slice before applying the box in
> +             produce_image_glyph. */
> +
> +          if (g->type == IMAGE_GLYPH)
> +            {
> +	      if (!s->row->reversed_p)
> +		{
> +		  struct image *img = IMAGE_FROM_ID (s->f, g->u.img_id);
> +		  do_left_box_p = g->left_box_line_p &&
> +		    g->slice.img.x == 0;
> +		  do_right_box_p = g->right_box_line_p &&
> +		    g->slice.img.x + g->slice.img.width == img->width;
> +		}
> +	      else
> +		{
> +		  struct image *img = IMAGE_FROM_ID (s->f, 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;
> +		}
> +            }
> +
> +          /* If the glyph has a left box line, subtract it from the
> +	     offset.  */
> +          if (do_left_box_p)
> +            s->background_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 (do_right_box_p)
> +            s->background_width -= max (0, regular_face->box_vertical_line_width);
> +          /* Now add the line widths from the new face.  */
> +          if (g->left_box_line_p)
> +            s->background_width += max (0, s->face->box_vertical_line_width);
> +          if (g->right_box_line_p)
> +            s->background_width += max (0, s->face->box_vertical_line_width);
> +
> +	  /* s->width is probably worth adjusting here as well. */
> +	  s->width = s->background_width;
> +        }
> +#endif

This looks like the same code we have elsewhere, so can't we have a
function to call in both places?

Also, the indentation with/without TABs seems wrong here.

And finally, please always leave TWO spaces after the final period in
a comment, like this:

        /* s->width is probably worth adjusting here as well.  */
                                                            ^^^

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]

Eli Zaretskii <eliz@gnu.org> writes:

> This part doesn't look right to me: FACE_FOR_CHAR could potentially
> yield a face with a different font, but the glyph codes in the glyph
> string will reference the previous font, because
> get_glyph_face_and_encoding was called before the face was changed.

But now that I think of it, what if the original font has different
metrics than the mouse face?  In that case, shouldn't s->font be the
original font and not the font of the mouse face?  Thanks.

> Also, why did you not follow the more cautious code of xterm.c:
> FACE_FROM_ID can abort if the face is not in the face cache.

Thanks.

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: Sat, 16 Oct 2021 08:18:25 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > This part doesn't look right to me: FACE_FOR_CHAR could potentially
> > yield a face with a different font, but the glyph codes in the glyph
> > string will reference the previous font, because
> > get_glyph_face_and_encoding was called before the face was changed.
> 
> But now that I think of it, what if the original font has different
> metrics than the mouse face?  In that case, shouldn't s->font be the
> original font and not the font of the mouse face?  Thanks.

I don't think I follow: what is "the original font" in this context?
And when you say "shouldn't s->font be", do you mean what it should be
before or after the processing in fill_glyph_string?

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]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think I follow: what is "the original font" in this context?
> And when you say "shouldn't s->font be", do you mean what it should be
> before or after the processing in fill_glyph_string?

I meant what it should be after the processing, and by "the original
font", I meant the font of the original face, that was used to calculate
the metrics of the glyphs.

Apologies for the confusion.  Thanks.

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: Sat, 16 Oct 2021 14:16:12 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't think I follow: what is "the original font" in this context?
> > And when you say "shouldn't s->font be", do you mean what it should be
> > before or after the processing in fill_glyph_string?
> 
> I meant what it should be after the processing, and by "the original
> font", I meant the font of the original face, that was used to calculate
> the metrics of the glyphs.

FACE_FOR_CHAR will get you the face with the correct font, and calling
get_glyph_face_and_encoding after that will produce the glyph codes
from that font.  So that's exactly why I commented why your additional
code must be before the loop that produces the glyph codes (inside
get_glyph_face_and_encoding).

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]

Eli Zaretskii <eliz@gnu.org> writes:

>> I meant what it should be after the processing, and by "the original
>> font", I meant the font of the original face, that was used to calculate
>> the metrics of the glyphs.

> FACE_FOR_CHAR will get you the face with the correct font, and calling
> get_glyph_face_and_encoding after that will produce the glyph codes
> from that font.  So that's exactly why I commented why your additional
> code must be before the loop that produces the glyph codes (inside
> get_glyph_face_and_encoding).

We might be misunderstanding something: I'm asking whether to arrange
fill_glyph_string like such:

  /* The loop with get_glyph_face_and_encoding is above this comment */
  s->font = s->face->font;
  if (s->hl == DRAW_MOUSE_FACE
      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
    {
      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
      struct face *face
        = FACE_FROM_ID (s->f, hlinfo->mouse_face_face_id);
      s->face
        = FACE_FROM_ID (s->f, FACE_FOR_CHAR (s->f, face,
					     s->first_glyph->u.ch, -1, Qnil));
    }

Or like such:

  /* get_glyph_face_and_encoding is modified to produce glyphs with the
     mouse face in the loop here. */
  while (glyph < last
	 && glyph->type == CHAR_GLYPH
	 && glyph->voffset == voffset
	 /* Same face id implies same font, nowadays.  */
	 && glyph->face_id == face_id
	 && glyph->glyph_not_available_p == glyph_not_available_p)
    {
      s->face = get_glyph_face_and_encoding (s->f, glyph,
					     s->char2b + s->nchars,
                                             /* This argument controls whether or not
                                                get_glyph_face_and_encoding uses the mouse face */
                                             (s->hl == DRAW_MOUSE_FACE
                                              || (s->hl == DRAW_CURSOR
                                                  && cursor_in_mouse_face_p (s->w)));
      ++s->nchars;
      eassert (s->nchars <= end - start);
      s->width += glyph->pixel_width;
      if (glyph++->padding_p != s->padding_p)
	break;
    }

  s->font = s->face->font;

I think the first situation will work better, because we want the mouse
face to be drawn with the font that the regular face is under, not the
font of the mouse face.  This is how the old code in *term.c used to
behave, and prevents the text from being drawn with a font (font, not
face) that has metrics different from that of the mouse face's font.

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: Sat, 16 Oct 2021 14:39:58 +0800
> 
> I think the first situation will work better, because we want the mouse
> face to be drawn with the font that the regular face is under, not the
> font of the mouse face.

I don't understand: if the mouse-face changes the font, you want to
ignore that? why does that make sense?

And mouse-face is defined for ASCII font only anyway, which is why the
code calls FACE_FOR_CHAR.  You want to ignore the font that this call
produces?

> This is how the old code in *term.c used to
> behave, and prevents the text from being drawn with a font (font, not
> face) that has metrics different from that of the mouse face's font.

A face includes the font, so I don't understand why you want to
separate them, and how.

As for the old code: are you sure that's not a bug, part of the same
subtle issue you are trying to fix?

Btw, I'm not sure we need move the face selection into
get_glyph_face_and_encoding, I think it should be left outside.  That
selection needs to be done only once, whereas
get_glyph_face_and_encoding is called in a loop.  You just should move
the face determination before the loop, and momentarily change
glyph->face to the selected face and back around the call to
get_glyph_face_and_encoding.

Maybe I'm wrong (it won't be the first time), but in that case please
produce an example of a situation where my opinion produces incorrect
results, so I could study it and find what I am missing now.  Or maybe
you could describe in more detail what you discovered about the
artifacts I reported, which led you to these changes, because perhaps
I don't have a clear idea about what exactly are you trying to fix
here.

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]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't understand: if the mouse-face changes the font, you want to
> ignore that? why does that make sense?
>
> And mouse-face is defined for ASCII font only anyway, which is why the
> code calls FACE_FOR_CHAR.  You want to ignore the font that this call
> produces?

Yes, precisely.

> A face includes the font, so I don't understand why you want to
> separate them, and how.
>
> As for the old code: are you sure that's not a bug, part of the same
> subtle issue you are trying to fix?

I'm reasonably sure.  Under the old code in *term, moving the mouse over
the entry for `glyphless-char' in list-faces-display results in nothing,
while under the new code (where s->font == s->face->font even under
mouse face) the section under mouse face overlaps with its surroundings
and is otherwise glitchy, because the mouse face's font is larger than
the original face's font.

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: Sat, 16 Oct 2021 15:13:18 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't understand: if the mouse-face changes the font, you want to
> > ignore that? why does that make sense?
> >
> > And mouse-face is defined for ASCII font only anyway, which is why the
> > code calls FACE_FOR_CHAR.  You want to ignore the font that this call
> > produces?
> 
> Yes, precisely.
> 
> > A face includes the font, so I don't understand why you want to
> > separate them, and how.
> >
> > As for the old code: are you sure that's not a bug, part of the same
> > subtle issue you are trying to fix?
> 
> I'm reasonably sure.  Under the old code in *term, moving the mouse over
> the entry for `glyphless-char' in list-faces-display results in nothing,
> while under the new code (where s->font == s->face->font even under
> mouse face) the section under mouse face overlaps with its surroundings
> and is otherwise glitchy, because the mouse face's font is larger than
> the original face's font.

In the examples I used for testing the size of the font was the same,
so I'm no longer sure we are talking about the same thing.

I also asked to describe what exactly you found that causes the
artifacts I described when I installed the previous patch -- could you
please provide that description?  Because I'm no longer sure I
understand what is the problem with the existing code you are trying
to fix now.

AFAIU, the issue is with displaying the cursor inside mouse-face, and
that involves redrawing the character on which the cursor is
displayed, so that must use the same font as the one we used to
display the character itself, and use the same font.  But mouse-face's
font is not necessarily appropriate for every character shown in that
face.

Thanks.

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]

Eli Zaretskii <eliz@gnu.org> writes:

>> I'm reasonably sure.  Under the old code in *term, moving the mouse over
>> the entry for `glyphless-char' in list-faces-display results in nothing,
>> while under the new code (where s->font == s->face->font even under
>> mouse face) the section under mouse face overlaps with its surroundings
>> and is otherwise glitchy, because the mouse face's font is larger than
>> the original face's font.

> In the examples I used for testing the size of the font was the same,
> so I'm no longer sure we are talking about the same thing.

Yes, this has gone off on some kind of tangent.  We're discussing the
repercussions of a proposed method of moving the mouse face selection
into fill_XXX_glyph_string, instead of putting it in terminal specific
code.

> I also asked to describe what exactly you found that causes the
> artifacts I described when I installed the previous patch -- could you
> please provide that description?  Because I'm no longer sure I
> understand what is the problem with the existing code you are trying
> to fix now.

Okay.  The first issue, with the cursor put on the first character "s",
is caused by this snippet of xterm.c (in x_draw_glyph_string_foreground):

  /* If first glyph of S has a left box line, start drawing the text
     of S to the right of that box line.  */
  if (s->face->box != FACE_NO_BOX
      && s->first_glyph->left_box_line_p)
    x = s->x + max (s->face->box_vertical_line_width, 0);
  else
    x = s->x;

An identical snippet exists in w32term.

This happens because s->face is not the mouse face when s->hl is
DRAW_CURSOR and cursor_in_mouse_face_p, so it mistakenly assumes there
is a box for s (when there is in fact no box), and adds the original
face's vertical box width to x.

(Seeing this issue, you proposed to also move mouse face selection to
draw_glyphs, and I proposed to move it to fill_XXX_glyph_string, leading
to the above tangent about the semantics of s->font.)

The second issue was caused by testing for "start <=" end instead of
"start < end" in get_cursor_offset_for_mouse_face.

Thanks.

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: Sat, 16 Oct 2021 15:52:24 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I'm reasonably sure.  Under the old code in *term, moving the mouse over
> >> the entry for `glyphless-char' in list-faces-display results in nothing,
> >> while under the new code (where s->font == s->face->font even under
> >> mouse face) the section under mouse face overlaps with its surroundings
> >> and is otherwise glitchy, because the mouse face's font is larger than
> >> the original face's font.
> 
> > In the examples I used for testing the size of the font was the same,
> > so I'm no longer sure we are talking about the same thing.
> 
> Yes, this has gone off on some kind of tangent.

I don't think it's a tangent, see below.

> Okay.  The first issue, with the cursor put on the first character "s",
> is caused by this snippet of xterm.c (in x_draw_glyph_string_foreground):
> 
>   /* If first glyph of S has a left box line, start drawing the text
>      of S to the right of that box line.  */
>   if (s->face->box != FACE_NO_BOX
>       && s->first_glyph->left_box_line_p)
>     x = s->x + max (s->face->box_vertical_line_width, 0);
>   else
>     x = s->x;
> 
> An identical snippet exists in w32term.
> 
> This happens because s->face is not the mouse face when s->hl is
> DRAW_CURSOR and cursor_in_mouse_face_p, so it mistakenly assumes there
> is a box for s (when there is in fact no box), and adds the original
> face's vertical box width to x.

So you are saying that s->hl can only be either DRAW_CURSOR or
DRAW_MOUSE_FACE, whereas we need both?  If so, this matches what I
thought you were trying to solve.

So what happens here is that s->face is computed from the face of the
glyphs which "belong" to the glyph string s.  That face comes from the
glyph matrix which holds the glyphs.  That face was computed by
redisplay_window using FACE_FOR_CHAR, see get_next_display_element, so
it's the face at the character's buffer position adjusted for the font
suitable for the character at the cursor.  Now you want to display
that same character, but with the mouse-face.  FACE_FROM_ID gives you
that face, but it is for ASCII characters.  So you call FACE_FOR_CHAR
again, to obtain the mouse face adjusted for the font suitable for
displaying the character at the cursor.

The above sounds correct to me, so I don't understand why you want to
ignore the font of the face produced by FACE_FOR_CHAR.  What am I
missing?

> (Seeing this issue, you proposed to also move mouse face selection to
> draw_glyphs, and I proposed to move it to fill_XXX_glyph_string, leading
> to the above tangent about the semantics of s->font.)

Yes.  Btw, it would probably be cleaner to add an extra argument to
get_glyph_face_and_encoding, but make that argument be a pointer to
'struct face', not just an indication of which face to use.

> The second issue was caused by testing for "start <=" end instead of
> "start < end" in get_cursor_offset_for_mouse_face.

What was that second issue about? why did you need to change the
inequality?

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]

Eli Zaretskii <eliz@gnu.org> writes:

> So what happens here is that s->face is computed from the face of the
> glyphs which "belong" to the glyph string s.  That face comes from the
> glyph matrix which holds the glyphs.  That face was computed by
> redisplay_window using FACE_FOR_CHAR, see get_next_display_element, so
> it's the face at the character's buffer position adjusted for the font
> suitable for the character at the cursor.  Now you want to display
> that same character, but with the mouse-face.  FACE_FROM_ID gives you
> that face, but it is for ASCII characters.  So you call FACE_FOR_CHAR
> again, to obtain the mouse face adjusted for the font suitable for
> displaying the character at the cursor.
>
> The above sounds correct to me, so I don't understand why you want to
> ignore the font of the face produced by FACE_FOR_CHAR.  What am I
> missing?

The problem occurs when (adjusted_mouse_face)->font has different
metrics from the font originally used to display each glyph.

(For instance, if mouse face is `highlight' but the original face was
`custom-group-tag', which is variable pitch and has a much larger font
size than `highlight')

In this case, the text will be drawn with the wrong font for its
existing metrics!  If the mouse face has larger dimensions than the
original face, it will extend past the boundaries of the original text,
and if they are smaller, then the text will not be enough to fill the
original text.

And regardless of the relative dimensions of either faces, cursor
position will be even more incorrect.

> Yes.  Btw, it would probably be cleaner to add an extra argument to
> get_glyph_face_and_encoding, but make that argument be a pointer to
> 'struct face', not just an indication of which face to use.

Noted, thanks.

> What was that second issue about? why did you need to change the
> inequality?

get_cursor_offset_for_mouse_face was including the glyph under the
cursor in its offset calculations, which is invalid.

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: 50660@debbugs.gnu.org
> Date: Sat, 16 Oct 2021 20:12:55 +0800
> 
> The problem occurs when (adjusted_mouse_face)->font has different
> metrics from the font originally used to display each glyph.
> 
> (For instance, if mouse face is `highlight' but the original face was
> `custom-group-tag', which is variable pitch and has a much larger font
> size than `highlight')

That's a separate problem, which exists in the current code as well,
doesn't it?

The only solution to that is to merge the mouse-face with the original
face, ignoring the attributes of mouse-face that affect the text size,
as documented in the ELisp manual.

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]

Eli Zaretskii <eliz@gnu.org> writes:

> That's a separate problem, which exists in the current code as well,
> doesn't it?

It doesn't, because s->font isn't updated (while s->face is) in the
current code, which makes everything work properly.

> The only solution to that is to merge the mouse-face with the original
> face, ignoring the attributes of mouse-face that affect the text size,
> as documented in the ELisp manual.

Would this be easy to do?  If so, could you explain where to begin
implementing this?  Thanks.

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: 50660@debbugs.gnu.org
> Date: Sat, 16 Oct 2021 20:36:43 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > That's a separate problem, which exists in the current code as well,
> > doesn't it?
> 
> It doesn't, because s->font isn't updated (while s->face is) in the
> current code, which makes everything work properly.

Then maybe the simplest solution would be to do the same.

> > The only solution to that is to merge the mouse-face with the original
> > face, ignoring the attributes of mouse-face that affect the text size,
> > as documented in the ELisp manual.
> 
> Would this be easy to do?  If so, could you explain where to begin
> implementing this?  Thanks.

See merge_face_vectors.  But perhaps ignoring the font of what
FACE_FOR_CHAR produces has the same effect, but cheaper.

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]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Then maybe the simplest solution would be to do the same.

Yes, I tend to think the same way.  Do the attached changes make sense
to you?  Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-minor-issues-with-text-display-when-cursor-is-in.patch --]
[-- Type: text/x-patch, Size: 12965 bytes --]

From 92165a34d29af55723e9a141218f4f5325330351 Mon Sep 17 00:00:00 2001
From: Po Lu <luangruo@yahoo.com>
Date: Thu, 14 Oct 2021 18:38:26 +0800
Subject: [PATCH] Fix minor issues with text display when cursor is in mouse
 face

 * src/xdisp.c (get_cursor_offset_for_mouse_face): Don't calculate
offsets for the glyph the cursor is on.
(fill_composite_glyph_string)
(fill_gstring_glyph_string)
(fill_glyphless_glyph_string)
(fill_glyph_string)
(fill_image_glyph_string)
(fill_xwidget_glyph_string)
(fill_stretch_glyph_string): Set s->face to mouse face whenever
appropriate.
(set_glyph_string_background_width): Update background width and
s->width to take into account differing :box properties of the mouse
face, when producing strings for the cursor.
(erase_phys_cursor): Redraw mouse face when erasing a cursor on top of
the mouse face.
 * src/xterm.c (x_set_mouse_face_gc): Stop setting s->face when under
mouse face because redisplay now does that for us.
 * src/w32term.c (w32_set_mouse_face_gc): Likewise.
---
 src/w32term.c |  16 -----
 src/xdisp.c   | 162 ++++++++++++++++++++++++++++++++++++++++++++------
 src/xterm.c   |  16 -----
 3 files changed, 144 insertions(+), 50 deletions(-)

diff --git a/src/w32term.c b/src/w32term.c
index 9cf250cd73..07a5cd3564 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -954,22 +954,6 @@ w32_set_cursor_gc (struct glyph_string *s)
 static void
 w32_set_mouse_face_gc (struct glyph_string *s)
 {
-  int face_id;
-  struct face *face;
-
-  /* What face has to be used last for the mouse face?  */
-  face_id = MOUSE_HL_INFO (s->f)->mouse_face_face_id;
-  face = FACE_FROM_ID_OR_NULL (s->f, face_id);
-  if (face == NULL)
-    face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
-
-  if (s->first_glyph->type == CHAR_GLYPH)
-    face_id = FACE_FOR_CHAR (s->f, face, s->first_glyph->u.ch, -1, Qnil);
-  else
-    face_id = FACE_FOR_CHAR (s->f, face, 0, -1, Qnil);
-  s->face = FACE_FROM_ID (s->f, face_id);
-  prepare_face_for_display (s->f, s->face);
-
   /* If font in this face is same as S->font, use it.  */
   if (s->font == s->face->font)
     s->gc = s->face->gc;
diff --git a/src/xdisp.c b/src/xdisp.c
index 012c2ad8bf..876a68d99c 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -28128,6 +28128,19 @@ fill_composite_glyph_string (struct glyph_string *s, struct face *base_face,
       s->font = s->face->font;
     }
 
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      int c = COMPOSITION_GLYPH (s->cmp, 0);
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+
+      s->face = FACE_FROM_ID (s->f, FACE_FOR_CHAR (s->f, s->face, c, -1, Qnil));
+    }
+
   /* All glyph strings for the same composition has the same width,
      i.e. the width set for the first component of the composition.  */
   s->width = s->first_glyph->pixel_width;
@@ -28164,7 +28177,16 @@ fill_gstring_glyph_string (struct glyph_string *s, int face_id,
   s->cmp_id = glyph->u.cmp.id;
   s->cmp_from = glyph->slice.cmp.from;
   s->cmp_to = glyph->slice.cmp.to + 1;
-  s->face = FACE_FROM_ID (s->f, face_id);
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
+  else
+    s->face = FACE_FROM_ID (s->f, face_id);
   lgstring = composition_gstring_from_id (s->cmp_id);
   s->font = XFONT_OBJECT (LGSTRING_FONT (lgstring));
   /* The width of a composition glyph string is the sum of the
@@ -28220,6 +28242,14 @@ fill_glyphless_glyph_string (struct glyph_string *s, int face_id,
   voffset = glyph->voffset;
   s->face = FACE_FROM_ID (s->f, face_id);
   s->font = s->face->font ? s->face->font : FRAME_FONT (s->f);
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
   s->nchars = 1;
   s->width = glyph->pixel_width;
   glyph++;
@@ -28283,6 +28313,18 @@ fill_glyph_string (struct glyph_string *s, int face_id,
 
   s->font = s->face->font;
 
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+      s->face
+        = FACE_FROM_ID (s->f, FACE_FOR_CHAR (s->f, s->face,
+					     s->first_glyph->u.ch, -1, Qnil));
+    }
+
   /* If the specified font could not be loaded, use the frame's font,
      but record the fact that we couldn't load it in
      S->font_not_found_p so that we can draw rectangles for the
@@ -28312,6 +28354,14 @@ fill_image_glyph_string (struct glyph_string *s)
   s->slice = s->first_glyph->slice.img;
   s->face = FACE_FROM_ID (s->f, s->first_glyph->face_id);
   s->font = s->face->font;
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
   s->width = s->first_glyph->pixel_width;
 
   /* Adjust base line for subscript/superscript text.  */
@@ -28326,6 +28376,14 @@ fill_xwidget_glyph_string (struct glyph_string *s)
   eassert (s->first_glyph->type == XWIDGET_GLYPH);
   s->face = FACE_FROM_ID (s->f, s->first_glyph->face_id);
   s->font = s->face->font;
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
   s->width = s->first_glyph->pixel_width;
   s->ybase += s->first_glyph->voffset;
   s->xwidget = s->first_glyph->u.xwidget;
@@ -28351,6 +28409,14 @@ fill_stretch_glyph_string (struct glyph_string *s, int start, int end)
   face_id = glyph->face_id;
   s->face = FACE_FROM_ID (s->f, face_id);
   s->font = s->face->font;
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+    }
   s->width = glyph->pixel_width;
   s->nchars = 1;
   voffset = glyph->voffset;
@@ -28598,7 +28664,12 @@ right_overwriting (struct glyph_string *s)
 
 /* Set background width of glyph string S.  START is the index of the
    first glyph following S.  LAST_X is the right-most x-position + 1
-   in the drawing area.  */
+   in the drawing area.
+
+   If S's hl is DRAW_CURSOR, S->f is a window system frame, and the
+   cursor in S's window is currently under mouse face, s->width will
+   also be updated to take into account differing :box properties
+   between the original face and the mouse face. */
 
 static void
 set_glyph_string_background_width (struct glyph_string *s, int start, int last_x)
@@ -28620,7 +28691,67 @@ set_glyph_string_background_width (struct glyph_string *s, int start, int last_x
   if (s->extends_to_end_of_line_p)
     s->background_width = last_x - s->x + 1;
   else
-    s->background_width = s->width;
+    {
+      s->background_width = s->width;
+#ifdef HAVE_WINDOW_SYSTEM
+      if (FRAME_WINDOW_P (s->f)
+	  && s->hl == DRAW_CURSOR
+	  && cursor_in_mouse_face_p (s->w))
+	{
+	  /* We will have to adjust the background width of the string
+	     in this situation, because the glyph's pixel_width might
+	     be inconsistent with the box of the mouse face, which
+	     leads to an ugly over-wide cursor. */
+
+	  struct glyph *g = s->first_glyph;
+	  struct face *regular_face = FACE_FROM_ID (s->f, g->face_id);
+
+          bool do_left_box_p = g->left_box_line_p;
+          bool do_right_box_p = g->right_box_line_p;
+
+          /* This is required because we test some parameters
+             of the image slice before applying the box in
+             produce_image_glyph. */
+
+          if (g->type == IMAGE_GLYPH)
+            {
+	      if (!s->row->reversed_p)
+		{
+		  struct image *img = IMAGE_FROM_ID (s->f, g->u.img_id);
+		  do_left_box_p = g->left_box_line_p &&
+		    g->slice.img.x == 0;
+		  do_right_box_p = g->right_box_line_p &&
+		    g->slice.img.x + g->slice.img.width == img->width;
+		}
+	      else
+		{
+		  struct image *img = IMAGE_FROM_ID (s->f, 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;
+		}
+            }
+
+          /* If the glyph has a left box line, subtract it from the
+	     offset.  */
+          if (do_left_box_p)
+            s->background_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 (do_right_box_p)
+            s->background_width -= max (0, regular_face->box_vertical_line_width);
+          /* Now add the line widths from the new face.  */
+          if (g->left_box_line_p)
+            s->background_width += max (0, s->face->box_vertical_line_width);
+          if (g->right_box_line_p)
+            s->background_width += max (0, s->face->box_vertical_line_width);
+
+	  /* s->width is probably worth adjusting here as well. */
+	  s->width = s->background_width;
+        }
+#endif
+    }
 }
 
 
@@ -31755,10 +31886,6 @@ erase_phys_cursor (struct window *w)
   Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
   int hpos = w->phys_cursor.hpos;
   int vpos = w->phys_cursor.vpos;
-#ifdef HAVE_WINDOW_SYSTEM
-  int mouse_delta;
-  int phys_x = w->phys_cursor.x;
-#endif
   bool mouse_face_here_p = false;
   struct glyph_matrix *active_glyphs = w->current_matrix;
   struct glyph_row *cursor_row;
@@ -31829,13 +31956,16 @@ erase_phys_cursor (struct window *w)
     mouse_face_here_p = true;
 
 #ifdef HAVE_WINDOW_SYSTEM
-  /* Adjust the physical cursor's X coordinate if needed.  The problem
-     solved by the code below is outlined in the comment above
-     'get_cursor_offset_for_mouse_face'.  */
-  if (mouse_face_here_p)
+  /* Since erasing the phys cursor will probably lead to corruption of
+     the mouse face display if the glyph's pixel_width is not kept up
+     to date with the :box property of the mouse face, just redraw the
+     mouse face. */
+  if (FRAME_WINDOW_P (WINDOW_XFRAME (w)) && mouse_face_here_p)
     {
-      get_cursor_offset_for_mouse_face (w, cursor_row, &mouse_delta);
-      w->phys_cursor.x += mouse_delta;
+      w->phys_cursor_on_p = false;
+      w->phys_cursor_type = NO_CURSOR;
+      show_mouse_face (MOUSE_HL_INFO (WINDOW_XFRAME (w)), DRAW_MOUSE_FACE);
+      return;
     }
 #endif
 
@@ -31874,10 +32004,6 @@ erase_phys_cursor (struct window *w)
   draw_phys_cursor_glyph (w, cursor_row, hl);
 
  mark_cursor_off:
-#ifdef HAVE_WINDOW_SYSTEM
-  /* Restore the original cursor position.  */
-  w->phys_cursor.x = phys_x;
-#endif
   w->phys_cursor_on_p = false;
   w->phys_cursor_type = NO_CURSOR;
 }
@@ -36042,7 +36168,7 @@ get_cursor_offset_for_mouse_face (struct window *w, struct glyph_row *row,
   /* Calculate the offset to correct phys_cursor x if we are
      drawing the cursor inside mouse-face highlighted text.  */
 
-  for (; row->reversed_p ? start >= end : start <= end;
+  for (; row->reversed_p ? start > end : start < end;
        row->reversed_p ? --start : ++start)
     {
       struct glyph *g = start;
diff --git a/src/xterm.c b/src/xterm.c
index 2b365929a1..0435ad341c 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -1573,22 +1573,6 @@ x_set_cursor_gc (struct glyph_string *s)
 static void
 x_set_mouse_face_gc (struct glyph_string *s)
 {
-  int face_id;
-  struct face *face;
-
-  /* What face has to be used last for the mouse face?  */
-  face_id = MOUSE_HL_INFO (s->f)->mouse_face_face_id;
-  face = FACE_FROM_ID_OR_NULL (s->f, face_id);
-  if (face == NULL)
-    face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
-
-  if (s->first_glyph->type == CHAR_GLYPH)
-    face_id = FACE_FOR_CHAR (s->f, face, s->first_glyph->u.ch, -1, Qnil);
-  else
-    face_id = FACE_FOR_CHAR (s->f, face, 0, -1, Qnil);
-  s->face = FACE_FROM_ID (s->f, face_id);
-  prepare_face_for_display (s->f, s->face);
-
   if (s->font == s->face->font)
     s->gc = s->face->gc;
   else
-- 
2.31.1

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: 50660@debbugs.gnu.org
> Date: Sat, 16 Oct 2021 21:18:02 +0800
> 
> > Then maybe the simplest solution would be to do the same.
> 
> Yes, I tend to think the same way.  Do the attached changes make sense
> to you?  Thanks.

There are two minor issues still:

  1) After producing the mouse face, you should call
     prepare_face_for_display.

  2) The code you add in set_glyph_string_background_width is almost
     identical to what we already have inside the loop in
     get_cursor_offset_for_mouse_face, and I wonder whether we could
     have a function to be called from these two places.

Other than that, I think this is fine, thanks.

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]

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

Eli Zaretskii <eliz@gnu.org> writes:

> There are two minor issues still:
>
>   1) After producing the mouse face, you should call
>      prepare_face_for_display.
>
>   2) The code you add in set_glyph_string_background_width is almost
>      identical to what we already have inside the loop in
>      get_cursor_offset_for_mouse_face, and I wonder whether we could
>      have a function to be called from these two places.
>
> Other than that, I think this is fine, thanks.

Does this resolve your concerns?  Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-minor-issues-with-text-display-when-cursor-is-in.patch --]
[-- Type: text/x-patch, Size: 16662 bytes --]

From 3a283bf727f5adb494689315c9e89483525402e3 Mon Sep 17 00:00:00 2001
From: Po Lu <luangruo@yahoo.com>
Date: Thu, 14 Oct 2021 18:38:26 +0800
Subject: [PATCH] Fix minor issues with text display when cursor is in mouse
 face

 * src/xdisp.c (get_cursor_offset_for_mouse_face): Don't calculate
offsets for the glyph the cursor is on, and move some logic to
get_glyph_pixel_width_delta_for_mouse_face.
(fill_composite_glyph_string)
(fill_gstring_glyph_string)
(fill_glyphless_glyph_string)
(fill_glyph_string)
(fill_image_glyph_string)
(fill_xwidget_glyph_string)
(fill_stretch_glyph_string): Set s->face to mouse face whenever
appropriate.
(get_glyph_pixel_width_delta_for_mouse_face): New function.
(set_glyph_string_background_width): Update background width and
s->width to take into account differing :box properties of the mouse
face, when producing strings for the cursor.
(erase_phys_cursor): Redraw mouse face when erasing a cursor on top of
the mouse face.
 * src/xterm.c (x_set_mouse_face_gc): Stop setting s->face when under
mouse face because redisplay now does that for us.
 * src/w32term.c (w32_set_mouse_face_gc): Likewise.
---
 src/w32term.c |  16 ----
 src/xdisp.c   | 244 +++++++++++++++++++++++++++++++++++++-------------
 src/xterm.c   |  16 ----
 3 files changed, 180 insertions(+), 96 deletions(-)

diff --git a/src/w32term.c b/src/w32term.c
index 9cf250cd73..07a5cd3564 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -954,22 +954,6 @@ w32_set_cursor_gc (struct glyph_string *s)
 static void
 w32_set_mouse_face_gc (struct glyph_string *s)
 {
-  int face_id;
-  struct face *face;
-
-  /* What face has to be used last for the mouse face?  */
-  face_id = MOUSE_HL_INFO (s->f)->mouse_face_face_id;
-  face = FACE_FROM_ID_OR_NULL (s->f, face_id);
-  if (face == NULL)
-    face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
-
-  if (s->first_glyph->type == CHAR_GLYPH)
-    face_id = FACE_FOR_CHAR (s->f, face, s->first_glyph->u.ch, -1, Qnil);
-  else
-    face_id = FACE_FOR_CHAR (s->f, face, 0, -1, Qnil);
-  s->face = FACE_FROM_ID (s->f, face_id);
-  prepare_face_for_display (s->f, s->face);
-
   /* If font in this face is same as S->font, use it.  */
   if (s->font == s->face->font)
     s->gc = s->face->gc;
diff --git a/src/xdisp.c b/src/xdisp.c
index f4ea7de190..7fb6cb8bfd 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -1179,6 +1179,11 @@ #define face_after_it_pos(IT)  face_before_or_after_it_pos (IT, false)
 static Lisp_Object get_it_property (struct it *, Lisp_Object);
 static Lisp_Object calc_line_height_property (struct it *, Lisp_Object,
 					      struct font *, int, bool);
+static int get_glyph_pixel_width_delta_for_mouse_face (struct glyph *,
+						       struct glyph_row *,
+						       struct window *,
+						       struct face *,
+						       struct face *);
 static void get_cursor_offset_for_mouse_face (struct window *w,
 					      struct glyph_row *row,
 					      int *offset);
@@ -28125,6 +28130,20 @@ fill_composite_glyph_string (struct glyph_string *s, struct face *base_face,
       s->font = s->face->font;
     }
 
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      int c = COMPOSITION_GLYPH (s->cmp, 0);
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+
+      s->face = FACE_FROM_ID (s->f, FACE_FOR_CHAR (s->f, s->face, c, -1, Qnil));
+      prepare_face_for_display (s->f, s->face);
+    }
+
   /* All glyph strings for the same composition has the same width,
      i.e. the width set for the first component of the composition.  */
   s->width = s->first_glyph->pixel_width;
@@ -28161,7 +28180,17 @@ fill_gstring_glyph_string (struct glyph_string *s, int face_id,
   s->cmp_id = glyph->u.cmp.id;
   s->cmp_from = glyph->slice.cmp.from;
   s->cmp_to = glyph->slice.cmp.to + 1;
-  s->face = FACE_FROM_ID (s->f, face_id);
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+      prepare_face_for_display (s->f, s->face);
+    }
+  else
+    s->face = FACE_FROM_ID (s->f, face_id);
   lgstring = composition_gstring_from_id (s->cmp_id);
   s->font = XFONT_OBJECT (LGSTRING_FONT (lgstring));
   /* The width of a composition glyph string is the sum of the
@@ -28217,6 +28246,15 @@ fill_glyphless_glyph_string (struct glyph_string *s, int face_id,
   voffset = glyph->voffset;
   s->face = FACE_FROM_ID (s->f, face_id);
   s->font = s->face->font ? s->face->font : FRAME_FONT (s->f);
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+      prepare_face_for_display (s->f, s->face);
+    }
   s->nchars = 1;
   s->width = glyph->pixel_width;
   glyph++;
@@ -28280,6 +28318,19 @@ fill_glyph_string (struct glyph_string *s, int face_id,
 
   s->font = s->face->font;
 
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+      s->face
+        = FACE_FROM_ID (s->f, FACE_FOR_CHAR (s->f, s->face,
+					     s->first_glyph->u.ch, -1, Qnil));
+      prepare_face_for_display (s->f, s->face);
+    }
+
   /* If the specified font could not be loaded, use the frame's font,
      but record the fact that we couldn't load it in
      S->font_not_found_p so that we can draw rectangles for the
@@ -28309,6 +28360,15 @@ fill_image_glyph_string (struct glyph_string *s)
   s->slice = s->first_glyph->slice.img;
   s->face = FACE_FROM_ID (s->f, s->first_glyph->face_id);
   s->font = s->face->font;
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+      prepare_face_for_display (s->f, s->face);
+    }
   s->width = s->first_glyph->pixel_width;
 
   /* Adjust base line for subscript/superscript text.  */
@@ -28323,6 +28383,15 @@ fill_xwidget_glyph_string (struct glyph_string *s)
   eassert (s->first_glyph->type == XWIDGET_GLYPH);
   s->face = FACE_FROM_ID (s->f, s->first_glyph->face_id);
   s->font = s->face->font;
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+      prepare_face_for_display (s->f, s->face);
+    }
   s->width = s->first_glyph->pixel_width;
   s->ybase += s->first_glyph->voffset;
   s->xwidget = s->first_glyph->u.xwidget;
@@ -28348,6 +28417,15 @@ fill_stretch_glyph_string (struct glyph_string *s, int start, int end)
   face_id = glyph->face_id;
   s->face = FACE_FROM_ID (s->f, face_id);
   s->font = s->face->font;
+  if (s->hl == DRAW_MOUSE_FACE
+      || (s->hl == DRAW_CURSOR && cursor_in_mouse_face_p (s->w)))
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (s->f);
+      s->face = FACE_FROM_ID_OR_NULL (s->f, hlinfo->mouse_face_face_id);
+      if (!s->face)
+	s->face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
+      prepare_face_for_display (s->f, s->face);
+    }
   s->width = glyph->pixel_width;
   s->nchars = 1;
   voffset = glyph->voffset;
@@ -28595,7 +28673,12 @@ right_overwriting (struct glyph_string *s)
 
 /* Set background width of glyph string S.  START is the index of the
    first glyph following S.  LAST_X is the right-most x-position + 1
-   in the drawing area.  */
+   in the drawing area.
+
+   If S's hl is DRAW_CURSOR, S->f is a window system frame, and the
+   cursor in S's window is currently under mouse face, s->width will
+   also be updated to take into account differing :box properties
+   between the original face and the mouse face. */
 
 static void
 set_glyph_string_background_width (struct glyph_string *s, int start, int last_x)
@@ -28617,7 +28700,28 @@ set_glyph_string_background_width (struct glyph_string *s, int start, int last_x
   if (s->extends_to_end_of_line_p)
     s->background_width = last_x - s->x + 1;
   else
-    s->background_width = s->width;
+    {
+      s->background_width = s->width;
+#ifdef HAVE_WINDOW_SYSTEM
+      if (FRAME_WINDOW_P (s->f)
+	  && s->hl == DRAW_CURSOR
+	  && cursor_in_mouse_face_p (s->w))
+	{
+	  /* We will have to adjust the background width of the string
+	     in this situation, because the glyph's pixel_width might
+	     be inconsistent with the box of the mouse face, which
+	     leads to an ugly over-wide cursor. */
+
+	  struct glyph *g = s->first_glyph;
+	  struct face *regular_face = FACE_FROM_ID (s->f, g->face_id);
+	  s->background_width +=
+	    get_glyph_pixel_width_delta_for_mouse_face (g, s->row, s->w,
+							regular_face, s->face);
+	  /* s->width is probably worth adjusting here as well. */
+	  s->width = s->background_width;
+        }
+#endif
+    }
 }
 
 
@@ -31752,10 +31856,6 @@ erase_phys_cursor (struct window *w)
   Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
   int hpos = w->phys_cursor.hpos;
   int vpos = w->phys_cursor.vpos;
-#ifdef HAVE_WINDOW_SYSTEM
-  int mouse_delta;
-  int phys_x = w->phys_cursor.x;
-#endif
   bool mouse_face_here_p = false;
   struct glyph_matrix *active_glyphs = w->current_matrix;
   struct glyph_row *cursor_row;
@@ -31826,13 +31926,16 @@ erase_phys_cursor (struct window *w)
     mouse_face_here_p = true;
 
 #ifdef HAVE_WINDOW_SYSTEM
-  /* Adjust the physical cursor's X coordinate if needed.  The problem
-     solved by the code below is outlined in the comment above
-     'get_cursor_offset_for_mouse_face'.  */
-  if (mouse_face_here_p)
+  /* Since erasing the phys cursor will probably lead to corruption of
+     the mouse face display if the glyph's pixel_width is not kept up
+     to date with the :box property of the mouse face, just redraw the
+     mouse face. */
+  if (FRAME_WINDOW_P (WINDOW_XFRAME (w)) && mouse_face_here_p)
     {
-      get_cursor_offset_for_mouse_face (w, cursor_row, &mouse_delta);
-      w->phys_cursor.x += mouse_delta;
+      w->phys_cursor_on_p = false;
+      w->phys_cursor_type = NO_CURSOR;
+      show_mouse_face (MOUSE_HL_INFO (WINDOW_XFRAME (w)), DRAW_MOUSE_FACE);
+      return;
     }
 #endif
 
@@ -31871,10 +31974,6 @@ erase_phys_cursor (struct window *w)
   draw_phys_cursor_glyph (w, cursor_row, hl);
 
  mark_cursor_off:
-#ifdef HAVE_WINDOW_SYSTEM
-  /* Restore the original cursor position.  */
-  w->phys_cursor.x = phys_x;
-#endif
   w->phys_cursor_on_p = false;
   w->phys_cursor_type = NO_CURSOR;
 }
@@ -35993,6 +36092,65 @@ cancel_hourglass (void)
     }
 }
 
+/* Return a delta that must be applied to g->pixel_width in order to
+   obtain the correct pixel_width of G when drawn under MOUSE_FACE.
+   ORIGINAL_FACE is the face G was originally drawn in, and MOUSE_FACE
+   is the face it will be drawn in now.  ROW should be the row G is
+   located in.  W should be the window G is located in.  */
+static int
+get_glyph_pixel_width_delta_for_mouse_face (struct glyph *g,
+					    struct glyph_row *row,
+					    struct window *w,
+					    struct face *original_face,
+					    struct face *mouse_face)
+{
+  int sum = 0;
+
+  bool do_left_box_p = g->left_box_line_p;
+  bool do_right_box_p = g->right_box_line_p;
+
+  /* This is required because we test some parameters
+     of the image slice before applying the box in
+     produce_image_glyph. */
+
+  if (g->type == IMAGE_GLYPH)
+    {
+      if (!row->reversed_p)
+	{
+	  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 == 0;
+	  do_right_box_p = g->right_box_line_p &&
+	    g->slice.img.x + g->slice.img.width == img->width;
+	}
+      else
+	{
+	  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;
+	}
+    }
+
+  /* If the glyph has a left box line, subtract it from the offset.  */
+  if (do_left_box_p)
+    sum -= max (0, original_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)
+        sum -= max (0, original_face->box_vertical_line_width);
+  /* Now add the line widths from the new face.  */
+  if (g->left_box_line_p)
+    sum += max (0, mouse_face->box_vertical_line_width);
+  if (g->right_box_line_p)
+    sum += max (0, mouse_face->box_vertical_line_width);
+
+  return sum;
+}
+
 /* Get the offset due to mouse-highlight to apply before drawing
    phys_cursor, and return it in OFFSET.  ROW should be the row that
    is under mouse face and contains the phys cursor.
@@ -36036,57 +36194,15 @@ get_cursor_offset_for_mouse_face (struct window *w, struct glyph_row *row,
 	start = &row->glyphs[TEXT_AREA][row->used[TEXT_AREA] - 1];
     }
 
-  /* Calculate the offset to correct phys_cursor x if we are
+  /* Calculate the offset by which to correct phys_cursor x if we are
      drawing the cursor inside mouse-face highlighted text.  */
 
-  for (; row->reversed_p ? start >= end : start <= end;
+  for (; row->reversed_p ? start > end : start < end;
        row->reversed_p ? --start : ++start)
     {
-      struct glyph *g = start;
-      struct face *mouse = mouse_face;
-      struct face *regular_face = FACE_FROM_ID (f, g->face_id);
-
-      bool do_left_box_p = g->left_box_line_p;
-      bool do_right_box_p = g->right_box_line_p;
-
-      /* This is required because we test some parameters
-	 of the image slice before applying the box in
-	 produce_image_glyph. */
-
-      if (g->type == IMAGE_GLYPH)
-	{
-	  if (!row->reversed_p)
-	    {
-	      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 == 0;
-	      do_right_box_p = g->right_box_line_p &&
-		g->slice.img.x + g->slice.img.width == img->width;
-	    }
-	  else
-	    {
-	      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;
-	    }
-	}
-
-      /* If the glyph has a left box line, subtract it from the offset.  */
-      if (do_left_box_p)
-        sum -= 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)
-        sum -= max (0, regular_face->box_vertical_line_width);
-      /* Now add the line widths from the new face.  */
-      if (g->left_box_line_p)
-        sum += max (0, mouse->box_vertical_line_width);
-      if (g->right_box_line_p)
-        sum += max (0, mouse->box_vertical_line_width);
+      sum += get_glyph_pixel_width_delta_for_mouse_face (start, row, w,
+							 FACE_FROM_ID (f, start->face_id),
+							 mouse_face);
     }
 
   if (row->reversed_p)
diff --git a/src/xterm.c b/src/xterm.c
index 2b365929a1..0435ad341c 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -1573,22 +1573,6 @@ x_set_cursor_gc (struct glyph_string *s)
 static void
 x_set_mouse_face_gc (struct glyph_string *s)
 {
-  int face_id;
-  struct face *face;
-
-  /* What face has to be used last for the mouse face?  */
-  face_id = MOUSE_HL_INFO (s->f)->mouse_face_face_id;
-  face = FACE_FROM_ID_OR_NULL (s->f, face_id);
-  if (face == NULL)
-    face = FACE_FROM_ID (s->f, MOUSE_FACE_ID);
-
-  if (s->first_glyph->type == CHAR_GLYPH)
-    face_id = FACE_FOR_CHAR (s->f, face, s->first_glyph->u.ch, -1, Qnil);
-  else
-    face_id = FACE_FOR_CHAR (s->f, face, 0, -1, Qnil);
-  s->face = FACE_FROM_ID (s->f, face_id);
-  prepare_face_for_display (s->f, s->face);
-
   if (s->font == s->face->font)
     s->gc = s->face->gc;
   else
-- 
2.31.1

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: 50660@debbugs.gnu.org
> Date: Sun, 17 Oct 2021 08:32:46 +0800
> 
> >   1) After producing the mouse face, you should call
> >      prepare_face_for_display.
> >
> >   2) The code you add in set_glyph_string_background_width is almost
> >      identical to what we already have inside the loop in
> >      get_cursor_offset_for_mouse_face, and I wonder whether we could
> >      have a function to be called from these two places.
> >
> > Other than that, I think this is fine, thanks.
> 
> Does this resolve your concerns?  Thanks.

Thanks, installed with some minor adaptations (the main of which was
to find a shorter name for the new function).

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]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, installed with some minor adaptations (the main of which was
> to find a shorter name for the new function).

It works here, LGTM.  Thanks!