Skip to content
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

Add command key bindings to macOS text editing and fix selection. #44130

Merged
merged 10 commits into from Nov 18, 2019

Conversation

@gspencergoog
Copy link
Contributor

gspencergoog commented Nov 4, 2019

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

  • Added tests for multiple platforms to make sure that editing works there.

Breaking Change

  • No, this is not a breaking change.
@googlebot googlebot added the cla: yes label Nov 4, 2019
@gspencergoog gspencergoog changed the title Add command key Add command key bindings to macOS text editing. Nov 4, 2019
@gspencergoog gspencergoog force-pushed the gspencergoog:add_command_key branch 3 times, most recently from a4fc1a2 to 5fc34cd Nov 6, 2019
@gspencergoog gspencergoog changed the title Add command key bindings to macOS text editing. Add command key bindings to macOS text editing and fix selection. Nov 7, 2019
@gspencergoog gspencergoog force-pushed the gspencergoog:add_command_key branch from 5fc34cd to e956ff9 Nov 13, 2019
@gspencergoog gspencergoog requested a review from jonahwilliams Nov 18, 2019
_cursorResetLocation += 1;

int nextNonWhitespace(int extent) {
final String plain = text.toPlainText();

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Nov 18, 2019

Contributor

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

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Nov 18, 2019

Author Contributor

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.

@gspencergoog gspencergoog force-pushed the gspencergoog:add_command_key branch from e956ff9 to 60c4f45 Nov 18, 2019
Copy link
Contributor

jonahwilliams left a comment

I took a quick pass through this one. Generally looks good, but I'm a bit concerned about performance on larger buffers

Copy link
Contributor

jonahwilliams left a comment

LGTM

@gspencergoog gspencergoog merged commit 21158d8 into flutter:master Nov 18, 2019
63 checks passed
63 checks passed
WIP Ready for review
Details
analyze-linux Task Summary
Details
analyze-linux
Details
build_tests-linux Task Summary
Details
build_tests-linux
Details
cla/google All necessary CLAs are signed
customer_testing-linux Task Summary
Details
customer_testing-linux
Details
customer_testing-macos Task Summary
Details
customer_testing-macos
Details
customer_testing-windows Task Summary
Details
customer_testing-windows
Details
deploy_gallery-linux Task Summary
Details
deploy_gallery-linux
Details
deploy_gallery-macos Task Summary
Details
deploy_gallery-macos
Details
docs-linux Task Summary
Details
docs-linux
Details
firebase_test_lab_tests-linux Task Summary
Details
firebase_test_lab_tests-linux
Details
flutter-build
Details
framework_tests-libraries-linux Task Summary
Details
framework_tests-libraries-linux
Details
framework_tests-libraries-macos Task Summary
Details
framework_tests-libraries-macos
Details
framework_tests-libraries-windows Task Summary
Details
framework_tests-libraries-windows
Details
framework_tests-misc-linux Task Summary
Details
framework_tests-misc-linux
Details
framework_tests-misc-macos Task Summary
Details
framework_tests-misc-macos
Details
framework_tests-misc-windows Task Summary
Details
framework_tests-misc-windows
Details
framework_tests-widgets-linux Task Summary
Details
framework_tests-widgets-linux
Details
framework_tests-widgets-macos Task Summary
Details
framework_tests-widgets-macos
Details
framework_tests-widgets-windows Task Summary
Details
framework_tests-widgets-windows
Details
hostonly_devicelab_tests-0-linux Task Summary
Details
hostonly_devicelab_tests-0-linux
Details
hostonly_devicelab_tests-1-linux Task Summary
Details
hostonly_devicelab_tests-1-linux
Details
hostonly_devicelab_tests-2-linux Task Summary
Details
hostonly_devicelab_tests-2-linux
Details
hostonly_devicelab_tests-3_last-linux Task Summary
Details
hostonly_devicelab_tests-3_last-linux
Details
web_tests-0-linux Task Summary
Details
web_tests-0-linux
Details
web_tests-1-linux Task Summary
Details
web_tests-1-linux
Details
web_tests-2-linux Task Summary
Details
web_tests-2-linux
Details
web_tests-3-linux Task Summary
Details
web_tests-3-linux
Details
web_tests-4-linux Task Summary
Details
web_tests-4-linux
Details
web_tests-5-linux Task Summary
Details
web_tests-5-linux
Details
web_tests-6-linux Task Summary
Details
web_tests-6-linux
Details
web_tests-7_last-linux Task Summary
Details
web_tests-7_last-linux
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.