Keyboard shortcuts are set on the top-level scaffold #3458

elliette · 2021-10-18T23:19:46Z

Work towards #3457

Moves keyboard shortcuts to the top-level scaffold so that they can be triggered for a particular screen no matter where the focus is.

kenzieschmoll · 2021-10-19T00:06:53Z

packages/devtools_app/lib/src/debugger/codeview.dart

+    return MouseRegion(
+      onEnter: (_) {
+        if (!focusNode.hasFocus) {
+          focusNode.requestFocus();


2 scenarios I'm curious about:

what if a user is performing an eval and then clicks an auto complete suggestion that is overlapping with the code view area? Will the code view then take focus away from the eval text field?

What if a user is searching (cmd + f), and then moves the mouse over the code view (for example maybe to click the forward / backward arrows in that search field)? Will the code view then take focus away from the search field?

We should find a different way to fix this. Mouse hover should not change focus.

Ok, I'm now tackling this in a completely different way.

A Screen widget exposes a public method that can be called to return all the keyboard shortcuts that should be registered for that screen

When the Screens are built in DevToolsScaffold, the keyboard shortcuts are wrapped around the top-level Scaffold widget

This fixes the issue seen in #3457 (comment). I still need to reproduce landing on the empty debugger page to confirm that the file opener can now be triggered from there.

This reverts commit ec93e70.

Moves shortcuts to the top level

kenzieschmoll · 2021-10-20T21:42:59Z

packages/devtools_app/lib/src/screen.dart

+  final Map<ShortcutActivator, Intent> shortcuts;
+  final Map<Type, Action<Intent>> actions;
+
+  static ShortcutsConfiguration empty =


make this a factory constructor

I agree this should be a const constructor taking a child widget once this is refactored to be a widget.
If this wasn't a widget, probably makes the most sense as a const static field.

Let me know what you think of it now - it's a factory constructor, and it's passed to the widget

kenzieschmoll · 2021-10-20T21:43:19Z

packages/devtools_app/lib/src/screen.dart

@@ -67,6 +67,10 @@ abstract class Screen {
  /// Whether to show the console for this screen.
  bool showConsole(bool embed) => false;

+  // Which keyboard shortcuts shoud be enabled for this screen.


nit: use /// instead of // for dartdoc

Good to know! Switched to ///

kenzieschmoll · 2021-10-20T21:44:15Z

packages/devtools_app/lib/src/screen.dart

+  ShortcutsConfiguration({
+    @required this.shortcuts,
+    @required this.actions,
+  });


assert that shortcuts.length == actions.length

kenzieschmoll · 2021-10-20T21:44:49Z

packages/devtools_app/lib/src/scaffold.dart

-                bottomNavigationBar: widget.embed ? null : _buildStatusLine(),
-              ),
+              child: _maybeWrapInShortcuts(
+                  scaffold, _currentScreen.keyboardShortcuts(context)),


nit: trailing comma

Thanks! This line is now gone

kenzieschmoll · 2021-10-20T21:46:23Z

packages/devtools_app/lib/src/scaffold.dart

@@ -470,6 +473,22 @@ class DevToolsScaffoldState extends State<DevToolsScaffold>
    );
  }

+  Widget _maybeWrapInShortcuts(


instead of making this a method, create a widget class KeyboardShortcuts that takes required named params. And you can change widget to child to match flutter naming conventions. The KeyboardShortcuts widget can handle the logic here in its build method

Makes sense, switched it to a widget

kenzieschmoll · 2021-10-20T21:49:29Z

packages/devtools_app/lib/src/scaffold.dart

@@ -326,6 +326,29 @@ class DevToolsScaffoldState extends State<DevToolsScaffold>
      ],
    );
    final theme = Theme.of(context);
+    final scaffold = Scaffold(


instead of breaking this out into its own var, put back inline in the return statement and see my comment below regarding KeyboardShortcuts. Nesting widgets directly is preferred for build methods when possible. There are some cases where we need to break widgets out into local variables, but in this case, the var scaffold is only being used once, so we can leave directly in the widget tree.

jacob314 · 2021-10-20T22:36:54Z

packages/devtools_app/lib/src/debugger/debugger_screen.dart

@@ -44,6 +44,24 @@ class DebuggerScreen extends Screen {
  @override
  bool showConsole(bool embed) => true;

+  @override
+  ShortcutsConfiguration keyboardShortcuts(BuildContext context) {


nit: maybe call this buildKeyboardShortcuts?

jacob314 · 2021-10-20T22:38:50Z

packages/devtools_app/lib/src/screen.dart

+  static ShortcutsConfiguration empty =
+      ShortcutsConfiguration(shortcuts: {}, actions: {});
+
+  bool isEmpty() {


isEmpty should be a getter not a method in Dart.

Done, switched to getter

jacob314 · 2021-10-20T22:42:57Z

packages/devtools_app/lib/src/screen.dart

@@ -304,3 +308,20 @@ class BadgePainter extends CustomPainter {
    return true;
  }
 }
+
+class ShortcutsConfiguration {
+  ShortcutsConfiguration({


this could be a const constructor.

Done, thanks!

Tries to claim focus when mouse enters codeview

ec93e70

kenzieschmoll reviewed Oct 19, 2021

View reviewed changes

elliette added 3 commits October 20, 2021 14:13

Move shortcuts to top level

2e530a0

Revert "Tries to claim focus when mouse enters codeview"

80711e1

This reverts commit ec93e70.

Merge branch 'focus' into file-opener-bug

0f1641d

Moves shortcuts to the top level

elliette changed the title ~~Codeview tries to claim focus when the mouse enters its area~~ Keyboard shortcuts are set on the top-level scaffold Oct 20, 2021

kenzieschmoll approved these changes Oct 20, 2021

View reviewed changes

jacob314 reviewed Oct 20, 2021

View reviewed changes

Responding to PR comments

86bc140

kenzieschmoll approved these changes Oct 21, 2021

View reviewed changes

Merge branch 'master' into file-opener-bug

1033277

elliette merged commit 3f21103 into flutter:master Oct 25, 2021

elliette deleted the file-opener-bug branch December 24, 2021 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyboard shortcuts are set on the top-level scaffold #3458

Keyboard shortcuts are set on the top-level scaffold #3458

elliette commented Oct 18, 2021 •

edited

Loading

kenzieschmoll Oct 19, 2021

jacob314 Oct 19, 2021

elliette Oct 20, 2021

kenzieschmoll Oct 20, 2021

jacob314 Oct 20, 2021

elliette Oct 21, 2021

kenzieschmoll Oct 20, 2021

elliette Oct 21, 2021

kenzieschmoll Oct 20, 2021

elliette Oct 21, 2021

kenzieschmoll Oct 20, 2021

elliette Oct 21, 2021

kenzieschmoll Oct 20, 2021

elliette Oct 21, 2021

kenzieschmoll Oct 20, 2021

elliette Oct 21, 2021

jacob314 Oct 20, 2021

elliette Oct 21, 2021

jacob314 Oct 20, 2021

elliette Oct 21, 2021

jacob314 Oct 20, 2021

elliette Oct 21, 2021

Keyboard shortcuts are set on the top-level scaffold #3458

Keyboard shortcuts are set on the top-level scaffold #3458

Conversation

elliette commented Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elliette commented Oct 18, 2021 •

edited

Loading