Skip to content

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog 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.

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. 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 add_command_key branch 3 times, most recently from a4fc1a2 to 5fc34cd Compare November 7, 2019 23:33
@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
@Piinks Piinks added a: desktop Running on desktop platform-mac Building on or for macOS specifically customer: octopod labels Nov 11, 2019
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@jonahwilliams jonahwilliams left a 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

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gspencergoog gspencergoog merged commit 21158d8 into flutter:master Nov 18, 2019
@gspencergoog gspencergoog deleted the add_command_key branch March 13, 2020 16:10
@lock
Copy link

lock bot commented Apr 2, 2020

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 flutter doctor -v and a minimal reproduction of the issue.

@lock lock bot locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: desktop Running on desktop customer: octopod framework flutter/packages/flutter repository. See also f: labels. platform-mac Building on or for macOS specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support multi-line text editing on desktop platforms

6 participants