From 767ffc4eefb6ad156bab93cec394abd6d86bc89b Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Wed, 5 Feb 2025 05:19:40 -0800 Subject: [PATCH] Reduce flicker when changing fonts/adding tabs in go+=k/fullscreen MacVim would previously show a quick flicker when adjusting font (e.g. Cmd =/-) or showing/hiding tabs/scroll bar when in fixed window size mode (guioptions+=k or full screen). This was because after the state change, Vim requests a resize asynchronously to the GUI to fit it in the window. MacVim does so after changing the font/showing the tab, leading to a momentary incorrect result before Vim then redraws the resized grid. In normal GVim this is not an issue because Vim requests the resize synchronously in a single-process environment, and we would like to avoid that as the message passing between Vim/MacVim and designed to be mostly non-blocking. To fix this, after receiving the Vim resize request, we block all further text rendering commands, until Vim has resized / redrawn, preventing the short period of time where text view is drawing the old state using the new font. For tabs / scroll bars, the text view itself has moved after the new layout, so we temporarily apply a render offset to make the text view pretend it didn't move and looks mostly the same to the user while we wait for Vim to redraw with the updated grid. There are some potential ways to still see flicker, but they are mostly edge cases: - When changing fonts, if Vim is slow and the user gets MacVim to re-draw the text view (e.g. dragging the window to resize) while we wait for Vim to resize, it would still draw an incorrect result (since it has the new font, but old text grid). This should realistically only happen if Vim takes an abnormal amount of time to respond. - For tabs / scrollbars we have a similar issue. We immediately place/remove them while we wait for Vim to resize, which could cause a small visual discontinuity (easiest way is to toggle `go+=e`). From testing, having the tab bar / etc immediately show up and hide feels better as the user feels like something has happened, so keeping the responsiveness is more important than delaying showing/hiding the tab bar for visual stability (not to mention the deferral is more complicated to implement). If Vim takes a long time to resize/redraw, this change could make font size change *feel* less responsive because nothing happens on the screen until the fully redrawn screen is shown. This is ok, and if Vim takes so long to resize then that's the actual issue to address. This change also removes unnecessary code: - Excessive and unnecessary redraws when showing/hiding tabs and setting fonts. They were written a long time ago as temporary hacks which survived till now. From testing this makes changing font size and showing/hiding tabs feel a fair bit more responsive because Vim isn't trying to redraw over and over again now. - Stale "maximize" code that has long been unused. It was trying to solve a similar issue but long obsolete and disabled. --- src/MacVim/MMBackend.m | 8 +- src/MacVim/MMCoreTextView.h | 6 +- src/MacVim/MMCoreTextView.m | 18 +- src/MacVim/MMTextView.h | 2 + src/MacVim/MMVimController.m | 15 +- src/MacVim/MMVimView.m | 11 +- src/MacVim/MMWindowController.h | 15 +- src/MacVim/MMWindowController.m | 266 ++++++++++++--------------- src/MacVim/MacVim.h | 2 +- src/MacVim/MacVimTests/MacVimTests.m | 165 ++++++++++++++++- src/MacVim/gui_macvim.m | 6 + src/gui.c | 3 +- 12 files changed, 328 insertions(+), 189 deletions(-) diff --git a/src/MacVim/MMBackend.m b/src/MacVim/MMBackend.m index 0d35f841a3..66ea170252 100644 --- a/src/MacVim/MMBackend.m +++ b/src/MacVim/MMBackend.m @@ -2206,10 +2206,8 @@ - (void)handleInputEvent:(int)msgid data:(NSData *)data const void *bytes = [data bytes]; int idx = *((int*)bytes) + 1; send_tabline_menu_event(idx, TABLINE_MENU_CLOSE); - [self redrawScreen]; } else if (AddNewTabMsgID == msgid) { send_tabline_menu_event(0, TABLINE_MENU_NEW); - [self redrawScreen]; } else if (DraggedTabMsgID == msgid) { if (!data) return; const void *bytes = [data bytes]; @@ -2259,8 +2257,6 @@ - (void)handleInputEvent:(int)msgid data:(NSData *)data [self queueMessage:msgid data:d]; gui_resize_shell(cols, rows); - } else if (ResizeViewMsgID == msgid) { - [self queueMessage:msgid data:data]; } else if (ExecuteMenuMsgID == msgid) { NSDictionary *attrs = [NSDictionary dictionaryWithData:data]; if (attrs) { @@ -2332,7 +2328,7 @@ - (void)handleInputEvent:(int)msgid data:(NSData *)data [self setImState:YES]; } else if (DeactivatedImMsgID == msgid) { [self setImState:NO]; - } else if (BackingPropertiesChangedMsgID == msgid) { + } else if (RedrawMsgID == msgid) { [self redrawScreen]; } else if (LoopBackMsgID == msgid) { // This is a debug message used for confirming a message has been @@ -2725,8 +2721,6 @@ - (void)handleSetFont:(NSData *)data CONVERT_FROM_UTF8_FREE(ws); } CONVERT_FROM_UTF8_FREE(s); - - [self redrawScreen]; } - (void)handleDropFiles:(NSData *)data diff --git a/src/MacVim/MMCoreTextView.h b/src/MacVim/MMCoreTextView.h index 6dd64125d4..15ec0b9c64 100644 --- a/src/MacVim/MMCoreTextView.h +++ b/src/MacVim/MMCoreTextView.h @@ -71,10 +71,6 @@ NS_ASSUME_NONNULL_BEGIN MMTextViewHelper *helper; - NSMutableDictionary *fontVariants; - NSMutableSet *characterStrings; - NSMutableDictionary *> *characterLines; - // These are used in MMCoreTextView+ToolTip.m id trackingRectOwner_; // (not retained) void *trackingRectUserData_; @@ -82,6 +78,8 @@ NS_ASSUME_NONNULL_BEGIN NSString* toolTip_; } +@property (nonatomic) NSSize drawRectOffset; ///< A render offset to apply to the draw rects. This is currently only used in specific situations when rendering is blocked. + - (instancetype)initWithFrame:(NSRect)frame; // diff --git a/src/MacVim/MMCoreTextView.m b/src/MacVim/MMCoreTextView.m index 5b91b1ad51..57f67a42a5 100644 --- a/src/MacVim/MMCoreTextView.m +++ b/src/MacVim/MMCoreTextView.m @@ -222,6 +222,10 @@ static void grid_free(Grid *grid) { @implementation MMCoreTextView { Grid grid; + NSMutableSet *characterStrings; ///< Storage for characters in the grid + + NSMutableDictionary *fontVariants; ///< Cache for fonts used for each variant (e.g. italic) + NSMutableDictionary *> *characterLines; ///< Cache for built CTLine objects BOOL alignCmdLineToBottom; ///< Whether to pin the Vim command-line to the bottom of the window int cmdlineRow; ///< Row number (0-indexed) where the cmdline starts. Used for pinning it to the bottom if desired. @@ -251,8 +255,9 @@ - (instancetype)initWithFrame:(NSRect)frame antialias = YES; [self setFont:[NSFont userFixedPitchFontOfSize:0]]; - fontVariants = [[NSMutableDictionary alloc] init]; characterStrings = [[NSMutableSet alloc] init]; + + fontVariants = [[NSMutableDictionary alloc] init]; characterLines = [[NSMutableDictionary alloc] init]; helper = [[MMTextViewHelper alloc] init]; @@ -276,8 +281,8 @@ - (void)dealloc [fontWide release]; fontWide = nil; [defaultBackgroundColor release]; defaultBackgroundColor = nil; [defaultForegroundColor release]; defaultForegroundColor = nil; - [fontVariants release]; fontVariants = nil; [characterStrings release]; characterStrings = nil; + [fontVariants release]; fontVariants = nil; [characterLines release]; characterLines = nil; [helper setTextView:nil]; @@ -478,9 +483,7 @@ - (void)setFont:(NSFont *)newFont cellSize.width = columnspace + ceil(em * cellWidthMultiplier); cellSize.height = linespace + defaultLineHeightForFont(font); - [self clearAll]; [fontVariants removeAllObjects]; - [characterStrings removeAllObjects]; [characterLines removeAllObjects]; } @@ -498,9 +501,7 @@ - (void)setWideFont:(NSFont *)newFont fontWide = [newFont retain]; } - [self clearAll]; [fontVariants removeAllObjects]; - [characterStrings removeAllObjects]; [characterLines removeAllObjects]; } @@ -1485,6 +1486,9 @@ - (NSRect)rectForRow:(int)row column:(int)col numRows:(int)nr rect.size.width = nc*cellSize.width; rect.size.height = nr*cellSize.height; + rect.origin.x += _drawRectOffset.width; + rect.origin.y += _drawRectOffset.height; + // Under smooth resizing, full screen, or guioption-k; we frequently have a frame size that's not // aligned with the exact grid size. If the user has 'cursorline' set, or the color scheme uses // the NonText highlight group, this will leave a small gap on the right filled with bg color looking @@ -1505,6 +1509,7 @@ - (NSRect)rectForRow:(int)row column:(int)col numRows:(int)nr const CGFloat gapHeight = frame.size.height - grid.rows*cellSize.height - insetSize.height - insetBottom; if (row >= cmdlineRow) { rect.origin.y -= gapHeight; + rect.origin.y -= _drawRectOffset.height; // Pinning ignores draw rect offset for stability } else if (row + nr - 1 >= cmdlineRow) { // This is an odd case where the gap between cmdline and the top-aligned content is inside // the rect so we need to adjust the height as well. During rendering we draw line-by-line @@ -1512,6 +1517,7 @@ - (NSRect)rectForRow:(int)row column:(int)col numRows:(int)nr // the rect in setNeedsDisplayFromRow:. rect.size.height += gapHeight; rect.origin.y -= gapHeight; + rect.origin.y -= _drawRectOffset.height; // Pinning ignores draw rect offset for stability } } return rect; diff --git a/src/MacVim/MMTextView.h b/src/MacVim/MMTextView.h index 570dc1c3f6..b18ee08203 100644 --- a/src/MacVim/MMTextView.h +++ b/src/MacVim/MMTextView.h @@ -31,6 +31,8 @@ MMTextViewHelper *helper; } +@property (nonatomic) NSSize drawRectOffset; // Unused. Only used by MMCoreTextView + - (id)initWithFrame:(NSRect)frame; - (void)setPreEditRow:(int)row column:(int)col; diff --git a/src/MacVim/MMVimController.m b/src/MacVim/MMVimController.m index 2137dedf43..bff551ac1d 100644 --- a/src/MacVim/MMVimController.m +++ b/src/MacVim/MMVimController.m @@ -697,6 +697,11 @@ - (void)handleMessage:(int)msgid data:(NSData *)data break; case BatchDrawMsgID: { + if ([windowController isRenderBlocked]) { + // Drop all batch draw commands while blocked. If we end up + // changing out mind later we will need to ask Vim to redraw. + break; + } [[[windowController vimView] textView] performBatchDrawWithData:data]; } break; @@ -717,13 +722,11 @@ - (void)handleMessage:(int)msgid data:(NSData *)data case ShowTabBarMsgID: { [windowController showTabline:YES]; - [self sendMessage:BackingPropertiesChangedMsgID data:nil]; } break; case HideTabBarMsgID: { [windowController showTabline:NO]; - [self sendMessage:BackingPropertiesChangedMsgID data:nil]; } break; @@ -753,7 +756,13 @@ - (void)handleMessage:(int)msgid data:(NSData *)data case ResizeViewMsgID: { - [windowController resizeView]; + // This is sent when Vim wants MacVim to resize Vim to fit + // everything within the GUI window, usually because go+=k is set. + // Other gVim usually blocks on this but for MacVim it is async + // to reduce synchronization points so we schedule a resize for + // later. We ask to block any render from happening until we are + // done resizing to avoid a momentary annoying flicker. + [windowController resizeVimViewBlockRender]; } break; case SetWindowTitleMsgID: diff --git a/src/MacVim/MMVimView.m b/src/MacVim/MMVimView.m index 9a7a5a4781..c9840471ed 100644 --- a/src/MacVim/MMVimView.m +++ b/src/MacVim/MMVimView.m @@ -914,16 +914,7 @@ - (void)frameSizeMayHaveChanged:(BOOL)keepGUISize [textView setFrame:textViewRect]; // Immediately place the scrollbars instead of deferring till later here. - // Deferral ended up causing some bugs, in particular when in <10.14 - // CoreText renderer where [NSAnimationContext beginGrouping] is used to - // bundle state changes together and the deferred placeScrollbars would get - // the wrong data to use. An alternative would be to check for that and only - // call finishPlaceScrollbars once we call [NSAnimationContext endGrouping] - // but that makes the code mode complicated. Just do it here and the - // performance is fine as this gets called occasionally only - // (pendingPlaceScrollbars is mostly for the case if we are adding a lot of - // scrollbars at once we want to only call placeScrollbars once instead of - // doing it N times). + // Otherwise in situations like live resize we will see the scroll bars lag. self.pendingPlaceScrollbars = NO; [self placeScrollbars]; diff --git a/src/MacVim/MMWindowController.h b/src/MacVim/MMWindowController.h index 544385f8c2..4b8a0a2278 100644 --- a/src/MacVim/MMWindowController.h +++ b/src/MacVim/MMWindowController.h @@ -28,18 +28,22 @@ MMVimView *vimView; BOOL setupDone; BOOL windowPresented; - BOOL shouldResizeVimView; - BOOL shouldKeepGUISize; + + BOOL shouldResizeVimView; ///< Indicates there is a pending command to resize the Vim view + BOOL shouldKeepGUISize; ///< If on, the Vim view resize will try to fit in the existing window. If off, the window resizes to fit Vim view. + BOOL blockRenderUntilResize; ///< Indicates that there should be no text rendering until a Vim view resize is completed to avoid flicker. + BOOL shouldRestoreUserTopLeft; - BOOL shouldMaximizeWindow; int updateToolbarFlag; BOOL keepOnScreen; NSString *windowAutosaveKey; + BOOL fullScreenEnabled; ///< Whether full screen is on (native or not) MMFullScreenWindow *fullScreenWindow; ///< The window used for non-native full screen. Will only be non-nil when in non-native full screen. int fullScreenOptions; BOOL delayEnterFullScreen; NSRect preFullScreenFrame; + MMWindow *decoratedWindow; NSString *lastSetTitle; NSString *documentFilename; ///< File name of document being edited, used for the icon at the title bar. @@ -68,7 +72,10 @@ - (void)setTextDimensionsWithRows:(int)rows columns:(int)cols isLive:(BOOL)live keepGUISize:(BOOL)keepGUISize keepOnScreen:(BOOL)onScreen; -- (void)resizeView; +- (void)resizeVimViewAndWindow; +- (void)resizeVimView; +- (void)resizeVimViewBlockRender; +- (BOOL)isRenderBlocked; - (void)zoomWithRows:(int)rows columns:(int)cols state:(int)state; - (void)setTitle:(NSString *)title; - (void)setDocumentFilename:(NSString *)filename; diff --git a/src/MacVim/MMWindowController.m b/src/MacVim/MMWindowController.m index 8b195e28e5..05890bcedf 100644 --- a/src/MacVim/MMWindowController.m +++ b/src/MacVim/MMWindowController.m @@ -91,7 +91,6 @@ - (void)updateTablineSeparator; - (void)hideTablineSeparator:(BOOL)hide; - (void)doFindNext:(BOOL)next; - (void)updateToolbar; -- (BOOL)maximizeWindow:(int)options; - (void)applicationDidChangeScreenParameters:(NSNotification *)notification; - (void)enterNativeFullScreen; - (void)processAfterWindowPresentedQueue; @@ -414,6 +413,10 @@ - (void)setTextDimensionsWithRows:(int)rows columns:(int)cols isLive:(BOOL)live [vimView setDesiredRows:rows columns:cols]; vimView.pendingLiveResize = NO; + if (blockRenderUntilResize) { + blockRenderUntilResize = NO; + [vimView.textView setDrawRectOffset:NSZeroSize]; + } if (vimView.pendingLiveResizeQueued) { // There was already a new size queued while Vim was still processing // the last one. We need to immediately request another resize now that @@ -426,11 +429,11 @@ - (void)setTextDimensionsWithRows:(int)rows columns:(int)cols isLive:(BOOL)live // only one outstanding resize message at a time // inframeSizeMayHaveChanged:. vimView.pendingLiveResizeQueued = NO; - [self resizeView]; + [self resizeVimView]; } if (setupDone && !live && !keepGUISize) { - shouldResizeVimView = YES; + [self resizeVimViewAndWindow]; keepOnScreen = onScreen; } @@ -458,7 +461,20 @@ - (void)setTextDimensionsWithRows:(int)rows columns:(int)cols isLive:(BOOL)live } } -- (void)resizeView +/// Resize the Vim view to its desired size based on number of rows/cols. +/// Ignores the window size and force the window to resize along with it. +- (void)resizeVimViewAndWindow +{ + if (setupDone) + { + shouldResizeVimView = YES; + } +} + +/// Resize the Vim view to match the GUI window size. This makes sure the GUI +/// doesn't change its size and fit everything within it to match. If the text +/// view ends up changing size, will send a message to Vim to resize itself. +- (void)resizeVimView { if (setupDone) { @@ -467,6 +483,26 @@ - (void)resizeView } } +/// Resize the Vim view to match GUI window size, but also block any text +/// rendering from happening while we wait for Vim to be resized. This is used +/// to avoid any flickering as the current rendered texts are going to be +/// invalidated very soon as Vim will be resized and issue new draw commands. +/// +/// This happens when say we have guioptions+=k and the user changes the font +/// or shows the tab bar. +- (void)resizeVimViewBlockRender +{ + [self resizeVimView]; + if (shouldResizeVimView) { + blockRenderUntilResize = YES; + } +} + +- (BOOL)isRenderBlocked +{ + return blockRenderUntilResize; +} + - (void)zoomWithRows:(int)rows columns:(int)cols state:(int)state { [self setTextDimensionsWithRows:rows @@ -805,7 +841,6 @@ - (void)setFont:(NSFont *)font [[vimView textView] setFont:font]; [self updateResizeConstraints:NO]; - shouldMaximizeWindow = YES; } - (void)setWideFont:(NSFont *)font @@ -834,69 +869,89 @@ - (void)processInputQueueDidFinish if (updateToolbarFlag != 0) [self updateToolbar]; + const int oldTextViewRows = vimView.textView.pendingMaxRows; + const int oldTextViewCols = vimView.textView.pendingMaxColumns; + const NSRect oldTextViewFrame = vimView.textView.frame; + BOOL vimViewSizeChanged = NO; + // NOTE: If the window has not been presented then we must avoid resizing // the views since it will cause them to be constrained to the screen which // has not yet been set! if (windowPresented && shouldResizeVimView) { shouldResizeVimView = NO; - // Make sure full-screen window stays maximized (e.g. when scrollbar or - // tabline is hidden) according to 'fuopt'. - - BOOL didMaximize = NO; - if (shouldMaximizeWindow && fullScreenEnabled && - (fullScreenOptions & (FUOPT_MAXVERT|FUOPT_MAXHORZ)) != 0) - didMaximize = [self maximizeWindow:fullScreenOptions]; - - shouldMaximizeWindow = NO; - - // Resize Vim view and window, but don't do this now if the window was - // just reszied because this would make the window "jump" unpleasantly. - // Instead wait for Vim to respond to the resize message and do the - // resizing then. - // TODO: What if the resize message fails to make it back? - if (!didMaximize) { - NSSize originalSize = [vimView frame].size; - int rows = 0, cols = 0; - - // Setting 'guioptions+=k' will make shouldKeepGUISize true, which - // means avoid resizing the window. Instead, resize the view instead - // to keep the GUI window's size consistent. - bool avoidWindowResize = shouldKeepGUISize || fullScreenEnabled; - - if (!avoidWindowResize) { - NSSize contentSize = [vimView constrainRows:&rows columns:&cols - toSize: - fullScreenWindow ? [fullScreenWindow frame].size : - fullScreenEnabled ? desiredWindowSize : - [self constrainContentSizeToScreenSize:[vimView desiredSize]]]; - - [vimView setFrameSize:contentSize]; - - [self resizeWindowToFitContentSize:contentSize - keepOnScreen:keepOnScreen]; - } - else { - NSSize frameSize; - if (fullScreenWindow) { - // Non-native full screen mode. - NSRect desiredFrame = [fullScreenWindow getDesiredFrame]; - frameSize = desiredFrame.size; - [vimView setFrameOrigin:desiredFrame.origin]; // This will get set back to normal in MMFullScreenWindow::leaveFullScreen. - } else if (fullScreenEnabled) { - // Native full screen mode. - frameSize = desiredWindowSize; - } else { - frameSize = originalSize; - } - [vimView setFrameSizeKeepGUISize:frameSize]; + NSSize originalSize = [vimView frame].size; + int rows = 0, cols = 0; + + // Setting 'guioptions+=k' will make shouldKeepGUISize true, which + // means avoid resizing the window. Instead, resize the view instead + // to keep the GUI window's size consistent. + // Note: Vim should always have requested shouldKeepGUISize to be true + // when in full screen, but we check for it anyway for safety. + bool avoidWindowResize = shouldKeepGUISize || fullScreenEnabled; + + if (!avoidWindowResize) { + NSSize contentSize = [vimView constrainRows:&rows columns:&cols + toSize: + fullScreenWindow ? [fullScreenWindow frame].size : + fullScreenEnabled ? desiredWindowSize : + [self constrainContentSizeToScreenSize:[vimView desiredSize]]]; + + [vimView setFrameSize:contentSize]; + + [self resizeWindowToFitContentSize:contentSize + keepOnScreen:keepOnScreen]; + } + else { + NSSize frameSize; + if (fullScreenWindow) { + // Non-native full screen mode. + NSRect desiredFrame = [fullScreenWindow getDesiredFrame]; + frameSize = desiredFrame.size; + [vimView setFrameOrigin:desiredFrame.origin]; // This will get set back to normal in MMFullScreenWindow::leaveFullScreen. + } else if (fullScreenEnabled) { + // Native full screen mode. + frameSize = desiredWindowSize; + } else { + frameSize = originalSize; } + [vimView setFrameSizeKeepGUISize:frameSize]; } keepOnScreen = NO; shouldKeepGUISize = NO; + + vimViewSizeChanged = (vimView.textView.pendingMaxColumns != oldTextViewCols || vimView.textView.pendingMaxRows != oldTextViewRows); } - + + if (blockRenderUntilResize) { + if (vimViewSizeChanged) { + const NSRect newTextViewFrame = vimView.textView.frame; + + // We are currently blocking all rendering to prevent flicker. If + // the view frame moved (this happens if the tab or left scroll bar + // were shown/hidden) the user will see a temporary flicker as the + // text view was moved before Vim has udpated us with new draw calls + // to match the new size. To alleviate this, we temporarily apply + // a drawing offset in the text view to counter the offset. To the + // user it would appear that the text view hasn't moved at all. + [vimView.textView setDrawRectOffset:NSMakeSize(NSMinX(oldTextViewFrame) - NSMinX(newTextViewFrame), NSMaxY(oldTextViewFrame) - NSMaxY(newTextViewFrame))]; + } else { + // We were blocking all rendering until Vim has been resized. However + // in situations where we turned out to not need to resize Vim to + // begin with, we need to remedy the situation as we dropped some + // draw commands before. We simply ask Vim to redraw itself for + // simplicity instead of caching all the draw commands for this. + // This could happen if we changed guifont (which makes Vim think + // we need to resize) but turned out we set it to the same font so + // the grid size is the same and no need to resize. + blockRenderUntilResize = NO; + [vimView.textView setDrawRectOffset:NSZeroSize]; + + [vimController sendMessage:RedrawMsgID data:nil]; + } + } + // Tell Vim view to update its scrollbars which is done once per update. // Do it last so whatever resizing we have done above will take effect // immediate too instead of waiting till next frame. @@ -921,7 +976,6 @@ - (void)showTabline:(BOOL)on { [vimView showTabline:on]; [self updateTablineSeparator]; - shouldMaximizeWindow = YES; if([[NSUserDefaults standardUserDefaults] boolForKey:MMWindowUseTabBackgroundColorKey]) { if (on) { @@ -1010,7 +1064,7 @@ - (void)liveResizeDidEnd // with live resizing to make sure we don't havae any stale sizes due // to rate limiting of IPC messages during live resizing.. vimView.pendingLiveResizeQueued = NO; - [self resizeView]; + [self resizeVimView]; } } @@ -1378,12 +1432,6 @@ - (void)windowDidResize:(id)sender if (!setupDone) return; - // NOTE: We need to update the window frame size for Split View even though - // in full-screen on El Capitan or later. - if (floor(NSAppKitVersionNumber) <= NSAppKitVersionNumber10_10_Max - && fullScreenEnabled) - return; - // NOTE: Since we have no control over when the window may resize (Cocoa // may resize automatically) we simply set the view to fill the entire // window. The vim view takes care of notifying Vim if the number of @@ -1408,7 +1456,7 @@ - (void)windowDidResize:(id)sender - (void)windowDidChangeBackingProperties:(NSNotification *)notification { ASLogDebug(@""); - [vimController sendMessage:BackingPropertiesChangedMsgID data:nil]; + [vimController sendMessage:RedrawMsgID data:nil]; } // This is not an NSWindow delegate method, our custom MMWindow class calls it @@ -1638,12 +1686,6 @@ - (void)windowWillEnterFullScreen:(NSNotification *)notification - (void)windowDidEnterFullScreen:(NSNotification *)notification { - if (floor(NSAppKitVersionNumber) > NSAppKitVersionNumber10_10_Max) { - // NOTE: On El Capitan, we need to redraw the view when entering - // full-screen using :fullscreen option (including Ctrl-Cmd-f). - [vimController sendMessage:BackingPropertiesChangedMsgID data:nil]; - } - // Sometimes full screen will de-focus the text view. This seems to happen // when titlebar is configured as hidden. Simply re-assert it to make sure // text is still focused. @@ -1701,12 +1743,6 @@ - (void)windowWillExitFullScreen:(NSNotification *)notification - (void)windowDidExitFullScreen:(NSNotification *)notification { - if (floor(NSAppKitVersionNumber) > NSAppKitVersionNumber10_10_Max) { - // NOTE: On El Capitan, we need to redraw the view when leaving - // full-screen by moving the window out from Split View. - [vimController sendMessage:BackingPropertiesChangedMsgID data:nil]; - } - // We set the resize increment to 1,1 above just to sure window size was // restored properly. We want to set it back to the correct value, which // would not be 1,1 if we are not using smooth resize. @@ -1734,7 +1770,6 @@ - (void)windowDidFailToExitFullScreen:(NSWindow *)window ASLogNotice(@"Failed to EXIT full-screen, maximizing window..."); fullScreenEnabled = YES; - [self maximizeWindow:fullScreenOptions]; // Sometimes full screen will de-focus the text view. This seems to happen // when titlebar is configured as hidden. Simply re-assert it to make sure @@ -1793,7 +1828,7 @@ - (void)updateResizeConstraints:(BOOL)resizeWindow // We only want to resize the window down to match the Vim size if not using smooth resizing. // This resizing is going to re-snap the Window size to multiples of grid size. Otherwise // the resize constraint is always going to be at an offset to the desired size. - shouldResizeVimView = YES; + [self resizeVimViewAndWindow]; } } } @@ -2069,83 +2104,10 @@ - (void)updateToolbar updateToolbarFlag = 0; } -- (BOOL)maximizeWindow:(int)options -{ - // Note: - // This is deprecated code and will be removed later. 'fuopt' should be - // handled in processInputQueueDidFinish instead. - - if (floor(NSAppKitVersionNumber) > NSAppKitVersionNumber10_10_Max) { - // NOTE: Prevent to resize the window in Split View on El Capitan or - // later. - return NO; - } - - int currRows, currColumns; - [[vimView textView] getMaxRows:&currRows columns:&currColumns]; - - // NOTE: Do not use [NSScreen visibleFrame] when determining the screen - // size since it compensates for menu and dock. - int maxRows, maxColumns; - NSScreen *screen = [decoratedWindow screen]; - if (!screen) { - ASLogNotice(@"Window not on screen, using main screen"); - screen = [NSScreen mainScreen]; - } - NSSize size = [screen frame].size; - [vimView constrainRows:&maxRows columns:&maxColumns toSize:size]; - - ASLogDebug(@"Window dimensions max: %dx%d current: %dx%d", - maxRows, maxColumns, currRows, currColumns); - - // Compute current fu size - int fuRows = currRows, fuColumns = currColumns; - if (options & FUOPT_MAXVERT) - fuRows = maxRows; - if (options & FUOPT_MAXHORZ) - fuColumns = maxColumns; - - // If necessary, resize vim to target fu size - if (currRows != fuRows || currColumns != fuColumns) { - // The size sent here is queued and sent to vim when it's in - // event processing mode again. Make sure to only send the values we - // care about, as they override any changes that were made to 'lines' - // and 'columns' after 'fu' was set but before the event loop is run. - NSData *data = nil; - int msgid = 0; - if (currRows != fuRows && currColumns != fuColumns) { - int newSize[2] = { fuRows, fuColumns }; - data = [NSData dataWithBytes:newSize length:2*sizeof(int)]; - msgid = SetTextDimensionsMsgID; - } else if (currRows != fuRows) { - data = [NSData dataWithBytes:&fuRows length:sizeof(int)]; - msgid = SetTextRowsMsgID; - } else if (currColumns != fuColumns) { - data = [NSData dataWithBytes:&fuColumns length:sizeof(int)]; - msgid = SetTextColumnsMsgID; - } - NSParameterAssert(data != nil && msgid != 0); - - ASLogDebug(@"%s: %dx%d", MMVimMsgIDStrings[msgid], fuRows, fuColumns); - MMVimController *vc = [self vimController]; - [vc sendMessage:msgid data:data]; - [[vimView textView] setMaxRows:fuRows columns:fuColumns]; - - // Indicate that window was resized - return YES; - } - - // Indicate that window was not resized - return NO; -} - - (void)applicationDidChangeScreenParameters:(NSNotification *)notification { if (fullScreenWindow) { [fullScreenWindow applicationDidChangeScreenParameters:notification]; - } else if (fullScreenEnabled) { - ASLogDebug(@"Re-maximizing full-screen window..."); - [self maximizeWindow:fullScreenOptions]; } } diff --git a/src/MacVim/MacVim.h b/src/MacVim/MacVim.h index 6bcb61057d..b863bad620 100644 --- a/src/MacVim/MacVim.h +++ b/src/MacVim/MacVim.h @@ -348,7 +348,7 @@ extern const char * const MMVimMsgIDStrings[]; MSG(SetTooltipMsgID) \ MSG(GestureMsgID) \ MSG(AddToMRUMsgID) \ - MSG(BackingPropertiesChangedMsgID) \ + MSG(RedrawMsgID) \ MSG(SetBlurRadiusMsgID) \ MSG(SetBackgroundOptionMsgID) \ MSG(NotifyAppearanceChangeMsgID) \ diff --git a/src/MacVim/MacVimTests/MacVimTests.m b/src/MacVim/MacVimTests/MacVimTests.m index 4e632ad5e9..7fb3551f13 100644 --- a/src/MacVim/MacVimTests/MacVimTests.m +++ b/src/MacVim/MacVimTests/MacVimTests.m @@ -25,6 +25,7 @@ // Expose private methods for testing purposes @interface MMAppController (Private) + (NSDictionary*)parseOpenURL:(NSURL*)url; +- (void)processInputQueues:(id)sender; @end @interface MMVimController (Private) @@ -225,7 +226,90 @@ - (void)waitForEventHandling { method_setImplementation(method, origIMP); } -/// Wait for Vim to process all pending messages in its queue. +static BOOL vimProcessInputBlocked = NO; + +/// Block / unblock all Vim message handling from happening. This allows tests +/// to run with a strong guarantee of ordering of events without them being +/// subject to timing variations. Any outstanding block will be cleared at the +/// end of the tests during teardown automatically. +- (void)blockVimProcessInput:(BOOL)block { + SEL sel = @selector(processInputQueues:); + Method method = class_getInstanceMethod([MMAppController class], sel); + + __weak __typeof__(self) self_weak = self; + + static IMP origIMP = nil; + static BOOL blockedMethodCalled = NO; + static BOOL teardownAdded = NO; + if (block) { + if (origIMP == nil) + origIMP = method_getImplementation(method); + IMP newIMP = imp_implementationWithBlock(^(id self, id sender) { + blockedMethodCalled = YES; + }); + method_setImplementation(method, newIMP); + vimProcessInputBlocked = YES; + + if (!teardownAdded) { + [self addTeardownBlock:^{ + if (vimProcessInputBlocked) { + [self_weak blockVimProcessInput:NO]; + } + teardownAdded = NO; + }]; + teardownAdded = YES; + } + } else { + if (origIMP != nil) { + method_setImplementation(method, origIMP); + + if (blockedMethodCalled) { + MMAppController *app = MMAppController.sharedInstance; + [app performSelectorOnMainThread:@selector(processInputQueues:) withObject:nil waitUntilDone:NO]; + blockedMethodCalled = NO; + } + } + vimProcessInputBlocked = NO; + } +} + +/// Wait for a specific message from Vim. Optionally, after receiving the +/// mesasge, block all future Vim message handling until manually unblocked +/// or this method was called again. This is useful for tests that want to +/// test a sequence of events with tight ordering and not be subject to timing +/// issues. +- (void)waitForVimMessage:(int)messageID blockFutureMessages:(BOOL)blockMsgs { + XCTestExpectation *expectation = [self expectationWithDescription:@"VimMessage"]; + + SEL sel = @selector(handleMessage:data:); + Method method = class_getInstanceMethod([MMVimController class], sel); + + __weak __typeof__(self) self_weak = self; + + IMP origIMP = method_getImplementation(method); + IMP newIMP = imp_implementationWithBlock(^(id self, int msgid, NSData *data) { + typedef void (*fn)(id,SEL,int,NSData*); + ((fn)origIMP)(self, sel, msgid, data); + if (msgid == messageID) { + [expectation fulfill]; + if (blockMsgs) { + [self_weak blockVimProcessInput:YES]; + } + } + }); + + if (vimProcessInputBlocked) { + // Make sure unblock message handling first or we will deadlock. + [self blockVimProcessInput:NO]; + } + + method_setImplementation(method, newIMP); + [self waitForExpectations:@[expectation] timeout:10]; + method_setImplementation(method, origIMP); +} + +/// Wait for Vim to process all pending messages in its queue. In future we +/// should migrate to having tests directly call waitForVimMessage directly. - (void)waitForVimProcess { // Implement this by sending a loopback message (Vim will send the message // back to us) as a synchronization mechanism as Vim handles its messages @@ -776,6 +860,85 @@ - (void) testWindowResize { } } +/// Test resizing the Vim view to match window size (go+=k / full screen) works +/// and produces a stable image. +- (void) testResizeVimView { + [self createTestVimWindow]; + + MMAppController *app = MMAppController.sharedInstance; + MMWindowController *win = [[app keyVimController] windowController]; + MMTextView *textView = [[[[app keyVimController] windowController] vimView] textView]; + + [self sendStringToVim:@":set guioptions+=k guifont=Menlo:h10\n" withMods:0]; + [self waitForEventHandlingAndVimProcess]; + + // Set a default 30,80 base size for the entire test + [self sendStringToVim:@":set lines=30 columns=80\n" withMods:0]; + [self waitForEventHandlingAndVimProcess]; + XCTAssertEqual(textView.maxRows, 30); + XCTAssertEqual(textView.maxColumns, 80); + + // Test that setting a bigger font will trigger a resize of Vim view to + // smaller grid, but also block intermediary rendering to avoid flicker. + [self sendStringToVim:@":set guifont=Menlo:h13\n" withMods:0]; + [self waitForVimMessage:SetFontMsgID blockFutureMessages:YES]; + XCTAssertEqual(textView.maxRows, 30); + XCTAssertEqual(textView.maxColumns, 80); + XCTAssertLessThan(textView.pendingMaxRows, 30); // confirms that we have an outstanding resize request to make it smaller + XCTAssertLessThan(textView.pendingMaxColumns, 80); + XCTAssertTrue(win.isRenderBlocked); + // Vim has responded to the size change. We should now have unblocked rendering. + [self waitForVimMessage:SetTextDimensionsNoResizeWindowMsgID blockFutureMessages:YES]; + XCTAssertLessThan(textView.maxRows, 30); + XCTAssertLessThan(textView.maxColumns, 80); + XCTAssertFalse(win.isRenderBlocked); + + // Make sure if we set it again to the same font, we won't block since we + // didn't actually resize anything. + [self sendStringToVim:@":set guifont=Menlo:h13\n" withMods:0]; + [self waitForVimMessage:SetFontMsgID blockFutureMessages:YES]; + XCTAssertFalse(win.isRenderBlocked); + + // Set it back and make sure it went back to the original rows/cols + [self sendStringToVim:@":set guifont=Menlo:h10\n" withMods:0]; + [self waitForVimMessage:SetTextDimensionsNoResizeWindowMsgID blockFutureMessages:YES]; + XCTAssertEqual(textView.maxRows, 30); + XCTAssertEqual(textView.maxColumns, 80); + + // Test making a new tab would do the same + [self sendStringToVim:@":tabnew\n" withMods:0]; + [self waitForVimMessage:ShowTabBarMsgID blockFutureMessages:YES]; + XCTAssertEqual(textView.maxRows, 30); + XCTAssertLessThan(textView.pendingMaxRows, 30); + XCTAssertGreaterThan(textView.drawRectOffset.height, 0); + XCTAssertTrue(win.isRenderBlocked); + [self waitForVimMessage:SetTextDimensionsNoResizeWindowMsgID blockFutureMessages:YES]; + XCTAssertLessThan(textView.maxRows, 30); + XCTAssertEqual(textView.drawRectOffset.height, 0); + XCTAssertFalse(win.isRenderBlocked); + [self blockVimProcessInput:NO]; + + // Repeat the same font size change test in full screen to exercise that + // code path. In particular, it should act like go+=k even if the option + // was not explicitly set. + [self setDefault:MMNativeFullScreenKey toValue:@NO]; // non-native is faster so use that + [self sendStringToVim:@":set guioptions-=k fullscreen\n" withMods:0]; + [self waitForFullscreenTransitionIsEnter:YES isNative:NO]; + int fuRows = textView.maxRows; + int fuCols = textView.maxColumns; + [self sendStringToVim:@":set guifont=Menlo:h13\n" withMods:0]; + [self waitForVimMessage:SetFontMsgID blockFutureMessages:YES]; + XCTAssertEqual(textView.maxRows, fuRows); + XCTAssertEqual(textView.maxColumns, fuCols); + XCTAssertLessThan(textView.pendingMaxRows, fuRows); + XCTAssertLessThan(textView.pendingMaxColumns, fuCols); + XCTAssertTrue(win.isRenderBlocked); + [self waitForVimMessage:SetTextDimensionsNoResizeWindowMsgID blockFutureMessages:YES]; + XCTAssertLessThan(textView.maxRows, fuRows); + XCTAssertLessThan(textView.maxColumns, fuCols); + XCTAssertFalse(win.isRenderBlocked); +} + #pragma mark Full screen tests - (void)waitForNativeFullscreenEnter { diff --git a/src/MacVim/gui_macvim.m b/src/MacVim/gui_macvim.m index 0d030f04ca..7ea4212c11 100644 --- a/src/MacVim/gui_macvim.m +++ b/src/MacVim/gui_macvim.m @@ -1945,6 +1945,12 @@ * Re-calculates size of the Vim view to fit within the window without having * to resize the window. Usually happens after UI elements have changed (e.g. * adding / removing a toolbar) when guioptions 'k' is set. + * + * In other GVim implementations this is a synchronous operation (via + * gui_mch_newfont). In MacVim we need to request the GUI process to resize us, + * and we do this asynchronously to avoid introducing sync points. It does mean + * Vim will temporarily draw/behave using the old size until the it receives + * the resize message from the GUI. */ void gui_mch_resize_view(void) diff --git a/src/gui.c b/src/gui.c index 1bc329c764..6a10ef5e2a 100644 --- a/src/gui.c +++ b/src/gui.c @@ -1714,7 +1714,8 @@ gui_set_shellsize( #endif #ifdef FEAT_GUI_MACVIM - if (!mustset && (vim_strchr(p_go, GO_KEEPWINSIZE) != NULL)) + if (!mustset && (vim_strchr(p_go, GO_KEEPWINSIZE) != NULL + || p_fullscreen)) { /* We don't want to resize the window, so instruct the GUI to resize * the view to be within the constraints of the current window's size */