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`.

Author: ychin

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