-
Notifications
You must be signed in to change notification settings - Fork 6k
WIP: Smooth window resizing on macOS #21110
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial review, need to take a deeper look still. Have you thought about a testing strategy for this change?
@@ -262,7 +261,7 @@ - (BOOL)runWithEntrypoint:(NSString*)entrypoint { | |||
const FlutterCustomTaskRunners custom_task_runners = { | |||
.struct_size = sizeof(FlutterCustomTaskRunners), | |||
.platform_task_runner = &cocoa_task_runner_description, | |||
.render_task_runner = &cocoa_task_runner_description, | |||
.render_task_runner = nullptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line can be removed.
@@ -79,7 +80,9 @@ source_set("flutter_framework_source") { | |||
|
|||
libs = [ | |||
"Cocoa.framework", | |||
"QuartzCore.framework", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: arrange these in alphabetical order.
-self.layer.bounds.size.height, 0); | ||
contentLayer.frame = self.layer.bounds; | ||
|
||
[contentLayer setContents:nil]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment explaining why we need to set to nil
before setting it to the contents of the ioSurface
@@ -26,6 +26,11 @@ | |||
*/ | |||
- (void)updateWindowMetrics; | |||
|
|||
/** | |||
* Schedules the block on rater thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: rater -> raster
void* copy = Block_copy((__bridge void*)block); | ||
FlutterEnginePostRenderThreadTask( | ||
_engine, | ||
[](void* cb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: rename to callback
@@ -334,6 +337,18 @@ - (void)updateWindowMetrics { | |||
FlutterEngineSendWindowMetricsEvent(_engine, &event); | |||
} | |||
|
|||
- (void)scheduleOnRasterTread:(dispatch_block_t)block { | |||
void* copy = Block_copy((__bridge void*)block); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, why do we need to copy the block here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocks are by default allocated on stack. It needs to be moved to heap otherwise it will get destroyed on scope exit.
@@ -280,7 +279,8 @@ - (BOOL)runWithEntrypoint:(NSString*)entrypoint { | |||
} | |||
|
|||
[self sendUserLocales]; | |||
[self updateWindowMetrics]; | |||
|
|||
[self.viewController.flutterView start]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to have start
rather than doing it in init
or ctor
for FlutterView
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC start causes flutterView to drive viewport size change. I think I'd prefer that to be separate method rather than doing it from constructor.
@@ -7,32 +7,39 @@ | |||
/** | |||
* Listener for view resizing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix the doc.
Superseded by #21252 |
Description
Make window resizing on macOS smooth, responsive and synchronous with windows size.
Related design doc: https://flutter.dev/go/desktop-resize-macos
Depends on: #21108
Related Issues
flutter/flutter#44136
Tests
I added the following tests:
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.