Unverified Commit 34f39a20 authored by Ian Hickson's avatar Ian Hickson Committed by GitHub

ContextAction.isEnabled needs a context (#127721)

...and lots of things that fall out from that
parent 4effd9c4
......@@ -233,8 +233,19 @@ abstract class Action<T extends Intent> with Diagnosticable {
///
/// This will be called by the [ActionDispatcher] before attempting to invoke
/// the action.
///
/// If the action's enable state depends on a [BuildContext], subclass
/// [ContextAction] instead of [Action].
bool isEnabled(T intent) => isActionEnabled;
bool _isEnabled(T intent, BuildContext? context) {
final Action<T> self = this;
if (self is ContextAction<T>) {
return self.isEnabled(intent, context);
}
return self.isEnabled(intent);
}
/// Whether this [Action] is inherently enabled.
///
/// If [isActionEnabled] is false, then this [Action] is disabled for any
......@@ -313,9 +324,20 @@ abstract class Action<T extends Intent> with Diagnosticable {
/// To receive the result of invoking an action, it must be invoked using
/// [Actions.invoke], or by invoking it using an [ActionDispatcher]. An action
/// invoked via a [Shortcuts] widget will have its return value ignored.
///
/// If the action's behavior depends on a [BuildContext], subclass
/// [ContextAction] instead of [Action].
@protected
Object? invoke(T intent);
Object? _invoke(T intent, BuildContext? context) {
final Action<T> self = this;
if (self is ContextAction<T>) {
return self.invoke(intent, context);
}
return self.invoke(intent);
}
/// Register a callback to listen for changes to the state of this action.
///
/// If you call this, you must call [removeActionListener] a matching number
......@@ -487,11 +509,22 @@ class _ActionListenerState extends State<ActionListener> {
}
/// An abstract [Action] subclass that adds an optional [BuildContext] to the
/// [invoke] method to be able to provide context to actions.
/// [isEnabled] and [invoke] methods to be able to provide context to actions.
///
/// [ActionDispatcher.invokeAction] checks to see if the action it is invoking
/// is a [ContextAction], and if it is, supplies it with a context.
abstract class ContextAction<T extends Intent> extends Action<T> {
/// Returns true if the action is enabled and is ready to be invoked.
///
/// This will be called by the [ActionDispatcher] before attempting to invoke
/// the action.
///
/// The optional `context` parameter is the context of the invocation of the
/// action, and in the case of an action invoked by a [ShortcutManager], via
/// a [Shortcuts] widget, will be the context of the [Shortcuts] widget.
@override
bool isEnabled(T intent, [BuildContext? context]) => super.isEnabled(intent);
/// Called when the action is to be performed.
///
/// This is called by the [ActionDispatcher] when an action is invoked via
......@@ -598,20 +631,47 @@ class ActionDispatcher with Diagnosticable {
/// Returns the object returned from [Action.invoke].
///
/// The caller must receive a `true` result from [Action.isEnabled] before
/// calling this function. This function will assert if the action is not
/// enabled when called.
/// calling this function (or [ContextAction.isEnabled] with the same
/// `context`, if the `action` is a [ContextAction]). This function will
/// assert if the action is not enabled when called.
///
/// Consider using [invokeActionIfEnabled] to invoke the action conditionally
/// based on whether it is enabled or not, without having to check first.
Object? invokeAction(
covariant Action<Intent> action,
covariant Intent intent, [
BuildContext? context,
]) {
assert(action.isEnabled(intent), 'Action must be enabled when calling invokeAction');
if (action is ContextAction) {
context ??= primaryFocus?.context;
return action.invoke(intent, context);
} else {
return action.invoke(intent);
final BuildContext? target = context ?? primaryFocus?.context;
assert(action._isEnabled(intent, target), 'Action must be enabled when calling invokeAction');
return action._invoke(intent, target);
}
/// Invokes the given `action`, passing it the given `intent`, but only if the
/// action is enabled.
///
/// The action will be invoked with the given `context`, if given, but only if
/// the action is a [ContextAction] subclass. If no `context` is given, and
/// the action is a [ContextAction], then the context from the [primaryFocus]
/// is used.
///
/// The return value has two components. The first is a boolean indicating if
/// the action was enabled (as per [Action.isEnabled]). If this is false, the
/// second return value is null. Otherwise, the second return value is the
/// object returned from [Action.invoke].
///
/// Consider using [invokeAction] if the enabled state of the action is not in
/// question; this avoids calling [Action.isEnabled] redundantly.
(bool, Object?) invokeActionIfEnabled(
covariant Action<Intent> action,
covariant Intent intent, [
BuildContext? context,
]) {
final BuildContext? target = context ?? primaryFocus?.context;
if (action._isEnabled(intent, target)) {
return (true, action._invoke(intent, target));
}
return (false, null);
}
}
......@@ -734,11 +794,11 @@ class Actions extends StatefulWidget {
/// [Actions.invoke] instead.
static VoidCallback? handler<T extends Intent>(BuildContext context, T intent) {
final Action<T>? action = Actions.maybeFind<T>(context);
if (action != null && action.isEnabled(intent)) {
if (action != null && action._isEnabled(intent, context)) {
return () {
// Could be that the action was enabled when the closure was created,
// but is now no longer enabled, so check again.
if (action.isEnabled(intent)) {
if (action._isEnabled(intent, context)) {
Actions.of(context).invokeAction(action, intent, context);
}
};
......@@ -907,7 +967,7 @@ class Actions extends StatefulWidget {
final bool actionFound = _visitActionsAncestors(context, (InheritedElement element) {
final _ActionsScope actions = element.widget as _ActionsScope;
final Action<T>? result = _castAction(actions, intent: intent);
if (result != null && result.isEnabled(intent)) {
if (result != null && result._isEnabled(intent, context)) {
// Invoke the action we found using the relevant dispatcher from the Actions
// Element we found.
returnValue = _findDispatcher(element).invokeAction(result, intent, context);
......@@ -954,11 +1014,10 @@ class Actions extends StatefulWidget {
T intent,
) {
Object? returnValue;
_visitActionsAncestors(context, (InheritedElement element) {
final _ActionsScope actions = element.widget as _ActionsScope;
final Action<T>? result = _castAction(actions, intent: intent);
if (result != null && result.isEnabled(intent)) {
if (result != null && result._isEnabled(intent, context)) {
// Invoke the action we found using the relevant dispatcher from the Actions
// element we found.
returnValue = _findDispatcher(element).invokeAction(result, intent, context);
......@@ -1540,12 +1599,17 @@ class PrioritizedIntents extends Intent {
/// An [Action] that iterates through a list of [Intent]s, invoking the first
/// that is enabled.
class PrioritizedAction extends Action<PrioritizedIntents> {
///
/// The [isEnabled] method must be called before [invoke]. Calling [isEnabled]
/// configures the object by seeking the first intent with an enabled action.
/// If the actions have an opportunity to change enabled state, [isEnabled]
/// must be called again before calling [invoke].
class PrioritizedAction extends ContextAction<PrioritizedIntents> {
late Action<dynamic> _selectedAction;
late Intent _selectedIntent;
@override
bool isEnabled(PrioritizedIntents intent) {
bool isEnabled(PrioritizedIntents intent, [ BuildContext? context ]) {
final FocusNode? focus = primaryFocus;
if (focus == null || focus.context == null) {
return false;
......@@ -1555,7 +1619,7 @@ class PrioritizedAction extends Action<PrioritizedIntents> {
focus.context!,
intent: candidateIntent,
);
if (candidateAction != null && candidateAction.isEnabled(candidateIntent)) {
if (candidateAction != null && candidateAction._isEnabled(candidateIntent, context)) {
_selectedAction = candidateAction;
_selectedIntent = candidateIntent;
return true;
......@@ -1565,8 +1629,8 @@ class PrioritizedAction extends Action<PrioritizedIntents> {
}
@override
void invoke(PrioritizedIntents intent) {
_selectedAction.invoke(_selectedIntent);
void invoke(PrioritizedIntents intent, [ BuildContext? context ]) {
_selectedAction._invoke(_selectedIntent, context);
}
}
......@@ -1610,9 +1674,7 @@ mixin _OverridableActionMixin<T extends Intent> on Action<T> {
return true;
}());
overrideAction._updateCallingAction(defaultAction);
final Object? returnValue = overrideAction is ContextAction<T>
? overrideAction.invoke(intent, context)
: overrideAction.invoke(intent);
final Object? returnValue = overrideAction._invoke(intent, context);
overrideAction._updateCallingAction(null);
assert(() {
debugAssertMutuallyRecursive = false;
......@@ -1656,7 +1718,7 @@ mixin _OverridableActionMixin<T extends Intent> on Action<T> {
}
@override
bool isEnabled(T intent) {
bool isEnabled(T intent, [BuildContext? context]) {
assert(!debugAssertIsEnabledMutuallyRecursive);
assert(() {
debugAssertIsEnabledMutuallyRecursive = true;
......@@ -1665,7 +1727,7 @@ mixin _OverridableActionMixin<T extends Intent> on Action<T> {
final Action<T>? overrideAction = getOverrideAction();
overrideAction?._updateCallingAction(defaultAction);
final bool returnValue = (overrideAction ?? defaultAction).isEnabled(intent);
final bool returnValue = (overrideAction ?? defaultAction)._isEnabled(intent, context);
overrideAction?._updateCallingAction(null);
assert(() {
debugAssertIsEnabledMutuallyRecursive = false;
......@@ -1747,9 +1809,7 @@ class _OverridableContextAction<T extends Intent> extends ContextAction<T> with
// calling BuildContext.
final Action<T> wrappedDefault = _ContextActionToActionAdapter<T>(invokeContext: context!, action: defaultAction);
overrideAction._updateCallingAction(wrappedDefault);
final Object? returnValue = overrideAction is ContextAction<T>
? overrideAction.invoke(intent, context)
: overrideAction.invoke(intent);
final Object? returnValue = overrideAction._invoke(intent, context);
overrideAction._updateCallingAction(null);
assert(() {
......@@ -1790,7 +1850,7 @@ class _ContextActionToActionAdapter<T extends Intent> extends Action<T> {
Action<T>? get callingAction => action.callingAction;
@override
bool isEnabled(T intent) => action.isEnabled(intent);
bool isEnabled(T intent) => action.isEnabled(intent, invokeContext);
@override
bool get isActionEnabled => action.isActionEnabled;
......
......@@ -320,6 +320,10 @@ class Scrollable extends StatefulWidget {
/// the nearest enclosing [ScrollableState] in that [Axis] is returned, or
/// null if there is none.
///
/// This finds the nearest _ancestor_ [Scrollable] of the `context`. This
/// means that if the `context` is that of a [Scrollable], it will _not_ find
/// _that_ [Scrollable].
///
/// See also:
///
/// * [Scrollable.of], which is similar to this method, but asserts
......@@ -359,6 +363,10 @@ class Scrollable extends StatefulWidget {
/// target [Scrollable] is not the closest instance. When [axis] is provided,
/// the nearest enclosing [ScrollableState] in that [Axis] is returned.
///
/// This finds the nearest _ancestor_ [Scrollable] of the `context`. This
/// means that if the `context` is that of a [Scrollable], it will _not_ find
/// _that_ [Scrollable].
///
/// If no [Scrollable] ancestor is found, then this method will assert in
/// debug mode, and throw an exception in release mode.
///
......@@ -943,7 +951,6 @@ class ScrollableState extends State<Scrollable> with TickerProviderStateMixin, R
Widget result = _ScrollableScope(
scrollable: this,
position: position,
// TODO(ianh): Having all these global keys is sad.
child: Listener(
onPointerSignal: _receivedPointerSignal,
child: RawGestureDetector(
......
......@@ -10,7 +10,6 @@ import 'package:flutter/rendering.dart';
import 'actions.dart';
import 'basic.dart';
import 'focus_manager.dart';
import 'framework.dart';
import 'primary_scroll_controller.dart';
import 'scroll_configuration.dart';
......@@ -377,10 +376,10 @@ class ScrollIntent extends Intent {
final ScrollIncrementType type;
}
/// An [Action] that scrolls the [Scrollable] that encloses the current
/// [primaryFocus] by the amount configured in the [ScrollIntent] given to it.
/// An [Action] that scrolls the relevant [Scrollable] by the amount configured
/// in the [ScrollIntent] given to it.
///
/// If a Scrollable cannot be found above the current [primaryFocus], the
/// If a Scrollable cannot be found above the given [BuildContext], the
/// [PrimaryScrollController] will be considered for default handling of
/// [ScrollAction]s.
///
......@@ -388,21 +387,17 @@ class ScrollIntent extends Intent {
/// for a [ScrollIntent.type] set to [ScrollIncrementType.page] is 80% of the
/// size of the scroll window, and for [ScrollIncrementType.line], 50 logical
/// pixels.
class ScrollAction extends Action<ScrollIntent> {
class ScrollAction extends ContextAction<ScrollIntent> {
@override
bool isEnabled(ScrollIntent intent) {
final FocusNode? focus = primaryFocus;
final bool contextIsValid = focus != null && focus.context != null;
if (contextIsValid) {
// Check for primary scrollable within the current context
if (Scrollable.maybeOf(focus.context!) != null) {
return true;
}
// Check for fallback scrollable with context from PrimaryScrollController
final ScrollController? primaryScrollController = PrimaryScrollController.maybeOf(focus.context!);
return primaryScrollController != null && primaryScrollController.hasClients;
bool isEnabled(ScrollIntent intent, [BuildContext? context]) {
if (context == null) {
return false;
}
if (Scrollable.maybeOf(context) != null) {
return true;
}
return false;
final ScrollController? primaryScrollController = PrimaryScrollController.maybeOf(context);
return (primaryScrollController != null) && (primaryScrollController.hasClients);
}
/// Returns the scroll increment for a single scroll request, for use when
......@@ -480,10 +475,11 @@ class ScrollAction extends Action<ScrollIntent> {
}
@override
void invoke(ScrollIntent intent) {
ScrollableState? state = Scrollable.maybeOf(primaryFocus!.context!);
void invoke(ScrollIntent intent, [BuildContext? context]) {
assert(context != null, 'Cannot scroll without a context.');
ScrollableState? state = Scrollable.maybeOf(context!);
if (state == null) {
final ScrollController primaryScrollController = PrimaryScrollController.of(primaryFocus!.context!);
final ScrollController primaryScrollController = PrimaryScrollController.of(context);
assert (() {
if (primaryScrollController.positions.length != 1) {
throw FlutterError.fromParts(<DiagnosticsNode>[
......
......@@ -843,9 +843,13 @@ class ShortcutManager with Diagnosticable, ChangeNotifier {
primaryContext,
intent: matchedIntent,
);
if (action != null && action.isEnabled(matchedIntent)) {
final Object? invokeResult = Actions.of(primaryContext).invokeAction(action, matchedIntent, primaryContext);
return action.toKeyEventResult(matchedIntent, invokeResult);
if (action != null) {
final (bool enabled, Object? invokeResult) = Actions.of(primaryContext).invokeActionIfEnabled(
action, matchedIntent, primaryContext,
);
if (enabled) {
return action.toKeyEventResult(matchedIntent, invokeResult);
}
}
}
}
......
......@@ -549,4 +549,33 @@ void main() {
equals(const Rect.fromLTRB(0.0, 100.0, 800.0, 200.0)),
);
}, variant: KeySimulatorTransitModeVariant.all());
testWidgets('Can scroll using intents only', (WidgetTester tester) async {
await tester.pumpWidget(
MaterialApp(
home: ListView(
children: const <Widget>[
SizedBox(height: 600.0, child: Text('The cow as white as milk')),
SizedBox(height: 600.0, child: Text('The cape as red as blood')),
SizedBox(height: 600.0, child: Text('The hair as yellow as corn')),
],
),
),
);
expect(find.text('The cow as white as milk'), findsOneWidget);
expect(find.text('The cape as red as blood'), findsNothing);
expect(find.text('The hair as yellow as corn'), findsNothing);
Actions.invoke(tester.element(find.byType(SliverList)), const ScrollIntent(direction: AxisDirection.down, type: ScrollIncrementType.page));
await tester.pump(); // start scroll
await tester.pump(const Duration(milliseconds: 1000)); // end scroll
expect(find.text('The cow as white as milk'), findsOneWidget);
expect(find.text('The cape as red as blood'), findsOneWidget);
expect(find.text('The hair as yellow as corn'), findsNothing);
Actions.invoke(tester.element(find.byType(SliverList)), const ScrollIntent(direction: AxisDirection.down, type: ScrollIncrementType.page));
await tester.pump(); // start scroll
await tester.pump(const Duration(milliseconds: 1000)); // end scroll
expect(find.text('The cow as white as milk'), findsNothing);
expect(find.text('The cape as red as blood'), findsOneWidget);
expect(find.text('The hair as yellow as corn'), findsOneWidget);
});
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment