Merge pull request #1518 from ychin/fix-window-resize-stale-bug

Fix resizing MacVim window occasionally result in a stale wrong Vim size

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;

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

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

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];

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;
 

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];
         }
 

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

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