GNU bug report logs - #77336
[PATCH] Fix mouse highlighting for compact mode lines

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 77336 in the body.
You can then email your comments to 77336 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pengji Zhang <me <at> pengjiz.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix mouse highlighting for compact mode lines
Date: Fri, 28 Mar 2025 18:55:30 +0800

[Message part 1 (text/plain, inline)]

Hello,

I found that with 'mode-line-compact' being non-nil, mouse highlighting
would not work properly. To reproduce:

  - emacs -Q
  - (setq mode-line-compact t) or (setq mode-line-compact 'long)
  - Hover mouse cursor over the major mode part on the mode line

Now the minor mode lighters are also highlighted. That is presumably
because when 'mode-line-compact' is non-nil, the whole mode line string
is displayed at once. So the glyphs produced all have the same 'object'
field. That confuses the computation of boundaries for mouse
highlighting.

In this patch we instead split the mode line string into elements and
display them one by one.

Thanks!

[0001-Fix-mouse-highlighting-for-compact-mode-lines.patch (text/x-patch, inline)]

From 5c7538aceb7e80ea44be7d74890d36e88d36169c Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me <at> pengjiz.com>
Date: Sun, 23 Mar 2025 20:19:09 +0800
Subject: [PATCH] Fix mouse highlighting for compact mode lines

When 'mode-line-compact' is non-nil, the mode line string is
displayed as a whole.  That confuses the computation of ranges
of mouse highlighting on the mode line because all the glyphs
have the same Lisp object source.  As such, in this commit we
instead split the mode line string by sources, and display those
elements one by one, so the boundaries of each element could be
correctly detected for the purpose of mouse highlighting.

* src/xdisp.c (display_mode_line): Display mode line elements
one by one when 'mode-line-compact' is non-nil.
(display_mode_element): Record source of the stored string via a
text property.
(syms_of_xdisp): New symbol for the text property.
---
 src/xdisp.c | 132 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 93 insertions(+), 39 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index f2b158f00e3..193f9767c6f 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -27745,57 +27745,102 @@ display_mode_line (struct window *w, enum face_id face_id, Lisp_Object format)
 	{
 	  /* The window is wide enough; just display the mode line we
 	     just computed. */
-	  display_string (NULL, mode_string, Qnil,
-			  0, 0, &it, 0, 0, 0,
-			  STRING_MULTIBYTE (mode_string));
+	  Lisp_Object start = make_fixnum (0), end;
+	  Lisp_Object elt;
+	  AUTO_LIST2 (props, Qmode_line_element, Qnil);
+
+	  /* Display the mode line string one element by one element.
+	     This is to make the ranges of mouse highlighting
+	     correct. */
+	  do
+	    {
+	      end = Fnext_single_property_change (start, Qmode_line_element,
+						  mode_string, Qnil);
+	      elt = Fsubstring (mode_string, start, end);
+	      Fremove_text_properties (make_fixnum (0),
+				       make_fixnum (SCHARS (elt)),
+				       props, elt);
+	      display_string (NULL, elt, Qnil, 0, 0, &it, 0, 0, 0,
+			      STRING_MULTIBYTE (elt));
+	      start = end;
+	    }
+	  while (!NILP (end));
 	}
       else
 	{
 	  /* Compress the mode line. */
-	  ptrdiff_t i = 0, i_byte = 0, start = 0;
+	  ptrdiff_t i = 0, i_byte = 0;
 	  int prev = 0;
+	  Lisp_Object start = make_fixnum (0), end;
+	  Lisp_Object elt = empty_unibyte_string;
+	  AUTO_LIST2 (props, Qmode_line_element, Qnil);
 
-	  while (i < SCHARS (mode_string))
+	  /* Display the mode line string one element by one element.
+	     This is to make the ranges of mouse highlighting
+	     correct. */
+	  do
 	    {
-	      int c = fetch_string_char_advance (mode_string, &i, &i_byte);
-	      if (c == ' ' && prev == ' ')
+	      end = Fnext_single_property_change (start,
+						  Qmode_line_element,
+						  mode_string,
+						  make_fixnum (SCHARS (mode_string)));
+	      while (i < XFIXNUM (end))
 		{
-		  Lisp_Object prev_pos = make_fixnum (i - 1);
-
-		  /* SPC characters with 'display' properties are not
-                     really "empty", since they have non-trivial visual
-                     effects on the mode line.  */
-		  if (NILP (Fget_text_property (prev_pos, Qdisplay,
-						mode_string)))
+		  int c = fetch_string_char_advance (mode_string, &i, &i_byte);
+		  if (c == ' ' && prev == ' ')
 		    {
-		      display_string (NULL,
-				      Fsubstring (mode_string,
-						  make_fixnum (start),
-						  prev_pos),
-				      Qnil, 0, 0, &it, 0, 0, 0,
-				      STRING_MULTIBYTE (mode_string));
-		      /* Skip past the rest of the space characters. */
-		      while (c == ' ' && i < SCHARS (mode_string)
-			     && NILP (Fget_text_property (make_fixnum (i),
-							  Qdisplay,
-							  mode_string)))
+		      Lisp_Object prev_pos = make_fixnum (i - 1);
+		      Lisp_Object display = Fget_text_property (prev_pos,
+								Qdisplay,
+								mode_string);
+
+		      /* SPC characters with 'display' properties are not
+			 really "empty", since they have non-trivial visual
+			 effects on the mode line.  */
+		      if (NILP (display))
 			{
-			  c = fetch_string_char_advance (mode_string,
-							 &i, &i_byte);
+			  elt = concat2 (elt, Fsubstring (mode_string,
+							  start,
+							  prev_pos));
+
+			  /* Skip past the rest of the space characters. */
+			  Lisp_Object display = Fget_text_property (make_fixnum (i),
+								    Qdisplay,
+								    mode_string);
+			  while (c == ' ' && i < XFIXNUM (end)
+				 && NILP (display))
+			    {
+			      c = fetch_string_char_advance (mode_string,
+							     &i, &i_byte);
+			      display = Fget_text_property (make_fixnum (i),
+							    Qdisplay,
+							    mode_string);
+			    }
+
+			  /* Skip the final space no matter how the loop
+			     above ends. */
+			  if (c == ' ' && NILP (display))
+			    start = end;
+			  else
+			    start = make_fixnum (i - 1);
 			}
-		      start = i - 1;
 		    }
+		  prev = c;
 		}
-	      prev = c;
-	    }
 
-	  /* Display the final bit. */
-	  if (start < i)
-	    display_string (NULL,
-			    Fsubstring (mode_string, make_fixnum (start),
-					make_fixnum (i)),
-			    Qnil, 0, 0, &it, 0, 0, 0,
-			    STRING_MULTIBYTE (mode_string));
+	      /* Append the final bit. */
+	      if (XFIXNUM (start) < XFIXNUM (end))
+		elt = concat2 (elt, Fsubstring (mode_string, start, end));
+
+	      Fremove_text_properties (make_fixnum (0),
+				       make_fixnum (SCHARS (elt)),
+				       props, elt);
+	      display_string (NULL, elt, Qnil, 0, 0, &it, 0, 0, 0,
+			      STRING_MULTIBYTE (elt));
+	      elt = empty_unibyte_string;
+	      start = end;
+	    }
+	  while (XFIXNUM (end) < SCHARS (mode_string));
 	}
     }
   pop_kboard ();
@@ -28014,7 +28059,10 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 		n += store_mode_line_noprop (SSDATA (elt), -1, prec);
 		break;
 	      case MODE_LINE_STRING:
-		n += store_mode_line_string (NULL, elt, true, 0, prec, Qnil);
+		{
+		  AUTO_LIST2 (src, Qmode_line_element, elt);
+		  n += store_mode_line_string (NULL, elt, true, 0, prec, src);
+		}
 		break;
 	      case MODE_LINE_DISPLAY:
 		n += display_string (NULL, elt, Qnil, 0, 0, it,
@@ -28067,8 +28115,9 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 		      Lisp_Object mode_string
 			= Fsubstring (elt, make_fixnum (charpos),
 				      make_fixnum (endpos));
+		      AUTO_LIST2 (src, Qmode_line_element, elt);
 		      n += store_mode_line_string (NULL, mode_string, false,
-						   0, 0, Qnil);
+						   0, 0, src);
 		    }
 		    break;
 		  case MODE_LINE_DISPLAY:
@@ -28148,6 +28197,7 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 			{
 			  Lisp_Object tem = build_string (spec);
 			  props = Ftext_properties_at (make_fixnum (charpos), elt);
+			  props = plist_put (props, Qmode_line_element, elt);
 			  /* Should only keep face property in props */
 			  n += store_mode_line_string (NULL, tem, false,
 						       field, prec, props);
@@ -28363,6 +28413,9 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 	  n += store_mode_line_noprop ("", field_width - n, 0);
 	  break;
 	case MODE_LINE_STRING:
+	  /* NOTE: Padding implicitly has 'mode-line-element' property
+	     to be nil.  Normally padding spaces are between two real
+	     elements, so this should be good. */
 	  n += store_mode_line_string ("", Qnil, false, field_width - n, 0,
 				       Qnil);
 	  break;
@@ -37657,6 +37710,7 @@ syms_of_xdisp (void)
   DEFSYM (Qfontification_functions, "fontification-functions");
   DEFSYM (Qlong_line_optimizations_in_fontification_functions,
 	  "long-line-optimizations-in-fontification-functions");
+  DEFSYM (Qmode_line_element, "mode-line-element");
 
   /* Name of the symbol which disables Lisp evaluation in 'display'
      properties.  This is used by enriched.el.  */
-- 
2.49.0

Message #8 received at 77336 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pengji Zhang <me <at> pengjiz.com>
Cc: 77336 <at> debbugs.gnu.org
Subject: Re: bug#77336: [PATCH] Fix mouse highlighting for compact mode lines
Date: Sat, 29 Mar 2025 12:21:01 +0300

> From: Pengji Zhang <me <at> pengjiz.com>
> Date: Fri, 28 Mar 2025 18:55:30 +0800
> 
> @@ -28014,7 +28059,10 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
>  		n += store_mode_line_noprop (SSDATA (elt), -1, prec);
>  		break;
>  	      case MODE_LINE_STRING:
> -		n += store_mode_line_string (NULL, elt, true, 0, prec, Qnil);
> +		{
> +		  AUTO_LIST2 (src, Qmode_line_element, elt);
> +		  n += store_mode_line_string (NULL, elt, true, 0, prec, src);
> +		}
>  		break;
>  	      case MODE_LINE_DISPLAY:
>  		n += display_string (NULL, elt, Qnil, 0, 0, it,
> @@ -28067,8 +28115,9 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
>  		      Lisp_Object mode_string
>  			= Fsubstring (elt, make_fixnum (charpos),
>  				      make_fixnum (endpos));
> +		      AUTO_LIST2 (src, Qmode_line_element, elt);
>  		      n += store_mode_line_string (NULL, mode_string, false,
> -						   0, 0, Qnil);
> +						   0, 0, src);
>  		    }
>  		    break;
>  		  case MODE_LINE_DISPLAY:
> @@ -28148,6 +28197,7 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
>  			{
>  			  Lisp_Object tem = build_string (spec);
>  			  props = Ftext_properties_at (make_fixnum (charpos), elt);
> +			  props = plist_put (props, Qmode_line_element, elt);
>  			  /* Should only keep face property in props */
>  			  n += store_mode_line_string (NULL, tem, false,
>  						       field, prec, props);

Thanks, but why do we need to make the value of this mode-line-element
property be the element itself?  Why not give it some simple value,
like the ordinal number of the element (which should be incremented
whenever we process another element)?  So we could call this property
mode-line-elt-number and assign the elements values 1, 2, 3, etc.

I'm concerned that having the elements themselves be values will
prevent them from being GC'd, which might increase the memory
pressure.  By contrast, fixnums are immediate values that don't need
to be GC'ed, and the element itself will not have any references after
the mode line is processed.

Message #11 received at 77336 <at> debbugs.gnu.org (full text, mbox):

From: Pengji Zhang <me <at> pengjiz.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77336 <at> debbugs.gnu.org
Subject: Re: bug#77336: [PATCH] Fix mouse highlighting for compact mode lines
Date: Sun, 30 Mar 2025 19:44:59 +0800

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

> Thanks, but why do we need to make the value of this mode-line-element
> property be the element itself?  Why not give it some simple value,
> like the ordinal number of the element (which should be incremented
> whenever we process another element)?  So we could call this property
> mode-line-elt-number and assign the elements values 1, 2, 3, etc.

Is casting the 'Lisp_Object' to a number acceptable here? Please have a
look at the updated patch.

I tried an incremental counter but gave up. Because
'display_mode_element' could be called recursively, we may have to
change its signature or introduce a new global variable. That feels a
bit less clean to me.

> I'm concerned that having the elements themselves be values will
> prevent them from being GC'd, which might increase the memory
> pressure.  By contrast, fixnums are immediate values that don't need
> to be GC'ed, and the element itself will not have any references after
> the mode line is processed.

I thought all objects would be GC'd. Since fixnums are not, I guess now
we do not need to remove the text property before displaying the string?
Please let me know what you think and I will update the patch if needed.

[0001-Fix-mouse-highlighting-for-compact-mode-lines.patch (text/x-patch, inline)]

From 73bc97111ace409a5de27930d0dadcd18490fa5e Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me <at> pengjiz.com>
Date: Sun, 30 Mar 2025 19:24:10 +0800
Subject: [PATCH] Fix mouse highlighting for compact mode lines (bug#77336)

When 'mode-line-compact' is non-nil, the mode line string is
displayed as a whole.  That confuses the computation of ranges
of mouse highlighting on the mode line because all the glyphs
have the same Lisp object source.  As such, in this commit we
instead split the mode line string by sources, and display those
elements one by one, so the boundaries of each element could be
correctly detected for the purpose of mouse highlighting.

* src/xdisp.c (display_mode_line): Display mode line elements
one by one when 'mode-line-compact' is non-nil.
(display_mode_element): Record source of the stored string via a
text property.
(syms_of_xdisp): New symbol for the text property.
---
 src/xdisp.c | 132 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 93 insertions(+), 39 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 2c676c09827..d712e3d02ae 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -27754,57 +27754,102 @@ display_mode_line (struct window *w, enum face_id face_id, Lisp_Object format)
 	{
 	  /* The window is wide enough; just display the mode line we
 	     just computed. */
-	  display_string (NULL, mode_string, Qnil,
-			  0, 0, &it, 0, 0, 0,
-			  STRING_MULTIBYTE (mode_string));
+	  Lisp_Object start = make_fixnum (0), end;
+	  Lisp_Object elt;
+	  AUTO_LIST2 (props, Qmode_line_element, Qnil);
+
+	  /* Display the mode line string one element by one element.
+	     This is to make the ranges of mouse highlighting
+	     correct. */
+	  do
+	    {
+	      end = Fnext_single_property_change (start, Qmode_line_element,
+						  mode_string, Qnil);
+	      elt = Fsubstring (mode_string, start, end);
+	      Fremove_text_properties (make_fixnum (0),
+				       make_fixnum (SCHARS (elt)),
+				       props, elt);
+	      display_string (NULL, elt, Qnil, 0, 0, &it, 0, 0, 0,
+			      STRING_MULTIBYTE (elt));
+	      start = end;
+	    }
+	  while (!NILP (end));
 	}
       else
 	{
 	  /* Compress the mode line. */
-	  ptrdiff_t i = 0, i_byte = 0, start = 0;
+	  ptrdiff_t i = 0, i_byte = 0;
 	  int prev = 0;
+	  Lisp_Object start = make_fixnum (0), end;
+	  Lisp_Object elt = empty_unibyte_string;
+	  AUTO_LIST2 (props, Qmode_line_element, Qnil);
 
-	  while (i < SCHARS (mode_string))
+	  /* Display the mode line string one element by one element.
+	     This is to make the ranges of mouse highlighting
+	     correct. */
+	  do
 	    {
-	      int c = fetch_string_char_advance (mode_string, &i, &i_byte);
-	      if (c == ' ' && prev == ' ')
+	      end = Fnext_single_property_change (start,
+						  Qmode_line_element,
+						  mode_string,
+						  make_fixnum (SCHARS (mode_string)));
+	      while (i < XFIXNUM (end))
 		{
-		  Lisp_Object prev_pos = make_fixnum (i - 1);
-
-		  /* SPC characters with 'display' properties are not
-                     really "empty", since they have non-trivial visual
-                     effects on the mode line.  */
-		  if (NILP (Fget_text_property (prev_pos, Qdisplay,
-						mode_string)))
+		  int c = fetch_string_char_advance (mode_string, &i, &i_byte);
+		  if (c == ' ' && prev == ' ')
 		    {
-		      display_string (NULL,
-				      Fsubstring (mode_string,
-						  make_fixnum (start),
-						  prev_pos),
-				      Qnil, 0, 0, &it, 0, 0, 0,
-				      STRING_MULTIBYTE (mode_string));
-		      /* Skip past the rest of the space characters. */
-		      while (c == ' ' && i < SCHARS (mode_string)
-			     && NILP (Fget_text_property (make_fixnum (i),
-							  Qdisplay,
-							  mode_string)))
+		      Lisp_Object prev_pos = make_fixnum (i - 1);
+		      Lisp_Object display = Fget_text_property (prev_pos,
+								Qdisplay,
+								mode_string);
+
+		      /* SPC characters with 'display' properties are not
+			 really "empty", since they have non-trivial visual
+			 effects on the mode line.  */
+		      if (NILP (display))
 			{
-			  c = fetch_string_char_advance (mode_string,
-							 &i, &i_byte);
+			  elt = concat2 (elt, Fsubstring (mode_string,
+							  start,
+							  prev_pos));
+
+			  /* Skip past the rest of the space characters. */
+			  Lisp_Object display = Fget_text_property (make_fixnum (i),
+								    Qdisplay,
+								    mode_string);
+			  while (c == ' ' && i < XFIXNUM (end)
+				 && NILP (display))
+			    {
+			      c = fetch_string_char_advance (mode_string,
+							     &i, &i_byte);
+			      display = Fget_text_property (make_fixnum (i),
+							    Qdisplay,
+							    mode_string);
+			    }
+
+			  /* Skip the final space no matter how the loop
+			     above ends. */
+			  if (c == ' ' && NILP (display))
+			    start = end;
+			  else
+			    start = make_fixnum (i - 1);
 			}
-		      start = i - 1;
 		    }
+		  prev = c;
 		}
-	      prev = c;
-	    }
 
-	  /* Display the final bit. */
-	  if (start < i)
-	    display_string (NULL,
-			    Fsubstring (mode_string, make_fixnum (start),
-					make_fixnum (i)),
-			    Qnil, 0, 0, &it, 0, 0, 0,
-			    STRING_MULTIBYTE (mode_string));
+	      /* Append the final bit. */
+	      if (XFIXNUM (start) < XFIXNUM (end))
+		elt = concat2 (elt, Fsubstring (mode_string, start, end));
+
+	      Fremove_text_properties (make_fixnum (0),
+				       make_fixnum (SCHARS (elt)),
+				       props, elt);
+	      display_string (NULL, elt, Qnil, 0, 0, &it, 0, 0, 0,
+			      STRING_MULTIBYTE (elt));
+	      elt = empty_unibyte_string;
+	      start = end;
+	    }
+	  while (XFIXNUM (end) < SCHARS (mode_string));
 	}
     }
   pop_kboard ();
@@ -28023,7 +28068,10 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 		n += store_mode_line_noprop (SSDATA (elt), -1, prec);
 		break;
 	      case MODE_LINE_STRING:
-		n += store_mode_line_string (NULL, elt, true, 0, prec, Qnil);
+		{
+		  AUTO_LIST2 (src, Qmode_line_element, make_fixnum (XLI (elt)));
+		  n += store_mode_line_string (NULL, elt, true, 0, prec, src);
+		}
 		break;
 	      case MODE_LINE_DISPLAY:
 		n += display_string (NULL, elt, Qnil, 0, 0, it,
@@ -28076,8 +28124,9 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 		      Lisp_Object mode_string
 			= Fsubstring (elt, make_fixnum (charpos),
 				      make_fixnum (endpos));
+		      AUTO_LIST2 (src, Qmode_line_element, make_fixnum (XLI (elt)));
 		      n += store_mode_line_string (NULL, mode_string, false,
-						   0, 0, Qnil);
+						   0, 0, src);
 		    }
 		    break;
 		  case MODE_LINE_DISPLAY:
@@ -28157,6 +28206,7 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 			{
 			  Lisp_Object tem = build_string (spec);
 			  props = Ftext_properties_at (make_fixnum (charpos), elt);
+			  props = plist_put (props, Qmode_line_element, make_fixnum (XLI (elt)));
 			  /* Should only keep face property in props */
 			  n += store_mode_line_string (NULL, tem, false,
 						       field, prec, props);
@@ -28372,6 +28422,9 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 	  n += store_mode_line_noprop ("", field_width - n, 0);
 	  break;
 	case MODE_LINE_STRING:
+	  /* NOTE: Padding implicitly has 'mode-line-element' property
+	     to be nil.  Normally padding spaces are between two real
+	     elements, so this should be good. */
 	  n += store_mode_line_string ("", Qnil, false, field_width - n, 0,
 				       Qnil);
 	  break;
@@ -37666,6 +37719,7 @@ syms_of_xdisp (void)
   DEFSYM (Qfontification_functions, "fontification-functions");
   DEFSYM (Qlong_line_optimizations_in_fontification_functions,
 	  "long-line-optimizations-in-fontification-functions");
+  DEFSYM (Qmode_line_element, "mode-line-element");
 
   /* Name of the symbol which disables Lisp evaluation in 'display'
      properties.  This is used by enriched.el.  */
-- 
2.49.0

Message #14 received at 77336 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pengji Zhang <me <at> pengjiz.com>
Cc: 77336 <at> debbugs.gnu.org
Subject: Re: bug#77336: [PATCH] Fix mouse highlighting for compact mode lines
Date: Sun, 30 Mar 2025 15:01:21 +0300

> From: Pengji Zhang <me <at> pengjiz.com>
> Cc: 77336 <at> debbugs.gnu.org
> Date: Sun, 30 Mar 2025 19:44:59 +0800
> 
> > Thanks, but why do we need to make the value of this mode-line-element
> > property be the element itself?  Why not give it some simple value,
> > like the ordinal number of the element (which should be incremented
> > whenever we process another element)?  So we could call this property
> > mode-line-elt-number and assign the elements values 1, 2, 3, etc.
> 
> Is casting the 'Lisp_Object' to a number acceptable here?

No, because Lisp_Object could be a struct (if Emacs was configured with
"--enable-check-lisp-object-type").

What I had in mind was to have a static global int variable named, say
elt_no, initialized to zero when display_mode_line is called, and then
give each segment of the mode line a value like this:

   AUTO_LIST2 (src, Qmode_line_element, make_fixnum (elt_no++));
   n += store_mode_line_string (NULL, elt, true, 0, prec, src);   

> I tried an incremental counter but gave up. Because
> 'display_mode_element' could be called recursively, we may have to
> change its signature or introduce a new global variable. That feels a
> bit less clean to me.

A global variable is what I had in mind.  I don't see why it would be
less clean.

> > I'm concerned that having the elements themselves be values will
> > prevent them from being GC'd, which might increase the memory
> > pressure.  By contrast, fixnums are immediate values that don't need
> > to be GC'ed, and the element itself will not have any references after
> > the mode line is processed.
> 
> I thought all objects would be GC'd. Since fixnums are not, I guess now
> we do not need to remove the text property before displaying the string?
> Please let me know what you think and I will update the patch if needed.

If you remove the text property because of GC, then indeed you don't
need to do that anymore with fixnums.

Message #17 received at 77336 <at> debbugs.gnu.org (full text, mbox):

From: Pengji Zhang <me <at> pengjiz.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77336 <at> debbugs.gnu.org
Subject: Re: bug#77336: [PATCH] Fix mouse highlighting for compact mode lines
Date: Mon, 31 Mar 2025 11:31:36 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Pengji Zhang <me <at> pengjiz.com>
>> Cc: 77336 <at> debbugs.gnu.org
>> Date: Sun, 30 Mar 2025 19:44:59 +0800
>> 
>> > Thanks, but why do we need to make the value of this mode-line-element
>> > property be the element itself?  Why not give it some simple value,
>> > like the ordinal number of the element (which should be incremented
>> > whenever we process another element)?  So we could call this property
>> > mode-line-elt-number and assign the elements values 1, 2, 3, etc.
>> 
>> Is casting the 'Lisp_Object' to a number acceptable here?
>
> No, because Lisp_Object could be a struct (if Emacs was configured with
> "--enable-check-lisp-object-type").

Sorry I did not make it clear. I meant 'make_fixnum(XLI(elt))'. I
suppose 'XLI' will handle that case correctly?

I am actually concerned that there could be a collision between two
different 'elt's, e.g., a symbol and a string, because 'make_fixnum'
alters the tag of 'elt'. I think it might be fine because 'elt' should
always be a 'Lisp_String' when this is called.

> What I had in mind was to have a static global int variable named, say
> elt_no, initialized to zero when display_mode_line is called, and then
> give each segment of the mode line a value like this:
>
>    AUTO_LIST2 (src, Qmode_line_element, make_fixnum (elt_no++));
>    n += store_mode_line_string (NULL, elt, true, 0, prec, src);   
>
>> I tried an incremental counter but gave up. Because
>> 'display_mode_element' could be called recursively, we may have to
>> change its signature or introduce a new global variable. That feels a
>> bit less clean to me.
>
> A global variable is what I had in mind.  I don't see why it would be
> less clean.

It is just that maintaining a global state is not as tasteful to me. If
'make_fixnum(XLI(elt))' is not good here, I am happy to go this way.

>> > I'm concerned that having the elements themselves be values will
>> > prevent them from being GC'd, which might increase the memory
>> > pressure.  By contrast, fixnums are immediate values that don't need
>> > to be GC'ed, and the element itself will not have any references after
>> > the mode line is processed.
>> 
>> I thought all objects would be GC'd. Since fixnums are not, I guess now
>> we do not need to remove the text property before displaying the string?
>> Please let me know what you think and I will update the patch if needed.
>
> If you remove the text property because of GC, then indeed you don't
> need to do that anymore with fixnums.

Thanks. I will update the patch accordingly.

Message #20 received at 77336 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pengji Zhang <me <at> pengjiz.com>
Cc: 77336 <at> debbugs.gnu.org
Subject: Re: bug#77336: [PATCH] Fix mouse highlighting for compact mode lines
Date: Mon, 31 Mar 2025 16:03:18 +0300

> From: Pengji Zhang <me <at> pengjiz.com>
> Cc: 77336 <at> debbugs.gnu.org
> Date: Mon, 31 Mar 2025 11:31:36 +0800
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> Is casting the 'Lisp_Object' to a number acceptable here?
> >
> > No, because Lisp_Object could be a struct (if Emacs was configured with
> > "--enable-check-lisp-object-type").
> 
> Sorry I did not make it clear. I meant 'make_fixnum(XLI(elt))'. I
> suppose 'XLI' will handle that case correctly?

I'd prefer not to do that unless there's no other reasonable solution.

> I am actually concerned that there could be a collision between two
> different 'elt's, e.g., a symbol and a string, because 'make_fixnum'
> alters the tag of 'elt'. I think it might be fine because 'elt' should
> always be a 'Lisp_String' when this is called.

Sorry, I don't understand: what do you mean by "alters the tag of
'elt'", and how could make_fixnum affect 'elt'?

> > A global variable is what I had in mind.  I don't see why it would be
> > less clean.
> 
> It is just that maintaining a global state is not as tasteful to me. If
> 'make_fixnum(XLI(elt))' is not good here, I am happy to go this way.

Yes, a global variable is cleaner than using XLI for this purpose.

Message #23 received at 77336 <at> debbugs.gnu.org (full text, mbox):

From: Pengji Zhang <me <at> pengjiz.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77336 <at> debbugs.gnu.org
Subject: Re: bug#77336: [PATCH] Fix mouse highlighting for compact mode lines
Date: Wed, 02 Apr 2025 21:02:19 +0800

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Pengji Zhang <me <at> pengjiz.com>
>> Cc: 77336 <at> debbugs.gnu.org
>> Date: Mon, 31 Mar 2025 11:31:36 +0800
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> >> Is casting the 'Lisp_Object' to a number acceptable here?
>> >
>> > No, because Lisp_Object could be a struct (if Emacs was configured with
>> > "--enable-check-lisp-object-type").
>> 
>> Sorry I did not make it clear. I meant 'make_fixnum(XLI(elt))'. I
>> suppose 'XLI' will handle that case correctly?
>
> I'd prefer not to do that unless there's no other reasonable solution.
>
>> I am actually concerned that there could be a collision between two
>> different 'elt's, e.g., a symbol and a string, because 'make_fixnum'
>> alters the tag of 'elt'. I think it might be fine because 'elt' should
>> always be a 'Lisp_String' when this is called.
>
> Sorry, I don't understand: what do you mean by "alters the tag of
> 'elt'", and how could make_fixnum affect 'elt'?

It does not affect 'elt'. I meant that 'make_fixnum' may change the
value of the number from the tagged pointer:

    INLINE Lisp_Object
    make_fixnum (EMACS_INT n)
    {
      ...
        {
          n &= INTMASK;
          n += (int0 << VALBITS);
        }
      return XIL (n);
    }

So if I understand it correctly, that means 'make_fixnum(XLI(x))' could
give the same result for, e.g., a fixnum and a string.

>> > A global variable is what I had in mind.  I don't see why it would be
>> > less clean.
>> 
>> It is just that maintaining a global state is not as tasteful to me. If
>> 'make_fixnum(XLI(elt))' is not good here, I am happy to go this way.
>
> Yes, a global variable is cleaner than using XLI for this purpose.

Thanks. Please see the attached patch.

[0001-Fix-mouse-highlighting-for-compact-mode-lines-bug-77.patch (text/x-patch, inline)]

From e6ca2165f2e49364f57c53c5c1472a127a7b76a4 Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me <at> pengjiz.com>
Date: Wed, 2 Apr 2025 20:52:30 +0800
Subject: [PATCH] Fix mouse highlighting for compact mode lines (bug#77336)

When 'mode-line-compact' is non-nil, the mode line string is
displayed as a whole.  That confuses the computation of ranges
of mouse highlighting on the mode line because all the glyphs
have the same Lisp object source.  As such, in this commit we
instead split the mode line string by sources, and display those
elements one by one, so the boundaries of each element could be
correctly detected for the purpose of mouse highlighting.

* src/xdisp.c (display_mode_line): Display mode line elements
one by one when 'mode-line-compact' is non-nil.
(display_mode_element): Record source element number of the
stored string via a text property.
(Fformat_mode_line): Initialize 'mode_line_elt_no' to 0.
(syms_of_xdisp): New symbol for the text property.
---
 src/xdisp.c | 145 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 106 insertions(+), 39 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 886f070753f..673b5c19c20 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -13674,6 +13674,9 @@ #define MODE_LINE_NOPROP_LEN(start) \
 /* List of strings making up the mode-line.  */
 static Lisp_Object mode_line_string_list;
 
+/* Element number for the stored mode line string. */
+static ptrdiff_t mode_line_elt_no;
+
 /* Base face property when building propertized mode line string.  */
 static Lisp_Object mode_line_string_face;
 static Lisp_Object mode_line_string_face_prop;
@@ -27754,57 +27757,94 @@ display_mode_line (struct window *w, enum face_id face_id, Lisp_Object format)
 	{
 	  /* The window is wide enough; just display the mode line we
 	     just computed. */
-	  display_string (NULL, mode_string, Qnil,
-			  0, 0, &it, 0, 0, 0,
-			  STRING_MULTIBYTE (mode_string));
+	  Lisp_Object start = make_fixnum (0), end;
+	  Lisp_Object elt;
+
+	  /* Display the mode line string one element by one element.
+	     This is to make the ranges of mouse highlighting
+	     correct. */
+	  do
+	    {
+	      end = Fnext_single_property_change (start, Qmode_line_elt_no,
+						  mode_string, Qnil);
+	      elt = Fsubstring (mode_string, start, end);
+	      display_string (NULL, elt, Qnil, 0, 0, &it, 0, 0, 0,
+			      STRING_MULTIBYTE (elt));
+	      start = end;
+	    }
+	  while (!NILP (end));
 	}
       else
 	{
 	  /* Compress the mode line. */
-	  ptrdiff_t i = 0, i_byte = 0, start = 0;
+	  ptrdiff_t i = 0, i_byte = 0;
 	  int prev = 0;
+	  Lisp_Object start = make_fixnum (0), end;
+	  Lisp_Object elt = empty_unibyte_string;
 
-	  while (i < SCHARS (mode_string))
+	  /* Display the mode line string one element by one element.
+	     This is to make the ranges of mouse highlighting
+	     correct. */
+	  do
 	    {
-	      int c = fetch_string_char_advance (mode_string, &i, &i_byte);
-	      if (c == ' ' && prev == ' ')
+	      end = Fnext_single_property_change (start,
+						  Qmode_line_elt_no,
+						  mode_string,
+						  make_fixnum (SCHARS (mode_string)));
+	      while (i < XFIXNUM (end))
 		{
-		  Lisp_Object prev_pos = make_fixnum (i - 1);
-
-		  /* SPC characters with 'display' properties are not
-                     really "empty", since they have non-trivial visual
-                     effects on the mode line.  */
-		  if (NILP (Fget_text_property (prev_pos, Qdisplay,
-						mode_string)))
+		  int c = fetch_string_char_advance (mode_string, &i, &i_byte);
+		  if (c == ' ' && prev == ' ')
 		    {
-		      display_string (NULL,
-				      Fsubstring (mode_string,
-						  make_fixnum (start),
-						  prev_pos),
-				      Qnil, 0, 0, &it, 0, 0, 0,
-				      STRING_MULTIBYTE (mode_string));
-		      /* Skip past the rest of the space characters. */
-		      while (c == ' ' && i < SCHARS (mode_string)
-			     && NILP (Fget_text_property (make_fixnum (i),
-							  Qdisplay,
-							  mode_string)))
+		      Lisp_Object prev_pos = make_fixnum (i - 1);
+		      Lisp_Object display = Fget_text_property (prev_pos,
+								Qdisplay,
+								mode_string);
+
+		      /* SPC characters with 'display' properties are not
+			 really "empty", since they have non-trivial visual
+			 effects on the mode line.  */
+		      if (NILP (display))
 			{
-			  c = fetch_string_char_advance (mode_string,
-							 &i, &i_byte);
+			  elt = concat2 (elt, Fsubstring (mode_string,
+							  start,
+							  prev_pos));
+
+			  /* Skip past the rest of the space characters. */
+			  Lisp_Object display = Fget_text_property (make_fixnum (i),
+								    Qdisplay,
+								    mode_string);
+			  while (c == ' ' && i < XFIXNUM (end)
+				 && NILP (display))
+			    {
+			      c = fetch_string_char_advance (mode_string,
+							     &i, &i_byte);
+			      display = Fget_text_property (make_fixnum (i),
+							    Qdisplay,
+							    mode_string);
+			    }
+
+			  /* Skip the final space no matter how the loop
+			     above ends. */
+			  if (c == ' ' && NILP (display))
+			    start = end;
+			  else
+			    start = make_fixnum (i - 1);
 			}
-		      start = i - 1;
 		    }
+		  prev = c;
 		}
-	      prev = c;
-	    }
 
-	  /* Display the final bit. */
-	  if (start < i)
-	    display_string (NULL,
-			    Fsubstring (mode_string, make_fixnum (start),
-					make_fixnum (i)),
-			    Qnil, 0, 0, &it, 0, 0, 0,
-			    STRING_MULTIBYTE (mode_string));
+	      /* Append the final bit. */
+	      if (XFIXNUM (start) < XFIXNUM (end))
+		elt = concat2 (elt, Fsubstring (mode_string, start, end));
+
+	      display_string (NULL, elt, Qnil, 0, 0, &it, 0, 0, 0,
+			      STRING_MULTIBYTE (elt));
+	      elt = empty_unibyte_string;
+	      start = end;
+	    }
+	  while (XFIXNUM (end) < SCHARS (mode_string));
 	}
     }
   pop_kboard ();
@@ -27926,6 +27966,20 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
   if (depth > 100)
     elt = build_string ("*too-deep*");
 
+  /* NOTE: Increment the number only for 'MODE_LINE_STRING', because for
+     other cases this variable is not used.  So we do not need to reset
+     the variable in every callers of this function.
+
+     NOTE: This might result in gaps in the stored element numbers
+     because some elements are processed recursively.  However, we
+     cannot increment this number when the number is stored because an
+     element could be stored by parts.  For example, "a%b" is stored as
+     two elements in the 'mode_line_string_list', but they should be
+     considered as one element, so we cannot increment this number when
+     "a" is stored. */
+  if (mode_line_target == MODE_LINE_STRING)
+    mode_line_elt_no++;
+
   depth++;
 
   switch (XTYPE (elt))
@@ -28023,7 +28077,11 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 		n += store_mode_line_noprop (SSDATA (elt), -1, prec);
 		break;
 	      case MODE_LINE_STRING:
-		n += store_mode_line_string (NULL, elt, true, 0, prec, Qnil);
+		{
+		  AUTO_LIST2 (src, Qmode_line_elt_no,
+			      make_fixnum (mode_line_elt_no));
+		  n += store_mode_line_string (NULL, elt, true, 0, prec, src);
+		}
 		break;
 	      case MODE_LINE_DISPLAY:
 		n += display_string (NULL, elt, Qnil, 0, 0, it,
@@ -28076,8 +28134,10 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 		      Lisp_Object mode_string
 			= Fsubstring (elt, make_fixnum (charpos),
 				      make_fixnum (endpos));
+		      AUTO_LIST2 (src, Qmode_line_elt_no,
+				  make_fixnum (mode_line_elt_no));
 		      n += store_mode_line_string (NULL, mode_string, false,
-						   0, 0, Qnil);
+						   0, 0, src);
 		    }
 		    break;
 		  case MODE_LINE_DISPLAY:
@@ -28157,6 +28217,8 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 			{
 			  Lisp_Object tem = build_string (spec);
 			  props = Ftext_properties_at (make_fixnum (charpos), elt);
+			  props = plist_put (props, Qmode_line_elt_no,
+					     make_fixnum (mode_line_elt_no));
 			  /* Should only keep face property in props */
 			  n += store_mode_line_string (NULL, tem, false,
 						       field, prec, props);
@@ -28372,6 +28434,9 @@ display_mode_element (struct it *it, int depth, int field_width, int precision,
 	  n += store_mode_line_noprop ("", field_width - n, 0);
 	  break;
 	case MODE_LINE_STRING:
+	  /* NOTE: Padding implicitly has 'mode-line-elt-no' property
+	     to be nil.  Normally padding spaces are between two real
+	     elements, so this should be good. */
 	  n += store_mode_line_string ("", Qnil, false, field_width - n, 0,
 				       Qnil);
 	  break;
@@ -28579,6 +28644,7 @@ DEFUN ("format-mode-line", Fformat_mode_line, Sformat_mode_line,
     }
 
   push_kboard (FRAME_KBOARD (it.f));
+  mode_line_elt_no = 0;
   display_mode_element (&it, 0, 0, 0, format, Qnil, false);
   pop_kboard ();
 
@@ -37675,6 +37741,7 @@ syms_of_xdisp (void)
   DEFSYM (Qfontification_functions, "fontification-functions");
   DEFSYM (Qlong_line_optimizations_in_fontification_functions,
 	  "long-line-optimizations-in-fontification-functions");
+  DEFSYM (Qmode_line_elt_no, "mode-line-elt-no");
 
   /* Name of the symbol which disables Lisp evaluation in 'display'
      properties.  This is used by enriched.el.  */
-- 
2.49.0

Message #28 received at 77336-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pengji Zhang <me <at> pengjiz.com>
Cc: 77336-done <at> debbugs.gnu.org
Subject: Re: bug#77336: [PATCH] Fix mouse highlighting for compact mode lines
Date: Sat, 05 Apr 2025 12:36:46 +0300

> From: Pengji Zhang <me <at> pengjiz.com>
> Cc: 77336 <at> debbugs.gnu.org
> Date: Wed, 02 Apr 2025 21:02:19 +0800
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Pengji Zhang <me <at> pengjiz.com>
> >> Cc: 77336 <at> debbugs.gnu.org
> >> Date: Mon, 31 Mar 2025 11:31:36 +0800
> >> 
> >> Eli Zaretskii <eliz <at> gnu.org> writes:
> >> 
> >> >> Is casting the 'Lisp_Object' to a number acceptable here?
> >> >
> >> > No, because Lisp_Object could be a struct (if Emacs was configured with
> >> > "--enable-check-lisp-object-type").
> >> 
> >> Sorry I did not make it clear. I meant 'make_fixnum(XLI(elt))'. I
> >> suppose 'XLI' will handle that case correctly?
> >
> > I'd prefer not to do that unless there's no other reasonable solution.
> >
> >> I am actually concerned that there could be a collision between two
> >> different 'elt's, e.g., a symbol and a string, because 'make_fixnum'
> >> alters the tag of 'elt'. I think it might be fine because 'elt' should
> >> always be a 'Lisp_String' when this is called.
> >
> > Sorry, I don't understand: what do you mean by "alters the tag of
> > 'elt'", and how could make_fixnum affect 'elt'?
> 
> It does not affect 'elt'. I meant that 'make_fixnum' may change the
> value of the number from the tagged pointer:
> 
>     INLINE Lisp_Object
>     make_fixnum (EMACS_INT n)
>     {
>       ...
>         {
>           n &= INTMASK;
>           n += (int0 << VALBITS);
>         }
>       return XIL (n);
>     }
> 
> So if I understand it correctly, that means 'make_fixnum(XLI(x))' could
> give the same result for, e.g., a fixnum and a string.
> 
> >> > A global variable is what I had in mind.  I don't see why it would be
> >> > less clean.
> >> 
> >> It is just that maintaining a global state is not as tasteful to me. If
> >> 'make_fixnum(XLI(elt))' is not good here, I am happy to go this way.
> >
> > Yes, a global variable is cleaner than using XLI for this purpose.
> 
> Thanks. Please see the attached patch.

Thanks, I've now installed this on the master branch, and I'm
therefore closing this bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.