[Proposal] Add `onPop` to GoRoute

[benPesso]

Since the Navigator is hidden behind GoRouter.navigatorBuilder, we don't have a way to define the onPagePop handler. It would make sense to have a onPop for each GoRoute, so we can handle page popping per route.

[csells]

what would the signature be? could you cancel the pop? what is the scenario?

[benPesso]

Well, similar the Navigator 2.0's onPopPage callback, I guess.

Possible use cases:

• Record the view time of a video when the user navigates back from a video before it's finished.
• Trigger a refresh for a list in the underlying page (which has its state retained, so it won't otherwise know it should refresh).

Generally speaking, there are many cases where you want some logic to run when the user navigates back without explicitly triggering that logic with a CTA on the popping page.

[csells]

where would the listener be? on the route being popped? or the route being uncovered? also, would you want an event that a page is being covered up, e.g. to stop a video playback?

[benPesso]

I think keeping it inline with the redirect and refreshListener properties would keep things simple for users, so I'd imagine having something like this GoRouter(onPop: _onPopHandler, ...);.

also, would you want an event that a page is being covered up, e.g. to stop a video playback?

I don't think that's necessary, as it can (should?) be handled by the widget that is triggering the navigation away.

[csells]

I'm sorry, @benPesso. I'm not sure I understand your answer. I was thinking that the onPop would be specified on the route itself and not globally, although now I understand the syntax you're asking for. What would be an example implementation of _onPopHandler?

[benPesso]

Apologies for not conveying myself better across our communications, but glad we're making progress. 😄

I'm suggesting to expose the built-in underlying functionality of Flutter's Navigator, which supports an onPopPage callback. The Flutter docs say this about the callback:

Called when pop is invoked but the current Route corresponds to a Page found in the pages list.

To equate it to GoRouter's methodology, I would expect a GoRouter to have a similar callback. Something like this, maybe:

GoRouter(
  onPop: _handlePop, // Called every time a page (`GoRoute` route) is about to pop.
  redirect: _handleRedirect,
  refreshListenable: refreshListenable,
  routes: []
);

This implementation is useful for many cases, such as logging, clean-up, etc.

Does this make sense?

[benPesso]

One prime example for the need for this: Calling SystemChrome.setSystemUIOverlayStyle once a route has popped, to change the overlay style back to what the underlying page needs, without forcing a page rebuild.

[csells]

Logging is supported via the NavigatorObserver support. I would think cleanup would be on a per-page basis. The biggest use for this I can figure is to be able to prevent someone close closing a page if it's "dirty", i.e. if there's some changed data on the page that hasn't been saved by the user, so you'll want them to confirm. I don't know what the API for such a think would be, but I think that support would be per-page and not global.

[benPesso]

I think what you're talking about is a pre-pop callback, which is what the WillPopScope widget is for. Did you read the documentation I've linked to?

The scenario I'm talking about has nothing to do with tracking. In my last example, it's about updating the system UI state after transitioning from a dark-themed page to a light-themed page (and vis a versa), to change the color of text in the status bar, for example. In order to achieve this right now, one has to implement a navigation observer and implement the same logic for Pop, Push and Replace events. And because the GoRoute is not exposed to the navigation observer, one has to also manage a mapping of routes and their themes.

If GoRouter had an onPop callback with a signature like void function(GoRoute poppedRoute), for example, it would simplify everything. And it would enable other functionalities, beyond the one I mentioned above.

[csells]

Is there someone stopping you from accessing the GoRouter instance in your NavigatorObserver?

[benPesso]

For one, the fact that the GoRouter instance is not a global variable, and there's no context in a navigator observer. But aside from that, it is also inefficient to (1) create a special observer and (2) duplicate logic for all route states, when a single callback within the GoRouter could do the trick and keep everything self-contained.

[csells]

you can make your GoRouter instance global -- I've done that in some apps and it's handy.

you don't have to duplicate logic for all states -- you can implement one shared implementation and share it.

[benPesso]

The GoRouter doesn't offer access to the route and its context, though. Only to the location data. So that will still require a separate datastore to manage a mapping between the location data and the desired settings to be applied.

The duplication is still needed because the properties of each state are different. So using a route observer, and the location name, such an implementation looks something like this:

  @override
  void didPop(Route<dynamic> route, Route<dynamic>? previousRoute) {
    super.didPop(route, previousRoute);
    if (previousRoute is! PageRoute || route is! PageRoute) return;

    _setSystemStyleByLocation(route.settings.name);
  }

  @override
  void didPush(Route<dynamic> route, Route<dynamic>? previousRoute) {
    super.didPush(route, previousRoute);
    if (route is! PageRoute) return;

    _setSystemStyleByLocation(previousRoute?.settings.name);
  }

  @override
  void didReplace({Route<dynamic>? newRoute, Route<dynamic>? oldRoute}) {
    super.didReplace(newRoute: newRoute, oldRoute: oldRoute);
    if (newRoute is! PageRoute) return;

    _setSystemStyleByLocation(newRoute.settings.name);
  }

And that's excluding the 3 other states...

  void didStartUserGesture(Route<dynamic> route, Route<dynamic>? previousRoute) { }

  void didStopUserGesture() { }

  void didRemove(Route<dynamic> route, Route<dynamic>? previousRoute) { }

[csells]

It does seem like one implementation of NavigatorObserver that the community could share would solve this problem w/o changing the DevExp or API for go_router.