From 2d22da9f4fee78801fbffaf5ffabbc250ccbd1cd Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Fri, 20 May 2022 16:14:29 -0700 Subject: [PATCH] Remove the shortcut widget's of and maybeOf, and add Shortcuts.manager --- packages/flutter/lib/src/widgets/actions.dart | 4 + .../flutter/lib/src/widgets/shortcuts.dart | 121 ++++----- .../flutter/test/widgets/shortcuts_test.dart | 236 ++++++++++-------- 3 files changed, 195 insertions(+), 166 deletions(-) diff --git a/packages/flutter/lib/src/widgets/actions.dart b/packages/flutter/lib/src/widgets/actions.dart index 0c9c519bff27..e219d9919cc8 100644 --- a/packages/flutter/lib/src/widgets/actions.dart +++ b/packages/flutter/lib/src/widgets/actions.dart @@ -97,6 +97,10 @@ typedef ActionListenerCallback = void Function(Action action); /// developers to change that if they add an ancestor [Actions] widget that maps /// [SelectAllTextIntent] to a different [Action]. /// +/// See the article on [Using Actions and +/// Shortcuts](https://docs.flutter.dev/development/ui/advanced/actions_and_shortcuts) +/// for a detailed explanation. +/// /// See also: /// /// * [Shortcuts], which is a widget that contains a key map, in which it looks diff --git a/packages/flutter/lib/src/widgets/shortcuts.dart b/packages/flutter/lib/src/widgets/shortcuts.dart index 7315d5c25e50..8117e11c5952 100644 --- a/packages/flutter/lib/src/widgets/shortcuts.dart +++ b/packages/flutter/lib/src/widgets/shortcuts.dart @@ -636,13 +636,15 @@ class _ActivatorIntentPair with Diagnosticable { } } -/// A manager of keyboard shortcut bindings. -/// -/// A `ShortcutManager` is obtained by calling [Shortcuts.of] on the context of -/// the widget that you want to find a manager for. +/// A manager of keyboard shortcut bindings used by [Shortcuts] to handle key +/// events. /// /// The manager may be listened to (with [addListener]/[removeListener]) for /// change notifications when the shortcuts change. +/// +/// Typically, a [Shortcuts] widget supplies its own manager, but in uncommon +/// cases where overriding the usual shortcut manager behavior is desired, a +/// subclassed [ShortcutManager] may be supplied. class ShortcutManager with Diagnosticable, ChangeNotifier { /// Constructs a [ShortcutManager]. ShortcutManager({ @@ -773,6 +775,10 @@ class ShortcutManager with Diagnosticable, ChangeNotifier { /// when invoking an [Action] via a keyboard key combination that maps to an /// [Intent]. /// +/// See the article on [Using Actions and +/// Shortcuts](https://docs.flutter.dev/development/ui/advanced/actions_and_shortcuts) +/// for a detailed explanation. +/// /// {@tool dartpad} /// Here, we will use the [Shortcuts] and [Actions] widgets to add and subtract /// from a counter. When the child widget has keyboard focus, and a user presses @@ -810,35 +816,61 @@ class ShortcutManager with Diagnosticable, ChangeNotifier { /// * [Action], a class for defining an invocation of a user action. /// * [CallbackAction], a class for creating an action from a callback. class Shortcuts extends StatefulWidget { - /// Creates a const [Shortcuts] widget. + /// Creates a const [Shortcuts] widget that owns the map of shortcuts and + /// creates its own manager. + /// + /// When using this constructor, [manager] will return null. /// /// The [child] and [shortcuts] arguments are required. + /// + /// See also: + /// + /// * [Shortcuts.manager], a constructor that uses a [ShortcutManager] to + /// manage the shortcuts list instead. const Shortcuts({ super.key, - this.manager, - required this.shortcuts, + required Map shortcuts, + required this.child, + this.debugLabel, + }) : _shortcuts = shortcuts, + manager = null, + assert(shortcuts != null), + assert(child != null); + + /// Creates a const [Shortcuts] widget that uses the [manager] to + /// manage the map of shortcuts. + /// + /// If this constructor is used, [shortcuts] will return the contents of + /// [ShortcutManager.shortcuts]. + /// + /// The [child] and [manager] arguments are required. + const Shortcuts.manager({ + super.key, + required ShortcutManager this.manager, required this.child, this.debugLabel, - }) : assert(shortcuts != null), + }) : _shortcuts = const {}, + assert(manager != null), assert(child != null); /// The [ShortcutManager] that will manage the mapping between key /// combinations and [Action]s. /// - /// If not specified, uses a default-constructed [ShortcutManager]. - /// - /// This manager will be given new [shortcuts] to manage whenever the - /// [shortcuts] change materially. + /// If this widget was created with [Shortcuts.manager], then + /// [ShortcutManager.shortcuts] will be used as the source for shortcuts. If + /// the unnamed constructor is used, this manager will be null, and a + /// default-constructed `ShortcutsManager` will be used. final ShortcutManager? manager; /// {@template flutter.widgets.shortcuts.shortcuts} - /// The map of shortcuts that the [ShortcutManager] will be given to manage. - /// - /// For performance reasons, it is recommended that a pre-built map is passed - /// in here (e.g. a final variable from your widget class) instead of defining - /// it inline in the build function. + /// The map of shortcuts that describes the mapping between a key sequence + /// defined by a [ShortcutActivator] and the [Intent] that will be emitted + /// when that key sequence is pressed. /// {@endtemplate} - final Map shortcuts; + Map get shortcuts { + return manager == null ? _shortcuts : manager!.shortcuts; + } + final Map _shortcuts; /// The child widget for this [Shortcuts] widget. /// @@ -854,52 +886,6 @@ class Shortcuts extends StatefulWidget { /// unnecessarily with large default shortcut maps. final String? debugLabel; - /// Returns the [ShortcutManager] that most tightly encloses the given - /// [BuildContext]. - /// - /// If no [Shortcuts] widget encloses the context given, will assert in debug - /// mode and throw an exception in release mode. - /// - /// See also: - /// - /// * [maybeOf], which is similar to this function, but will return null if - /// it doesn't find a [Shortcuts] ancestor. - static ShortcutManager of(BuildContext context) { - assert(context != null); - final _ShortcutsMarker? inherited = context.dependOnInheritedWidgetOfExactType<_ShortcutsMarker>(); - assert(() { - if (inherited == null) { - throw FlutterError( - 'Unable to find a $Shortcuts widget in the context.\n' - '$Shortcuts.of() was called with a context that does not contain a ' - '$Shortcuts widget.\n' - 'No $Shortcuts ancestor could be found starting from the context that was ' - 'passed to $Shortcuts.of().\n' - 'The context used was:\n' - ' $context', - ); - } - return true; - }()); - return inherited!.manager; - } - - /// Returns the [ShortcutManager] that most tightly encloses the given - /// [BuildContext]. - /// - /// If no [Shortcuts] widget encloses the context given, will return null. - /// - /// See also: - /// - /// * [of], which is similar to this function, but returns a non-nullable - /// result, and will throw an exception if it doesn't find a [Shortcuts] - /// ancestor. - static ShortcutManager? maybeOf(BuildContext context) { - assert(context != null); - final _ShortcutsMarker? inherited = context.dependOnInheritedWidgetOfExactType<_ShortcutsMarker>(); - return inherited?.manager; - } - @override State createState() => _ShortcutsState(); @@ -926,8 +912,8 @@ class _ShortcutsState extends State { super.initState(); if (widget.manager == null) { _internalManager = ShortcutManager(); + _internalManager!.shortcuts = widget.shortcuts; } - manager.shortcuts = widget.shortcuts; } @override @@ -941,7 +927,7 @@ class _ShortcutsState extends State { _internalManager ??= ShortcutManager(); } } - manager.shortcuts = widget.shortcuts; + _internalManager?.shortcuts = widget.shortcuts; } KeyEventResult _handleOnKey(FocusNode node, RawKeyEvent event) { @@ -1331,8 +1317,7 @@ class _ShortcutRegistrarState extends State { @override Widget build(BuildContext context) { - return Shortcuts( - shortcuts: registry.shortcuts, + return Shortcuts.manager( manager: manager, child: _ShortcutRegistrarMarker( registry: registry, diff --git a/packages/flutter/test/widgets/shortcuts_test.dart b/packages/flutter/test/widgets/shortcuts_test.dart index 8e9ba8a82e51..4d63ae48c692 100644 --- a/packages/flutter/test/widgets/shortcuts_test.dart +++ b/packages/flutter/test/widgets/shortcuts_test.dart @@ -532,69 +532,81 @@ void main() { group(Shortcuts, () { testWidgets('Default constructed Shortcuts has empty shortcuts', (WidgetTester tester) async { + const Shortcuts shortcuts = Shortcuts(shortcuts: {}, child: SizedBox()); + await tester.pumpWidget(shortcuts); + expect(shortcuts.shortcuts, isNotNull); + expect(shortcuts.shortcuts, isEmpty); + }); + testWidgets('Default constructed Shortcuts.manager has empty shortcuts', (WidgetTester tester) async { final ShortcutManager manager = ShortcutManager(); expect(manager.shortcuts, isNotNull); expect(manager.shortcuts, isEmpty); - const Shortcuts shortcuts = Shortcuts(shortcuts: {}, child: SizedBox()); + final Shortcuts shortcuts = Shortcuts.manager(manager: manager, child: const SizedBox()); await tester.pumpWidget(shortcuts); expect(shortcuts.shortcuts, isNotNull); expect(shortcuts.shortcuts, isEmpty); }); - testWidgets('Shortcuts.of and maybeOf find shortcuts', (WidgetTester tester) async { + testWidgets('Shortcuts.manager passes on shortcuts', (WidgetTester tester) async { + final Map testShortcuts = { + LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), + }; + final ShortcutManager manager = ShortcutManager(shortcuts: testShortcuts); + expect(manager.shortcuts, isNotNull); + expect(manager.shortcuts, equals(testShortcuts)); + final Shortcuts shortcuts = Shortcuts.manager(manager: manager, child: const SizedBox()); + await tester.pumpWidget(shortcuts); + expect(shortcuts.shortcuts, isNotNull); + expect(shortcuts.shortcuts, equals(testShortcuts)); + }); + testWidgets('ShortcutManager handles shortcuts', (WidgetTester tester) async { final GlobalKey containerKey = GlobalKey(); final List pressedKeys = []; - final TestShortcutManager testManager = TestShortcutManager(pressedKeys); + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + shortcuts: { + LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), + }, + ); + bool invoked = false; await tester.pumpWidget( - Shortcuts( - manager: testManager, - shortcuts: { - LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), + Actions( + actions: >{ + TestIntent: TestAction( + onInvoke: (Intent intent) { + invoked = true; + return true; + }, + ), }, - child: Focus( - autofocus: true, - child: SizedBox(key: containerKey, width: 100, height: 100), + child: Shortcuts.manager( + manager: testManager, + child: Focus( + autofocus: true, + child: SizedBox(key: containerKey, width: 100, height: 100), + ), ), ), ); await tester.pump(); - expect(Shortcuts.maybeOf(containerKey.currentContext!), isNotNull); - expect(Shortcuts.maybeOf(containerKey.currentContext!), equals(testManager)); - expect(Shortcuts.of(containerKey.currentContext!), equals(testManager)); - }); - testWidgets('Shortcuts.of and maybeOf work correctly without shortcuts', (WidgetTester tester) async { - final GlobalKey containerKey = GlobalKey(); - await tester.pumpWidget(Container(key: containerKey)); - expect(Shortcuts.maybeOf(containerKey.currentContext!), isNull); - late FlutterError error; - try { - Shortcuts.of(containerKey.currentContext!); - } on FlutterError catch (e) { - error = e; - } finally { - expect(error, isNotNull); - expect(error.diagnostics.length, 5); - expect(error.diagnostics[2].level, DiagnosticLevel.info); - expect( - error.diagnostics[2].toStringDeep(), - 'No Shortcuts ancestor could be found starting from the context\n' - 'that was passed to Shortcuts.of().\n', - ); - expect(error.toStringDeep(), equalsIgnoringHashCodes( - 'FlutterError\n' - ' Unable to find a Shortcuts widget in the context.\n' - ' Shortcuts.of() was called with a context that does not contain a\n' - ' Shortcuts widget.\n' - ' No Shortcuts ancestor could be found starting from the context\n' - ' that was passed to Shortcuts.of().\n' - ' The context used was:\n' - ' Container-[GlobalKey#00000]\n', - )); - } + await tester.sendKeyDownEvent(LogicalKeyboardKey.shiftLeft); + expect(invoked, isTrue); + expect(pressedKeys, equals([LogicalKeyboardKey.shiftLeft])); }); - testWidgets('ShortcutManager handles shortcuts', (WidgetTester tester) async { + testWidgets('Shortcuts.manager lets manager handle shortcuts', (WidgetTester tester) async { final GlobalKey containerKey = GlobalKey(); final List pressedKeys = []; - final TestShortcutManager testManager = TestShortcutManager(pressedKeys); + bool shortcutsSet = false; + void onShortcutsSet() { + shortcutsSet = true; + } + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + onShortcutsSet: onShortcutsSet, + shortcuts: { + LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), + }, + ); + shortcutsSet = false; bool invoked = false; await tester.pumpWidget( Actions( @@ -606,11 +618,8 @@ void main() { }, ), }, - child: Shortcuts( + child: Shortcuts.manager( manager: testManager, - shortcuts: { - LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), - }, child: Focus( autofocus: true, child: SizedBox(key: containerKey, width: 100, height: 100), @@ -619,15 +628,20 @@ void main() { ), ); await tester.pump(); - expect(Shortcuts.of(containerKey.currentContext!), isNotNull); await tester.sendKeyDownEvent(LogicalKeyboardKey.shiftLeft); expect(invoked, isTrue); + expect(shortcutsSet, isFalse); expect(pressedKeys, equals([LogicalKeyboardKey.shiftLeft])); }); - testWidgets('ShortcutManager ignores keypresses with no primary focus', (WidgetTester tester) async { + testWidgets('ShortcutManager ignores key presses with no primary focus', (WidgetTester tester) async { final GlobalKey containerKey = GlobalKey(); final List pressedKeys = []; - final TestShortcutManager testManager = TestShortcutManager(pressedKeys); + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + shortcuts: { + LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), + }, + ); bool invoked = false; await tester.pumpWidget( Actions( @@ -639,18 +653,14 @@ void main() { }, ), }, - child: Shortcuts( + child: Shortcuts.manager( manager: testManager, - shortcuts: { - LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), - }, child: SizedBox(key: containerKey, width: 100, height: 100), ), ), ); await tester.pump(); expect(primaryFocus, isNull); - expect(Shortcuts.of(containerKey.currentContext!), isNotNull); await tester.sendKeyDownEvent(LogicalKeyboardKey.shiftLeft); expect(invoked, isFalse); expect(pressedKeys, isEmpty); @@ -658,14 +668,16 @@ void main() { testWidgets("Shortcuts passes to the next Shortcuts widget if it doesn't map the key", (WidgetTester tester) async { final GlobalKey containerKey = GlobalKey(); final List pressedKeys = []; - final TestShortcutManager testManager = TestShortcutManager(pressedKeys); + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + shortcuts: { + LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), + }, + ); bool invoked = false; await tester.pumpWidget( - Shortcuts( + Shortcuts.manager( manager: testManager, - shortcuts: { - LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), - }, child: Actions( actions: >{ TestIntent: TestAction( @@ -688,7 +700,6 @@ void main() { ), ); await tester.pump(); - expect(Shortcuts.of(containerKey.currentContext!), isNotNull); await tester.sendKeyDownEvent(LogicalKeyboardKey.shiftLeft); expect(invoked, isTrue); expect(pressedKeys, equals([LogicalKeyboardKey.shiftLeft])); @@ -696,15 +707,17 @@ void main() { testWidgets('Shortcuts can disable a shortcut with Intent.doNothing', (WidgetTester tester) async { final GlobalKey containerKey = GlobalKey(); final List pressedKeys = []; - final TestShortcutManager testManager = TestShortcutManager(pressedKeys); + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + shortcuts: { + LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), + }, + ); bool invoked = false; await tester.pumpWidget( MaterialApp( - home: Shortcuts( + home: Shortcuts.manager( manager: testManager, - shortcuts: { - LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(), - }, child: Actions( actions: >{ TestIntent: TestAction( @@ -728,7 +741,6 @@ void main() { ), ); await tester.pump(); - expect(Shortcuts.of(containerKey.currentContext!), isNotNull); await tester.sendKeyDownEvent(LogicalKeyboardKey.shiftLeft); expect(invoked, isFalse); expect(pressedKeys, isEmpty); @@ -736,22 +748,23 @@ void main() { testWidgets("Shortcuts that aren't bound to an action don't absorb keys meant for text fields", (WidgetTester tester) async { final GlobalKey textFieldKey = GlobalKey(); final List pressedKeys = []; - final TestShortcutManager testManager = TestShortcutManager(pressedKeys); + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + shortcuts: { + LogicalKeySet(LogicalKeyboardKey.keyA): const TestIntent(), + }, + ); await tester.pumpWidget( MaterialApp( home: Material( - child: Shortcuts( + child: Shortcuts.manager( manager: testManager, - shortcuts: { - LogicalKeySet(LogicalKeyboardKey.keyA): const TestIntent(), - }, child: TextField(key: textFieldKey, autofocus: true), ), ), ), ); await tester.pump(); - expect(Shortcuts.of(textFieldKey.currentContext!), isNotNull); final bool handled = await tester.sendKeyEvent(LogicalKeyboardKey.keyA); expect(handled, isFalse); expect(pressedKeys, equals([LogicalKeyboardKey.keyA])); @@ -759,16 +772,18 @@ void main() { testWidgets('Shortcuts that are bound to an action do override text fields', (WidgetTester tester) async { final GlobalKey textFieldKey = GlobalKey(); final List pressedKeys = []; - final TestShortcutManager testManager = TestShortcutManager(pressedKeys); + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + shortcuts: { + LogicalKeySet(LogicalKeyboardKey.keyA): const TestIntent(), + }, + ); bool invoked = false; await tester.pumpWidget( MaterialApp( home: Material( - child: Shortcuts( + child: Shortcuts.manager( manager: testManager, - shortcuts: { - LogicalKeySet(LogicalKeyboardKey.keyA): const TestIntent(), - }, child: Actions( actions: >{ TestIntent: TestAction( @@ -785,7 +800,6 @@ void main() { ), ); await tester.pump(); - expect(Shortcuts.of(textFieldKey.currentContext!), isNotNull); final bool result = await tester.sendKeyEvent(LogicalKeyboardKey.keyA); expect(result, isTrue); expect(pressedKeys, equals([LogicalKeyboardKey.keyA])); @@ -794,16 +808,18 @@ void main() { testWidgets('Shortcuts can override intents that apply to text fields', (WidgetTester tester) async { final GlobalKey textFieldKey = GlobalKey(); final List pressedKeys = []; - final TestShortcutManager testManager = TestShortcutManager(pressedKeys); + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + shortcuts: { + LogicalKeySet(LogicalKeyboardKey.keyA): const TestIntent(), + }, + ); bool invoked = false; await tester.pumpWidget( MaterialApp( home: Material( - child: Shortcuts( + child: Shortcuts.manager( manager: testManager, - shortcuts: { - LogicalKeySet(LogicalKeyboardKey.keyA): const TestIntent(), - }, child: Actions( actions: >{ TestIntent: TestAction( @@ -825,7 +841,6 @@ void main() { ), ); await tester.pump(); - expect(Shortcuts.of(textFieldKey.currentContext!), isNotNull); final bool result = await tester.sendKeyEvent(LogicalKeyboardKey.keyA); expect(result, isFalse); expect(invoked, isFalse); @@ -833,16 +848,18 @@ void main() { testWidgets('Shortcuts can override intents that apply to text fields with DoNothingAndStopPropagationIntent', (WidgetTester tester) async { final GlobalKey textFieldKey = GlobalKey(); final List pressedKeys = []; - final TestShortcutManager testManager = TestShortcutManager(pressedKeys); + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + shortcuts: { + LogicalKeySet(LogicalKeyboardKey.keyA): const TestIntent(), + }, + ); bool invoked = false; await tester.pumpWidget( MaterialApp( home: Material( - child: Shortcuts( + child: Shortcuts.manager( manager: testManager, - shortcuts: { - LogicalKeySet(LogicalKeyboardKey.keyA): const TestIntent(), - }, child: Actions( actions: >{ TestIntent: TestAction( @@ -864,7 +881,6 @@ void main() { ), ); await tester.pump(); - expect(Shortcuts.of(textFieldKey.currentContext!), isNotNull); final bool result = await tester.sendKeyEvent(LogicalKeyboardKey.keyA); expect(result, isFalse); expect(invoked, isFalse); @@ -919,11 +935,10 @@ void main() { expect(description.length, equals(1)); expect(description[0], equals('shortcuts: ')); }); - test('Shortcuts diagnostics work when manager specified.', () { + test('Shortcuts diagnostics work when manager not specified.', () { final DiagnosticPropertiesBuilder builder = DiagnosticPropertiesBuilder(); Shortcuts( - manager: ShortcutManager(), shortcuts: { LogicalKeySet( LogicalKeyboardKey.keyA, @@ -937,11 +952,35 @@ void main() { return !node.isFiltered(DiagnosticLevel.info); }).map((DiagnosticsNode node) => node.toString()).toList(); + expect(description.length, equals(1)); + expect(description[0], equalsIgnoringHashCodes('shortcuts: {{Key A + Key B}: ActivateIntent#00000}')); + }); + test('Shortcuts diagnostics work when manager specified.', () { + final DiagnosticPropertiesBuilder builder = DiagnosticPropertiesBuilder(); + final List pressedKeys = []; + final TestShortcutManager testManager = TestShortcutManager( + pressedKeys, + shortcuts: { + LogicalKeySet( + LogicalKeyboardKey.keyA, + LogicalKeyboardKey.keyB, + ): const ActivateIntent(), + }, + ); + + Shortcuts.manager( + manager: testManager, + child: const SizedBox(), + ).debugFillProperties(builder); + + final List description = builder.properties.where((DiagnosticsNode node) { + return !node.isFiltered(DiagnosticLevel.info); + }).map((DiagnosticsNode node) => node.toString()).toList(); + expect(description.length, equals(2)); - expect(description[0], equalsIgnoringHashCodes('manager: ShortcutManager#00000(shortcuts: {})')); + expect(description[0], equalsIgnoringHashCodes('manager: TestShortcutManager#00000(shortcuts: {LogicalKeySet#00000(keys: Key A + Key B): ActivateIntent#00000})')); expect(description[1], equalsIgnoringHashCodes('shortcuts: {{Key A + Key B}: ActivateIntent#00000}')); }); - testWidgets('Shortcuts support multiple intents', (WidgetTester tester) async { tester.binding.focusManager.highlightStrategy = FocusHighlightStrategy.alwaysTraditional; bool? value = true; @@ -1787,9 +1826,10 @@ class TestIntent2 extends Intent { } class TestShortcutManager extends ShortcutManager { - TestShortcutManager(this.keys); + TestShortcutManager(this.keys, { super.shortcuts, this.onShortcutsSet }); List keys; + VoidCallback? onShortcutsSet; @override KeyEventResult handleKeypress(BuildContext context, RawKeyEvent event) {