-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Add command key bindings to macOS text editing and fix selection. #44130
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
a4fc1a2 to
5fc34cd
Compare
5fc34cd to
e956ff9
Compare
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.
Seems like this code would result in use computing plain text every time a key is pressed, which it should be cache-able as long as the TextPainter doesn't update. I'm not sure how large the performance impact would be, but it might be an easy win
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 added _plainText that lazily evaluates and caches the plain text version of the text. It didn't add too much complexity (actually seems easier to read to me), and can only result in a win performance-wise. The only danger is that the TextPainter updates its text member somehow without telling the RenderEditable, but I don't think that's possible, since the RenderEditable owns the painter and doesn't expose it.
e956ff9 to
60c4f45
Compare
jonahwilliams
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.
I took a quick pass through this one. Generally looks good, but I'm a bit concerned about performance on larger buffers
jonahwilliams
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
|
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
Description
This adds support for the command key for text selection/editing on macOS. I had ported the text editing code (in #42879), but forgot to add support for the command key itself. This also adds a test that tests the text editing on multiple platforms instead of just testing Android.
There appears to still be a bug (filed #44135) where we're losing key events sometimes on macOS, leaving some keys "stuck" on, but this PR at least allows the right key combinations to be used.
Related Issues
Addresses #30709
Fixes #36316
Tests
Breaking Change