Mac cmd + shift + left/right by justinmc · Pull Request #95948 · flutter/flutter

justinmc · 2021-12-29T23:45:38Z

Command + shift + left/right arrow should expand to the beginning/end of the line, not extend or pivot like other platforms.

Mac single line

words |words words
=>
words |words words|
<=
|words words words|

Other platforms single line

words |words words
=>
words |words words|
<=
|words |words words

Mac multi line

words |words words
words |words words
=>
words |words words
words words words|
<=
|words words words
words words words|

Mac single line

words |words words
=>
words |words words|
<=
|words words words|

Other platforms multi line

words |words words
words |words words
=>
words |words words
words words words|
<=
|words words words
|words words words

Fixes #93883 (comment)

justinmc · 2021-12-30T00:27:30Z

packages/flutter/lib/src/widgets/default_text_editing_shortcuts.dart


-    const SingleActivator(LogicalKeyboardKey.arrowLeft, shift: true, meta: true): const ExtendSelectionToLineBreakIntent(forward: false, collapseSelection: false),
-    const SingleActivator(LogicalKeyboardKey.arrowRight, shift: true, meta: true): const ExtendSelectionToLineBreakIntent(forward: true, collapseSelection: false),
+    const SingleActivator(LogicalKeyboardKey.arrowLeft, shift: true, meta: true): const ExtendSelectionToLineBreakIntent(forward: false, collapseSelection: false, expand: true),


@LongCatIsLooong What do you think of adding this expand parameter here, or would it be better to do something like creating an all new intent?

Seems reasonable, but I would slightly prefer having a different intent since collapseSelection and expand can't both be true and currently it relies on a run time assert.

I've created a new Intent as a subclass of DirectionalTextEditingIntent and I think it worked out cleaner 👍 .

LongCatIsLooong

LGTM with nits

LongCatIsLooong · 2022-01-11T19:31:25Z

packages/flutter/lib/src/widgets/editable_text.dart

    }

-    final TextPosition extent = textBoundarySelection.extent;
+    late final TextPosition position;


nit: is the late needed?

Undone with refactoring to use a new Intent.

LongCatIsLooong · 2022-01-11T19:50:33Z

packages/flutter/lib/src/widgets/text_editing_intents.dart

+  /// be held in place while the extent is moved.
+  ///
+  /// When set to true, the selection will expand, meaning that the selection
+  /// will grow by moving either the base or the extent.


"whichever is closest to the target location" or something similar?

Also maybe say this is typically only for macOS?

This exact comment was removed because I created a separate Intent. I made sure that Intent's comments were clear and I added a line about MacOS there.

LongCatIsLooong · 2022-01-11T21:12:43Z

packages/flutter/lib/src/widgets/editable_text.dart

-    final TextSelection newSelection = collapseSelection
-      ? TextSelection.fromPosition(newExtent)
-      : textBoundarySelection.extendTo(newExtent);
+    late final TextSelection newSelection;


unnecessary late?

Undone with refactoring to use a new Intent.

LongCatIsLooong · 2022-01-11T21:26:42Z

packages/flutter/lib/src/widgets/editable_text.dart

+    if (collapseSelection) {
+      newSelection = TextSelection.fromPosition(newExtent);
+    } else {
+      newSelection = intent.expand


I tried this and it seems macos does not move the extent when it expands the selection? cmd+shift+-> and then press -> the cursor moves to 1 character to the right from its original position.

I think I've got this right now, the behavior might have changed since you tested it. Here's what I'm testing that seems to match with TextEdit:

With cursor in the middle of the line, cmd + left. Left end is the extent.

With cursor in the middle of the line, cmd + right. Right end is the extent.

With cursor in the middle of the line, cmd + left then cmd + right. Left end is the extent.

With cursor in the middle of the line, cmd + right then cmd + left. Right end is the extent.

With an RTL selection in the middle of the line, cmd + left. Left end is the extent.

With an RTL selection in the middle of the line, cmd + right. Left end is the extent.

With an LTR selection in the middle of the line, cmd + right. Right end is the extent.

With an LTR selection in the middle of the line, cmd + left. Right end is the extent.

I just added tests for all of these cases too. By "testing" in the previous comment I meant manually trying myself vs. TextEdit.

LongCatIsLooong · 2022-01-11T21:33:30Z

packages/flutter/lib/src/widgets/default_text_editing_shortcuts.dart


-    const SingleActivator(LogicalKeyboardKey.arrowLeft, shift: true, meta: true): const ExtendSelectionToLineBreakIntent(forward: false, collapseSelection: false),
-    const SingleActivator(LogicalKeyboardKey.arrowRight, shift: true, meta: true): const ExtendSelectionToLineBreakIntent(forward: true, collapseSelection: false),
+    const SingleActivator(LogicalKeyboardKey.arrowLeft, shift: true, meta: true): const ExtendSelectionToLineBreakIntent(forward: false, collapseSelection: false, expand: true),


Seems reasonable, but I would slightly prefer having a different intent since collapseSelection and expand can't both be true and currently it relies on a run time assert.

…ime asserts

…comments

Works by hacking in an expand param

b408d27

justinmc self-assigned this Dec 29, 2021

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Dec 29, 2021

Update tests

5136086

justinmc force-pushed the mac-expand-to-linebreak branch from 2f10e2e to 5136086 Compare December 29, 2021 23:48

Handle multi-line selection like native Mac does

5abd6f2

justinmc requested a review from LongCatIsLooong December 30, 2021 00:24

justinmc marked this pull request as ready for review December 30, 2021 00:24

justinmc commented Dec 30, 2021

View reviewed changes

Restore accidental change to test's platforms

a0a7237

justinmc mentioned this pull request Jan 11, 2022

Windows/Linux keyboard shortcuts at a wordwrap #96323

Merged

LongCatIsLooong approved these changes Jan 11, 2022

View reviewed changes

justinmc added 2 commits January 11, 2022 15:07

New intent type for expand

be8a5a8

Completely separate intent from DirectionalCaretMovement, avoids runt…

fa01357

…ime asserts

justinmc force-pushed the mac-expand-to-linebreak branch from d10f76e to fa01357 Compare January 11, 2022 23:46

justinmc added 3 commits January 11, 2022 16:10

Test extent position in Mac shortcuts, which came up as confusing in …

c24b1d4

…comments

Clarify extend/expand in docs

2bd76f0

Analyzer

ded635c

justinmc added the waiting for tree to go green label Jan 13, 2022

fluttergithubbot merged commit 8b46014 into flutter:master Jan 13, 2022

justinmc deleted the mac-expand-to-linebreak branch January 13, 2022 22:30

engine-flutter-autoroll mentioned this pull request Jan 18, 2022

Roll Flutter from 3e6e996f9eff to a3fd36a11500 (77 revisions) flutter/plugins#4677

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 18, 2022

8b46014 Mac cmd + shift + left/right (flutter/flutter#95948)

27272f9

engine-flutter-autoroll mentioned this pull request Jan 18, 2022

Roll Flutter from 3e6e996f9eff to 4e468f0122a9 (78 revisions) flutter/plugins#4678

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 18, 2022

8b46014 Mac cmd + shift + left/right (flutter/flutter#95948)

2d523b2

engine-flutter-autoroll mentioned this pull request Jan 18, 2022

Roll Flutter from 3e6e996f9eff to a2ac0e30fd41 (82 revisions) flutter/plugins#4680

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 18, 2022

8b46014 Mac cmd + shift + left/right (flutter/flutter#95948)

35d4569

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 4, 2022

8b46014 Mac cmd + shift + left/right (flutter/flutter#95948)

5ec9f6a

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022

Mac cmd + shift + left/right (flutter#95948)

d059ddd

Conversation

justinmc commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

justinmc commented Dec 29, 2021 •

edited

Loading