From 0db547d4593483c12f68a713b9884883a9eaa66d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= Date: Sun, 2 Dec 2018 00:06:15 -0500 Subject: [PATCH 1/2] Fix rendering issues with the 10.14 SDK. Before this commit, the Core Text renderer relied on a legacy behavior of NSView to keep its content across draws. setNeedsDisplayInRect: drew over parts of the existing content as drawing commands were received, without ever redrawing old content. Layer backed views don't preserve content between draws and may be asked to redraw at any time, and layer backing is default on in the 10.14 SDK. This change adds a way to draw to a CGBitmapContext and then display that in the view's layer. It's similar to the CGLayer path, but I wasn't able to get the CGLayer path to work without hanging or crashing when I scrolled. My best guess from looking at stack traces is that using CGContextDrawLayerInRect to draw a layer into itself doesn't actually copy pixels, but adds the self-draw as an action to be performed when the CGLayer is drawn into a bitmap context. Scrolling stacks these actions, which either hangs or overflows the stack when drawn. The new code is controlled by the MMBufferedDrawing user default, which is on by default on macOS >= 10.14 with this change. Fixes #751. --- src/MacVim/MMAppController.m | 1 + src/MacVim/MMCoreTextView.h | 2 ++ src/MacVim/MMCoreTextView.m | 59 ++++++++++++++++++++++++++++++--- src/MacVim/MMVimView.m | 3 +- src/MacVim/MMWindowController.m | 1 + src/MacVim/Miscellaneous.h | 2 ++ src/MacVim/Miscellaneous.m | 12 +++++++ 7 files changed, 74 insertions(+), 6 deletions(-) diff --git a/src/MacVim/MMAppController.m b/src/MacVim/MMAppController.m index 157319d5a4..91e0fdb08c 100644 --- a/src/MacVim/MMAppController.m +++ b/src/MacVim/MMAppController.m @@ -248,6 +248,7 @@ + (void)initialize [NSNumber numberWithBool:YES], MMNativeFullScreenKey, [NSNumber numberWithDouble:0.25], MMFullScreenFadeTimeKey, [NSNumber numberWithBool:NO], MMUseCGLayerAlwaysKey, + @(shouldUseBufferedDrawing()), MMBufferedDrawingKey, [NSNumber numberWithBool:YES], MMShareFindPboardKey, nil]; diff --git a/src/MacVim/MMCoreTextView.h b/src/MacVim/MMCoreTextView.h index a085013057..ab58b89e61 100644 --- a/src/MacVim/MMCoreTextView.h +++ b/src/MacVim/MMCoreTextView.h @@ -46,6 +46,8 @@ 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 index 96610dcdc6..4d7aa8438b 100644 --- a/src/MacVim/MMCoreTextView.m +++ b/src/MacVim/MMCoreTextView.m @@ -168,6 +168,8 @@ - (void)dealloc [drawData release]; drawData = nil; [fontCache release]; fontCache = nil; + CGContextRelease(cgContext); cgContext = nil; + [helper setTextView:nil]; [helper release]; helper = nil; @@ -215,6 +217,7 @@ - (void)setDefaultColorsBackground:(NSColor *)bgColor [defaultForegroundColor release]; defaultForegroundColor = fgColor ? [fgColor retain] : nil; } + [self setNeedsDisplay:YES]; } - (NSColor *)defaultBackgroundColor @@ -444,11 +447,18 @@ - (BOOL)_wantsKeyDownForEvent:(id)event } - (void)setFrameSize:(NSSize)newSize { - if (!drawPending && !NSEqualSizes(newSize, self.frame.size) && drawData.count == 0) { + if (NSEqualSizes(newSize, self.bounds.size)) + return; + if (!drawPending) { [NSAnimationContext beginGrouping]; drawPending = YES; } [super setFrameSize:newSize]; + [self updateCGContext]; +} + +- (void)viewDidChangeBackingProperties { + [self updateCGContext]; } - (void)keyDown:(NSEvent *)event @@ -606,6 +616,26 @@ - (BOOL)isFlipped return NO; } +- (void)updateCGContext { + if (cgContext || [[NSUserDefaults standardUserDefaults] + boolForKey:MMBufferedDrawingKey]) { + 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); + } +} + +- (BOOL)wantsUpdateLayer { + return cgContext != nil; +} + +- (void)updateLayer { + CGImageRef contentsImage = CGBitmapContextCreateImage(cgContext); + self.layer.contents = (id)contentsImage; + CGImageRelease(contentsImage); +} + - (void)drawRect:(NSRect)rect { NSGraphicsContext *context = [NSGraphicsContext currentContext]; @@ -653,7 +683,12 @@ - (void)drawRect:(NSRect)rect - (void)performBatchDrawWithData:(NSData *)data { - if (cgLayerEnabled && drawData.count == 0 && [self getCGContext]) { + if (cgContext) { + [NSGraphicsContext saveGraphicsState]; + NSGraphicsContext.currentContext = [NSGraphicsContext graphicsContextWithCGContext:cgContext flipped:self.flipped]; + [self batchDrawData:data]; + [NSGraphicsContext restoreGraphicsState]; + } else if (cgLayerEnabled && drawData.count == 0 && [self getCGContext]) { [cgLayerLock lock]; [self batchDrawData:data]; [cgLayerLock unlock]; @@ -669,6 +704,9 @@ - (void)performBatchDrawWithData:(NSData *)data - (void)setCGLayerEnabled:(BOOL)enabled { + if (cgContext) + return; + cgLayerEnabled = enabled; if (!cgLayerEnabled) @@ -710,13 +748,13 @@ - (CGContextRef)getCGContext - (void)setNeedsDisplayCGLayerInRect:(CGRect)rect { - if (cgLayerEnabled) + if (cgLayerEnabled || cgContext) [self setNeedsDisplayInRect:rect]; } - (void)setNeedsDisplayCGLayer:(BOOL)flag { - if (cgLayerEnabled) + if (cgLayerEnabled || cgContext) [self setNeedsDisplay:flag]; } @@ -1491,7 +1529,18 @@ - (void)drawString:(const UniChar *)chars length:(UniCharCount)length - (void)scrollRect:(NSRect)rect lineCount:(int)count { - if (cgLayerEnabled) { + if (cgContext) { + NSRect fromRect = NSOffsetRect(self.bounds, 0, -count * cellSize.height); + NSRect toRect = NSOffsetRect(rect, 0, -count * cellSize.height); + CGContextSaveGState(cgContext); + CGContextClipToRect(cgContext, toRect); + CGContextSetBlendMode(cgContext, kCGBlendModeCopy); + CGImageRef contentsImage = CGBitmapContextCreateImage(cgContext); + CGContextDrawImage(cgContext, fromRect, contentsImage); + CGImageRelease(contentsImage); + CGContextRestoreGState(cgContext); + [self setNeedsDisplayCGLayerInRect:toRect]; + } else if (cgLayerEnabled) { CGContextRef context = [self getCGContext]; int yOffset = count * cellSize.height; NSRect clipRect = rect; diff --git a/src/MacVim/MMVimView.m b/src/MacVim/MMVimView.m index fa5baeb759..79c9c2e881 100644 --- a/src/MacVim/MMVimView.m +++ b/src/MacVim/MMVimView.m @@ -189,7 +189,7 @@ - (void)dealloc - (BOOL)isOpaque { - return YES; + return textView.defaultBackgroundColor.alphaComponent == 1; } - (void)drawRect:(NSRect)rect @@ -511,6 +511,7 @@ - (void)setDefaultColorsBackground:(NSColor *)back foreground:(NSColor *)fore MMScroller *sb = [scrollbars objectAtIndex:i]; [sb setNeedsDisplay:YES]; } + [self setNeedsDisplay:YES]; } diff --git a/src/MacVim/MMWindowController.m b/src/MacVim/MMWindowController.m index 5fa8ab47a7..3795090587 100644 --- a/src/MacVim/MMWindowController.m +++ b/src/MacVim/MMWindowController.m @@ -543,6 +543,7 @@ - (void)setDefaultColorsBackground:(NSColor *)back foreground:(NSColor *)fore [decoratedWindow setOpaque:isOpaque]; if (fullScreenWindow) [fullScreenWindow setOpaque:isOpaque]; + [decoratedWindow setBackgroundColor:back]; [vimView setDefaultColorsBackground:back foreground:fore]; } diff --git a/src/MacVim/Miscellaneous.h b/src/MacVim/Miscellaneous.h index d7f6227e1a..db11562c73 100644 --- a/src/MacVim/Miscellaneous.h +++ b/src/MacVim/Miscellaneous.h @@ -53,6 +53,7 @@ extern NSString *MMNativeFullScreenKey; extern NSString *MMUseMouseTimeKey; extern NSString *MMFullScreenFadeTimeKey; extern NSString *MMUseCGLayerAlwaysKey; +extern NSString *MMBufferedDrawingKey; // Enum for MMUntitledWindowKey @@ -156,3 +157,4 @@ NSArray *normalizeFilenames(NSArray *filenames); BOOL shouldUseYosemiteTabBarStyle(); BOOL shouldUseMojaveTabBarStyle(); +BOOL shouldUseBufferedDrawing(); diff --git a/src/MacVim/Miscellaneous.m b/src/MacVim/Miscellaneous.m index 2796712115..657fdba3e1 100644 --- a/src/MacVim/Miscellaneous.m +++ b/src/MacVim/Miscellaneous.m @@ -49,6 +49,7 @@ NSString *MMUseMouseTimeKey = @"MMUseMouseTime"; NSString *MMFullScreenFadeTimeKey = @"MMFullScreenFadeTime"; NSString *MMUseCGLayerAlwaysKey = @"MMUseCGLayerAlways"; +NSString *MMBufferedDrawingKey = @"MMBufferedDrawing"; @@ -316,3 +317,14 @@ - (NSInteger)tag #endif return false; } + +BOOL +shouldUseBufferedDrawing() +{ +#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_14 + if (@available(macos 10.14, *)) { + return YES; + } +#endif + return NO; +} From ab71bd8d697c042879594bbe30b64bc681411426 Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Mon, 3 Dec 2018 01:40:52 -0800 Subject: [PATCH 2/2] Fix resizing issues with the flicker fix and hide the tabline 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. --- runtime/doc/gui_mac.txt | 3 + src/MacVim/MMCoreTextView.h | 23 +++++- src/MacVim/MMCoreTextView.m | 131 +++++++++++++++++++++++++++----- src/MacVim/MMWindowController.m | 14 +++- src/MacVim/MacVim.h | 3 + 5 files changed, 148 insertions(+), 26 deletions(-) diff --git a/runtime/doc/gui_mac.txt b/runtime/doc/gui_mac.txt index d6c620368e..b77af5b092 100644 --- 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 index ab58b89e61..cfabfa66bd 100644 --- 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 index 4d7aa8438b..7f7a47a98e 100644 --- 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 index 3795090587..d8578aefc5 100644 --- 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 index 36ae1edb48..e7fae69f85 100644 --- 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