Fix resizing issues with the flicker fix and hide the tabline

ychin · ychin · commit ab71bd8d697c · 2018-12-04T00:09:53.000-08:00
Fix the misc resizing issues with the previous CoreText renderer commit,
in particular cases where zoom button was clicked, Vim initiated
resizing (e.g. ":set lines+=10"), font size changes (Cmd-+/-),
fullscreen toggles, etc.

- The core issue is that the order of operation for those are not
  consistent. Sometimes, MacVim changes window size first before letting
  Vim knows, but other times it lets Vim handle it before resizing (e.g.
  zoom).

- The new CoreText renderer's buffer needs to know when the size
  change in order to resize the buffer, and it wasn't doing it in the
  right spot. Fix it so that it's delayed until updateLayer: is called.
  By that time both MacVim and Vim should have already come to an
  agreement on the new size.

- Also, when using the new 10.14 buffer renderer, don't use
  [NSAnimationContext beginGrouping] to block the system from resizing
  the window, because it also suffers from the order of operation issue
  and sometimes endGrouping could get called before beginGrouping,
  causing the UI to appear frozen. Instead, just have updateLayer make a
  new image and copy over the old one to avoid the black flickering when
  resizing (which was what the begin/endGrouping was trying to solve to
  begin with), and the UI now works smoother as well (e.g. double
  clicking the border now works smoothly).

The previous change also set the window background color to whatever
default background color is which is fine but it affects the tabline
separator as well and makes it look jarring. The tabline separator is
mostly a relic of the older macOS versions, so disable it on new-ish
macOS verisons.

Also, update docs in the known issues section to make it clear there's
currently an issue in performance under Mojave. That will be removed
when the performance is fixed in the future.
diff --git a/runtime/doc/gui_mac.txt b/runtime/doc/gui_mac.txt
@@ -715,6 +715,9 @@ to use in normal mode and type ":set imd" followed by ":set noimd".
 This list is by no means exhaustive, it only enumerates some of the more
 prominent bugs/missing features.
 
+- Under macOS Mojave (10.14), the default renderer (Core Text renderer) has
+  some performance issues and scrolling is not as smooth as previous macOS
+  versions (10.13 or below).
 - Localized menus are not supported.  Choosing anything but "English" in the
   "International" pane of "System Prefences" may break the menus (and
   toolbar).
diff --git a/src/MacVim/MMCoreTextView.h b/src/MacVim/MMCoreTextView.h
@@ -41,13 +41,32 @@
     CGPoint                     *positions;
     NSMutableArray              *fontCache;
 
+    // Issue draws onto an CGImage that caches the drawn results instead of
+    // directly in drawRect:. This is the default behavior in cases where simply
+    // drawing incrementally in drawRect: doesn't work. Those cases are:
+    // 1. Non-native fullscreen
+    // 2. 10.14+ (views are always layer-backed which means the view buffer will
+    //    be cleared and we can't incrementally draw in drawRect:)
+    //    
+    // This can be configured by setting MMBufferedDrawingKey in user defaults.
+    BOOL                        cgBufferDrawEnabled;
+    BOOL                        cgBufferDrawNeedsUpdateContext;
+    CGContextRef                cgContext;
+    
+    // *Deprecated*
+    // Draw onto a CGLayer instead of lazily updating the view's buffer in
+    // drawRect: which is error-prone and relying on undocumented behaviors
+    // (that the OS will preserve the old buffer).
+    //
+    // This is deprecated. Use cgBufferDrawEnabled instead which is more
+    // efficient.
+    //    
+    // This can be configured by setting MMUseCGLayerAlwaysKey in user defaults.
     BOOL                        cgLayerEnabled;
     CGLayerRef                  cgLayer;
     CGContextRef                cgLayerContext;
     NSLock                      *cgLayerLock;
 
-    CGContextRef                cgContext;
-
     // These are used in MMCoreTextView+ToolTip.m
     id trackingRectOwner_;              // (not retained)
     void *trackingRectUserData_;
diff --git a/src/MacVim/MMCoreTextView.m b/src/MacVim/MMCoreTextView.m
@@ -132,6 +132,10 @@ - (id)initWithFrame:(NSRect)frame
 {
     if (!(self = [super initWithFrame:frame]))
         return nil;
+    
+    cgBufferDrawEnabled = [[NSUserDefaults standardUserDefaults]
+            boolForKey:MMBufferedDrawingKey];
+    cgBufferDrawNeedsUpdateContext = NO;
 
     cgLayerEnabled = [[NSUserDefaults standardUserDefaults]
             boolForKey:MMUseCGLayerAlwaysKey];
@@ -447,18 +451,31 @@ - (BOOL)_wantsKeyDownForEvent:(id)event
 }
 
 - (void)setFrameSize:(NSSize)newSize {
-    if (NSEqualSizes(newSize, self.bounds.size))
-        return;
-    if (!drawPending) {
-        [NSAnimationContext beginGrouping];
-        drawPending = YES;
+    if (!NSEqualSizes(newSize, self.bounds.size)) {
+        if (!drawPending && !cgBufferDrawEnabled) {
+            // When resizing a window, it will invalidate the buffer and cause
+            // MacVim to draw black until we get the draw commands from Vim and
+            // we draw them out in drawRect. Use beginGrouping to stop the
+            // window resize from happening until we get the draw calls.
+            //
+            // The updateLayer/cgBufferDrawEnabled path handles this differently
+            // and don't need this.
+            [NSAnimationContext beginGrouping];
+            drawPending = YES;
+        }
+        if (cgBufferDrawEnabled) {
+            cgBufferDrawNeedsUpdateContext = YES;
+        }
     }
+    
     [super setFrameSize:newSize];
-    [self updateCGContext];
 }
 
 - (void)viewDidChangeBackingProperties {
-    [self updateCGContext];
+    if (cgBufferDrawEnabled) {
+        cgBufferDrawNeedsUpdateContext = YES;
+    }
+    [super viewDidChangeBackingProperties];
 }
 
 - (void)keyDown:(NSEvent *)event
@@ -617,20 +634,81 @@ - (BOOL)isFlipped
 }
 
 - (void)updateCGContext {
-    if (cgContext || [[NSUserDefaults standardUserDefaults]
-                      boolForKey:MMBufferedDrawingKey]) {
+    if (cgContext) {
         CGContextRelease(cgContext);
-        NSRect backingRect = [self convertRectToBacking:self.bounds];
-        cgContext = CGBitmapContextCreate(NULL, NSWidth(backingRect), NSHeight(backingRect), 8, 0, self.window.colorSpace.CGColorSpace, kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host);
-        CGContextScaleCTM(cgContext, self.window.backingScaleFactor, self.window.backingScaleFactor);
+        cgContext = nil;
     }
+
+    NSRect backingRect = [self convertRectToBacking:self.bounds];
+    cgContext = CGBitmapContextCreate(NULL, NSWidth(backingRect), NSHeight(backingRect), 8, 0, self.window.colorSpace.CGColorSpace, kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host);
+    CGContextScaleCTM(cgContext, self.window.backingScaleFactor, self.window.backingScaleFactor);
+    
+    cgBufferDrawNeedsUpdateContext = NO;
 }
 
 - (BOOL)wantsUpdateLayer {
-    return cgContext != nil;
+    return cgBufferDrawEnabled;
 }
 
 - (void)updateLayer {
+    if (!cgContext) {
+        [self updateCGContext];
+    } else if (cgBufferDrawNeedsUpdateContext) {
+        if ([drawData count] != 0) {
+            [self updateCGContext];
+        } else {
+            // In this case, we don't have a single draw command, meaning that
+            // Vim hasn't caught up yet and hasn't issued draw commands. We
+            // don't want to use [NSAnimationContext beginGrouping] as it's
+            // fragile (we may miss the endGrouping call due to order of
+            // operation), and also it makes the animation jerky.
+            // Instead, copy the image to the new context and align it to the
+            // top left and make sure it doesn't stretch. This makes the
+            // resizing smooth while Vim tries to catch up in issuing draws.
+            CGImageRef oldImage = CGBitmapContextCreateImage(cgContext);
+
+            [self updateCGContext]; // This will make a new cgContext
+            
+            CGContextSaveGState(cgContext);
+            CGContextSetBlendMode(cgContext, kCGBlendModeCopy);
+            
+            // Filling the background so the edge won't be black.
+            NSRect newRect = [self bounds];
+            float r = [defaultBackgroundColor redComponent];
+            float g = [defaultBackgroundColor greenComponent];
+            float b = [defaultBackgroundColor blueComponent];
+            float a = [defaultBackgroundColor alphaComponent];
+            CGContextSetRGBFillColor(cgContext, r, g, b, a);
+            CGContextFillRect(cgContext, *(CGRect*)&newRect);
+            CGContextSetBlendMode(cgContext, kCGBlendModeNormal);
+
+            // Copy the old image over to the new image, and make sure to
+            // respect scaling and remember that CGImage's Y origin is
+            // bottom-left.
+            CGFloat scale = self.window.backingScaleFactor;
+            size_t oldWidth = CGImageGetWidth(oldImage) / scale;
+            size_t oldHeight = CGImageGetHeight(oldImage) / scale;
+            CGFloat newHeight = newRect.size.height;
+            NSRect imageRect = NSMakeRect(0, newHeight - oldHeight, (CGFloat)oldWidth, (CGFloat)oldHeight);
+
+            CGContextDrawImage(cgContext, imageRect, oldImage);
+            CGImageRelease(oldImage);
+            CGContextRestoreGState(cgContext);
+        }
+    }
+    
+    // Now issue the batched draw commands
+    if ([drawData count] != 0) {
+        [NSGraphicsContext saveGraphicsState];
+        NSGraphicsContext.currentContext = [NSGraphicsContext graphicsContextWithCGContext:cgContext flipped:self.flipped];
+        id data;
+        NSEnumerator *e = [drawData objectEnumerator];
+        while ((data = [e nextObject]))
+            [self batchDrawData:data];
+        [drawData removeAllObjects];
+        [NSGraphicsContext restoreGraphicsState];
+    }
+
     CGImageRef contentsImage = CGBitmapContextCreateImage(cgContext);
     self.layer.contents = (id)contentsImage;
     CGImageRelease(contentsImage);
@@ -683,11 +761,24 @@ - (void)drawRect:(NSRect)rect
 
 - (void)performBatchDrawWithData:(NSData *)data
 {
-    if (cgContext) {
-        [NSGraphicsContext saveGraphicsState];
-        NSGraphicsContext.currentContext = [NSGraphicsContext graphicsContextWithCGContext:cgContext flipped:self.flipped];
-        [self batchDrawData:data];
-        [NSGraphicsContext restoreGraphicsState];
+    if (cgBufferDrawEnabled) {
+        // We batch up all the commands and actually perform the draw at
+        // updateLayer. The reason is that right now MacVim has a lot of
+        // different paths that could change the view size (zoom, user resizing
+        // from either dragging border or another program, Cmd-+/- to change
+        // font size, fullscreen, etc). Those different paths don't currently
+        // have a consistent order of operation of (Vim or MacVim go first), so
+        // sometimes Vim gets updated and issue a batch draw first, but
+        // sometimes MacVim gets notified first (e.g. when window is resized).
+        // If frame size has changed we need to call updateCGContext but we
+        // can't do it here because of the order of operation issue. That's why
+        // we wait till updateLayer to do it where everything has already been
+        // done and settled.
+        //
+        // Note: Should probably refactor the different ways window size could
+        // be changed and unify them instead of the status quo of spaghetti.
+        [drawData addObject:data];
+        [self setNeedsDisplay:YES];
     } else if (cgLayerEnabled && drawData.count == 0 && [self getCGContext]) {
         [cgLayerLock lock];
         [self batchDrawData:data];
@@ -748,13 +839,13 @@ - (CGContextRef)getCGContext
 
 - (void)setNeedsDisplayCGLayerInRect:(CGRect)rect
 {
-    if (cgLayerEnabled || cgContext)
+    if (cgLayerEnabled)
        [self setNeedsDisplayInRect:rect];
 }
 
 - (void)setNeedsDisplayCGLayer:(BOOL)flag
 {
-    if (cgLayerEnabled || cgContext)
+    if (cgLayerEnabled)
        [self setNeedsDisplay:flag];
 }
 
diff --git a/src/MacVim/MMWindowController.m b/src/MacVim/MMWindowController.m
@@ -1544,11 +1544,17 @@ - (void)updateTablineSeparator
     BOOL windowTextured = ([decoratedWindow styleMask] &
                             NSWindowStyleMaskTexturedBackground) != 0;
     BOOL hideSeparator  = NO;
-
-    if (fullScreenEnabled || tabBarVisible)
+    
+    if (floor(NSAppKitVersionNumber) >= NSAppKitVersionNumber10_10) {
+        // The tabline separator is mostly an old feature and not necessary
+        // modern macOS versions.
         hideSeparator = YES;
-    else
-        hideSeparator = toolbarHidden && !windowTextured;
+    } else {
+        if (fullScreenEnabled || tabBarVisible)
+            hideSeparator = YES;
+        else
+            hideSeparator = toolbarHidden && !windowTextured;
+    }
 
     [self hideTablineSeparator:hideSeparator];
 }
diff --git a/src/MacVim/MacVim.h b/src/MacVim/MacVim.h
@@ -45,6 +45,9 @@
 #ifndef NSAppKitVersionNumber10_12
 # define NSAppKitVersionNumber10_12 1504
 #endif
+#ifndef NSAppKitVersionNumber10_13
+# define NSAppKitVersionNumber10_13 1561
+#endif
 
 #if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_12
 // Deprecated constants in 10.12 SDK
