Windows home/end shortcuts by justinmc · Pull Request #90840 · flutter/flutter

justinmc · 2021-09-27T23:20:47Z

This PR fixes some keyboard shortcuts on Windows that were missing/incorrect. From the issue, these were my observations before starting this PR:

Shortcut	Behavior (Windows)	Behavior (Flutter)
Ctrl+Home/End	move to start/end of all text	❌ nothing
Ctrl+Shift+Home/End	extend selection to start/end of all text	❌ nothing
Shift+Home/End	extend selection to start/end of line	❌ works, but pauses at inversion, and jumps lines

Fixes #77020
I've opened a separate issue for the incorrect movement between lines: #90993

…ersal intents/actions

justinmc · 2021-09-28T22:11:00Z

@LongCatIsLooong FYI this is going to conflict with your refactor #90684. I'm still working on this a bit so maybe I should just wait until your PR is merged and then make these changes in the new pattern.

goderbauer · 2021-11-10T22:49:46Z

From triage: Looks like #90684 is submitted now 🥳

justinmc · 2021-11-10T22:58:20Z

Will update this, thanks!

…een merged?

justinmc · 2021-11-11T01:12:34Z

      ? TextSelection.fromPosition(newExtent)
      : textBoundarySelection.extendTo(newExtent);

+    // If collapseAtReversal is true and would have an effect, collapse it.


@LongCatIsLooong How would you prefer that we implement this "collapseAtReversal" feature within your new Actions setup? Feel free to butcher this if it's totally off, I'm still getting the hang of your new refactor so I've just put down a quick attempt.

Yeah this makes sense to me!

LongCatIsLooong · 2021-11-11T20:44:01Z

    SingleActivator(LogicalKeyboardKey.end): ExtendSelectionToLineBreakIntent(forward: true, collapseSelection: true),
-    SingleActivator(LogicalKeyboardKey.home, shift: true): ExtendSelectionToLineBreakIntent(forward: false, collapseSelection: false),
-    SingleActivator(LogicalKeyboardKey.end, shift: true): ExtendSelectionToLineBreakIntent(forward: true, collapseSelection: false),
+    SingleActivator(LogicalKeyboardKey.home, shift: true): ExtendSelectionToLineBreakIntent(forward: false, collapseSelection: false, collapseAtReversal: true),


I tried on windows 10, it doesn't seem to collapse at reversal? E.g., the initial selection is aaaaaa|bbbbb, I pressed shift-End it became aaaaaa<bbbbb> and then I pressed shift-Home it became <aaaaaa>bbbbb. This was tested in the explorer's address bar.

Thanks good catch. My table in the description of this PR is correct, but I got this code backwards when I ported it. I've fixed it.

I also double checked Linux (gedit). It has the same behavior as Windows.

justinmc · 2021-11-19T20:28:31Z

@LongCatIsLooong In need of a re-review when you have a chance.

LongCatIsLooong · 2021-11-22T21:31:28Z

  //   * Meta + backspace
-  static const Map<ShortcutActivator, Intent> _windowsShortcuts = _linuxShortcuts;
+  static const Map<ShortcutActivator, Intent> _windowsShortcuts = <ShortcutActivator, Intent>{
+    ..._linuxShortcuts,


nit: _commonShortcuts is the same as _linuxShortcuts iirc? I probably should have referenced _commonShortcuts here it's a bit confusing why we are including _linuxShortcuts for windows.

I've reworked this to just use ..._commonShortcuts here. I had to duplicate 4 entries but I think that's fine.

LongCatIsLooong · 2021-11-22T21:48:01Z

    SingleActivator(LogicalKeyboardKey.arrowUp, shift: true): DoNothingAndStopPropagationTextIntent(),
    SingleActivator(LogicalKeyboardKey.end, shift: true): DoNothingAndStopPropagationTextIntent(),
    SingleActivator(LogicalKeyboardKey.home, shift: true): DoNothingAndStopPropagationTextIntent(),
+    SingleActivator(LogicalKeyboardKey.end, control: true): DoNothingAndStopPropagationTextIntent(),


Something related but doesn't have to be addressed in the pull request: would it be possible if we create a Map subclass that always returns DoNothingAndStopPropagationTextIntent? Or is it possible we relax what the Shortcuts widget use for Intent lookup, so it takes a function of type ShortcutActivator => Intent (instead of a Map)?

Yeah that might help us avoid a lot of this code. @gspencergoog Do you have any opinion?

We could make a different constructor for Shortcuts that takes a function instead of a Map. I'd want to keep the one that takes a Map, though. One additional benefit of the function over the map would be that it might be faster to construct: long maps can get slow to construct on each build.

That would work, so all of this _webShortcuts map would just be a check for kIsWeb and if so return DoNothingAndStopPropagationTextIntent. I'll keep this in mind next time I'm working on this code.

LongCatIsLooong · 2021-11-22T22:54:41Z

      ? TextSelection.fromPosition(newExtent)
      : textBoundarySelection.extendTo(newExtent);

+    // If collapseAtReversal is true and would have an effect, collapse it.


Yeah this makes sense to me!

LongCatIsLooong · 2021-11-22T22:58:02Z

      ),
    ));

-    // Focus on the EditableText, which is a TextEditingActionTarget.


I thought I removed this file, since TextEditingActionTarget is gone?

So did I but I also noticed that text_editing_action_target.dart was still in the repo unused! I removed it in this PR. Maybe it accidentally came back in a merge or something and we missed it?

I went ahead and deleted this test file too now.

justinmc added 4 commits September 27, 2021 14:56

Windows shift+home/end doesnt stop at reversal, and cleanup stopatrev…

b19dd00

…ersal intents/actions

Control + home/end on Windows

aa700c3

Control + home/end on Windows and test

d11227b

Windows control + shift + home/end and test

f7de5c1

justinmc self-assigned this Sep 27, 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 Sep 27, 2021

google-cla Bot added the cla: yes label Sep 27, 2021

Shift + home/end test

1985ac7

justinmc requested a review from LongCatIsLooong September 29, 2021 22:13

justinmc changed the title ~~(WIP) Windows home/end shortcuts~~ Windows home/end shortcuts Sep 29, 2021

Merge branch 'master' into windows-shortcuts-missing

b96a131

justinmc marked this pull request as ready for review September 29, 2021 22:18

Merge branch 'master' into windows-shortcuts-missing

1b53858

justinmc force-pushed the windows-shortcuts-missing branch from cda763a to 1b53858 Compare November 11, 2021 00:59

We can delete TextEditingActionTarget now that Weiyu's refactor has b…

9ce7e05

…een merged?

justinmc commented Nov 11, 2021

View reviewed changes

LongCatIsLooong reviewed Nov 11, 2021

View reviewed changes

justinmc added 2 commits November 12, 2021 14:42

Analyzer fixes

b002ad6

Fix ctrl + home/end windows directions

7970e0c

LongCatIsLooong approved these changes Nov 22, 2021

View reviewed changes

LongCatIsLooong reviewed Nov 22, 2021

View reviewed changes

justinmc added 2 commits November 22, 2021 16:21

CR: Dont use linuxShortcuts in windowsShortcuts

b6bf825

CR: Remove test file since main file was also removed

4bdc638

justinmc merged commit a1115b8 into flutter:master Nov 23, 2021

justinmc deleted the windows-shortcuts-missing branch November 23, 2021 22:49

Conversation

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

Uh oh!

Uh oh!

justinmc commented Sep 28, 2021

Uh oh!

goderbauer commented Nov 10, 2021

Uh oh!

justinmc commented Nov 10, 2021

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!

justinmc commented Nov 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

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!

4 participants

justinmc commented Sep 27, 2021 •

edited

Loading

LongCatIsLooong Nov 22, 2021 •

edited

Loading

justinmc Dec 4, 2021 •

edited

Loading