From e78ebf4803100caa684dffdf853c27300633368d Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Tue, 24 Dec 2024 06:17:22 -0800 Subject: [PATCH] Fix resizing MacVim window occasionally result in a stale wrong Vim size First issue was that if you resize MacVim quickly (could be simulated by stalling Vim by calling `:sleep 5`), the existing logic for caching the size was wrong. It was using `maxRows`/`maxColumns` from the text view to decide if we send an updated message to Vim, but those are only updated after Vim has responded back to us. That means if we have two resize events before Vim could respond, that could lead to wrong results. One example would be quickly resizing Vim, then resizing it back to original size. To fix this, add a new "pending" rows/cols whenever we update Vim, and use that for caching instead as it always reflects what we want Vim's size to be. Another issue was that in live resizing, if the user resizes Vim quickly (which again could be simulated by stalling Vim), the rate limiting logic would not send Vim with the latest size until the live event has finished (the user let go of the mouse button). This feels weird when it happens. Instead, fix it so that whenever we have rate limited the IPC commands, we immediately send the updated resize message once Vim has handled the last one, instead of waiting to do it in `liveResizeDidEnd`. --- src/MacVim/MMCoreTextView.h | 4 + src/MacVim/MMCoreTextView.m | 18 ++++ src/MacVim/MMTextView.h | 6 ++ src/MacVim/MMTextView.m | 18 ++++ src/MacVim/MMVimView.h | 3 +- src/MacVim/MMVimView.m | 23 ++-- src/MacVim/MMWindowController.m | 30 ++++-- src/MacVim/MacVimTests/MacVimTests.m | 150 +++++++++++++++++++++++++++ 8 files changed, 239 insertions(+), 13 deletions(-) diff --git a/src/MacVim/MMCoreTextView.h b/src/MacVim/MMCoreTextView.h index 9bdf59f7e4..42a246f3e4 100644 --- a/src/MacVim/MMCoreTextView.h +++ b/src/MacVim/MMCoreTextView.h @@ -48,6 +48,7 @@ NS_ASSUME_NONNULL_BEGIN { // From MMTextStorage int maxRows, maxColumns; + int pendingMaxRows, pendingMaxColumns; NSColor *defaultBackgroundColor; NSColor *defaultForegroundColor; NSSize cellSize; @@ -112,6 +113,9 @@ NS_ASSUME_NONNULL_BEGIN - (int)maxColumns; - (void)getMaxRows:(int*)rows columns:(int*)cols; - (void)setMaxRows:(int)rows columns:(int)cols; +- (int)pendingMaxRows; +- (int)pendingMaxColumns; +- (void)setPendingMaxRows:(int)rows columns:(int)cols; - (void)setDefaultColorsBackground:(NSColor *)bgColor foreground:(NSColor *)fgColor; - (NSColor *)defaultBackgroundColor; diff --git a/src/MacVim/MMCoreTextView.m b/src/MacVim/MMCoreTextView.m index 6c146e7e3f..c687cb8480 100644 --- a/src/MacVim/MMCoreTextView.m +++ b/src/MacVim/MMCoreTextView.m @@ -309,6 +309,24 @@ - (void)setMaxRows:(int)rows columns:(int)cols grid_resize(&grid, rows, cols); maxRows = rows; maxColumns = cols; + pendingMaxRows = rows; + pendingMaxColumns = cols; +} + +- (int)pendingMaxRows +{ + return pendingMaxRows; +} + +- (int)pendingMaxColumns +{ + return pendingMaxColumns; +} + +- (void)setPendingMaxRows:(int)rows columns:(int)cols +{ + pendingMaxRows = rows; + pendingMaxColumns = cols; } - (void)setDefaultColorsBackground:(NSColor *)bgColor diff --git a/src/MacVim/MMTextView.h b/src/MacVim/MMTextView.h index 036192c1a8..570dc1c3f6 100644 --- a/src/MacVim/MMTextView.h +++ b/src/MacVim/MMTextView.h @@ -25,6 +25,9 @@ NSRect *invertRects; int numInvertRects; + int pendingMaxRows; + int pendingMaxColumns; + MMTextViewHelper *helper; } @@ -57,6 +60,9 @@ - (int)maxColumns; - (void)getMaxRows:(int*)rows columns:(int*)cols; - (void)setMaxRows:(int)rows columns:(int)cols; +- (int)pendingMaxRows; +- (int)pendingMaxColumns; +- (void)setPendingMaxRows:(int)rows columns:(int)cols; - (NSRect)rectForRowsInRange:(NSRange)range; - (NSRect)rectForColumnsInRange:(NSRange)range; - (void)setDefaultColorsBackground:(NSColor *)bgColor diff --git a/src/MacVim/MMTextView.m b/src/MacVim/MMTextView.m index d092cabd74..e7c4923cf2 100644 --- a/src/MacVim/MMTextView.m +++ b/src/MacVim/MMTextView.m @@ -401,9 +401,27 @@ - (void)getMaxRows:(int*)rows columns:(int*)cols - (void)setMaxRows:(int)rows columns:(int)cols { + pendingMaxRows = rows; + pendingMaxColumns = cols; return [(MMTextStorage*)[self textStorage] setMaxRows:rows columns:cols]; } +- (int)pendingMaxRows +{ + return pendingMaxRows; +} + +- (int)pendingMaxColumns +{ + return pendingMaxColumns; +} + +- (void)setPendingMaxRows:(int)rows columns:(int)cols +{ + pendingMaxRows = rows; + pendingMaxColumns = cols; +} + - (NSRect)rectForRowsInRange:(NSRange)range { return [(MMTextStorage*)[self textStorage] rectForRowsInRange:range]; diff --git a/src/MacVim/MMVimView.h b/src/MacVim/MMVimView.h index ba04f90553..ba65674645 100644 --- a/src/MacVim/MMVimView.h +++ b/src/MacVim/MMVimView.h @@ -28,7 +28,8 @@ } @property BOOL pendingPlaceScrollbars; -@property BOOL pendingLiveResize; +@property BOOL pendingLiveResize; ///< An ongoing live resizing message to Vim is active +@property BOOL pendingLiveResizeQueued; ///< A new size has been queued while an ongoing live resize is already active - (MMVimView *)initWithFrame:(NSRect)frame vimController:(MMVimController *)c; diff --git a/src/MacVim/MMVimView.m b/src/MacVim/MMVimView.m index c062aedec5..0ce0167c66 100644 --- a/src/MacVim/MMVimView.m +++ b/src/MacVim/MMVimView.m @@ -953,19 +953,23 @@ - (void)frameSizeMayHaveChanged:(BOOL)keepGUISize [textView constrainRows:&constrained[0] columns:&constrained[1] toSize:textViewSize]; - int rows, cols; - [textView getMaxRows:&rows columns:&cols]; - - if (constrained[0] != rows || constrained[1] != cols) { + if (constrained[0] != textView.pendingMaxRows || constrained[1] != textView.pendingMaxColumns) { NSData *data = [NSData dataWithBytes:constrained length:2*sizeof(int)]; int msgid = [self inLiveResize] ? LiveResizeMsgID : (keepGUISize ? SetTextDimensionsNoResizeWindowMsgID : SetTextDimensionsMsgID); ASLogDebug(@"Notify Vim that text dimensions changed from %dx%d to " - "%dx%d (%s)", cols, rows, constrained[1], constrained[0], + "%dx%d (%s)", textView.pendingMaxColumns, textView.pendingMaxRows, constrained[1], constrained[0], MMVimMsgIDStrings[msgid]); - if (msgid != LiveResizeMsgID || !self.pendingLiveResize) { + if (msgid == LiveResizeMsgID && self.pendingLiveResize) { + // We are currently live resizing and there's already an ongoing + // resize message that we haven't finished handling yet. Wait until + // we are done with that since we don't want to overload Vim with + // messages. + self.pendingLiveResizeQueued = YES; + } + else { // Live resize messages can be sent really rapidly, especailly if // it's from double clicking the window border (to indicate filling // all the way to that side to the window manager). We want to rate @@ -976,6 +980,13 @@ - (void)frameSizeMayHaveChanged:(BOOL)keepGUISize // up resizing. self.pendingLiveResize = (msgid == LiveResizeMsgID); + // Cache the new pending size so we can use it to prevent resizing Vim again + // if we haven't changed the row/col count later. We don't want to + // immediately resize the textView (hence it's "pending") as we only + // do that when Vim has acknoledged the message and draws. This leads + // to a stable drawing. + [textView setPendingMaxRows:constrained[0] columns:constrained[1]]; + [vimController sendMessageNow:msgid data:data timeout:1]; } diff --git a/src/MacVim/MMWindowController.m b/src/MacVim/MMWindowController.m index 454532be09..0e389bd6c5 100644 --- a/src/MacVim/MMWindowController.m +++ b/src/MacVim/MMWindowController.m @@ -418,7 +418,22 @@ - (void)setTextDimensionsWithRows:(int)rows columns:(int)cols isLive:(BOOL)live // user drags to resize the window. [vimView setDesiredRows:rows columns:cols]; + vimView.pendingLiveResize = NO; + 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 + // Vim was done with the last message. + // + // This could happen if we are in the middle of rapid resize (e.g. + // double-clicking on the border/corner of window), as we would fire + // off a lot of LiveResizeMsgID messages where some will be + // intentionally omitted to avoid swamping IPC as we rate limit it to + // only one outstanding resize message at a time + // inframeSizeMayHaveChanged:. + vimView.pendingLiveResizeQueued = NO; + [self resizeView]; + } if (setupDone && !live && !keepGUISize) { shouldResizeVimView = YES; @@ -931,12 +946,15 @@ - (void)liveResizeDidEnd [decoratedWindow setTitle:lastSetTitle]; } - // If we are in the middle of rapid resize (e.g. double-clicking on the border/corner - // of window), we would fire off a lot of LiveResizeMsgID messages where some will be - // intentionally omitted to avoid swamping IPC. If that happens this will perform a - // final clean up that makes sure the Vim view is sized correctly within the window. - // See frameSizeMayHaveChanged: for where the omission/rate limiting happens. - [self resizeView]; + if (vimView.pendingLiveResizeQueued) { + // Similar to setTextDimensionsWithRows:, if there's still outstanding + // resize message queued, we just immediately flush it here to make + // sure Vim will get the most up-to-date size here when we are done + // 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]; + } } - (void)setBlurRadius:(int)radius diff --git a/src/MacVim/MacVimTests/MacVimTests.m b/src/MacVim/MacVimTests/MacVimTests.m index cdfd33c639..b87978726f 100644 --- a/src/MacVim/MacVimTests/MacVimTests.m +++ b/src/MacVim/MacVimTests/MacVimTests.m @@ -40,6 +40,16 @@ - (NSMutableArray*)vimControllers { } @end +static BOOL forceInLiveResize = NO; +@implementation MMVimView (testWindowResize) +- (BOOL)inLiveResize { + // Mock NSView's inLiveResize functionality + if (forceInLiveResize) + return YES; + return [super inLiveResize]; +} +@end + @interface MacVimTests : XCTestCase @end @@ -583,4 +593,144 @@ - (void) testTitlebarDocumentIcon { [self waitForVimClose]; } +/// Test resizing the MacVim window properly resizes Vim +- (void) testWindowResize { + MMAppController *app = MMAppController.sharedInstance; + + [app openNewWindow:NewWindowClean activate:YES]; + [self waitForVimOpenAndMessages]; + + NSWindow *win = [[[app keyVimController] windowController] window]; + MMVimView *vimView = [[[app keyVimController] windowController] vimView]; + MMTextView *textView = [[[[app keyVimController] windowController] vimView] textView]; + + // Set a default 30,80 base size for the entire test + [self sendStringToVim:@":set lines=30 columns=80\n" withMods:0]; + [self waitForEventHandlingAndVimProcess]; + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + + const NSRect winFrame = win.frame; + + { + // Test basic resizing functionality. Make sure text view is updated properly + NSRect newFrame = winFrame; + newFrame.size.width -= textView.cellSize.width; + newFrame.size.height -= textView.cellSize.height; + + [win setFrame:newFrame display:YES]; + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + [self waitForVimProcess]; + XCTAssertEqual(29, textView.maxRows); + XCTAssertEqual(79, textView.maxColumns); + + [win setFrame:winFrame display:YES]; + [self waitForVimProcess]; + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + } + + { + // Test rapid resizing where we resize faster than Vim can handle. We + // should be updating a pending size indicating what we expect Vim's + // size should be and use that as the cache. Previously we had a bug + // we we used the outdated size as cache instead leading to rapid + // resizing sometimes leading to stale sizes. + + // This kind of situation coudl occur if say Vim is stalled for a bit + // and we resized the window multiple times. We don't rate limit unlike + // live resizing since usually it's not needed. + NSRect newFrame = winFrame; + newFrame.size.width -= textView.cellSize.width; + newFrame.size.height -= textView.cellSize.height; + + [win setFrame:newFrame display:YES]; + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + XCTAssertEqual(29, textView.pendingMaxRows); + XCTAssertEqual(79, textView.pendingMaxColumns); + + [win setFrame:winFrame display:YES]; + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + XCTAssertEqual(30, textView.pendingMaxRows); + XCTAssertEqual(80, textView.pendingMaxColumns); + + [self waitForVimProcess]; + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + } + + { + // Test rapid resizing again, but this time we don't resize back to the + // original size, but instead incremented multiple times. Just to make + // sure we actually get set to the final size. + NSRect newFrame = winFrame; + for (int i = 0; i < 5; i++) { + newFrame.size.width += textView.cellSize.width; + newFrame.size.height += textView.cellSize.height; + [win setFrame:newFrame display:YES]; + } + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + XCTAssertEqual(35, textView.pendingMaxRows); + XCTAssertEqual(85, textView.pendingMaxColumns); + + [self waitForVimProcess]; + XCTAssertEqual(35, textView.maxRows); + XCTAssertEqual(85, textView.maxColumns); + + [win setFrame:winFrame display:YES]; // reset back to original size + [self waitForVimProcess]; + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + } + + { + // Test live resizing (e.g. when user drags the window edge to resize). + // We rate limit the number of messages we send to Vim so if there are + // multiple resize events they will be sequenced to avoid overloading Vim. + forceInLiveResize = YES; // simulate live resizing which can only be initiated by a user + [vimView viewWillStartLiveResize]; + + NSRect newFrame = winFrame; + for (int i = 0; i < 5; i++) { + newFrame.size.width += textView.cellSize.width; + newFrame.size.height += textView.cellSize.height; + [win setFrame:newFrame display:YES]; + } + + // The first time Vim processes this it should have only received the first message + // due to rate limiting. + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + XCTAssertEqual(31, textView.pendingMaxRows); + XCTAssertEqual(81, textView.pendingMaxColumns); + + // After Vim has processed the messages it should now have the final size + [self waitForVimProcess]; // first wait for Vim to respond it processed the first message, where we send off the second one + [self waitForVimProcess]; // Vim should now have processed the last message + XCTAssertEqual(35, textView.maxRows); + XCTAssertEqual(85, textView.maxColumns); + XCTAssertEqual(35, textView.pendingMaxRows); + XCTAssertEqual(85, textView.pendingMaxColumns); + + forceInLiveResize = NO; + [vimView viewDidEndLiveResize]; + [self waitForVimProcess]; + XCTAssertEqual(35, textView.maxRows); + XCTAssertEqual(85, textView.maxColumns); + + [win setFrame:winFrame display:YES]; // reset back to original size + [self waitForEventHandlingAndVimProcess]; + XCTAssertEqual(30, textView.maxRows); + XCTAssertEqual(80, textView.maxColumns); + } + + // Clean up + [[app keyVimController] sendMessage:VimShouldCloseMsgID data:nil]; + [self waitForVimClose]; +} + @end