bug#32932: [PATCH v2] Fix more drawing bugs in NS port (bug#32932)

[Alan Third <alan@idiocy.org>]

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

On Mon, Nov 19, 2018 at 06:30:05PM -0800, Aaron Jensen wrote:
> On November 19, 2018 at 2:35:51 PM, Alan Third
> (alan@idiocy.org(mailto:alan@idiocy.org)) wrote:
> 
> > Given that Emacs 26.2 is in pre‐release now I’m inclined to leave
> > emacs-26 as‐is for now, unless we require the 8th of November patch
> > (Further changes to NS drawing (bug#32932))? I don’t think we do as
> > iirc it just solves a minor redrawing issue (i.e. Emacs is still
> > usable without it). Is that right?
> 
> Yes, it is still usable, though if I remember correctly it lessens the
> overall blank glitches (which still happen even w/ the patch). So yes,
> it’s usable, but you do occasionally have to resize the window or do
> something else to get it to repaint. You still have to w/ the patch,
> just less often.

Final (hopefully) version of this patch attached.

I guess there are still issues with it, but we’re running out of time
to sort them for Emacs 26.2.

From a practical stand point I don’t think this is really any
different from the last one, but it has some comment changes and a
change to how we handle requests to copy parts of the screen, since
they were the major issue still with ghosting cursors.

I probably need to double check it’s OK on GNUstep, but I don’t
foresee any issues there.
-- 
Alan Third

[-- Attachment #2: v3-0001-Fix-more-drawing-bugs-in-NS-port-bug-32932.patch --]
[-- Type: text/plain, Size: 11236 bytes --]

From a55e5259341e054023727de4dc86b77c7a7d5db6 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Mon, 29 Oct 2018 15:37:35 +0000
Subject: [PATCH v3] Fix more drawing bugs in NS port (bug#32932)

* src/nsterm.m (ns_row_rect): New function.
(ns_clip_to_row): Remove function.
(ns_copy_bits): Fix mistake.
(ns_shift_glyphs_for_insert): Mark the frame as dirty instead of
directly copying.
(ns_draw_fringe_bitmap): Stop using ns_clip_to_row.
(ns_draw_window_cursor): Stop using ns_clip_to_row and perform a
display when not in redisplay.
(ns_update_window_begin): Remove redundant code that never executes.
([EmacsView drawRect:]): Show the rectangle being exposed in NSTRACE.
* src/xdisp.c (expose_window_tree) [HAVE_NS]:
(expose_frame) [HAVE_NS]: Redraw even if the frame is garbaged.
---
 src/nsterm.m | 149 +++++++++++++++++++++++++++------------------------
 src/xdisp.c  |  15 +++++-
 2 files changed, 91 insertions(+), 73 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index 4b5d025ee3..948dd1da2e 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -796,6 +796,27 @@ Free a pool and temporary objects it refers to (callable from C)
 }
 
 
+static NSRect
+ns_row_rect (struct window *w, struct glyph_row *row,
+               enum glyph_row_area area)
+/* Get the row as an NSRect.  */
+{
+  struct frame *f = XFRAME (WINDOW_FRAME (w));
+  NSRect rect;
+  int window_x, window_y, window_width;
+
+  window_box (w, area, &window_x, &window_y, &window_width, 0);
+
+  rect.origin.x = window_x;
+  rect.origin.y = WINDOW_TO_FRAME_PIXEL_Y (w, max (0, row->y));
+  rect.origin.y = max (rect.origin.y, window_y);
+  rect.size.width = window_width;
+  rect.size.height = row->visible_height;
+
+  return rect;
+}
+
+
 /* ==========================================================================
 
     Focus (clipping) and screen update
@@ -1048,29 +1069,6 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen)
     if (! tbar_visible != ! [toolbar isVisible])
       [toolbar setVisible: tbar_visible];
   }
-
-  /* drawRect may have been called for say the minibuffer, and then clip path
-     is for the minibuffer.  But the display engine may draw more because
-     we have set the frame as garbaged.  So reset clip path to the whole
-     view.  */
-  /* FIXME: I don't think we need to do this.  */
-  if ([NSView focusView] == FRAME_NS_VIEW (f))
-    {
-      NSBezierPath *bp;
-      NSRect r = [view frame];
-      NSRect cr = [[view window] frame];
-      /* If a large frame size is set, r may be larger than the window frame
-         before constrained.  In that case don't change the clip path, as we
-         will clear in to the tool bar and title bar.  */
-      if (r.size.height
-          + FRAME_NS_TITLEBAR_HEIGHT (f)
-          + FRAME_TOOLBAR_HEIGHT (f) <= cr.size.height)
-        {
-          bp = [[NSBezierPath bezierPathWithRect: r] retain];
-          [bp setClip];
-          [bp release];
-        }
-    }
 #endif
 }
 
@@ -1206,28 +1204,6 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen)
 }
 
 
-static BOOL
-ns_clip_to_row (struct window *w, struct glyph_row *row,
-		enum glyph_row_area area, BOOL gc)
-/* --------------------------------------------------------------------------
-     Internal (but parallels other terms): Focus drawing on given row
-   -------------------------------------------------------------------------- */
-{
-  struct frame *f = XFRAME (WINDOW_FRAME (w));
-  NSRect clip_rect;
-  int window_x, window_y, window_width;
-
-  window_box (w, area, &window_x, &window_y, &window_width, 0);
-
-  clip_rect.origin.x = window_x;
-  clip_rect.origin.y = WINDOW_TO_FRAME_PIXEL_Y (w, max (0, row->y));
-  clip_rect.origin.y = max (clip_rect.origin.y, window_y);
-  clip_rect.size.width = window_width;
-  clip_rect.size.height = row->visible_height;
-
-  return ns_clip_to_rect (f, &clip_rect, 1);
-}
-
 /* ==========================================================================
 
     Visible bell and beep.
@@ -2692,7 +2668,7 @@ so some key presses (TAB) are swallowed by the system. */
 ns_copy_bits (struct frame *f, NSRect src, NSRect dest)
 {
   NSSize delta = NSMakeSize (dest.origin.x - src.origin.x,
-                             dest.origin.y - src.origin.y)
+                             dest.origin.y - src.origin.y);
   NSTRACE ("ns_copy_bits");
 
   if (FRAME_NS_VIEW (f))
@@ -2825,12 +2801,20 @@ so some key presses (TAB) are swallowed by the system. */
     External (RIF): copy an area horizontally, don't worry about clearing src
    -------------------------------------------------------------------------- */
 {
-  NSRect srcRect = NSMakeRect (x, y, width, height);
+  //NSRect srcRect = NSMakeRect (x, y, width, height);
   NSRect dstRect = NSMakeRect (x+shift_by, y, width, height);
 
   NSTRACE ("ns_shift_glyphs_for_insert");
 
-  ns_copy_bits (f, srcRect, dstRect);
+  /* This doesn't work now as we copy the "bits" before we've had a
+     chance to actually draw any changes to the screen.  This means in
+     certain circumstances we end up with copies of the cursor all
+     over the place.  Just mark the area dirty so it is redrawn later.
+
+     FIXME: Work out how to do this properly.  */
+  // ns_copy_bits (f, srcRect, dstRect);
+
+  [FRAME_NS_VIEW (f) setNeedsDisplayInRect:dstRect];
 }
 
 
@@ -2911,6 +2895,9 @@ so some key presses (TAB) are swallowed by the system. */
   struct face *face = p->face;
   static EmacsImage **bimgs = NULL;
   static int nBimgs = 0;
+  NSRect clearRect = NSZeroRect;
+  NSRect imageRect = NSZeroRect;
+  NSRect rowRect = ns_row_rect (w, row, ANY_AREA);
 
   NSTRACE_WHEN (NSTRACE_GROUP_FRINGE, "ns_draw_fringe_bitmap");
   NSTRACE_MSG ("which:%d cursor:%d overlay:%d width:%d height:%d period:%d",
@@ -2925,25 +2912,40 @@ so some key presses (TAB) are swallowed by the system. */
       nBimgs = max_used_fringe_bitmap;
     }
 
-  /* Must clip because of partially visible lines.  */
-  if (ns_clip_to_row (w, row, ANY_AREA, YES))
+  /* Work out the rectangle we will composite into.  */
+  if (p->which)
+    imageRect = NSMakeRect (p->x, p->y, p->wd, p->h);
+
+  /* Work out the rectangle we will need to clear.  Because we're
+     compositing rather than blitting, we need to clear the area under
+     the image regardless of anything else.  */
+  if (!p->overlay_p)
+    {
+      clearRect = NSMakeRect (p->bx, p->by, p->nx, p->ny);
+      clearRect = NSUnionRect (clearRect, imageRect);
+    }
+  else
+    {
+      clearRect = imageRect;
+    }
+
+  /* Handle partially visible rows.  */
+  clearRect = NSIntersectionRect (clearRect, rowRect);
+
+  /* The visible portion of imageRect will always be contained within
+     clearRect.  */
+  if (ns_clip_to_rect (f, &clearRect, 1))
     {
-      if (!p->overlay_p)
+      if (! NSIsEmptyRect (clearRect))
         {
-          int bx = p->bx, by = p->by, nx = p->nx, ny = p->ny;
+          NSTRACE_RECT ("clearRect", clearRect);
 
-          if (bx >= 0 && nx > 0)
-            {
-              NSRect r = NSMakeRect (bx, by, nx, ny);
-              NSRectClip (r);
-              [ns_lookup_indexed_color (face->background, f) set];
-              NSRectFill (r);
-            }
+          [ns_lookup_indexed_color(face->background, f) set];
+          NSRectFill (clearRect);
         }
 
       if (p->which)
         {
-          NSRect r = NSMakeRect (p->x, p->y, p->wd, p->h);
           EmacsImage *img = bimgs[p->which - 1];
 
           if (!img)
@@ -2964,13 +2966,6 @@ so some key presses (TAB) are swallowed by the system. */
               xfree (cbits);
             }
 
-          NSTRACE_RECT ("r", r);
-
-          NSRectClip (r);
-          /* Since we composite the bitmap instead of just blitting it, we need
-             to erase the whole background.  */
-          [ns_lookup_indexed_color(face->background, f) set];
-          NSRectFill (r);
 
           {
             NSColor *bm_color;
@@ -2990,7 +2985,7 @@ so some key presses (TAB) are swallowed by the system. */
 
           NSTRACE_RECT ("fromRect", fromRect);
 
-          [img drawInRect: r
+          [img drawInRect: imageRect
                  fromRect: fromRect
                 operation: NSCompositingOperationSourceOver
                  fraction: 1.0
@@ -2998,7 +2993,7 @@ so some key presses (TAB) are swallowed by the system. */
                     hints: nil];
 #else
           {
-            NSPoint pt = r.origin;
+            NSPoint pt = imageRect.origin;
             pt.y += p->h;
             [img compositeToPoint: pt operation: NSCompositingOperationSourceOver];
           }
@@ -3088,7 +3083,9 @@ Note that CURSOR_WIDTH is meaningful only for (h)bar cursors.
   r.size.width = w->phys_cursor_width;
 
   /* Prevent the cursor from being drawn outside the text area.  */
-  if (ns_clip_to_row (w, glyph_row, TEXT_AREA, NO))
+  r = NSIntersectionRect (r, ns_row_rect (w, glyph_row, TEXT_AREA));
+
+  if (ns_clip_to_rect (f, &r, 1))
     {
       face = FACE_FROM_ID_OR_NULL (f, phys_cursor_glyph->face_id);
       if (face && NS_FACE_BACKGROUND (face)
@@ -3128,11 +3125,18 @@ Note that CURSOR_WIDTH is meaningful only for (h)bar cursors.
           NSRectFill (s);
           break;
         }
-      ns_reset_clipping (f);
 
       /* draw the character under the cursor */
       if (cursor_type != NO_CURSOR)
         draw_phys_cursor_glyph (w, glyph_row, DRAW_CURSOR);
+
+      ns_reset_clipping (f);
+    }
+  else if (! redisplaying_p)
+    {
+      /* If this function is called outside redisplay, it probably
+         means we need an immediate update.  */
+      [FRAME_NS_VIEW (f) display];
     }
 }
 
@@ -8096,6 +8100,9 @@ - (void)drawRect: (NSRect)rect
   for (int i = 0 ; i < numRects ; i++)
     {
       NSRect r = rectList[i];
+
+      NSTRACE_RECT ("r", r);
+
       expose_frame (emacsframe,
                     NSMinX (r), NSMinY (r),
                     NSWidth (r), NSHeight (r));
diff --git a/src/xdisp.c b/src/xdisp.c
index 357f0fb30c..808eab7e53 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -32258,7 +32258,14 @@ expose_window_tree (struct window *w, XRectangle *r)
   struct frame *f = XFRAME (w->frame);
   bool mouse_face_overwritten_p = false;
 
-  while (w && !FRAME_GARBAGED_P (f))
+  /* NS toolkits may have aleady modified the frame in expectation of
+     a successful redraw, so don't bail out here if the frame is
+     garbaged.  */
+  while (w
+#if !defined (HAVE_NS)
+         && !FRAME_GARBAGED_P (f)
+#endif
+         )
     {
       mouse_face_overwritten_p
 	|= (WINDOWP (w->contents)
@@ -32286,12 +32293,16 @@ expose_frame (struct frame *f, int x, int y, int w, int h)
 
   TRACE ((stderr, "expose_frame "));
 
-  /* No need to redraw if frame will be redrawn soon.  */
+#if !defined (HAVE_NS)
+  /* No need to redraw if frame will be redrawn soon except under NS
+     where the toolkit may have already modified the frame in
+     expectation of us redrawing it.  */
   if (FRAME_GARBAGED_P (f))
     {
       TRACE ((stderr, " garbaged\n"));
       return;
     }
+#endif
 
   /* If basic faces haven't been realized yet, there is no point in
      trying to redraw anything.  This can happen when we get an expose
-- 
2.19.1