-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[go_router] Fix Popping state and re-rendering scaffold at the same time doesn't update the URL on web #150312 #6953
base: main
Are you sure you want to change the base?
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
routerDelegate.pop<T>(result); | ||
routeInformationProvider.popSave<T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not work if the routerDelegate.pop
removes pageless route such as dialog or modalbottomroute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've try use go_router 14.2.0 of pub.dev and run demo follwing code also not work, I thought it was normal, see the picture above.
if misundestand please tell me how reproduce it.
demo code:
import 'dart:async';
import 'package:flutter/material.dart';
import 'package:go_router/go_router.dart';
class CounterStream {
int _counter = 0;
final _streamController = StreamController<int>.broadcast();
Stream<int> get stateStream => _streamController.stream.asBroadcastStream();
void increment() {
_streamController.sink.add(++_counter);
}
Future<void> delayedRerender() async {
increment();
increment();
}
void dispose() {
_streamController.close();
}
}
final CounterStream counterStream = CounterStream();
class StreamListener extends ChangeNotifier {
StreamListener(Stream<dynamic> stream) {
notifyListeners();
_subscription = stream.asBroadcastStream().listen((_) {
notifyListeners();
});
}
late final StreamSubscription<dynamic> _subscription;
@override
void notifyListeners() {
super.notifyListeners();
print('refreshing the router');
}
@override
void dispose() {
_subscription.cancel();
super.dispose();
}
}
void main() {
GoRouter.optionURLReflectsImperativeAPIs = true;
WidgetsFlutterBinding.ensureInitialized();
runApp(const MyApp());
}
class MyApp extends StatefulWidget {
const MyApp({super.key});
@override
State<MyApp> createState() => _MyAppState();
}
final _rootNavigatorKey = GlobalKey<NavigatorState>();
final GoRouter _router = GoRouter(
initialLocation: '/',
navigatorKey: _rootNavigatorKey,
refreshListenable: StreamListener(counterStream.stateStream),
routes: <RouteBase>[
ShellRoute(
builder: (context, state, child) {
return GenericPage(child: child);
},
routes: [
GoRoute(
path: '/',
builder: (BuildContext context, GoRouterState state) =>
const GenericPage(showPushButton: true, path: 'a'),
routes: [
GoRoute(
path: 'a',
name: 'a',
builder: (BuildContext context, GoRouterState state) =>
const GenericPage(showPushButton: true, path: 'b'),
routes: [
GoRoute(
path: 'b',
name: 'b',
builder: (BuildContext context, GoRouterState state) =>
const GenericPage(showBackButton: true),
),
],
),
],
),
],
),
],
);
class _MyAppState extends State<MyApp> {
late StreamSubscription<int> _stateSubscription;
int _currentState = 0;
@override
void initState() {
super.initState();
_stateSubscription = counterStream.stateStream.listen((state) {
setState(() {
_currentState = state;
});
});
}
@override
void dispose() {
_stateSubscription.cancel();
super.dispose();
}
@override
Widget build(BuildContext context) {
return MaterialApp.router(
title: 'Flutter Demo',
theme: ThemeData(
primarySwatch: Colors.blue,
),
routerConfig: _router,
);
}
}
class DialogTest extends StatelessWidget {
const DialogTest({super.key});
@override
Widget build(BuildContext context) {
return Material(
type: MaterialType.transparency,
child: Center(
child: Container(
color: Colors.white,
width: 300,
height: 300,
alignment: Alignment.center,
child: Column(
children: ['Navigator::pop', 'GoRouter::pop'].map((e) {
return InkWell(
child: Row(children: [
Text(e),
const Icon(Icons.close),
]),
onTap: () {
if (e == 'GoRouter::pop') {
GoRouter.of(context).pop();
} else {
Navigator.of(context).pop();
}
},
);
}).toList(),
),
),
),
);
}
}
class GenericPage extends StatefulWidget {
final Widget? child;
final bool showPushButton;
final bool showBackButton;
final String? path;
const GenericPage({
this.child,
Key? key,
this.showPushButton = false,
this.showBackButton = false,
this.path,
}) : super(key: key ?? const ValueKey<String>('ShellWidget'));
@override
State<GenericPage> createState() => _GenericPageState();
}
class _GenericPageState extends State<GenericPage> {
late StreamSubscription<int> _stateSubscription;
int _currentState = 0;
@override
void initState() {
super.initState();
_stateSubscription = counterStream.stateStream.listen((state) {
setState(() {
_currentState = state;
});
});
}
@override
void dispose() {
_stateSubscription.cancel();
super.dispose();
}
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: widget.child != null
? AppBar(
title: Text('Count: $_currentState'),
actions: [
TextButton(
onPressed: () {
showDialog(
context: context,
builder: (context) {
return DialogTest();
},
);
},
child: const Text('dialog1'),
),
TextButton(
onPressed: () {
showModalBottomSheet(
context: context,
builder: (context) {
return DialogTest();
},
);
},
child: const Text('dialog2'),
)
],
)
: null,
body: _buildWidget(context));
}
Widget _buildWidget(BuildContext context) {
if (widget.child != null) {
return widget.child!;
}
if (widget.showBackButton) {
return TextButton(
onPressed: () {
// WHEN THE USER PRESSES THIS BUTTON, THE URL
// DOESN'T CHANGE, BUT THE SCREEN DOES
counterStream
.increment(); // <- when removing this line the issue is gone
GoRouter.of(context).pop();
},
child: const Text('<- Go Back'),
);
}
if (widget.showPushButton) {
return TextButton(
onPressed: () {
GoRouter.of(context).goNamed(widget.path!);
},
child: const Text('Push ->'),
);
}
return Text('Current state: $_currentState');
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not looked into it more, but we can't guarantee the goRouter.pop will pop the last routematch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can guarantee the goRouter.pop will pop the last routematch, because we just mark the RouteInformation in GoRouteInformationProvider, the pop will use navigatorKey to pop the router and use currentConfiguration of GoRouterDelegate to check last router and we have no change currentConfiguration GoRouterDelegate value.
pop of NavigatorState won't be affected either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant is that the pop can pop a dialog instead of the page. It is also possible that the page can contain localhistory, such as open drawer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed not addressed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will continue to look into this.
Hi @ahyangnb Are you still planning on coming back to this pr? |
|
Change the 3 file palce, because issue #150312
The reason for the issue is when call
counterStream.increment();
will notifynotifyListeners()
ofStreamListener
, and finally affect_handleRouteInformationProviderNotification
of_RouterState
inpackages/flutter/lib/src/widgets/router.dart
.When pop should not call
_handleRouteInformationProviderNotification
otherwise will be path wrong.because the value of
GoRouteInformationProvider
is not the latest.solution:
always make the value of GoRouteInformationProvider knew the latest RouteInformation[but not to do
notifyListeners()
].i don't know if adding pop to NavigatingType is reasonable, if any place needs to be changed please tell me.
Pre-launch Checklist
i was run
shell_route_test.dart
to check the change I am making, all existing and new tests are passing.dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.md
to add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes].///
).