Skip to content

[go_router] Fix Popping state and re-rendering scaffold at the same time doesn't update the URL on web #150312 #6953

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.

Sign up for GitHub

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

Closed
ahyangnb wants to merge 12 commits into from
Closed

[go_router] Fix Popping state and re-rendering scaffold at the same time doesn't update the URL on web #150312 #6953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eeffe8e
[go_router] Save the pop state to value when remove top-most route.
Jun 20, 2024
76c6ed2
[go_router] Save the pop state to value when remove top-most route.
ahyangnb Jun 20, 2024
fb5a366
revert
ahyangnb Jun 20, 2024
503f301
[go_router] Save the pop state to value when remove top-most route.
ahyangnb Jun 20, 2024
66b32ac
code formart
ahyangnb Jun 20, 2024
ced3492
code format parser.dart
ahyangnb Jun 20, 2024
5af364d
tests the shell route.
ahyangnb Jun 20, 2024
c2b6454
Merge branch 'main' into main
ahyangnb Jun 20, 2024
a207889
Merge branch 'main' into main
ahyangnb Jun 20, 2024
5952190
Merge branch 'main' into main
ahyangnb Jun 21, 2024
e134caa
optimize _updateRouteMatchList.
ahyangnb Jun 21, 2024
546a495
Merge remote-tracking branch 'origin/main'
ahyangnb Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion packages/go_router/lib/src/information_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ enum NavigatingType {
/// Restore the current match list with
/// [RouteInformationState.baseRouteMatchList].
restore,

/// Pop Current route.
pop,
}

/// The data class to be stored in [RouteInformation.state] to be used by
Expand All @@ -48,7 +51,9 @@ class RouteInformationState<T> {
this.completer,
this.baseRouteMatchList,
required this.type,
}) : assert((type == NavigatingType.go || type == NavigatingType.restore) ==
}) : assert((type == NavigatingType.go ||
type == NavigatingType.restore ||
type == NavigatingType.pop) ==
(completer == null)),
assert((type != NavigatingType.go) == (baseRouteMatchList != null));

Expand Down Expand Up @@ -217,6 +222,17 @@ class GoRouteInformationProvider extends RouteInformationProvider
return completer.future;
}

/// Save the pop state to value when remove top-most route.
void popSave<T>(String location, {required RouteMatchList base}) {
_value = RouteInformation(
uri: Uri.parse(location),
state: RouteInformationState<T>(
baseRouteMatchList: base,
type: NavigatingType.pop,
),
);
}

RouteInformation _valueInEngine;

void _platformReportsNewRouteInformation(RouteInformation routeInformation) {
Expand Down
3 changes: 3 additions & 0 deletions packages/go_router/lib/src/parser.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,9 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
return baseRouteMatchList!.uri.toString() != newMatchList.uri.toString()
? newMatchList
: baseRouteMatchList;
case NavigatingType.pop:
// Will not do anything.
return baseRouteMatchList!;
}
}

Expand Down
6 changes: 6 additions & 0 deletions packages/go_router/lib/src/router.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,12 @@ class GoRouter implements RouterConfig<RouteMatchList> {
log('popping ${routerDelegate.currentConfiguration.uri}');
return true;
}());

routerDelegate.pop<T>(result);
routeInformationProvider.popSave<T>(
Copy link
Contributor

@chunhtai chunhtai Jun 20, 2024

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.

Copy link
Contributor Author

@ahyangnb ahyangnb Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go_router14 2 0

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');
  }
}

Copy link
Contributor

@chunhtai chunhtai Jun 21, 2024

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.

Copy link
Contributor Author

@ahyangnb ahyangnb Jun 25, 2024

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.

Copy link
Contributor Author

@ahyangnb ahyangnb Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value of GoRouteInformationProvider is self-management, will not use it outside.

so you can check the GoRouteInformationProvider in packages/go_router/lib/src/information_provider.dart

image

Copy link
Contributor

@chunhtai chunhtai Jul 11, 2024

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.

Copy link
Contributor

@chunhtai chunhtai Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seemed not addressed?

Copy link
Contributor Author

@ahyangnb ahyangnb Jul 19, 2024

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.

routerDelegate.currentConfiguration.uri.toString(),
base: routerDelegate.currentConfiguration,
);
}

/// Refresh the route.
Expand Down Expand Up @@ -556,6 +561,7 @@ class GoRouter implements RouterConfig<RouteMatchList> {
/// A routing config that is never going to change.
class _ConstantRoutingConfig extends ValueListenable<RoutingConfig> {
const _ConstantRoutingConfig(this.value);

@override
void addListener(VoidCallback listener) {
// Intentionally empty because listener will never be called.
Expand Down