Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address previous comments, fix Intent.doNothing. #41417

Merged
merged 4 commits into from Sep 27, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 31 additions & 12 deletions packages/flutter/lib/src/widgets/actions.dart
Expand Up @@ -29,11 +29,10 @@ class Intent extends Diagnosticable {

/// An intent that can't be mapped to an action.
///
/// This Intent is prevented from being mapped to an action in the
/// [ActionDispatcher], and as such can be used to indicate that a shortcut
/// should not do anything, allowing a shortcut defined at a higher level to
/// be disabled at a deeper level in the widget hierarchy.
static const Intent doNothing = Intent(ValueKey<Type>(Intent));
/// This Intent is mapped to an action in the [WidgetsApp] that does nothing,
/// so that it can be bound to a key in a [Shortcuts] widget in order to
/// disable a key binding made above it in the hierarchy.
static const Intent doNothing = DoNothingIntent();

/// The key for the action this intent is associated with.
final LocalKey key;
Expand Down Expand Up @@ -224,6 +223,7 @@ class Actions extends InheritedWidget {
// Stop visiting.
return false;
}

element.visitAncestorElements(visitAncestorElement);
}
return dispatcher ?? const ActionDispatcher();
Expand Down Expand Up @@ -278,9 +278,6 @@ class Actions extends InheritedWidget {
///
/// Setting `nullOk` to true means that if no ambient [Actions] widget is
/// found, then this method will return false instead of throwing.
///
/// If the `intent` argument is [Intent.doNothing], then this function will
/// return false, without looking for a matching action.
static bool invoke(
BuildContext context,
Intent intent, {
Expand All @@ -292,10 +289,6 @@ class Actions extends InheritedWidget {
Element actionsElement;
Action action;

if (identical(intent, Intent.doNothing)) {
return false;
}

bool visitAncestorElement(Element element) {
if (element.widget is! Actions) {
// Continue visiting.
Expand Down Expand Up @@ -358,3 +351,29 @@ class Actions extends InheritedWidget {
properties.add(DiagnosticsProperty<Map<LocalKey, ActionFactory>>('actions', actions));
}
}

/// An [Action], that, as the name implies, does nothing.
///
/// This action is bound to the [Intent.doNothing] intent inside of
/// [WidgetsApp.build] so that a [Shortcuts] widget can bind a key to it to
/// override another shortcut binding defined above it in the hierarchy.
class DoNothingAction extends Action {
/// Const constructor for [DoNothingAction].
const DoNothingAction() : super(key);

/// The Key used for the [DoNothingIntent] intent, and registered at the top
/// level actions in [WidgetsApp.build].
static const LocalKey key = ValueKey<Type>(DoNothingAction);

@override
void invoke(FocusNode node, Intent invocation) { }
}

/// An [Intent] that can be used to disable [Shortcuts] key bindings defined
/// above a widget in the hierarchy.
///
/// The [Intent.doNothing] intent is of this type.
class DoNothingIntent extends Intent {
/// Const constructor for [DoNothingIntent].
const DoNothingIntent() : super(DoNothingAction.key);
}
22 changes: 14 additions & 8 deletions packages/flutter/lib/src/widgets/app.dart
Expand Up @@ -8,6 +8,7 @@ import 'dart:collection' show HashMap;
import 'package:flutter/foundation.dart';
import 'package:flutter/rendering.dart';

import 'actions.dart';
import 'banner.dart';
import 'basic.dart';
import 'binding.dart';
Expand Down Expand Up @@ -1194,14 +1195,19 @@ class _WidgetsAppState extends State<WidgetsApp> implements WidgetsBindingObserv

assert(_debugCheckLocalizations(appLocale));

return DefaultFocusTraversal(
policy: ReadingOrderTraversalPolicy(),
child: MediaQuery(
data: MediaQueryData.fromWindow(WidgetsBinding.instance.window),
child: Localizations(
locale: appLocale,
delegates: _localizationsDelegates.toList(),
child: title,
return Actions(
actions: <LocalKey, ActionFactory>{
DoNothingAction.key: () => const DoNothingAction(),
},
child: DefaultFocusTraversal(
policy: ReadingOrderTraversalPolicy(),
child: MediaQuery(
data: MediaQueryData.fromWindow(WidgetsBinding.instance.window),
child: Localizations(
locale: appLocale,
delegates: _localizationsDelegates.toList(),
child: title,
),
),
),
);
Expand Down
10 changes: 5 additions & 5 deletions packages/flutter/test/widgets/actions_test.dart
Expand Up @@ -302,11 +302,11 @@ void main() {
).debugFillProperties(builder);

final List<String> description = builder.properties
.where((DiagnosticsNode node) {
return !node.isFiltered(DiagnosticLevel.info);
})
.map((DiagnosticsNode node) => node.toString())
.toList();
.where((DiagnosticsNode node) {
return !node.isFiltered(DiagnosticLevel.info);
})
.map((DiagnosticsNode node) => node.toString())
.toList();

expect(description[0], equalsIgnoringHashCodes('dispatcher: ActionDispatcher#00000'));
expect(description[1], equals('actions: {}'));
Expand Down
82 changes: 55 additions & 27 deletions packages/flutter/test/widgets/shortcuts_test.dart
Expand Up @@ -37,22 +37,6 @@ class TestIntent extends Intent {
const TestIntent() : super(TestAction.key);
}

class DoNothingAction extends Action {
const DoNothingAction({
@required OnInvokeCallback onInvoke,
}) : assert(onInvoke != null),
super(key);

static const LocalKey key = ValueKey<Type>(DoNothingAction);

@override
void invoke(FocusNode node, Intent invocation) {}
}

class DoNothingIntent extends Intent {
const DoNothingIntent() : super(DoNothingAction.key);
}

class TestShortcutManager extends ShortcutManager {
TestShortcutManager(this.keys);

Expand Down Expand Up @@ -143,8 +127,8 @@ void main() {
LogicalKeyboardKey.keyD,
LogicalKeyboardKey.keyC,
LogicalKeyboardKey.keyB,
LogicalKeyboardKey.keyA,}
);
LogicalKeyboardKey.keyA,
});
final Map<LogicalKeySet, String> map = <LogicalKeySet, String>{set1: 'one'};
expect(set2 == set3, isTrue);
expect(set2 == set4, isTrue);
Expand Down Expand Up @@ -192,10 +176,12 @@ void main() {
await tester.pumpWidget(
Actions(
actions: <LocalKey, ActionFactory>{
TestAction.key: () => TestAction(onInvoke: (FocusNode node, Intent intent) {
invoked = true;
return true;
}),
TestAction.key: () => TestAction(
onInvoke: (FocusNode node, Intent intent) {
invoked = true;
return true;
},
),
},
child: Shortcuts(
manager: testManager,
Expand Down Expand Up @@ -228,14 +214,16 @@ void main() {
},
child: Actions(
actions: <LocalKey, ActionFactory>{
TestAction.key: () => TestAction(onInvoke: (FocusNode node, Intent intent) {
invoked = true;
return true;
}),
TestAction.key: () => TestAction(
onInvoke: (FocusNode node, Intent intent) {
invoked = true;
return true;
},
),
},
child: Shortcuts(
shortcuts: <LogicalKeySet, Intent>{
LogicalKeySet(LogicalKeyboardKey.keyA): const DoNothingIntent(),
LogicalKeySet(LogicalKeyboardKey.keyA): Intent.doNothing,
},
child: Focus(
autofocus: true,
Expand All @@ -251,5 +239,45 @@ void main() {
expect(invoked, isTrue);
expect(pressedKeys, equals(<LogicalKeyboardKey>[LogicalKeyboardKey.shiftLeft]));
});
testWidgets('$Shortcuts can disable a shortcut with Intent.doNothing', (WidgetTester tester) async {
final GlobalKey containerKey = GlobalKey();
final List<LogicalKeyboardKey> pressedKeys = <LogicalKeyboardKey>[];
final TestShortcutManager testManager = TestShortcutManager(pressedKeys);
bool invoked = false;
await tester.pumpWidget(
MaterialApp(
home: Shortcuts(
manager: testManager,
shortcuts: <LogicalKeySet, Intent>{
LogicalKeySet(LogicalKeyboardKey.shift): const TestIntent(),
},
child: Actions(
actions: <LocalKey, ActionFactory>{
TestAction.key: () => TestAction(
onInvoke: (FocusNode node, Intent intent) {
invoked = true;
return true;
},
),
},
child: Shortcuts(
shortcuts: <LogicalKeySet, Intent>{
LogicalKeySet(LogicalKeyboardKey.shift): Intent.doNothing,
},
child: Focus(
autofocus: true,
child: Container(key: containerKey, width: 100, height: 100),
),
),
),
),
),
);
await tester.pump();
expect(Shortcuts.of(containerKey.currentContext), isNotNull);
await tester.sendKeyDownEvent(LogicalKeyboardKey.shiftLeft);
expect(invoked, isFalse);
expect(pressedKeys, isEmpty);
});
});
}