Skip to content

Commit d1c157e

Browse files
authored
Merge pull request #1518 from ychin/fix-window-resize-stale-bug
Fix resizing MacVim window occasionally result in a stale wrong Vim size
2 parents 2b11b01 + e78ebf4 commit d1c157e

8 files changed

+239
-13
lines changed

src/MacVim/MMCoreTextView.h

+4
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ NS_ASSUME_NONNULL_BEGIN
4848
{
4949
// From MMTextStorage
5050
int maxRows, maxColumns;
51+
int pendingMaxRows, pendingMaxColumns;
5152
NSColor *defaultBackgroundColor;
5253
NSColor *defaultForegroundColor;
5354
NSSize cellSize;
@@ -112,6 +113,9 @@ NS_ASSUME_NONNULL_BEGIN
112113
- (int)maxColumns;
113114
- (void)getMaxRows:(int*)rows columns:(int*)cols;
114115
- (void)setMaxRows:(int)rows columns:(int)cols;
116+
- (int)pendingMaxRows;
117+
- (int)pendingMaxColumns;
118+
- (void)setPendingMaxRows:(int)rows columns:(int)cols;
115119
- (void)setDefaultColorsBackground:(NSColor *)bgColor
116120
foreground:(NSColor *)fgColor;
117121
- (NSColor *)defaultBackgroundColor;

src/MacVim/MMCoreTextView.m

+18
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,24 @@ - (void)setMaxRows:(int)rows columns:(int)cols
309309
grid_resize(&grid, rows, cols);
310310
maxRows = rows;
311311
maxColumns = cols;
312+
pendingMaxRows = rows;
313+
pendingMaxColumns = cols;
314+
}
315+
316+
- (int)pendingMaxRows
317+
{
318+
return pendingMaxRows;
319+
}
320+
321+
- (int)pendingMaxColumns
322+
{
323+
return pendingMaxColumns;
324+
}
325+
326+
- (void)setPendingMaxRows:(int)rows columns:(int)cols
327+
{
328+
pendingMaxRows = rows;
329+
pendingMaxColumns = cols;
312330
}
313331

314332
- (void)setDefaultColorsBackground:(NSColor *)bgColor

src/MacVim/MMTextView.h

+6
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@
2525
NSRect *invertRects;
2626
int numInvertRects;
2727

28+
int pendingMaxRows;
29+
int pendingMaxColumns;
30+
2831
MMTextViewHelper *helper;
2932
}
3033

@@ -57,6 +60,9 @@
5760
- (int)maxColumns;
5861
- (void)getMaxRows:(int*)rows columns:(int*)cols;
5962
- (void)setMaxRows:(int)rows columns:(int)cols;
63+
- (int)pendingMaxRows;
64+
- (int)pendingMaxColumns;
65+
- (void)setPendingMaxRows:(int)rows columns:(int)cols;
6066
- (NSRect)rectForRowsInRange:(NSRange)range;
6167
- (NSRect)rectForColumnsInRange:(NSRange)range;
6268
- (void)setDefaultColorsBackground:(NSColor *)bgColor

src/MacVim/MMTextView.m

+18
Original file line numberDiff line numberDiff line change
@@ -401,9 +401,27 @@ - (void)getMaxRows:(int*)rows columns:(int*)cols
401401

402402
- (void)setMaxRows:(int)rows columns:(int)cols
403403
{
404+
pendingMaxRows = rows;
405+
pendingMaxColumns = cols;
404406
return [(MMTextStorage*)[self textStorage] setMaxRows:rows columns:cols];
405407
}
406408

409+
- (int)pendingMaxRows
410+
{
411+
return pendingMaxRows;
412+
}
413+
414+
- (int)pendingMaxColumns
415+
{
416+
return pendingMaxColumns;
417+
}
418+
419+
- (void)setPendingMaxRows:(int)rows columns:(int)cols
420+
{
421+
pendingMaxRows = rows;
422+
pendingMaxColumns = cols;
423+
}
424+
407425
- (NSRect)rectForRowsInRange:(NSRange)range
408426
{
409427
return [(MMTextStorage*)[self textStorage] rectForRowsInRange:range];

src/MacVim/MMVimView.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@
2828
}
2929

3030
@property BOOL pendingPlaceScrollbars;
31-
@property BOOL pendingLiveResize;
31+
@property BOOL pendingLiveResize; ///< An ongoing live resizing message to Vim is active
32+
@property BOOL pendingLiveResizeQueued; ///< A new size has been queued while an ongoing live resize is already active
3233

3334
- (MMVimView *)initWithFrame:(NSRect)frame vimController:(MMVimController *)c;
3435

src/MacVim/MMVimView.m

+17-6
Original file line numberDiff line numberDiff line change
@@ -953,19 +953,23 @@ - (void)frameSizeMayHaveChanged:(BOOL)keepGUISize
953953
[textView constrainRows:&constrained[0] columns:&constrained[1]
954954
toSize:textViewSize];
955955

956-
int rows, cols;
957-
[textView getMaxRows:&rows columns:&cols];
958-
959-
if (constrained[0] != rows || constrained[1] != cols) {
956+
if (constrained[0] != textView.pendingMaxRows || constrained[1] != textView.pendingMaxColumns) {
960957
NSData *data = [NSData dataWithBytes:constrained length:2*sizeof(int)];
961958
int msgid = [self inLiveResize] ? LiveResizeMsgID
962959
: (keepGUISize ? SetTextDimensionsNoResizeWindowMsgID : SetTextDimensionsMsgID);
963960

964961
ASLogDebug(@"Notify Vim that text dimensions changed from %dx%d to "
965-
"%dx%d (%s)", cols, rows, constrained[1], constrained[0],
962+
"%dx%d (%s)", textView.pendingMaxColumns, textView.pendingMaxRows, constrained[1], constrained[0],
966963
MMVimMsgIDStrings[msgid]);
967964

968-
if (msgid != LiveResizeMsgID || !self.pendingLiveResize) {
965+
if (msgid == LiveResizeMsgID && self.pendingLiveResize) {
966+
// We are currently live resizing and there's already an ongoing
967+
// resize message that we haven't finished handling yet. Wait until
968+
// we are done with that since we don't want to overload Vim with
969+
// messages.
970+
self.pendingLiveResizeQueued = YES;
971+
}
972+
else {
969973
// Live resize messages can be sent really rapidly, especailly if
970974
// it's from double clicking the window border (to indicate filling
971975
// all the way to that side to the window manager). We want to rate
@@ -976,6 +980,13 @@ - (void)frameSizeMayHaveChanged:(BOOL)keepGUISize
976980
// up resizing.
977981
self.pendingLiveResize = (msgid == LiveResizeMsgID);
978982

983+
// Cache the new pending size so we can use it to prevent resizing Vim again
984+
// if we haven't changed the row/col count later. We don't want to
985+
// immediately resize the textView (hence it's "pending") as we only
986+
// do that when Vim has acknoledged the message and draws. This leads
987+
// to a stable drawing.
988+
[textView setPendingMaxRows:constrained[0] columns:constrained[1]];
989+
979990
[vimController sendMessageNow:msgid data:data timeout:1];
980991
}
981992

src/MacVim/MMWindowController.m

+24-6
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,22 @@ - (void)setTextDimensionsWithRows:(int)rows columns:(int)cols isLive:(BOOL)live
418418
// user drags to resize the window.
419419

420420
[vimView setDesiredRows:rows columns:cols];
421+
421422
vimView.pendingLiveResize = NO;
423+
if (vimView.pendingLiveResizeQueued) {
424+
// There was already a new size queued while Vim was still processing
425+
// the last one. We need to immediately request another resize now that
426+
// Vim was done with the last message.
427+
//
428+
// This could happen if we are in the middle of rapid resize (e.g.
429+
// double-clicking on the border/corner of window), as we would fire
430+
// off a lot of LiveResizeMsgID messages where some will be
431+
// intentionally omitted to avoid swamping IPC as we rate limit it to
432+
// only one outstanding resize message at a time
433+
// inframeSizeMayHaveChanged:.
434+
vimView.pendingLiveResizeQueued = NO;
435+
[self resizeView];
436+
}
422437

423438
if (setupDone && !live && !keepGUISize) {
424439
shouldResizeVimView = YES;
@@ -931,12 +946,15 @@ - (void)liveResizeDidEnd
931946
[decoratedWindow setTitle:lastSetTitle];
932947
}
933948

934-
// If we are in the middle of rapid resize (e.g. double-clicking on the border/corner
935-
// of window), we would fire off a lot of LiveResizeMsgID messages where some will be
936-
// intentionally omitted to avoid swamping IPC. If that happens this will perform a
937-
// final clean up that makes sure the Vim view is sized correctly within the window.
938-
// See frameSizeMayHaveChanged: for where the omission/rate limiting happens.
939-
[self resizeView];
949+
if (vimView.pendingLiveResizeQueued) {
950+
// Similar to setTextDimensionsWithRows:, if there's still outstanding
951+
// resize message queued, we just immediately flush it here to make
952+
// sure Vim will get the most up-to-date size here when we are done
953+
// with live resizing to make sure we don't havae any stale sizes due
954+
// to rate limiting of IPC messages during live resizing..
955+
vimView.pendingLiveResizeQueued = NO;
956+
[self resizeView];
957+
}
940958
}
941959

942960
- (void)setBlurRadius:(int)radius

src/MacVim/MacVimTests/MacVimTests.m

+150
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,16 @@ - (NSMutableArray*)vimControllers {
4040
}
4141
@end
4242

43+
static BOOL forceInLiveResize = NO;
44+
@implementation MMVimView (testWindowResize)
45+
- (BOOL)inLiveResize {
46+
// Mock NSView's inLiveResize functionality
47+
if (forceInLiveResize)
48+
return YES;
49+
return [super inLiveResize];
50+
}
51+
@end
52+
4353
@interface MacVimTests : XCTestCase
4454

4555
@end
@@ -583,4 +593,144 @@ - (void) testTitlebarDocumentIcon {
583593
[self waitForVimClose];
584594
}
585595

596+
/// Test resizing the MacVim window properly resizes Vim
597+
- (void) testWindowResize {
598+
MMAppController *app = MMAppController.sharedInstance;
599+
600+
[app openNewWindow:NewWindowClean activate:YES];
601+
[self waitForVimOpenAndMessages];
602+
603+
NSWindow *win = [[[app keyVimController] windowController] window];
604+
MMVimView *vimView = [[[app keyVimController] windowController] vimView];
605+
MMTextView *textView = [[[[app keyVimController] windowController] vimView] textView];
606+
607+
// Set a default 30,80 base size for the entire test
608+
[self sendStringToVim:@":set lines=30 columns=80\n" withMods:0];
609+
[self waitForEventHandlingAndVimProcess];
610+
XCTAssertEqual(30, textView.maxRows);
611+
XCTAssertEqual(80, textView.maxColumns);
612+
613+
const NSRect winFrame = win.frame;
614+
615+
{
616+
// Test basic resizing functionality. Make sure text view is updated properly
617+
NSRect newFrame = winFrame;
618+
newFrame.size.width -= textView.cellSize.width;
619+
newFrame.size.height -= textView.cellSize.height;
620+
621+
[win setFrame:newFrame display:YES];
622+
XCTAssertEqual(30, textView.maxRows);
623+
XCTAssertEqual(80, textView.maxColumns);
624+
[self waitForVimProcess];
625+
XCTAssertEqual(29, textView.maxRows);
626+
XCTAssertEqual(79, textView.maxColumns);
627+
628+
[win setFrame:winFrame display:YES];
629+
[self waitForVimProcess];
630+
XCTAssertEqual(30, textView.maxRows);
631+
XCTAssertEqual(80, textView.maxColumns);
632+
}
633+
634+
{
635+
// Test rapid resizing where we resize faster than Vim can handle. We
636+
// should be updating a pending size indicating what we expect Vim's
637+
// size should be and use that as the cache. Previously we had a bug
638+
// we we used the outdated size as cache instead leading to rapid
639+
// resizing sometimes leading to stale sizes.
640+
641+
// This kind of situation coudl occur if say Vim is stalled for a bit
642+
// and we resized the window multiple times. We don't rate limit unlike
643+
// live resizing since usually it's not needed.
644+
NSRect newFrame = winFrame;
645+
newFrame.size.width -= textView.cellSize.width;
646+
newFrame.size.height -= textView.cellSize.height;
647+
648+
[win setFrame:newFrame display:YES];
649+
XCTAssertEqual(30, textView.maxRows);
650+
XCTAssertEqual(80, textView.maxColumns);
651+
XCTAssertEqual(29, textView.pendingMaxRows);
652+
XCTAssertEqual(79, textView.pendingMaxColumns);
653+
654+
[win setFrame:winFrame display:YES];
655+
XCTAssertEqual(30, textView.maxRows);
656+
XCTAssertEqual(80, textView.maxColumns);
657+
XCTAssertEqual(30, textView.pendingMaxRows);
658+
XCTAssertEqual(80, textView.pendingMaxColumns);
659+
660+
[self waitForVimProcess];
661+
XCTAssertEqual(30, textView.maxRows);
662+
XCTAssertEqual(80, textView.maxColumns);
663+
}
664+
665+
{
666+
// Test rapid resizing again, but this time we don't resize back to the
667+
// original size, but instead incremented multiple times. Just to make
668+
// sure we actually get set to the final size.
669+
NSRect newFrame = winFrame;
670+
for (int i = 0; i < 5; i++) {
671+
newFrame.size.width += textView.cellSize.width;
672+
newFrame.size.height += textView.cellSize.height;
673+
[win setFrame:newFrame display:YES];
674+
}
675+
XCTAssertEqual(30, textView.maxRows);
676+
XCTAssertEqual(80, textView.maxColumns);
677+
XCTAssertEqual(35, textView.pendingMaxRows);
678+
XCTAssertEqual(85, textView.pendingMaxColumns);
679+
680+
[self waitForVimProcess];
681+
XCTAssertEqual(35, textView.maxRows);
682+
XCTAssertEqual(85, textView.maxColumns);
683+
684+
[win setFrame:winFrame display:YES]; // reset back to original size
685+
[self waitForVimProcess];
686+
XCTAssertEqual(30, textView.maxRows);
687+
XCTAssertEqual(80, textView.maxColumns);
688+
}
689+
690+
{
691+
// Test live resizing (e.g. when user drags the window edge to resize).
692+
// We rate limit the number of messages we send to Vim so if there are
693+
// multiple resize events they will be sequenced to avoid overloading Vim.
694+
forceInLiveResize = YES; // simulate live resizing which can only be initiated by a user
695+
[vimView viewWillStartLiveResize];
696+
697+
NSRect newFrame = winFrame;
698+
for (int i = 0; i < 5; i++) {
699+
newFrame.size.width += textView.cellSize.width;
700+
newFrame.size.height += textView.cellSize.height;
701+
[win setFrame:newFrame display:YES];
702+
}
703+
704+
// The first time Vim processes this it should have only received the first message
705+
// due to rate limiting.
706+
XCTAssertEqual(30, textView.maxRows);
707+
XCTAssertEqual(80, textView.maxColumns);
708+
XCTAssertEqual(31, textView.pendingMaxRows);
709+
XCTAssertEqual(81, textView.pendingMaxColumns);
710+
711+
// After Vim has processed the messages it should now have the final size
712+
[self waitForVimProcess]; // first wait for Vim to respond it processed the first message, where we send off the second one
713+
[self waitForVimProcess]; // Vim should now have processed the last message
714+
XCTAssertEqual(35, textView.maxRows);
715+
XCTAssertEqual(85, textView.maxColumns);
716+
XCTAssertEqual(35, textView.pendingMaxRows);
717+
XCTAssertEqual(85, textView.pendingMaxColumns);
718+
719+
forceInLiveResize = NO;
720+
[vimView viewDidEndLiveResize];
721+
[self waitForVimProcess];
722+
XCTAssertEqual(35, textView.maxRows);
723+
XCTAssertEqual(85, textView.maxColumns);
724+
725+
[win setFrame:winFrame display:YES]; // reset back to original size
726+
[self waitForEventHandlingAndVimProcess];
727+
XCTAssertEqual(30, textView.maxRows);
728+
XCTAssertEqual(80, textView.maxColumns);
729+
}
730+
731+
// Clean up
732+
[[app keyVimController] sendMessage:VimShouldCloseMsgID data:nil];
733+
[self waitForVimClose];
734+
}
735+
586736
@end

0 commit comments

Comments
 (0)