[go_router] Implement `popUntil` #2728

ValentinVignal · 2022-10-22T13:37:04Z

So I wanted to work on flutter/flutter#107052 for pushAndRemoveUntil but it was closed saying it was a duplicate of flutter/flutter#106402 which was not because it didn't contain the request for pushAndRemoveUntil. And flutter/flutter#106402 got also closed because it got implemented.

So do you want me to re-create an issue? Or should we re-open flutter/flutter#107052?

In any case, this PR first implements popUntil which is simpler than pushAndRemoveUntil. I will implement pushAndRemoveUntil if the implementation of this one is acceptable.

Pre-launch Checklist

ValentinVignal · 2022-10-30T11:39:46Z

Also relates to flutter/flutter#99112

ValentinVignal · 2022-10-31T15:48:34Z

Is there something else I need to do in this PR?

chunhtai · 2022-11-02T17:40:52Z

I think these issues are closed due to we plan to use naive navigator API to handle imperative API. @johnpryan can chime in.

chunhtai · 2022-11-02T17:43:51Z

packages/go_router/lib/src/delegate.dart

@@ -146,6 +146,18 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
    notifyListeners();
  }

+  /// Calls [pop] repeatedly until the predicate returns true.
+  void popUntil(bool Function(RouteMatch) predicate) {


I think you can just use go to build up to a certain routematch list? If we do refactor the imperative API this will be obsolete

What do you mean?
Are you suggesting that instead of calling popUntil (as a dev using go_router), I should call go?

I don't think it would do the same right?

If I understood correctly, go is kind of a replaceAll, after using go, there is no page to pop and canPop is false.
While popUntil will allow you to only pop some pages but not all. The use case is when the user finishes a wizard to set up something, you want to remove all the wizard's pages from the history but still keep the history the user accumulated before starting the wizard.

go wouldn't work in this scenario, right?

no I meant it seems like the go can replace popUntil. Since developer creates their own route table, they will be able to use go to achieve functional equivalence of popUntil. For example if they have route table like this

GoRouteA( routes: [ GoRouteB( routes: [GoRouteC()] ) ], )

lets say route match list is [GoRouteA, GoRouteB, GoRouteC].
popUntil((route) => route == GoRouteB) will be the same as go(GoRouteB)

I think this comes back to flutter/flutter#99112 's discussion.

Here you are assuming that the match list is [GoRouteA, GoRouteB, GoRouteC] and matches perfectly the structure

GoRouteA( routes: [ GoRouteB( routes: [GoRouteC()] ) ], )

And pop from GoRouteC always goes "back" to GoRouteB.

But as I described in flutter/flutter#99112 (comment), this is not what users are usually expecting. They don't always access GoRouteC from GoRouteB. So always "going back" to GoRouteB from GoRouteC doesn't work.
The way I understand it is that you are trying to merge/combine "app location" and "history"? But that's 2 different things that should be independent.

Am I missing something?

no, you are right, the previous comment is under the assuming that the match list matches the structure. I am assuming history means the internal route stack, not the browser history. In that case, my proposal user can still modify the history through imperative API, it just that they won't be able to change app location with imperative API. Let's continue the discussion in the issue.

# Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

ValentinVignal · 2022-11-23T15:24:16Z

Hello @chunhtai , @johnpryan , can I ask for a follow up on this?

Should I close this PR? If yes, is there a plan to make the delegate and route match list public? And can I help on that?

chunhtai · 2022-12-08T17:37:18Z

packages/go_router/lib/src/delegate.dart

+  /// Calls [pop] repeatedly until the predicate returns true.
+  void popUntil(bool Function(RouteMatch) predicate) {
+    bool hasPopped = false;
+    while (!predicate(_matchList.last)) {


things has changed a bit, its probably the best to call pop directly since it will remove any pageless route that are on top such as dialog.

@chunhtai It's hard to use go router.... Us project previous use more ‘pop with value on pages’, now have the requirement need do the web compatible, but go router no this feature to provide, then use 'Go_Router_Flow' lib, but looks like Pop function have been changed....

No I mean the implementation of popUntil should use pop method. I am on board with implementing popUntil

@chunhtai I am calling pop in ♻️ Call pop repeatedly

# Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

ValentinVignal · 2022-12-16T10:49:23Z

packages/go_router/test/delegate_test.dart

+
+      await tester.pumpAndSettle();
+      expect(goRouter.routerDelegate.matches.matches.length, 3);
+      goRouter.routerDelegate.addListener(expectAsync0(count: 2, () {}));


With the current implementation (simply calling pop, the notifiers are notified for each pop). In this case, 2 times. Is it correct or should the router delegate notify only once?

we may need to refactor the code a bit, every time it notify the listener, the Gorouter will also notify its listener about location change, so if url = '/a/b/c/d/e' and popUtil to '/a'
if we notify each time it pop
goRoute will notify its listener with
'/a/b/c/d'
'/a/b/c'
'/a/b'
'/a'

instead just
'/a'

So, to be correct, this test should pass with count: 1, is that correct?

Hello @chunhtai , I'm not sure it will be possible to notify the listeners only once even when popping several times (and make the test pass with count: 1).

I pushed some code in 2c48f39 . Itis not correct as I guess it doesn't work with multiple nested navigators, it's more of a demonstration example. it uses directly popUntil from the NavigatorState and it still notifies the listeners twice.

Or would you know a way to make this work?

# Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

followthemoney1 · 2023-01-05T09:26:53Z

Will this work for dialogs? I mean if I've open the dialog and then will call this popUntill, will predicate return the right value?

stuartmorgan · 2023-01-17T21:08:28Z

@chunhtai What's the status of this PR? It looks like it's probably waiting for another review.

ValentinVignal · 2023-01-18T01:05:36Z

@stuartmorgan Actually, it requires some work from me first. #2728 (comment) I need to find a way to make the delegate notify its listeners only once.

I was waiting for #2846 to be merged. Because I personally have a bigger need for replace than popUntil in my projects so I was focusing my energy there

stuartmorgan · 2023-01-31T20:44:26Z

Thanks, I'm going to go ahead and mark this as a draft then just to get it off of our review queue. Please set it back to Ready for Review once you've had a chance to update it!

# Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/lib/src/misc/extensions.dart # packages/go_router/pubspec.yaml

chunhtai · 2023-03-16T21:40:50Z

@ValentinVignal The #2846 is about to be merged (just waiting for ci), will you work on this PR next?

ValentinVignal · 2023-03-17T01:08:31Z

@ValentinVignal The #2846 is about to be merged (just waiting for ci), will you work on this PR next?

I'm not sure I'll work on it right away as I'm quite busy right now, but I'll work on it at some point when #2846 is merged yes. But feel free to take it over if you want to

chunhtai · 2023-03-23T21:41:11Z

Let me close this one for now, feel free to reopen once you have time.

talamaska · 2023-04-12T17:24:56Z

I'm still missing popUntil. It is irreplace-able feature that I'm missing. go_router should have it if it wants to be a full featured navigator. Some guy told me I could just use .go instead, but isn't .go like a push, won't it create another item in the navigation stack? I don't want that. I want to pop several pages. Use case is very simple. Page with a button, when the button is clicked navigates to a page with camera view, take photo, navigate to a full page preview pf the photo, click on confirm button, pop the preview page and the camera view. The only way to make this now is with 2 pops and have a special logic to get a value from the first pop. What if I have a wizard form with multiple pages. Having to implement a bunch of custom logic and calls of unknown number of pop.

johnpryan · 2023-04-12T18:37:38Z

@talamaska go() is different from push(), go() replaces your current Navigator stack with a new one. For example, take this sample with two routes:

GoRoute(
  path: '/library',
  routes: [
    GoRoute(
      path: 'album/:albumId',
      routes: [
        GoRoute(
          path: 'song/:songId',
        ),
      ],
    ),
  ],
),

If you navigate to/library/album/1 by tapping on an item, the Navigator will have two Pages, one for /library and one for album/:albumId. If you tap the 'Library' button in the bottom navigation bar, the app will call go('/library') and the Navigator is rebuilt with only one Page, /library, so the second screen is popped from the Navigator and the correct transition animation is displayed, as if the user pressed the in-app back button.

ValentinVignal added 3 commits October 22, 2022 21:29

✨ Add popUntil

4a809f3

✅ Test popUntil

0047433

📝 Update the documentation

4196431

ValentinVignal requested a review from chunhtai as a code owner October 22, 2022 13:37

github-actions bot added the p: go_router label Oct 22, 2022

chunhtai reviewed Nov 2, 2022

View reviewed changes

ValentinVignal mentioned this pull request Nov 4, 2022

[go_router] Figure out how to support imperative APIs moving forward flutter/flutter#99112

Open

4 tasks

chunhtai self-requested a review November 17, 2022 22:55

Merge branch 'main' into go-router/pop-until

701d26d

# Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

chunhtai reviewed Dec 8, 2022

View reviewed changes

ValentinVignal added 4 commits December 16, 2022 18:22

Merge remote-tracking branch 'upstream/main' into go-router/pop-until

ab21137

# Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

♻️ Call pop repeatedly

f9e08c2

📝 Update the documentation

51395cd

✅ Update the tests

43ec575

ValentinVignal commented Dec 16, 2022

View reviewed changes

ValentinVignal requested a review from chunhtai December 16, 2022 10:49

Merge branch 'main' into go-router/pop-until

97cbf4e

# Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

stuartmorgan marked this pull request as draft January 31, 2023 20:44

ValentinVignal added 2 commits February 7, 2023 15:54

Merge remote-tracking branch 'upstream/main' into go-router/pop-until

59b89cd

# Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/lib/src/misc/extensions.dart # packages/go_router/pubspec.yaml

⚗️ Use navigator pop until

2c48f39

da-kami mentioned this pull request Mar 1, 2023

Receive payment UI get10101/10101#179

Merged

5 tasks

chunhtai closed this Mar 23, 2023

willhlas mentioned this pull request Apr 3, 2023

feat(game): add quit game dialog flutter/io_flip#147

Merged

7 tasks

[go_router] Implement `popUntil` #2728

[go_router] Implement `popUntil` #2728

ValentinVignal commented Oct 22, 2022 •

edited

ValentinVignal commented Oct 30, 2022

ValentinVignal commented Oct 31, 2022

chunhtai commented Nov 2, 2022

chunhtai Nov 2, 2022

ValentinVignal Nov 3, 2022

chunhtai Nov 3, 2022

ValentinVignal Nov 3, 2022

chunhtai Nov 3, 2022

ValentinVignal commented Nov 23, 2022

chunhtai Dec 8, 2022

AceChen1 Dec 9, 2022 •

edited

chunhtai Dec 15, 2022

ValentinVignal Dec 16, 2022

ValentinVignal Dec 16, 2022

chunhtai Dec 16, 2022

ValentinVignal Dec 17, 2022

chunhtai Jan 4, 2023

ValentinVignal Feb 7, 2023

followthemoney1 commented Jan 5, 2023

stuartmorgan commented Jan 17, 2023

ValentinVignal commented Jan 18, 2023

stuartmorgan commented Jan 31, 2023

chunhtai commented Mar 16, 2023

ValentinVignal commented Mar 17, 2023

chunhtai commented Mar 23, 2023

talamaska commented Apr 12, 2023 •

edited

johnpryan commented Apr 12, 2023

[go_router] Implement popUntil #2728

[go_router] Implement popUntil #2728

Conversation

ValentinVignal commented Oct 22, 2022 • edited

Pre-launch Checklist

ValentinVignal commented Oct 30, 2022

ValentinVignal commented Oct 31, 2022

chunhtai commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValentinVignal commented Nov 23, 2022

Choose a reason for hiding this comment

AceChen1 Dec 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

followthemoney1 commented Jan 5, 2023

stuartmorgan commented Jan 17, 2023

ValentinVignal commented Jan 18, 2023

stuartmorgan commented Jan 31, 2023

chunhtai commented Mar 16, 2023

ValentinVignal commented Mar 17, 2023

chunhtai commented Mar 23, 2023

talamaska commented Apr 12, 2023 • edited

johnpryan commented Apr 12, 2023

[go_router] Implement `popUntil` #2728

[go_router] Implement `popUntil` #2728

ValentinVignal commented Oct 22, 2022 •

edited

AceChen1 Dec 9, 2022 •

edited

talamaska commented Apr 12, 2023 •

edited