GNU bug report logs - #62573
29.0.60; Cursor color not being inverted in emacs-29

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 62573 in the body.
You can then email your comments to 62573 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: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.60; Cursor color not being inverted in emacs-29
Date: Fri, 31 Mar 2023 19:34:05 +0100

I noticed a change in the default cursor colour of emacs 29. It seems that in emacs 28.2 (at least) the cursor colour is inverted when it is on a background of the same colour but in emacs 29 the cursor colour is not changed making it impossible to see.

For example, the following code inserts text with a black background (same as the cursor colour). When the cursor is on top of this text in emacs 28.2 it becomes yellow'ish but it is invisible in emacs 29.

;; Start with emacs -Q

(fundamental-mode)

(defface my-back-face
  '((t :foreground "yellow"
       :background "black"))
  "Testing.")

(let ((current-string "\ntext to insert"))
  (put-text-property 0 (length current-string)
		     'face 'my-back-face
                     current-string)
  (insert current-string))

How can I get the emacs 28.2 behaviour? Maybe this should be the default behaviour in emacs 29?

-- Al

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Cc: 62573 <at> debbugs.gnu.org
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Fri, 31 Mar 2023 22:15:54 +0300

> From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
> Date: Fri, 31 Mar 2023 19:34:05 +0100
> 
> 
> I noticed a change in the default cursor colour of emacs 29. It seems that in emacs 28.2 (at least) the cursor colour is inverted when it is on a background of the same colour but in emacs 29 the cursor colour is not changed making it impossible to see.
> 
> For example, the following code inserts text with a black background (same as the cursor colour). When the cursor is on top of this text in emacs 28.2 it becomes yellow'ish but it is invisible in emacs 29.
> 
> ;; Start with emacs -Q
> 
> (fundamental-mode)
> 
> (defface my-back-face
>   '((t :foreground "yellow"
>        :background "black"))
>   "Testing.")
> 
> (let ((current-string "\ntext to insert"))
>   (put-text-property 0 (length current-string)
> 		     'face 'my-back-face
>                      current-string)
>   (insert current-string))

I cannot reproduce this problem here: I get a "yellowish" cursor in
Emacs 29 as well.  And I'm not aware of any changes in this area
between Emacs 28 and Emacs 29.

On which platform is that and with what Emacs configuration?  (Using
"M-x report-emacs-bug" would have collected this information
automatically for you.)

Thanks.

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

From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62573 <at> debbugs.gnu.org
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Fri, 31 Mar 2023 22:46:00 +0100

On 31/03/2023, Eli Zaretskii wrote:
> On which platform is that and with what Emacs configuration?  (Using
> "M-x report-emacs-bug" would have collected this information
> automatically for you.)
Apologies, below are the details. I can also add the config of emacs 28.2 if that's helpful.
I am not sure what the issue is or how to debug it, so any hints are appreciated.

---------------------------------------------------------------------------
In GNU Emacs 29.0.60 (build 1, aarch64-apple-darwin22.3.0, NS
 appkit-2299.40 Version 13.2.1 (Build 22D68)) of 2023-03-22 built on
 HW-R9XXWKPJ4D
Windowing system distributor 'Apple', version 10.3.2299
System Description:  macOS 13.2.1

Configured using:
 'configure --disable-dependency-tracking --disable-silent-rules
 --enable-locallisppath=/opt/homebrew/share/emacs/site-lisp
 --infodir=/opt/homebrew/Cellar/emacs-plus <at> 29/29.0.60/share/info/emacs
 --prefix=/opt/homebrew/Cellar/emacs-plus <at> 29/29.0.60 --with-xml2
 --with-gnutls --with-native-compilation --without-compress-install
 --without-dbus --with-imagemagick --with-modules --with-rsvg --with-ns
 --disable-ns-self-contained 'CFLAGS=-Os -w -pipe
 -mmacosx-version-min=13
 -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk
 -DFD_SETSIZE=10000 -DDARWIN_UNLIMITED_SELECT'
 'CPPFLAGS=-I/opt/homebrew/opt/zlib/include
 -I/opt/homebrew/opt/jpeg/include -I/opt/homebrew/opt/libomp/include
 -I/opt/homebrew/opt/icu4c/include
 -I/opt/homebrew/opt/openssl <at> 1.1/include -isystem/opt/homebrew/include
 -F/opt/homebrew/Frameworks
 -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk'
 'LDFLAGS=-L/opt/homebrew/opt/zlib/lib -L/opt/homebrew/opt/jpeg/lib
 -L/opt/homebrew/opt/libomp/lib -L/opt/homebrew/opt/icu4c/lib
 -L/opt/homebrew/opt/openssl <at> 1.1/lib -L/opt/homebrew/lib
 -F/opt/homebrew/Frameworks -Wl,-headerpad_max_install_names
 -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk''

Configured features:
ACL GIF GLIB GMP GNUTLS IMAGEMAGICK JPEG JSON LCMS2 LIBXML2 MODULES
NATIVE_COMP NOTIFY KQUEUE NS PDUMPER PNG RSVG SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM ZLIB

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

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-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
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util 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 help-fns
radix-tree cl-print byte-opt debug backtrace find-func comp comp-cstr
warnings icons subr-x rx cl-seq cl-macs gv cl-extra help-mode
cl-loaddefs cl-lib bytecomp byte-compile rmc iso-transl tooltip cconv
eldoc paren electric uniquify ediff-hook vc-hooks lisp-float-type
elisp-mode mwheel term/ns-win ns-win ucs-normalize mule-util
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-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 nadvice seq simple cl-generic indonesian philippine
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 emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads kqueue cocoa ns lcms2 multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 88096 7065)
 (symbols 48 7478 0)
 (strings 32 24488 3567)
 (string-bytes 1 788707)
 (vectors 16 20965)
 (vector-slots 8 357186 10640)
 (floats 8 37 39)
 (intervals 56 775 0)
 (buffers 984 13))

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Cc: 62573 <at> debbugs.gnu.org
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sat, 01 Apr 2023 08:38:48 +0300

> From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
> Cc: 62573 <at> debbugs.gnu.org
> Date: Fri, 31 Mar 2023 22:46:00 +0100
> 
> 
> On 31/03/2023, Eli Zaretskii wrote:
> > On which platform is that and with what Emacs configuration?  (Using
> > "M-x report-emacs-bug" would have collected this information
> > automatically for you.)
> Apologies, below are the details. I can also add the config of emacs 28.2 if that's helpful.
> I am not sure what the issue is or how to debug it, so any hints are appreciated.
> 
> ---------------------------------------------------------------------------
> In GNU Emacs 29.0.60 (build 1, aarch64-apple-darwin22.3.0, NS
>  appkit-2299.40 Version 13.2.1 (Build 22D68)) of 2023-03-22 built on
>  HW-R9XXWKPJ4D
> Windowing system distributor 'Apple', version 10.3.2299
> System Description:  macOS 13.2.1

This is macOS, so I suspect the problem is specific to macOS.  Can
someone with access to macOS please try reproducing this, and perhaps
debugging the problem?

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, 62573 <at> debbugs.gnu.org,
 Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sat, 01 Apr 2023 21:56:33 +0200

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

>> From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
>> Cc: 62573 <at> debbugs.gnu.org
>> Date: Fri, 31 Mar 2023 22:46:00 +0100
>> 
>> 
>> On 31/03/2023, Eli Zaretskii wrote:
>> > On which platform is that and with what Emacs configuration?  (Using
>> > "M-x report-emacs-bug" would have collected this information
>> > automatically for you.)
>> Apologies, below are the details. I can also add the config of emacs 28.2 if that's helpful.
>> I am not sure what the issue is or how to debug it, so any hints are appreciated.
>> 
>> ---------------------------------------------------------------------------
>> In GNU Emacs 29.0.60 (build 1, aarch64-apple-darwin22.3.0, NS
>>  appkit-2299.40 Version 13.2.1 (Build 22D68)) of 2023-03-22 built on
>>  HW-R9XXWKPJ4D
>> Windowing system distributor 'Apple', version 10.3.2299
>> System Description:  macOS 13.2.1
>
> This is macOS, so I suspect the problem is specific to macOS.  Can
> someone with access to macOS please try reproducing this, and perhaps
> debugging the problem?

This bug is a regression caused by
07715630ad9df9cb681cbadecbaf73fc9c698061.  (Adding Po Lu to the CC.)

From what I see, the font display refactor removed this code from
src/nsterm.m:

-  face = FACE_FROM_ID_OR_NULL (f, phys_cursor_glyph->face_id);
-  if (face && NS_FACE_BACKGROUND (face)
-      == ns_index_color (FRAME_CURSOR_COLOR (f), f))
-    {
-      [ns_lookup_indexed_color (NS_FACE_FOREGROUND (face), f) set];
-      hollow_color = FRAME_CURSOR_COLOR (f);
-    }
-  else

which seems to be responsible for the cursor color change when the
background face color is the same as the cursor color.  I can't find
that logic in the current code, so I think we miss it from the
refactoring.

I've solved the bug by replicating that logic in the appropriate places,
nsterm.m and macfont.m:

diff --git a/src/macfont.m b/src/macfont.m
index d0cdbcd08c7..d14cf5f9c98 100644
--- a/src/macfont.m
+++ b/src/macfont.m
@@ -2933,9 +2933,15 @@ So we use CTFontDescriptorCreateMatchingFontDescriptor (no
     {
       if (s->hl == DRAW_CURSOR)
         {
-	  CGColorRef colorref = get_cgcolor_from_nscolor (FRAME_CURSOR_COLOR (f), f);
-	  CGContextSetFillColorWithColor (context, colorref);
-	  CGColorRelease (colorref);
+          if (face && NS_FACE_BACKGROUND (face)
+              == [(NSColor*)FRAME_CURSOR_COLOR (f) unsignedLong])
+            CG_SET_FILL_COLOR_WITH_FACE_FOREGROUND (context, face);
+          else
+            {
+              CGColorRef colorref = get_cgcolor_from_nscolor (FRAME_CURSOR_COLOR (f), f);
+              CGContextSetFillColorWithColor (context, colorref);
+              CGColorRelease (colorref);
+            }
         }
       else
         CG_SET_FILL_COLOR_WITH_FACE_BACKGROUND (context, face);
@@ -2949,9 +2955,15 @@ So we use CTFontDescriptorCreateMatchingFontDescriptor (no
       CGContextScaleCTM (context, 1, -1);
       if (s->hl == DRAW_CURSOR)
         {
-	  CGColorRef colorref = get_cgcolor_from_nscolor (FRAME_BACKGROUND_COLOR (f), f);
-	  CGContextSetFillColorWithColor (context, colorref);
-	  CGColorRelease (colorref);
+          if (face && NS_FACE_BACKGROUND (face)
+              == [(NSColor*)FRAME_CURSOR_COLOR (f) unsignedLong])
+            CG_SET_FILL_COLOR_WITH_FACE_BACKGROUND (context, face);
+          else
+            {
+              CGColorRef colorref = get_cgcolor_from_nscolor (FRAME_BACKGROUND_COLOR (f), f);
+              CGContextSetFillColorWithColor (context, colorref);
+              CGColorRelease (colorref);
+            }
         }
       else
         CG_SET_FILL_COLOR_WITH_FACE_FOREGROUND (context, face);
diff --git a/src/nsterm.m b/src/nsterm.m
index 46007ec4fcb..2f31a279bfc 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3750,14 +3750,17 @@ Function modeled after x_draw_glyph_string_box ().
 	{
           struct face *face = s->face;
           if (!face->stipple)
-	    {
-	      if (s->hl != DRAW_CURSOR)
-		[(NS_FACE_BACKGROUND (face) != 0
-		  ? [NSColor colorWithUnsignedLong:NS_FACE_BACKGROUND (face)]
-		  : FRAME_BACKGROUND_COLOR (s->f)) set];
-	      else
-		[FRAME_CURSOR_COLOR (s->f) set];
-	    }
+            {
+              if (s->hl != DRAW_CURSOR)
+                [(NS_FACE_BACKGROUND (face) != 0
+                  ? [NSColor colorWithUnsignedLong:NS_FACE_BACKGROUND (face)]
+                  : FRAME_BACKGROUND_COLOR (s->f)) set];
+              else if (face && NS_FACE_BACKGROUND (face)
+                       == [(NSColor*)FRAME_CURSOR_COLOR (s->f) unsignedLong])
+                [[NSColor colorWithUnsignedLong:NS_FACE_FOREGROUND (face)] set];
+              else
+                [FRAME_CURSOR_COLOR (s->f) set];
+            }
           else
             {
               struct ns_display_info *dpyinfo = FRAME_DISPLAY_INFO (s->f);


Could you give it a try on macOS and GNUstep?  Thank you.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: Al Haji-Ali <abdo.haji.ali <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 62573 <at> debbugs.gnu.org
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sun, 02 Apr 2023 08:44:09 +0800

Daniel Martín <mardani29 <at> yahoo.es> writes:

> I've solved the bug by replicating that logic in the appropriate places,
> nsterm.m and macfont.m:

Nice, thanks.

> +              CGColorRef colorref = get_cgcolor_from_nscolor (FRAME_CURSOR_COLOR (f), f);

This row is now wider than 80 columns.  Please wrap it.

>            struct face *face = s->face;
>            if (!face->stipple)
> -	    {
> -	      if (s->hl != DRAW_CURSOR)
> -		[(NS_FACE_BACKGROUND (face) != 0
> -		  ? [NSColor colorWithUnsignedLong:NS_FACE_BACKGROUND (face)]
> -		  : FRAME_BACKGROUND_COLOR (s->f)) set];
> -	      else
> -		[FRAME_CURSOR_COLOR (s->f) set];
> -	    }
> +            {
> +              if (s->hl != DRAW_CURSOR)
> +                [(NS_FACE_BACKGROUND (face) != 0
> +                  ? [NSColor colorWithUnsignedLong:NS_FACE_BACKGROUND (face)]
> +                  : FRAME_BACKGROUND_COLOR (s->f)) set];
> +              else if (face && NS_FACE_BACKGROUND (face)
> +                       == [(NSColor*)FRAME_CURSOR_COLOR (s->f) unsignedLong])
> +                [[NSColor colorWithUnsignedLong:NS_FACE_FOREGROUND (face)] set];
> +              else
> +                [FRAME_CURSOR_COLOR (s->f) set];
> +            }
>            else
>              {
>                struct ns_display_info *dpyinfo = FRAME_DISPLAY_INFO (s->f);
>

Please write:

  (face && (NS_FACE_BACKGROUND (face)
  	    == [(NSColor *) FRAME_CURSOR_COLOR (s->f)
                  unsignedLong]))

instead.

> Could you give it a try on macOS and GNUstep?  Thank you.

Works for me, thanks.

P.S. I think what needs to be done is to make the NS port keep track of
glyph string colors through GCs, so that the relevant code can be easily
synchronized with X.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: luangruo <at> yahoo.com, 62573 <at> debbugs.gnu.org, abdo.haji.ali <at> gmail.com
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sun, 02 Apr 2023 08:09:12 +0300

> From: Daniel Martín <mardani29 <at> yahoo.es>
> Cc: Al Haji-Ali <abdo.haji.ali <at> gmail.com>,  62573 <at> debbugs.gnu.org,
>  luangruo <at> yahoo.com
> Date: Sat, 01 Apr 2023 21:56:33 +0200
> 
> This bug is a regression caused by
> 07715630ad9df9cb681cbadecbaf73fc9c698061.  (Adding Po Lu to the CC.)
> 
> >From what I see, the font display refactor removed this code from
> src/nsterm.m:
> 
> -  face = FACE_FROM_ID_OR_NULL (f, phys_cursor_glyph->face_id);
> -  if (face && NS_FACE_BACKGROUND (face)
> -      == ns_index_color (FRAME_CURSOR_COLOR (f), f))
> -    {
> -      [ns_lookup_indexed_color (NS_FACE_FOREGROUND (face), f) set];
> -      hollow_color = FRAME_CURSOR_COLOR (f);
> -    }
> -  else
> 
> which seems to be responsible for the cursor color change when the
> background face color is the same as the cursor color.  I can't find
> that logic in the current code, so I think we miss it from the
> refactoring.
> 
> I've solved the bug by replicating that logic in the appropriate places,
> nsterm.m and macfont.m:

This regression should be fixed on the release branch, but the patch
you propose is quite large.  Can't we simply reinstate the removed
code, and apply the changes you propose only on master?

Thanks.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62573 <at> debbugs.gnu.org, abdo.haji.ali <at> gmail.com,
 Daniel Martín <mardani29 <at> yahoo.es>
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sun, 02 Apr 2023 13:52:45 +0800

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

>> From: Daniel Martín <mardani29 <at> yahoo.es>
>> Cc: Al Haji-Ali <abdo.haji.ali <at> gmail.com>,  62573 <at> debbugs.gnu.org,
>>  luangruo <at> yahoo.com
>> Date: Sat, 01 Apr 2023 21:56:33 +0200
>> 
>> This bug is a regression caused by
>> 07715630ad9df9cb681cbadecbaf73fc9c698061.  (Adding Po Lu to the CC.)
>> 
>> >From what I see, the font display refactor removed this code from
>> src/nsterm.m:
>> 
>> -  face = FACE_FROM_ID_OR_NULL (f, phys_cursor_glyph->face_id);
>> -  if (face && NS_FACE_BACKGROUND (face)
>> -      == ns_index_color (FRAME_CURSOR_COLOR (f), f))
>> -    {
>> -      [ns_lookup_indexed_color (NS_FACE_FOREGROUND (face), f) set];
>> -      hollow_color = FRAME_CURSOR_COLOR (f);
>> -    }
>> -  else
>> 
>> which seems to be responsible for the cursor color change when the
>> background face color is the same as the cursor color.  I can't find
>> that logic in the current code, so I think we miss it from the
>> refactoring.
>> 
>> I've solved the bug by replicating that logic in the appropriate places,
>> nsterm.m and macfont.m:
>
> This regression should be fixed on the release branch, but the patch
> you propose is quite large.  Can't we simply reinstate the removed
> code, and apply the changes you propose only on master?

Unfortunately not, as that would involve removing the ability to
properly display mouse face with the Emacs 29 redisplay.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 62573 <at> debbugs.gnu.org, abdo.haji.ali <at> gmail.com, mardani29 <at> yahoo.es
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sun, 02 Apr 2023 09:58:03 +0300

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: Daniel Martín <mardani29 <at> yahoo.es>,
>   abdo.haji.ali <at> gmail.com,
>   62573 <at> debbugs.gnu.org
> Date: Sun, 02 Apr 2023 13:52:45 +0800
> 
> >> I've solved the bug by replicating that logic in the appropriate places,
> >> nsterm.m and macfont.m:
> >
> > This regression should be fixed on the release branch, but the patch
> > you propose is quite large.  Can't we simply reinstate the removed
> > code, and apply the changes you propose only on master?
> 
> Unfortunately not, as that would involve removing the ability to
> properly display mouse face with the Emacs 29 redisplay.

Sigh...

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, abdo.haji.ali <at> gmail.com, 62573 <at> debbugs.gnu.org
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sun, 02 Apr 2023 13:01:07 +0200

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

>> From: Daniel Martín <mardani29 <at> yahoo.es>
>> Cc: Al Haji-Ali <abdo.haji.ali <at> gmail.com>,  62573 <at> debbugs.gnu.org,
>>  luangruo <at> yahoo.com
>> Date: Sat, 01 Apr 2023 21:56:33 +0200
>> 
>> This bug is a regression caused by
>> 07715630ad9df9cb681cbadecbaf73fc9c698061.  (Adding Po Lu to the CC.)
>> 
>> >From what I see, the font display refactor removed this code from
>> src/nsterm.m:
>> 
>> -  face = FACE_FROM_ID_OR_NULL (f, phys_cursor_glyph->face_id);
>> -  if (face && NS_FACE_BACKGROUND (face)
>> -      == ns_index_color (FRAME_CURSOR_COLOR (f), f))
>> -    {
>> -      [ns_lookup_indexed_color (NS_FACE_FOREGROUND (face), f) set];
>> -      hollow_color = FRAME_CURSOR_COLOR (f);
>> -    }
>> -  else
>> 
>> which seems to be responsible for the cursor color change when the
>> background face color is the same as the cursor color.  I can't find
>> that logic in the current code, so I think we miss it from the
>> refactoring.
>> 
>> I've solved the bug by replicating that logic in the appropriate places,
>> nsterm.m and macfont.m:
>
> This regression should be fixed on the release branch, but the patch
> you propose is quite large.  Can't we simply reinstate the removed
> code, and apply the changes you propose only on master?
>

That's the first thing I tried, but it didn't fix the problem.  More
code changes would be needed because, since
07715630ad9df9cb681cbadecbaf73fc9c698061, the responsibility of drawing
the cursor now resides in the macOS font driver (macfont.m).

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: luangruo <at> yahoo.com, abdo.haji.ali <at> gmail.com, 62573 <at> debbugs.gnu.org
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sun, 02 Apr 2023 14:12:42 +0300

> From: Daniel Martín <mardani29 <at> yahoo.es>
> Cc: luangruo <at> yahoo.com,  62573 <at> debbugs.gnu.org,  abdo.haji.ali <at> gmail.com
> Date: Sun, 02 Apr 2023 13:01:07 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Daniel Martín <mardani29 <at> yahoo.es>
> >> Cc: Al Haji-Ali <abdo.haji.ali <at> gmail.com>,  62573 <at> debbugs.gnu.org,
> >>  luangruo <at> yahoo.com
> >> Date: Sat, 01 Apr 2023 21:56:33 +0200
> >> 
> >> This bug is a regression caused by
> >> 07715630ad9df9cb681cbadecbaf73fc9c698061.  (Adding Po Lu to the CC.)
> >> 
> >> >From what I see, the font display refactor removed this code from
> >> src/nsterm.m:
> >> 
> >> -  face = FACE_FROM_ID_OR_NULL (f, phys_cursor_glyph->face_id);
> >> -  if (face && NS_FACE_BACKGROUND (face)
> >> -      == ns_index_color (FRAME_CURSOR_COLOR (f), f))
> >> -    {
> >> -      [ns_lookup_indexed_color (NS_FACE_FOREGROUND (face), f) set];
> >> -      hollow_color = FRAME_CURSOR_COLOR (f);
> >> -    }
> >> -  else
> >> 
> >> which seems to be responsible for the cursor color change when the
> >> background face color is the same as the cursor color.  I can't find
> >> that logic in the current code, so I think we miss it from the
> >> refactoring.
> >> 
> >> I've solved the bug by replicating that logic in the appropriate places,
> >> nsterm.m and macfont.m:
> >
> > This regression should be fixed on the release branch, but the patch
> > you propose is quite large.  Can't we simply reinstate the removed
> > code, and apply the changes you propose only on master?
> >
> 
> That's the first thing I tried, but it didn't fix the problem.  More
> code changes would be needed because, since
> 07715630ad9df9cb681cbadecbaf73fc9c698061, the responsibility of drawing
> the cursor now resides in the macOS font driver (macfont.m).

OK, thanks, understood.

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

From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
To: Daniel Martín <mardani29 <at> yahoo.es>, Eli Zaretskii
 <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, 62573 <at> debbugs.gnu.org
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sun, 02 Apr 2023 15:28:58 +0100

On 01/04/2023, Daniel Martín wrote:
> This bug is a regression caused by
> 07715630ad9df9cb681cbadecbaf73fc9c698061.  (Adding Po Lu to the CC.)
>
> [snip]
>
> Could you give it a try on macOS and GNUstep?  Thank you.

Just managed to apply the path and it does seem to fix the issue for me.

Thanks!
-- Al

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 62573 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sun, 02 Apr 2023 23:24:15 +0200

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

Po Lu <luangruo <at> yahoo.com> writes:

> Daniel Martín <mardani29 <at> yahoo.es> writes:
>
>> I've solved the bug by replicating that logic in the appropriate places,
>> nsterm.m and macfont.m:
>
> Nice, thanks.
>
>> +              CGColorRef colorref = get_cgcolor_from_nscolor (FRAME_CURSOR_COLOR (f), f);
>
> This row is now wider than 80 columns.  Please wrap it.
>

Alright, I have also extracted a couple of macros because there were
already some macros for similar code patterns.

>>            struct face *face = s->face;
>>            if (!face->stipple)
>> -	    {
>> -	      if (s->hl != DRAW_CURSOR)
>> -		[(NS_FACE_BACKGROUND (face) != 0
>> -		  ? [NSColor colorWithUnsignedLong:NS_FACE_BACKGROUND (face)]
>> -		  : FRAME_BACKGROUND_COLOR (s->f)) set];
>> -	      else
>> -		[FRAME_CURSOR_COLOR (s->f) set];
>> -	    }
>> +            {
>> +              if (s->hl != DRAW_CURSOR)
>> +                [(NS_FACE_BACKGROUND (face) != 0
>> +                  ? [NSColor colorWithUnsignedLong:NS_FACE_BACKGROUND (face)]
>> +                  : FRAME_BACKGROUND_COLOR (s->f)) set];
>> +              else if (face && NS_FACE_BACKGROUND (face)
>> +                       == [(NSColor*)FRAME_CURSOR_COLOR (s->f) unsignedLong])
>> +                [[NSColor colorWithUnsignedLong:NS_FACE_FOREGROUND (face)] set];
>> +              else
>> +                [FRAME_CURSOR_COLOR (s->f) set];
>> +            }
>>            else
>>              {
>>                struct ns_display_info *dpyinfo = FRAME_DISPLAY_INFO (s->f);
>>
>
> Please write:
>
>   (face && (NS_FACE_BACKGROUND (face)
>   	    == [(NSColor *) FRAME_CURSOR_COLOR (s->f)
>                   unsignedLong]))
>
> instead.

Done.

>
>> Could you give it a try on macOS and GNUstep?  Thank you.
>
> Works for me, thanks.
>
> P.S. I think what needs to be done is to make the NS port keep track of
> glyph string colors through GCs, so that the relevant code can be easily
> synchronized with X.

Yes, I agree that we should refactor the code to use GCs in order to
avoid these differences with X.  Maybe I can work on that and suggest a
patch for the master branch.

[0001-Change-cursor-color-on-NS-port-when-it-matches-the-f.patch (text/x-patch, attachment)]

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

From: Po Lu <luangruo <at> yahoo.com>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 62573 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Mon, 03 Apr 2023 08:07:24 +0800

Daniel Martín <mardani29 <at> yahoo.es> writes:

> +#define CG_SET_FILL_COLOR_WITH_FRAME_CURSOR(context, frame)             \
> +  do {                                                                  \
> +    CGColorRef refcol_ =                                                \
> +      get_cgcolor_from_nscolor (FRAME_CURSOR_COLOR (frame), frame);     \
> +    CGContextSetFillColorWithColor (context, refcol_);                  \
> +    CGColorRelease (refcol_);                                           \
> +  } while (0)
> +#define CG_SET_FILL_COLOR_WITH_FRAME_BACKGROUND(context, frame)         \
> +  do {                                                                  \
> +    CGColorRef refcol_ =                                                \
> +      get_cgcolor_from_nscolor (FRAME_BACKGROUND_COLOR (frame), frame); \
> +    CGContextSetFillColorWithColor (context, refcol_);                  \
> +    CGColorRelease (refcol_);                                           \
> +  } while (0)

Thanks.  The GNU Coding Standards split expressions, before operators.
So this should read:

  do {
    CGColorRef refcol
      = ...

also, since you put this in a separate block, you don't have to worry
about shadowing, so there's no need to use names with a trailing
underscore.

>  #define CG_SET_STROKE_COLOR_WITH_FACE_FOREGROUND(context, face)         \
>    do {                                                                  \
>      CGColorRef refcol_ = get_cgcolor (NS_FACE_FOREGROUND (face));       \
> @@ -2933,9 +2947,12 @@ So we use CTFontDescriptorCreateMatchingFontDescriptor (no
>      {
>        if (s->hl == DRAW_CURSOR)
>          {
> -	  CGColorRef colorref = get_cgcolor_from_nscolor (FRAME_CURSOR_COLOR (f), f);
> -	  CGContextSetFillColorWithColor (context, colorref);
> -	  CGColorRelease (colorref);
> +          if (face && (NS_FACE_BACKGROUND (face)
> +                       == [(NSColor *) FRAME_CURSOR_COLOR (f)
> +                                       unsignedLong]))
> +            CG_SET_FILL_COLOR_WITH_FACE_FOREGROUND (context, face);
> +          else
> +            CG_SET_FILL_COLOR_WITH_FRAME_CURSOR (context, f);
>          }
>        else
>          CG_SET_FILL_COLOR_WITH_FACE_BACKGROUND (context, face);
> @@ -2949,9 +2966,12 @@ So we use CTFontDescriptorCreateMatchingFontDescriptor (no
>        CGContextScaleCTM (context, 1, -1);
>        if (s->hl == DRAW_CURSOR)
>          {
> -	  CGColorRef colorref = get_cgcolor_from_nscolor (FRAME_BACKGROUND_COLOR (f), f);
> -	  CGContextSetFillColorWithColor (context, colorref);
> -	  CGColorRelease (colorref);
> +          if (face && (NS_FACE_BACKGROUND (face)
> +                       == [(NSColor *) FRAME_CURSOR_COLOR (f)
> +                                       unsignedLong]))
> +            CG_SET_FILL_COLOR_WITH_FACE_BACKGROUND (context, face);
> +          else
> +            CG_SET_FILL_COLOR_WITH_FRAME_BACKGROUND (context, f);
>          }
>        else
>          CG_SET_FILL_COLOR_WITH_FACE_FOREGROUND (context, face);
> diff --git a/src/nsterm.m b/src/nsterm.m
> index 46007ec4fcb..637bc4b6419 100644
> --- a/src/nsterm.m
> +++ b/src/nsterm.m
> @@ -3750,14 +3750,18 @@ Function modeled after x_draw_glyph_string_box ().
>  	{
>            struct face *face = s->face;
>            if (!face->stipple)
> -	    {
> -	      if (s->hl != DRAW_CURSOR)
> -		[(NS_FACE_BACKGROUND (face) != 0
> -		  ? [NSColor colorWithUnsignedLong:NS_FACE_BACKGROUND (face)]
> -		  : FRAME_BACKGROUND_COLOR (s->f)) set];
> -	      else
> -		[FRAME_CURSOR_COLOR (s->f) set];
> -	    }
> +            {
> +              if (s->hl != DRAW_CURSOR)
> +                [(NS_FACE_BACKGROUND (face) != 0
> +                  ? [NSColor colorWithUnsignedLong:NS_FACE_BACKGROUND (face)]
> +                  : FRAME_BACKGROUND_COLOR (s->f)) set];
> +              else if (face && (NS_FACE_BACKGROUND (face)
> +                                == [(NSColor *) FRAME_CURSOR_COLOR (s->f)
> +                                                unsignedLong]))
> +                [[NSColor colorWithUnsignedLong:NS_FACE_FOREGROUND (face)] set];
> +              else
> +                [FRAME_CURSOR_COLOR (s->f) set];
> +            }
>            else
>              {
>                struct ns_display_info *dpyinfo = FRAME_DISPLAY_INFO (s->f);

LGTM.  Thanks.

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

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Al Haji-Ali <abdo.haji.ali <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 62573 <at> debbugs.gnu.org
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Thu, 06 Apr 2023 12:12:04 +0200

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

Po Lu <luangruo <at> yahoo.com> writes:

> Daniel Martín <mardani29 <at> yahoo.es> writes:
>
>> +#define CG_SET_FILL_COLOR_WITH_FRAME_CURSOR(context, frame)             \
>> +  do {                                                                  \
>> +    CGColorRef refcol_ =                                                \
>> +      get_cgcolor_from_nscolor (FRAME_CURSOR_COLOR (frame), frame);     \
>> +    CGContextSetFillColorWithColor (context, refcol_);                  \
>> +    CGColorRelease (refcol_);                                           \
>> +  } while (0)
>> +#define CG_SET_FILL_COLOR_WITH_FRAME_BACKGROUND(context, frame)         \
>> +  do {                                                                  \
>> +    CGColorRef refcol_ =                                                \
>> +      get_cgcolor_from_nscolor (FRAME_BACKGROUND_COLOR (frame), frame); \
>> +    CGContextSetFillColorWithColor (context, refcol_);                  \
>> +    CGColorRelease (refcol_);                                           \
>> +  } while (0)
>
> Thanks.  The GNU Coding Standards split expressions, before operators.
> So this should read:
>
>   do {
>     CGColorRef refcol
>       = ...
>
> also, since you put this in a separate block, you don't have to worry
> about shadowing, so there's no need to use names with a trailing
> underscore.
>

OK, I've removed the trailing underscore in the other macros as well.

Here's a new patch with the requested changes.  If everything looks
good, could someone install the patch for me?  (I don't have push access
to the repository.)

Thanks.

[0001-Change-cursor-color-on-NS-port-when-it-matches-the-f.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: luangruo <at> yahoo.com, abdo.haji.ali <at> gmail.com, 62573-done <at> debbugs.gnu.org
Subject: Re: bug#62573: 29.0.60; Cursor color not being inverted in emacs-29
Date: Sat, 08 Apr 2023 14:36:13 +0300

> From: Daniel Martín <mardani29 <at> yahoo.es>
> Cc: 62573 <at> debbugs.gnu.org,  Eli Zaretskii <eliz <at> gnu.org>,  Al Haji-Ali
>  <abdo.haji.ali <at> gmail.com>
> Date: Thu, 06 Apr 2023 12:12:04 +0200
> 
> OK, I've removed the trailing underscore in the other macros as well.
> 
> Here's a new patch with the requested changes.  If everything looks
> good, could someone install the patch for me?  (I don't have push access
> to the repository.)

Thanks, installed, and closing the bug.

Previous Next

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