Unverified Commit 00bbf543 authored by fzyzcjy's avatar fzyzcjy Committed by GitHub

Allow users to provide route settings to `showMenu` (#124935)

Background: I am adding logging to things like dialogs, bottom sheets and menus. Then I realized that, showMenu does not allow users to provide RouteSettings, while showDialog and showModalBottomSheet both allow. Therefore, IMHO a consistent API design may need to add this to showMenu.

I will add tests if this proposal looks OK :)
parent 5a1ef73d
...@@ -789,6 +789,7 @@ class _PopupMenuRoute<T> extends PopupRoute<T> { ...@@ -789,6 +789,7 @@ class _PopupMenuRoute<T> extends PopupRoute<T> {
required this.capturedThemes, required this.capturedThemes,
this.constraints, this.constraints,
required this.clipBehavior, required this.clipBehavior,
super.settings,
}) : itemSizes = List<Size?>.filled(items.length, null), }) : itemSizes = List<Size?>.filled(items.length, null),
// Menus always cycle focus through their items irrespective of the // Menus always cycle focus through their items irrespective of the
// focus traversal edge behavior set in the Navigator. // focus traversal edge behavior set in the Navigator.
...@@ -949,6 +950,7 @@ Future<T?> showMenu<T>({ ...@@ -949,6 +950,7 @@ Future<T?> showMenu<T>({
bool useRootNavigator = false, bool useRootNavigator = false,
BoxConstraints? constraints, BoxConstraints? constraints,
Clip clipBehavior = Clip.none, Clip clipBehavior = Clip.none,
RouteSettings? routeSettings,
}) { }) {
assert(items.isNotEmpty); assert(items.isNotEmpty);
assert(debugCheckHasMaterialLocalizations(context)); assert(debugCheckHasMaterialLocalizations(context));
...@@ -979,6 +981,7 @@ Future<T?> showMenu<T>({ ...@@ -979,6 +981,7 @@ Future<T?> showMenu<T>({
capturedThemes: InheritedTheme.capture(from: context, to: navigator.context), capturedThemes: InheritedTheme.capture(from: context, to: navigator.context),
constraints: constraints, constraints: constraints,
clipBehavior: clipBehavior, clipBehavior: clipBehavior,
settings: routeSettings,
)); ));
} }
......
...@@ -3180,6 +3180,48 @@ void main() { ...@@ -3180,6 +3180,48 @@ void main() {
paints..rrect(color: const Color(0xff0000ff)), paints..rrect(color: const Color(0xff0000ff)),
); );
}, variant: TargetPlatformVariant.desktop()); }, variant: TargetPlatformVariant.desktop());
testWidgets('Popup menu with RouteSettings', (WidgetTester tester) async {
late RouteSettings currentRouteSetting;
await tester.pumpWidget(
MaterialApp(
navigatorObservers: <NavigatorObserver>[
_ClosureNavigatorObserver(onDidChange: (Route<dynamic> newRoute) {
currentRouteSetting = newRoute.settings;
}),
],
home: const Material(
child: Center(
child: ElevatedButton(
onPressed: null,
child: Text('Go'),
),
),
),
),
);
final BuildContext context = tester.element(find.text('Go'));
const RouteSettings exampleSetting = RouteSettings(name: 'simple');
showMenu<void>(
context: context,
position: RelativeRect.fill,
items: const <PopupMenuItem<void>>[
PopupMenuItem<void>(child: Text('foo')),
],
routeSettings: exampleSetting,
);
await tester.pumpAndSettle();
expect(find.text('foo'), findsOneWidget);
expect(currentRouteSetting, exampleSetting);
await tester.tap(find.text('foo'));
await tester.pumpAndSettle();
expect(currentRouteSetting.name, '/');
});
} }
class TestApp extends StatelessWidget { class TestApp extends StatelessWidget {
...@@ -3232,3 +3274,21 @@ class MenuObserver extends NavigatorObserver { ...@@ -3232,3 +3274,21 @@ class MenuObserver extends NavigatorObserver {
super.didPush(route, previousRoute); super.didPush(route, previousRoute);
} }
} }
class _ClosureNavigatorObserver extends NavigatorObserver {
_ClosureNavigatorObserver({required this.onDidChange});
final void Function(Route<dynamic> newRoute) onDidChange;
@override
void didPush(Route<dynamic> route, Route<dynamic>? previousRoute) => onDidChange(route);
@override
void didPop(Route<dynamic> route, Route<dynamic>? previousRoute) => onDidChange(previousRoute!);
@override
void didRemove(Route<dynamic> route, Route<dynamic>? previousRoute) => onDidChange(previousRoute!);
@override
void didReplace({Route<dynamic>? newRoute, Route<dynamic>? oldRoute}) => onDidChange(newRoute!);
}
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