-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[macOS] Prepare TextInputPlugin for multi-view #164014
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4b578b3 to
429fc13
Compare
|
Ping @dkwingsmt or @justinmc perhaps? |
dkwingsmt
left a comment
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.
LGTM, thank you!
| } | ||
|
|
||
| _activeModel = std::make_unique<flutter::TextInputModel>(); | ||
| NSNumber* viewId = config[kViewId]; |
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.
Does it break existing apps that create such messages manually and doesn't add view ID? Should it default to implicit view ID?
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.
Are any applications out there creating the messages manually instead of using the text input client, which always puts the viewId? I'd hope not.
| // Returns the text input plugin associated with this view controller. | ||
| // This method only returns non nil instance if the text input plugin has active | ||
| // client with viewId matching this controller's view identifier. | ||
| - (FlutterTextInputPlugin*)textInputPlugin { |
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.
I'm concerned that this name might cause trouble if a maintainer does not read the doc carefully. Shall we rename it to something like matchingTextInputPlugin?
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.
That's a good point. I'll rename it to activeTextInputPlugin.
429fc13 to
c9b723b
Compare
Notable changes: - `FlutterTextInputPlugin` is now owned by `FlutterEngine`. - `FlutterTextInputPlugin` is only associated with a `FlutterViewController` while having active IME connection. - Changed private state of `TextInputPlugin` from properties to ivars. There is no need for these to be properties, and the state was accessed both as ivars and properties which seemed inconsistent. - Updated test to reflect the changes I've also tested everything including voice-over to make sure accessibility didn't regress. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Notable changes:
FlutterTextInputPluginis now owned byFlutterEngine.FlutterTextInputPluginis only associated with aFlutterViewControllerwhile having active IME connection.TextInputPluginfrom properties to ivars. There is no need for these to be properties, and the state was accessed both as ivars and properties which seemed inconsistent.I've also tested everything including voice-over to make sure accessibility didn't regress.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.