bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Al Haji-Ali]

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

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Eli Zaretskii]

> From: Al Haji-Ali <abdo.haji.ali@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.

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Al Haji-Ali]

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@29/29.0.60/share/info/emacs
 --prefix=/opt/homebrew/Cellar/emacs-plus@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@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@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))

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Eli Zaretskii]

> From: Al Haji-Ali <abdo.haji.ali@gmail.com>
> Cc: 62573@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?

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Al Haji-Ali <abdo.haji.ali@gmail.com>
>> Cc: 62573@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.

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Daniel Martín <mardani29@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.

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Po Lu <luangruo@yahoo.com> writes:

> Daniel Martín <mardani29@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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Change-cursor-color-on-NS-port-when-it-matches-the-f.patch --]
[-- Type: text/x-patch, Size: 5016 bytes --]

From f824d2e59921f14aa61ece17e8401c8a47c77d5f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es>
Date: Sun, 2 Apr 2023 22:39:44 +0200
Subject: [PATCH] Change cursor color on NS port when it matches the face
 background

* src/macfont.m (CG_SET_FILL_COLOR_WITH_FRAME_CURSOR): New macro.
(CG_SET_FILL_COLOR_WITH_FRAME_BACKGROUND): New macro.
(macfont_draw): When the cursor's color matches the face background,
set the fill color of the cursor to the face foreground.
* src/nsterm.m (ns_maybe_dumpglyphs_background): When dumping the
background of a glyph string, apply the logic mentioned
above.  (Bug#62573)
---
 src/macfont.m | 32 ++++++++++++++++++++++++++------
 src/nsterm.m  | 20 ++++++++++++--------
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/src/macfont.m b/src/macfont.m
index d0cdbcd08c7..a582bc1e38e 100644
--- a/src/macfont.m
+++ b/src/macfont.m
@@ -642,6 +642,20 @@ static void mac_font_get_glyphs_for_variants (CFDataRef, UTF32Char,
     CGContextSetFillColorWithColor (context, refcol_);                  \
     CGColorRelease (refcol_);                                           \
   } while (0)
+#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)
 #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);
-- 
2.34.1

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Daniel Martín <mardani29@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.

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Po Lu <luangruo@yahoo.com> writes:

> Daniel Martín <mardani29@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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Change-cursor-color-on-NS-port-when-it-matches-the-f.patch --]
[-- Type: text/x-patch, Size: 6498 bytes --]

From bbce06787debc564353ffc09ad74566e1fa254a0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es>
Date: Sun, 2 Apr 2023 22:39:44 +0200
Subject: [PATCH] Change cursor color on NS port when it matches the face
 background

* src/macfont.m (CG_SET_FILL_COLOR_WITH_FRAME_CURSOR): New macro.
(CG_SET_FILL_COLOR_WITH_FRAME_BACKGROUND): New macro.
(macfont_draw): When the cursor's color matches the face background,
set the fill color of the cursor to the face foreground.
* src/nsterm.m (ns_maybe_dumpglyphs_background): When dumping the
background of a glyph string, apply the logic mentioned
above.  (Bug#62573)
---
 src/macfont.m | 50 +++++++++++++++++++++++++++++++++++---------------
 src/nsterm.m  | 20 ++++++++++++--------
 2 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/src/macfont.m b/src/macfont.m
index d0cdbcd08c7..9f9f6f4efaf 100644
--- a/src/macfont.m
+++ b/src/macfont.m
@@ -632,21 +632,35 @@ static void mac_font_get_glyphs_for_variants (CFDataRef, UTF32Char,
 
 #define CG_SET_FILL_COLOR_WITH_FACE_FOREGROUND(context, face)           \
   do {                                                                  \
-    CGColorRef refcol_ = get_cgcolor (NS_FACE_FOREGROUND (face));       \
-    CGContextSetFillColorWithColor (context, refcol_) ;                 \
-    CGColorRelease (refcol_);                                           \
+    CGColorRef refcol = get_cgcolor (NS_FACE_FOREGROUND (face));        \
+    CGContextSetFillColorWithColor (context, refcol);                   \
+    CGColorRelease (refcol);                                            \
   } while (0)
 #define CG_SET_FILL_COLOR_WITH_FACE_BACKGROUND(context, face)           \
   do {                                                                  \
-    CGColorRef refcol_ = get_cgcolor (NS_FACE_BACKGROUND (face));       \
-    CGContextSetFillColorWithColor (context, refcol_);                  \
-    CGColorRelease (refcol_);                                           \
+    CGColorRef refcol = get_cgcolor (NS_FACE_BACKGROUND (face));        \
+    CGContextSetFillColorWithColor (context, refcol);                   \
+    CGColorRelease (refcol);                                            \
+  } while (0)
+#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)
 #define CG_SET_STROKE_COLOR_WITH_FACE_FOREGROUND(context, face)         \
   do {                                                                  \
-    CGColorRef refcol_ = get_cgcolor (NS_FACE_FOREGROUND (face));       \
-    CGContextSetStrokeColorWithColor (context, refcol_);                \
-    CGColorRelease (refcol_);                                           \
+    CGColorRef refcol = get_cgcolor (NS_FACE_FOREGROUND (face));        \
+    CGContextSetStrokeColorWithColor (context, refcol);                 \
+    CGColorRelease (refcol);                                            \
   } while (0)
 
 
@@ -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 c9f955000ac..37462cf49e2 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);
-- 
2.34.1

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Eli Zaretskii]

> From: Daniel Martín <mardani29@yahoo.es>
> Cc: 62573@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,  Al Haji-Ali
>  <abdo.haji.ali@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.

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Eli Zaretskii]

> From: Daniel Martín <mardani29@yahoo.es>
> Cc: Al Haji-Ali <abdo.haji.ali@gmail.com>,  62573@debbugs.gnu.org,
>  luangruo@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.

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Martín <mardani29@yahoo.es>
>> Cc: Al Haji-Ali <abdo.haji.ali@gmail.com>,  62573@debbugs.gnu.org,
>>  luangruo@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.

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Daniel Martín <mardani29@yahoo.es>,
>   abdo.haji.ali@gmail.com,
>   62573@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...

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Martín <mardani29@yahoo.es>
>> Cc: Al Haji-Ali <abdo.haji.ali@gmail.com>,  62573@debbugs.gnu.org,
>>  luangruo@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).

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Eli Zaretskii]

> From: Daniel Martín <mardani29@yahoo.es>
> Cc: luangruo@yahoo.com,  62573@debbugs.gnu.org,  abdo.haji.ali@gmail.com
> Date: Sun, 02 Apr 2023 13:01:07 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Daniel Martín <mardani29@yahoo.es>
> >> Cc: Al Haji-Ali <abdo.haji.ali@gmail.com>,  62573@debbugs.gnu.org,
> >>  luangruo@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.

bug#62573: 29.0.60; Cursor color not being inverted in emacs-29

[Al Haji-Ali]

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