[iOS] Migrate FlutterKeyboardInsetManager to Swift#189425
Conversation
|
NOTE TO REVIEWERS: This is stacked on top of #189353 which has been waiting to merge for ~8 hours over multiple attempts. Sending this out stacked on top of that PR to avoid a 24 hour review round trip. Once I rebase, any approvals will be lost so I'll address any feedback and re-request review once that's done. The actual change in the PR is the SECOND commit; please only review that one: 656d8a7 As noted in the PR description, this intentionally tries to stay as 1:1 with the Obj-C code as possible and avoids trying to make this Swifty. The goal here is to get to a 100% working and well-tested Swift version; we can make it Swiftier later :) |
There was a problem hiding this comment.
Code Review
This pull request rewrites FlutterKeyboardInsetManager from Objective-C++ to Swift (KeyboardInsetManager.swift), updating the build configuration, bridging headers, and associated tests to support this transition. The review feedback focuses on making the Swift implementation more idiomatic by casting directly to Swift types (such as Double and Bool) instead of using NSNumber, simplifying conditional checks, and removing redundant nil checks.
656d8a7 to
cd10d45
Compare
cd10d45 to
ddc2170
Compare
Ports FlutterKeyboardInsetManager from Objective-C to Swift. This is intended to be as close to a 1:1 translation of the original as possible: I've tried to preserve the structure, control flow, and behaviour of the original, and tried to use only a few Swift conveniences such as `guard` and optional binding. We can make this Swiftier in followups. We continue to expose the class to Objective-C++ via `@objc`. The keyboard mode enum and the manager's delegate protocol, which were previously declared in FlutterViewController_Internal.h and FlutterKeyboardInsetManager.h are now part of the Swift implementation, and having an _Internal.h header for an already-internal class was less than idiomatic, regardless. FlutterViewController now holds the manager through a new FlutterKeyboardInsetManagerProtocol rather than the concrete type, mirroring how the other migrated managers are wired up, which lets us substitute a fake and gives us an eventual path off of OCMock. I've added a uiTaskRunner accessor to the delegate protocol and implemented on FlutterViewController so the manager can reach the UI task runner directly rather than through `engine`, which keeps the vsync setup testable. There are a few details were kept deliberately as close as possible to the original, since they are easy to get subtly wrong when moving to Swift: * setUpKeyboardSpringAnimationIfNeeded is always invoked at the end of the keyboard animation block, including with a nil animation, so that the absence of a position animation clears the cached spring animation instead of leaving a stale one behind. hideKeyboardImmediately clears it as well. * isKeyboardNotificationForDifferentView compares the delegate against the engine's view controller by object identity (=== / !==), matching the original pointer comparison rather than -isEqual:. * The vsync callback is delivered with a plain dispatch to the main queue, as before, and the delegate is not retained for the duration of the animation. The tests in FlutterViewControllerTest.mm needed a more than a mechanical update. Swift classes don't co-operate with OCMock the way the original Objective-C class did: OCMock can't intercept a call one Swift method makes to another (they aren't dynamically dispatched like they are in Obj-C), and the generated header marks the class objc_subclassing_restricted, so it can't be subclassed from the test target to form a partial mock. I've reworked the affected tests to drive the real keyboard inset manager and assert on observable results (the computed inset, whether a vsync client was created, whether a pre-seeded spring animation was cleared) rather than verifying that one internal method called another; this seems like an improvement regardless. Normally I would have pushed to a followup, but as noted, Swift dispatch is more efficient than Obj-C but it means we can't intercept these calls in OCMock anymore, regardless. I've kept a minimal FakeFlutterKeyboardInsetManager conforming to the protocol for a test that confirms the view controller drives the manager on viewDidDisappear. This also adjusts the exported symbols check to allow symbols for Swift classes exported in the DATA section from InternalFlutterSwift (and also InternalFlutterSwiftCommon). The `_TtC` is standard Swift ABI name mangling: * `_Tt` indicates a Swift type exported to Obj-C. * `C` indicates a class. Issue: flutter#112232
ddc2170 to
8c1d5ff
Compare
hellohuanlin
left a comment
There was a problem hiding this comment.
Swift code looks great!
flutter/flutter@846664b...fc1ad95 2026-07-15 [email protected] [iOS] Migrate FlutterKeyboardInsetManager to Swift (flutter/flutter#189425) 2026-07-15 [email protected] Roll Dart SDK from 05bf153370c4 to 0c408ff6dce9 (4 revisions) (flutter/flutter#189487) 2026-07-15 [email protected] Implement UberSDF lines to replace LineContents-based AA lines (flutter/flutter#188514) 2026-07-15 [email protected] Fix stale eagerWinner reference in GestureArenaManager when rejected before arena close (flutter/flutter#187008) 2026-07-15 [email protected] Disable some Windows tests that are flaking on CI (flutter/flutter#189477) 2026-07-15 dmgr Added unified check-run user manual (flutter/flutter#189453) 2026-07-15 [email protected] Roll Skia from 88954ef8f36d to ab2410bc857c (9 revisions) (flutter/flutter#189474) 2026-07-14 [email protected] [agents] Refactor shepherd-prs skill into a pure Markdown runbook using native gh CLI (flutter/flutter#189095) 2026-07-14 [email protected] Add missing name to mirroring workflow (flutter/flutter#189439) 2026-07-14 [email protected] Add support for WASM deferred loading. (flutter/flutter#189308) 2026-07-14 [email protected] fix `templateDefaultGradleVersion` todo (flutter/flutter#189466) 2026-07-14 [email protected] Roll Fuchsia Linux SDK from oOETA0ISPouDt2xBo... to lLFbh5kFWbUGgC9Ek... (flutter/flutter#189469) 2026-07-14 [email protected] Use ServicesBinding.instance.exitApplication instead of exit(0) in multiple_windows example (flutter/flutter#189364) 2026-07-14 [email protected] Add CpuArch to the Device class (flutter/flutter#189207) 2026-07-14 [email protected] Fix space formatting in cherry-pick label for flutter_cp.dart (flutter/flutter#189463) 2026-07-14 [email protected] Roll pub packages (flutter/flutter#189454) 2026-07-14 [email protected] [flutter_tools] Format plugin example template to match dart format (flutter/flutter#188382) 2026-07-14 [email protected] Move renamed x64->ARM benchmarks out of bringup (flutter/flutter#189400) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Ports FlutterKeyboardInsetManager from Objective-C to Swift. This is intended to be as close to a 1:1 translation of the original as possible: I've tried to preserve the structure, control flow, and behaviour of the original, and tried to use only a few Swift conveniences such as
guardand optional binding. We can make this Swiftier in followups.We continue to expose the class to Objective-C++ via
@objc. The keyboard mode enum and the manager's delegate protocol, which were previously declared in FlutterViewController_Internal.h and FlutterKeyboardInsetManager.h are now part of the Swift implementation, and having an _Internal.h header for an already-internal class was less than idiomatic, regardless.FlutterViewController now holds the manager through a new FlutterKeyboardInsetManagerProtocol rather than the concrete type, mirroring how the other migrated managers are wired up, which lets us substitute a fake and gives us an eventual path off of OCMock. I've added a uiTaskRunner accessor to the delegate protocol and implemented on FlutterViewController so the manager can reach the UI task runner directly rather than through
engine, which keeps the vsync setup testable.There are a few details were kept deliberately as close as possible to the original, since they are easy to get subtly wrong when moving to Swift:
setUpKeyboardSpringAnimationIfNeeded is always invoked at the end of the keyboard animation block, including with a nil animation, so that the absence of a position animation clears the cached spring animation instead of leaving a stale one behind. hideKeyboardImmediately clears it as well.
isKeyboardNotificationForDifferentView compares the delegate against the engine's view controller by object identity (=== / !==), matching the original pointer comparison rather than -isEqual:.
The vsync callback is delivered with a plain dispatch to the main queue, as before, and the delegate is not retained for the duration of the animation.
The tests in FlutterViewControllerTest.mm needed a more than a mechanical update. Swift classes don't co-operate with OCMock the way the original Objective-C class did:
OCMock can't intercept a call one Swift method makes to another (they aren't dynamically dispatched like they are in Obj-C), and the generated header marks the class objc_subclassing_restricted, so it can't be subclassed from the test target to form a partial mock. I've reworked the affected tests to drive the real keyboard inset manager and assert on observable results (the computed inset, whether a vsync client was created, whether a pre-seeded spring animation was cleared) rather than verifying that one internal method called another; this seems like an improvement regardless. Normally I would have pushed to a followup, but as noted, Swift dispatch is more efficient than Obj-C but it means we can't intercept these calls in OCMock anymore, regardless.
I've kept a minimal FakeFlutterKeyboardInsetManager conforming to the protocol for a test that confirms the view controller drives the manager on viewDidDisappear.
This also adjusts the exported symbols check to allow symbols for Swift classes exported in the DATA section from InternalFlutterSwift (and also InternalFlutterSwiftCommon). The
_TtCis standard Swift ABI name mangling:_Ttindicates a Swift type exported to Obj-C.Cindicates a class.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.