Skip to content

Commit 879a219

Browse files
committed
Fix non-native full screen bad interaction with window delegate
This issue was detected when adding new tests for non-native full screen. When setting 'fuoptions' to empty and going full screen, the code would fail but also crash, due to a number of issues: 1. `enterFullScreen`'s setting of presentationOptions somehow triggers a window resize on the normal window, which leads to `windowDidResize` delegate being called. This is a degenerate situation as the window controller thinks we are already in full screen even though we haven't finished setting up yet, and in particular `nonFuVimViewSize` is still 0. This leads to the desired desired frame size being set incorrectly to 0. 2. `constrainRows:columns:toSize` does not sanity check the results when we have such degenerate small sizes, and end up calculating a desired rows to -1, which makes no sense. This leads to various wrong calculations. 3. MMCoreTextView loops through the grid using a size_t even though grid.rows is an int. This is a poor code practice in general and results in a crash as the loop comparison cast the -1 to size_t and the loop didn't correctly terminate. Fix 1 (the root cause) by making sure we detact the window delegate immediately when entering full screen to prevent any stray window messages from causing issues. Also safeguard `windowDidResize` so it only handles the full screen path if we have `fullScreenEnabled` set. For 2 and 3, add the sanity checks and also fix the size_t to use int when looping. For some reason this issue only showed up in CI but not in local testing, probably due to different screen environments.
1 parent 2b136db commit 879a219

4 files changed

Lines changed: 30 additions & 13 deletions

File tree

src/MacVim/MMCoreTextView.m

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -831,7 +831,7 @@ - (void)drawRect:(NSRect)rect
831831

832832
// Function to draw all rows
833833
void (^drawAllRows)(void (^)(CGContextRef,CGRect,int)) = ^(void (^drawFunc)(CGContextRef,CGRect,int)){
834-
for (size_t r = 0; r < grid.rows; r++) {
834+
for (int r = 0; r < grid.rows; r++) {
835835
const CGRect rowRect = [self rectForRow:(int)r
836836
column:0
837837
numRows:1
@@ -1276,6 +1276,9 @@ - (NSSize)constrainRows:(int *)rows columns:(int *)cols toSize:(NSSize)size
12761276
if (fh < 1.0f) fh = 1.0f;
12771277

12781278
desiredRows = floor((size.height - ih)/fh);
1279+
// Sanity checking in case unusual window sizes lead to degenerate results
1280+
if (desiredRows < 1)
1281+
desiredRows = 1;
12791282
desiredSize.height = fh*desiredRows + ih;
12801283
}
12811284

@@ -1285,6 +1288,9 @@ - (NSSize)constrainRows:(int *)rows columns:(int *)cols toSize:(NSSize)size
12851288
if (fw < 1.0f) fw = 1.0f;
12861289

12871290
desiredCols = floor((size.width - iw)/fw);
1291+
// Sanity checking in case unusual window sizes lead to degenerate results
1292+
if (desiredCols < 1)
1293+
desiredCols = 1;
12881294
desiredSize.width = fw*desiredCols + iw;
12891295
}
12901296

src/MacVim/MMFullScreenWindow.m

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,15 @@ - (void)enterFullScreen
136136
{
137137
ASLogDebug(@"Enter full-screen now");
138138

139+
// Detach the window delegate right now to prevent any stray window
140+
// messages (e.g. it may get resized when setting presentationOptions
141+
// below) being sent to the window controller while we are in the middle of
142+
// setting up the full screen window.
143+
NSWindowController *winController = [target windowController];
144+
id delegate = [target delegate];
145+
[winController setWindow:nil];
146+
[target setDelegate:nil];
147+
139148
// Hide Dock and menu bar when going to full screen. Only do so if the current screen
140149
// has a menu bar and dock.
141150
if ([self screenHasDockAndMenu]) {
@@ -162,13 +171,6 @@ - (void)enterFullScreen
162171
// this call so set the frame again just in case.
163172
[self setFrame:[[target screen] frame] display:NO];
164173

165-
// fool delegate
166-
id delegate = [target delegate];
167-
[target setDelegate:nil];
168-
169-
// make target's window controller believe that it's now controlling us
170-
[[target windowController] setWindow:self];
171-
172174
oldTabBarStyle = [[view tabBarControl] styleName];
173175

174176
NSString *style =
@@ -181,7 +183,7 @@ - (void)enterFullScreen
181183
[view removeFromSuperviewWithoutNeedingDisplay];
182184
[[self contentView] addSubview:view];
183185
[self setInitialFirstResponder:[view textView]];
184-
186+
185187
// NOTE: Calling setTitle:nil causes an exception to be raised (and it is
186188
// possible that 'target' has no title when we get here).
187189
if ([target title]) {
@@ -196,8 +198,10 @@ - (void)enterFullScreen
196198

197199
[self setOpaque:[target isOpaque]];
198200

201+
// reassign target's window controller to believe that it's now controlling us
199202
// don't set this sooner, so we don't get an additional
200-
// focus gained message
203+
// focus gained message
204+
[winController setWindow:self];
201205
[self setDelegate:delegate];
202206

203207
// Store view dimension used before entering full-screen, then resize the

src/MacVim/MMTextStorage.m

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -895,6 +895,9 @@ - (NSSize)fitToSize:(NSSize)size rows:(int *)rows columns:(int *)columns
895895
if (fh < 1.0f) fh = 1.0f;
896896

897897
fitRows = floor(size.height/fh);
898+
// Sanity checking in case unusual window sizes lead to degenerate results
899+
if (fitRows < 1)
900+
fitRows = 1;
898901
fitSize.height = fh*fitRows;
899902
}
900903

@@ -903,6 +906,9 @@ - (NSSize)fitToSize:(NSSize)size rows:(int *)rows columns:(int *)columns
903906
if (fw < 1.0f) fw = 1.0f;
904907

905908
fitCols = floor(size.width/fw);
909+
// Sanity checking in case unusual window sizes lead to degenerate results
910+
if (fitCols < 1)
911+
fitCols = 1;
906912
fitSize.width = fw*fitCols;
907913
}
908914

src/MacVim/MMWindowController.m

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1307,9 +1307,7 @@ - (void)windowDidResize:(id)sender
13071307
// Calling setFrameSizeKeepGUISize: instead of setFrameSize: prevents a
13081308
// degenerate case where frameSizeMayHaveChanged: ends up resizing the window
13091309
// *again* causing windowDidResize: to be called.
1310-
if (fullScreenWindow == nil) {
1311-
[vimView setFrameSizeKeepGUISize:[self contentSize]];
1312-
} else {
1310+
if (fullScreenEnabled && fullScreenWindow != nil) {
13131311
// Non-native full screen mode is more complicated and needs to
13141312
// re-layout the Vim view to properly account for the menu bar / notch,
13151313
// and misc fuopt configuration.
@@ -1318,6 +1316,9 @@ - (void)windowDidResize:(id)sender
13181316
[vimView setFrameOrigin:desiredFrame.origin];
13191317
[vimView setFrameSizeKeepGUISize:desiredFrame.size];
13201318
}
1319+
else {
1320+
[vimView setFrameSizeKeepGUISize:[self contentSize]];
1321+
}
13211322
}
13221323

13231324
- (void)windowDidChangeBackingProperties:(NSNotification *)notification

0 commit comments

Comments
 (0)