[iOS] Migrate to explicit VSyncClient initialisers - #185916
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes convenience initializers for FlutterVSyncClient, requiring the use of the designated initializer with explicit variable refresh rate and max refresh rate parameters. This change is implemented across FlutterKeyboardInsetManager, FlutterViewController, and vsync_waiter_ios, with updates to the corresponding unit tests. Feedback recommends using std::move for the callback in vsync_waiter_ios.mm to prevent unnecessary copies and updating the documentation for the modified initializers to include descriptions for the new parameters as required by the style guide.
I am having trouble creating individual review comments. Click here to see my feedback.
engine/src/flutter/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm (28)
The callback lambda should be moved into the initializer to avoid an unnecessary copy, consistent with the previous implementation in FlutterVSyncClient.mm.
callback:std::move(callback)];
engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterVSyncClient.h (69-76)
With the removal of the convenience initializer, the designated initializer initWithTaskRunner:isVariableRefreshRateEnabled:maxRefreshRate:callback: is now the primary public entry point. It should be updated to include full documentation for its parameters, adhering to the Flutter style guide for public members.
References
- All public members should have documentation. Documentation should be useful: Explain the why and the how. (link)
engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterVSyncClient+FML.h (17-18)
The documentation for the remaining internal initializer should be updated to describe the new parameters isVariableRefreshRateEnabled and maxRefreshRate, as the current description is now incomplete.
References
- All public members should have documentation. Documentation should be useful: Explain the why and the how. (link)
e9bff8a to
e121a23
Compare
Good point. Done.
Bad feedback. It's already documented.
Bad feedback. This is also already documented and I'm going to migrate the last use of it in the next PR and delete it anyways. |
Migrates `FlutterVSyncClient` to use explicit designated initialisers and removes the convenience initialisers that implicitly injected refresh rate details from `FlutterDisplayLinkManager`. Previously, `FlutterVSyncClient` provided initializers that defaulted the refresh rate settings by reading directly from `FlutterDisplayLinkManager` which created an implicit dependency on global state and made the class harder to test in isolation or with specific configurations. Issue: flutter#112232
e121a23 to
07ac300
Compare
| _keyboardAnimationVSyncClient = [[FlutterVSyncClient alloc] | ||
| initWithTaskRunner:delegate.engine.uiTaskRunner | ||
| isVariableRefreshRateEnabled:FlutterDisplayLinkManager.maxRefreshRateEnabledOnIPhone | ||
| maxRefreshRate:FlutterDisplayLinkManager.displayRefreshRate |
There was a problem hiding this comment.
Sanity check - This maxRefreshRate means the resolved max refresh rate, not the physical capability of the screen, right?
There was a problem hiding this comment.
Correct yep -- don't want to lose the approval bit but will send a followup that adds that to the docs! Thanks for calling it out!
Migrates
FlutterVSyncClientto use explicit designated initialisers and removes the convenience initialisers that implicitly injected refresh rate details fromFlutterDisplayLinkManager.Previously,
FlutterVSyncClientprovided initializers that defaulted the refresh rate settings by reading directly fromFlutterDisplayLinkManagerwhich created an implicit dependency on global state and made the class harder to test in isolation or with specific configurations.Issue: #112232
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.