-
Notifications
You must be signed in to change notification settings - Fork 6k
Add FlTextInputPlugin #18314
Add FlTextInputPlugin #18314
Conversation
|
This will fix flutter/flutter#54853 and flutter/flutter#30662. |
stuartmorgan-g
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.
From a quick skim, my initial feedback (other than we should share a slightly refactored text input model if possible, per Discord discussion, since I would rather we not maintain N implementations of the same embedding-side model indefinitely) is to wonder why things like editing state callback handlers are in FlView. I think the text input "plugin" (poorly named, as it's not an actual plugin) structure that's used on most of the other embeddings (at least iOS, macOS, Windows, and GLFW) is a good separation of concerns. FlView is the kind of thing that can become increasingly large over time if we're not careful.
6c751e5 to
74ecb71
Compare
|
Looks like I pushed this to the flutter/engine repo not robert-ancell/engine. I've been updating this in the wrong branch. I've pushed all my changes now and this should be ready for review. |
a49ecc2 to
10119bc
Compare
ce78670 to
c40a19b
Compare
stuartmorgan-g
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 with some minor changes.
c40a19b to
b0cf343
Compare
Builds on #18220 and #18313