firka/flutter
3
0
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[CupertinoActionSheet] Make _ActionSheetButtonBackground stateless (#152283)

Browse Source 
This PR is a refactor that makes `_ActionSheetButtonBackground` widgets no longer record their own `pressed` state, but instead receive this state from their parent. In this way, `_ActionSheetButtonBackground` becomes a stateless widget. The children states are duplicate because the parent has to keep track of the state for rendering dividers.

An obstacle with this change is that `_ActionSheetButtonBackground` needs an object that is persistent across rebuilds to provide to `Metadata.data`. Either it is kept as a stateful widget without any actual states, or it is made stateless and its `Element` as the object. After discussion, the first option is used.

`_ActionSheetSlideTarget` is renamed to `_SlideTarget` since the alert dialog will soon use this class as well.

This refactor shouldn't need additional tests. Still, one test is added for a behavior that I broke during development and found not covered by the unit tests then.
This commit is contained in:
Tong Mu
2024-07-31 16:39:25 -07:00
committed by GitHub
parent 788a0e3d2f
commit 3303973d99
2 changed files with 98 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files

171
packages/flutter/lib/src/cupertino/dialog.dart
View File

@@ -663,14 +663,14 @@ class _SlidingTapGestureRecognizer extends VerticalDragGestureRecognizer {
// enters, leaves, or ends this `MetaData` widget, corresponding methods of this
// class will be called.
//
// Multiple `_ActionSheetSlideTarget`s might be nested.
// Multiple `_SlideTarget`s might be nested.
// `_TargetSelectionGestureRecognizer` uses a simple algorithm that only
// compares if the inner-most slide target has changed (which suffices our use
// case). Semantically, this means that all outer targets will be treated as
// identical to the inner-most one, i.e. when the pointer enters or leaves a
// slide target, the corresponding method will be called on all targets that
// nest it.
abstract class _ActionSheetSlideTarget {
abstract class _SlideTarget {
// A pointer has entered this region.
//
// This includes:
@@ -702,7 +702,7 @@ abstract class _ActionSheetSlideTarget {
}
// Recognizes sliding taps and thereupon interacts with
// `_ActionSheetSlideTarget`s.
// `_SlideTarget`s.
class _TargetSelectionGestureRecognizer extends GestureRecognizer {
_TargetSelectionGestureRecognizer({super.debugOwner, required this.hitTest})
: _slidingTap = _SlidingTapGestureRecognizer(debugOwner: debugOwner) {
@@ -715,7 +715,7 @@ class _TargetSelectionGestureRecognizer extends GestureRecognizer {
final _HitTester hitTest;
final List<_ActionSheetSlideTarget> _currentTargets = <_ActionSheetSlideTarget>[];
final List<_SlideTarget> _currentTargets = <_SlideTarget>[];
final _SlidingTapGestureRecognizer _slidingTap;
@override
@@ -744,7 +744,7 @@ class _TargetSelectionGestureRecognizer extends GestureRecognizer {
super.dispose();
}
// Collect the `_ActionSheetSlideTarget`s that are currently hit by the
// Collect the `_SlideTarget`s that are currently hit by the
// pointer, check whether the current target have changed, and invoke their
// methods if necessary.
//
@@ -755,27 +755,27 @@ class _TargetSelectionGestureRecognizer extends GestureRecognizer {
// A slide target might nest other targets, therefore multiple targets might
// be found.
final List<_ActionSheetSlideTarget> foundTargets = <_ActionSheetSlideTarget>[];
final List<_SlideTarget> foundTargets = <_SlideTarget>[];
for (final HitTestEntry entry in result.path) {
if (entry.target case final RenderMetaData target) {
if (target.metaData is _ActionSheetSlideTarget) {
foundTargets.add(target.metaData as _ActionSheetSlideTarget);
if (target.metaData is _SlideTarget) {
foundTargets.add(target.metaData as _SlideTarget);
}
}
}
// Compare whether the active target has changed by simply comparing the
// first (inner-most) avatar of the nest, ignoring the cases where
// _currentTargets intersect with foundTargets (see _ActionSheetSlideTarget's
// _currentTargets intersect with foundTargets (see _SlideTarget's
// document for more explanation).
if (_currentTargets.firstOrNull != foundTargets.firstOrNull) {
for (final _ActionSheetSlideTarget target in _currentTargets) {
for (final _SlideTarget target in _currentTargets) {
target.didLeave();
}
_currentTargets
..clear()
..addAll(foundTargets);
for (final _ActionSheetSlideTarget target in _currentTargets) {
for (final _SlideTarget target in _currentTargets) {
target.didEnter(fromPointerDown: fromPointerDown);
}
}
@@ -791,14 +791,14 @@ class _TargetSelectionGestureRecognizer extends GestureRecognizer {
void _onEnd(Offset globalPosition) {
_updateDrag(globalPosition, fromPointerDown: false);
for (final _ActionSheetSlideTarget target in _currentTargets) {
for (final _SlideTarget target in _currentTargets) {
target.didConfirm();
}
_currentTargets.clear();
}
void _onCancel() {
for (final _ActionSheetSlideTarget target in _currentTargets) {
for (final _SlideTarget target in _currentTargets) {
target.didLeave();
}
_currentTargets.clear();
@@ -949,6 +949,9 @@ class CupertinoActionSheet extends StatefulWidget {
}
class _CupertinoActionSheetState extends State<CupertinoActionSheet> {
int? _pressedIndex;
static const int _kCancelButtonIndex = -1;
ScrollController? _backupMessageScrollController;
ScrollController? _backupActionScrollController;
@@ -1004,6 +1007,20 @@ class _CupertinoActionSheetState extends State<CupertinoActionSheet> {
);
}
void _onPressedUpdate(int actionIndex, bool state) {
if (!state) {
if (_pressedIndex == actionIndex) {
setState(() {
_pressedIndex = null;
});
}
} else {
setState(() {
_pressedIndex = actionIndex;
});
}
}
Widget _buildCancelButton() {
assert(widget.cancelButton != null);
final double cancelPadding = (widget.actions != null || widget.message != null || widget.title != null)
@@ -1012,7 +1029,10 @@ class _CupertinoActionSheetState extends State<CupertinoActionSheet> {
padding: EdgeInsets.only(top: cancelPadding),
child: _ActionSheetButtonBackground(
isCancel: true,
onPressStateChange: (_) {},
pressed: _pressedIndex == _kCancelButtonIndex,
onPressStateChange: (bool state) {
_onPressedUpdate(_kCancelButtonIndex, state);
},
child: widget.cancelButton!,
),
);
@@ -1106,6 +1126,8 @@ class _CupertinoActionSheetState extends State<CupertinoActionSheet> {
child: BackdropFilter(
filter: ImageFilter.blur(sigmaX: _kBlurAmount, sigmaY: _kBlurAmount),
child: _ActionSheetMainSheet(
pressedIndex: _pressedIndex,
onPressedUpdate: _onPressedUpdate,
scrollController: _effectiveActionScrollController,
contentSection: _buildContent(context),
actions: widget.actions ?? List<Widget>.empty(),
@@ -1208,16 +1230,16 @@ class CupertinoActionSheetAction extends StatefulWidget {
}
class _CupertinoActionSheetActionState extends State<CupertinoActionSheetAction>
implements _ActionSheetSlideTarget {
// |_ActionSheetSlideTarget|
implements _SlideTarget {
// |_SlideTarget|
@override
void didEnter({required bool fromPointerDown}) {}
// |_ActionSheetSlideTarget|
// |_SlideTarget|
@override
void didLeave() {}
// |_ActionSheetSlideTarget|
// |_SlideTarget|
@override
void didConfirm() {
widget.onPressed();
@@ -1307,15 +1329,23 @@ class _CupertinoActionSheetActionState extends State<CupertinoActionSheetAction>
// Renders the background of a button (both the pressed background and the idle
// background) and reports its state to the parent with `onPressStateChange`.
//
// Although this class doesn't keep any states, it's still a stateful widget
// because the state is used as a persistent object across rebuilds to provide
// to [MetaData.data].
class _ActionSheetButtonBackground extends StatefulWidget {
const _ActionSheetButtonBackground({
this.isCancel = false,
required this.pressed,
this.onPressStateChange,
required this.child,
});
final bool isCancel;
/// Whether the user is holding on this button.
final bool pressed;
/// Called when the user taps down or lifts up on the button.
///
/// The boolean value is true if the user is tapping down on the button.
@@ -1330,9 +1360,7 @@ class _ActionSheetButtonBackground extends StatefulWidget {
_ActionSheetButtonBackgroundState createState() => _ActionSheetButtonBackgroundState();
}
class _ActionSheetButtonBackgroundState extends State<_ActionSheetButtonBackground> implements _ActionSheetSlideTarget {
bool isBeingPressed = false;
class _ActionSheetButtonBackgroundState extends State<_ActionSheetButtonBackground> implements _SlideTarget {
void _emitVibration(){
switch (defaultTargetPlatform) {
case TargetPlatform.iOS:
@@ -1346,27 +1374,24 @@ class _ActionSheetButtonBackgroundState extends State<_ActionSheetButtonBackgrou
}
}
// |_ActionSheetSlideTarget|
// |_SlideTarget|
@override
void didEnter({required bool fromPointerDown}) {
setState(() { isBeingPressed = true; });
widget.onPressStateChange?.call(true);
if (!fromPointerDown) {
_emitVibration();
}
}
// |_ActionSheetSlideTarget|
// |_SlideTarget|
@override
void didLeave() {
setState(() { isBeingPressed = false; });
widget.onPressStateChange?.call(false);
}
// |_ActionSheetSlideTarget|
// |_SlideTarget|
@override
void didConfirm() {
setState(() { isBeingPressed = false; });
widget.onPressStateChange?.call(false);
}
@@ -1375,11 +1400,11 @@ class _ActionSheetButtonBackgroundState extends State<_ActionSheetButtonBackgrou
late final Color backgroundColor;
BorderRadius? borderRadius;
if (!widget.isCancel) {
backgroundColor = isBeingPressed
backgroundColor = widget.pressed
? _kActionSheetPressedColor
: _kActionSheetBackgroundColor;
} else {
backgroundColor = isBeingPressed
backgroundColor = widget.pressed
? _kActionSheetCancelPressedColor
: _kActionSheetCancelColor;
borderRadius = const BorderRadius.all(Radius.circular(_kCornerRadius));
@@ -1566,6 +1591,7 @@ class _ActionSheetActionSection extends StatelessWidget {
));
}
column.add(_ActionSheetButtonBackground(
pressed: pressedIndex == actionIndex,
onPressStateChange: (bool state) {
onPressedUpdate(actionIndex, state);
},
@@ -1586,71 +1612,52 @@ class _ActionSheetActionSection extends StatelessWidget {
}
// The part of an action sheet without the cancel button.
class _ActionSheetMainSheet extends StatefulWidget {
class _ActionSheetMainSheet extends StatelessWidget {
const _ActionSheetMainSheet({
required this.pressedIndex,
required this.onPressedUpdate,
required this.scrollController,
required this.actions,
required this.contentSection,
required this.dividerColor,
});
final int? pressedIndex;
final _PressedUpdateHandler onPressedUpdate;
final ScrollController? scrollController;
final List<Widget> actions;
final Widget? contentSection;
final Color dividerColor;
@override
_ActionSheetMainSheetState createState() => _ActionSheetMainSheetState();
}
class _ActionSheetMainSheetState extends State<_ActionSheetMainSheet> {
int? _pressedIndex;
bool get _hasContent => widget.contentSection != null;
bool get _hasActions => widget.actions.isNotEmpty;
void _onPressedUpdate(int actionIndex, bool state) {
if (!state) {
if (_pressedIndex == actionIndex) {
setState(() {
_pressedIndex = null;
});
}
} else {
setState(() {
_pressedIndex = actionIndex;
});
}
Widget _scrolledActionsSection(BuildContext context) {
final Color backgroundColor = CupertinoDynamicColor.resolve(_kActionSheetBackgroundColor, context);
return _OverscrollBackground(
scrollController: scrollController,
color: backgroundColor,
child: _ActionSheetActionSection(
actions: actions,
scrollController: scrollController,
dividerColor: dividerColor,
backgroundColor: backgroundColor,
pressedIndex: pressedIndex,
onPressedUpdate: onPressedUpdate,
),
);
}
Widget _dividerAndActionsSection(BuildContext context) {
if (!_hasActions) {
return _empty;
}
final Color backgroundColor = CupertinoDynamicColor.resolve(_kActionSheetBackgroundColor, context);
return Column(
mainAxisSize: MainAxisSize.min,
crossAxisAlignment: CrossAxisAlignment.stretch,
children: <Widget>[
if (_hasContent)
_Divider(
dividerColor: widget.dividerColor,
hiddenColor: backgroundColor,
hidden: false,
),
_Divider(
dividerColor: dividerColor,
hiddenColor: backgroundColor,
hidden: false,
),
Flexible(
child: _OverscrollBackground(
scrollController: widget.scrollController,
color: backgroundColor,
child: _ActionSheetActionSection(
actions: widget.actions,
scrollController: widget.scrollController,
pressedIndex: _pressedIndex,
dividerColor: widget.dividerColor,
backgroundColor: backgroundColor,
onPressedUpdate: _onPressedUpdate,
),
),
child: _scrolledActionsSection(context),
),
],
);
@@ -1658,14 +1665,16 @@ class _ActionSheetMainSheetState extends State<_ActionSheetMainSheet> {
@override
Widget build(BuildContext context) {
return LayoutBuilder(
builder: (BuildContext context, BoxConstraints constraints) {
return _PriorityColumn(
top: widget.contentSection ?? _empty,
bottom: _dividerAndActionsSection(context),
bottomMinHeight: _kActionSheetActionsSectionMinHeight + _kDividerThickness,
);
},
if (actions.isEmpty) {
return contentSection ?? _empty;
}
if (contentSection == null) {
return _scrolledActionsSection(context);
}
return _PriorityColumn(
top: contentSection!,
bottom: _dividerAndActionsSection(context),
bottomMinHeight: _kActionSheetActionsSectionMinHeight + _kDividerThickness,
);
}

10
packages/flutter/test/cupertino/action_sheet_test.dart
View File

@@ -1484,13 +1484,19 @@ void main() {
expect(wasPressed, isFalse);
await tester.tap(find.text('Cancel'));
final TestGesture gesture = await tester.startGesture(tester.getCenter(find.text('Cancel')));
await tester.pumpAndSettle();
// Verify that the cancel button shows the pressed color.
await expectLater(
find.byType(CupertinoActionSheet),
matchesGoldenFile('cupertinoActionSheet.pressedCancel.png'),
);
await gesture.up();
expect(wasPressed, isTrue);
await tester.pump();
await tester.pump(const Duration(seconds: 1));
expect(find.text('Cancel'), findsNothing);
});