bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Daniel Pettersson]

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


On macOS overlay-arrow is not draw on top of bitmaps already present
in the fringe, like its done on X Window System (src/xterm.c).  This is
to the detriment of gdb-mi.el where the overlay-arrow hides the
breakpoint when they exist on the same row.

It can be reproduced with the following:
(require 'gdb-mi)
(defvar overlay-arrow (make-marker))
(add-to-list 'overlay-arrow-variable-list 'overlay-arrow)
(setq fringe-indicator-alist '((overlay-arrow . hollow-right-triangle)))
(overlay-put (make-overlay (pos-bol) (1+ (pos-bol))) 'before-string
             (propertize " " 'display
                         `(left-fringe breakpoint warning)))
(move-marker overlay-arrow (pos-bol))

I expect that macOS is not using overlay_p param as intended as fixed by
attached patch.  But I was having an hard time finding any documentation
proving my point other then the implementation of it's X Window System
sibling in src/xterm.c.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-inconsistency-in-bitmap-overlay-drawing-for-macO.patch --]
[-- Type: text/x-patch, Size: 2285 bytes --]

From 492a26a4296fc8a7558b488796840c22c977d75c Mon Sep 17 00:00:00 2001
From: Daniel Pettersson <daniel@dpettersson.net>
Date: Mon, 24 Jun 2024 23:16:59 +0200
Subject: [PATCH] Fix inconsistency in bitmap overlay drawing for macOS

* src/nsterm.m (ns_draw_fringe_bitmap): Respect overlay_p by not
clearing fringe if set, as its done in xterm.
---
 src/nsterm.m | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index 794630de1c1..fc9133071ed 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2961,24 +2961,28 @@ Hide the window (X11 semantics)
   NSTRACE_MSG ("which:%d cursor:%d overlay:%d width:%d height:%d period:%d",
                p->which, p->cursor_p, p->overlay_p, p->wd, p->h, p->dh);
 
-  /* Work out the rectangle we will need to clear.  */
-  clearRect = NSMakeRect (p->x, p->y, p->wd, p->h);
+  /* Clear screen unless overlay.  */
+  if ( !p->overlay_p )
+    {
+      /* Work out the rectangle we will need to clear.  */
+      clearRect = NSMakeRect (p->x, p->y, p->wd, p->h);
 
-  if (p->bx >= 0 && !p->overlay_p)
-    clearRect = NSUnionRect (clearRect, NSMakeRect (p->bx, p->by, p->nx, p->ny));
+      if ( p->bx >= 0 )
+        clearRect = NSUnionRect (clearRect, NSMakeRect (p->bx, p->by, p->nx, p->ny));
 
-  /* Handle partially visible rows.  */
-  clearRect = NSIntersectionRect (clearRect, rowRect);
+      /* Handle partially visible rows.  */
+      clearRect = NSIntersectionRect (clearRect, rowRect);
 
-  /* The visible portion of imageRect will always be contained within
-     clearRect.  */
-  ns_focus (f, &clearRect, 1);
-  if (! NSIsEmptyRect (clearRect))
-    {
-      NSTRACE_RECT ("clearRect", clearRect);
+      /* The visible portion of imageRect will always be contained
+	 within clearRect.  */
+      ns_focus (f, &clearRect, 1);
+      if ( !NSIsEmptyRect (clearRect) )
+        {
+          NSTRACE_RECT ("clearRect", clearRect);
 
-      [[NSColor colorWithUnsignedLong:face->background] set];
-      NSRectFill (clearRect);
+          [[NSColor colorWithUnsignedLong:face->background] set];
+          NSRectFill (clearRect);
+        }
     }
 
   NSBezierPath *bmp = [fringe_bmp objectForKey:[NSNumber numberWithInt:p->which]];
-- 
2.39.3 (Apple Git-145)


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


In GNU Emacs 30.0.50 (build 1, aarch64-apple-darwin23.1.0, NS
 appkit-2487.20 Version 14.1.1 (Build 23B81)) of 2024-06-24 built on
 Daniels-Air
Windowing system distributor 'Apple', version 10.3.2487
System Description:  macOS 14.1.1

Configured using:
 'configure --with-xwidgets'

Configured features:
ACL DBUS GLIB GNUTLS LCMS2 LIBXML2 MODULES NOTIFY KQUEUE NS PDUMPER PNG
RSVG SQLITE3 THREADS TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM XWIDGETS
ZLIB

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Eli Zaretskii]

> From: Daniel Pettersson <daniel@dpettersson.net>
> Date: Tue, 25 Jun 2024 00:04:27 +0200
> 
> On macOS overlay-arrow is not draw on top of bitmaps already present
> in the fringe, like its done on X Window System (src/xterm.c).  This is
> to the detriment of gdb-mi.el where the overlay-arrow hides the
> breakpoint when they exist on the same row.
> 
> It can be reproduced with the following:
> (require 'gdb-mi)
> (defvar overlay-arrow (make-marker))
> (add-to-list 'overlay-arrow-variable-list 'overlay-arrow)
> (setq fringe-indicator-alist '((overlay-arrow . hollow-right-triangle)))
> (overlay-put (make-overlay (pos-bol) (1+ (pos-bol))) 'before-string
>              (propertize " " 'display
>                          `(left-fringe breakpoint warning)))
> (move-marker overlay-arrow (pos-bol))
> 
> I expect that macOS is not using overlay_p param as intended as fixed by
> attached patch.  But I was having an hard time finding any documentation
> proving my point other then the implementation of it's X Window System
> sibling in src/xterm.c.

Can some macOS expert please review the proposed patch?

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Daniel Pettersson]

Eli Zaretskii <eliz@gnu.org> writes:

> Can some macOS expert please review the proposed patch?
Seems like they are hard to come by.

I have been using this patch since I reported the bug, how can I help
move this along?

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Eli Zaretskii]

> From: Daniel Pettersson <daniel@dpettersson.net>
> Cc: 71763@debbugs.gnu.org
> Date: Sun, 27 Oct 2024 11:26:50 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can some macOS expert please review the proposed patch?
> Seems like they are hard to come by.
> 
> I have been using this patch since I reported the bug, how can I help
> move this along?

Thanks, but maybe Gerd (CCed) will agree to look at this?

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Pettersson <daniel@dpettersson.net>
>> Cc: 71763@debbugs.gnu.org
>> Date: Sun, 27 Oct 2024 11:26:50 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Can some macOS expert please review the proposed patch?
>> Seems like they are hard to come by.
>> 
>> I have been using this patch since I reported the bug, how can I help
>> move this along?
>
> Thanks, but maybe Gerd (CCed) will agree to look at this?

The struct draw_fringe_bitmap_params is not documented very well, so
I've looked at fringe.c for what its overlay_p member means. I landed in
draw_fringe_bitmap_1, where draw_fringe_bitmap_params::overlay_p is set
if that function is called with an overlay parameter that fits some
criterion, which one can try to figure out in its caller
draw_fringe_bitmap. It apparently has something to do with drawing a
cursor in a fringe, and then also drawing an overlay arrow.

Didn't even know until a few minutes ago that the cursor can land in
the fringe. And I'm wondering if that ever happens in the left fringe,
and/or if the overlay arrow appears only on the left. Questions upon
questions, as usual.

Anyway. Assuming that I read the code correctly, that we are drawing a
cursor and overlay arrow on top of each other, then I think Daniel's
patch makes sense, because drawing the overlay arrow should not clear
under it and erase what was drawn for the cursor before.

That's about what I could find out. NS should do something different if
overlay_p is set or not, I think that's for sure. And if Daniel says it
works, that's goog enough for me I guess.

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Daniel Pettersson <daniel@dpettersson.net>,  71763@debbugs.gnu.org
> Date: Sun, 27 Oct 2024 14:23:49 +0100
> 
> > Thanks, but maybe Gerd (CCed) will agree to look at this?
> 
> The struct draw_fringe_bitmap_params is not documented very well, so
> I've looked at fringe.c for what its overlay_p member means. I landed in
> draw_fringe_bitmap_1, where draw_fringe_bitmap_params::overlay_p is set
> if that function is called with an overlay parameter that fits some
> criterion, which one can try to figure out in its caller
> draw_fringe_bitmap. It apparently has something to do with drawing a
> cursor in a fringe, and then also drawing an overlay arrow.

Thanks for taking a look.

> Didn't even know until a few minutes ago that the cursor can land in
> the fringe.

See overflow-newline-into-fringe.  It was added in Emacs 22 (and is ON
by default, as you can see by typing enough characters on a line and
ending the line with a newline, such that the last character before
the newline fits completely on the screen line).

> And I'm wondering if that ever happens in the left fringe,

It can, if the window shows RTL text, which starts at the right edge
of the window.

> and/or if the overlay arrow appears only on the left.

In an RTL buffer, the arrow should appear on the right.

> Anyway. Assuming that I read the code correctly, that we are drawing a
> cursor and overlay arrow on top of each other, then I think Daniel's
> patch makes sense, because drawing the overlay arrow should not clear
> under it and erase what was drawn for the cursor before.
> 
> That's about what I could find out. NS should do something different if
> overlay_p is set or not, I think that's for sure. And if Daniel says it
> works, that's goog enough for me I guess.

Thanks.  If possible, can you also tell what the current code does
wrong, that this patch fixes?

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> Didn't even know until a few minutes ago that the cursor can land in
>> the fringe.
>
> See overflow-newline-into-fringe.  It was added in Emacs 22 (and is ON
> by default, as you can see by typing enough characters on a line and
> ending the line with a newline, such that the last character before
> the newline fits completely on the screen line).

Indeed, never noticed, thanks!

>
>> And I'm wondering if that ever happens in the left fringe,
>
> It can, if the window shows RTL text, which starts at the right edge
> of the window.
>
>> and/or if the overlay arrow appears only on the left.
>
> In an RTL buffer, the arrow should appear on the right.

Yeah, of course :-).

>
>> Anyway. Assuming that I read the code correctly, that we are drawing a
>> cursor and overlay arrow on top of each other, then I think Daniel's
>> patch makes sense, because drawing the overlay arrow should not clear
>> under it and erase what was drawn for the cursor before.
>> 
>> That's about what I could find out. NS should do something different if
>> overlay_p is set or not, I think that's for sure. And if Daniel says it
>> works, that's goog enough for me I guess.
>
> Thanks.  If possible, can you also tell what the current code does
> wrong, that this patch fixes?

diff --git a/src/nsterm.m b/src/nsterm.m
index 794630de1c1..fc9133071ed 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2961,24 +2961,28 @@ Hide the window (X11 semantics)
   NSTRACE_MSG ("which:%d cursor:%d overlay:%d width:%d height:%d period:%d",
                p->which, p->cursor_p, p->overlay_p, p->wd, p->h, p->dh);
 
-  /* Work out the rectangle we will need to clear.  */
-  clearRect = NSMakeRect (p->x, p->y, p->wd, p->h);
+  /* Clear screen unless overlay.  */
+  if ( !p->overlay_p )
+    {
+      /* Work out the rectangle we will need to clear.  */
+      clearRect = NSMakeRect (p->x, p->y, p->wd, p->h);

If I read the diff correctly, this puts the clearing under the bitmap in
an if-statement so that it is only done if we do not draw intentionally
on top something drawn before.

Right, Daniel?

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Daniel Pettersson]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> If I read the diff correctly, this puts the clearing under the bitmap in
> an if-statement so that it is only done if we do not draw intentionally
> on top something drawn before.

> Right, Daniel?

Yup, w/o this we clear fringe on draw when I we should keep it intact
and draw over.

The only example I found and the reason for this fix is that we clear
fringe of overlays before drawing overlay-arrow.  Such that breakpoint
bitmap is removed when overlay-arrow is on the same line in gdb-mi.el.

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Gerd Möllmann]

Daniel Pettersson <daniel@dpettersson.net> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> If I read the diff correctly, this puts the clearing under the bitmap in
>> an if-statement so that it is only done if we do not draw intentionally
>> on top something drawn before.
>
>> Right, Daniel?
>
> Yup, w/o this we clear fringe on draw when I we should keep it intact
> and draw over.
>
> The only example I found and the reason for this fix is that we clear
> fringe of overlays before drawing overlay-arrow.  Such that breakpoint
> bitmap is removed when overlay-arrow is on the same line in gdb-mi.el.

From my POV, it could be installed. Eli will decide that.

bug#71763: [PATCH] Inconsistency in bitmap overlay drawing for macOS

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  71763@debbugs.gnu.org
> Date: Wed, 30 Oct 2024 08:11:17 +0100
> 
> Daniel Pettersson <daniel@dpettersson.net> writes:
> 
> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> >
> >> If I read the diff correctly, this puts the clearing under the bitmap in
> >> an if-statement so that it is only done if we do not draw intentionally
> >> on top something drawn before.
> >
> >> Right, Daniel?
> >
> > Yup, w/o this we clear fringe on draw when I we should keep it intact
> > and draw over.
> >
> > The only example I found and the reason for this fix is that we clear
> > fringe of overlays before drawing overlay-arrow.  Such that breakpoint
> > bitmap is removed when overlay-arrow is on the same line in gdb-mi.el.
> 
> >From my POV, it could be installed. Eli will decide that.

OK, thanks.  I installed this on the master branch, and I'm closing
this bug.