Skip to content
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

[suggestion][navigator 2.0] Make it a lot simpler for the end developer #69315

Closed
tomasbaran opened this issue Oct 29, 2020 · 70 comments
Closed
Labels
c: proposal A detailed proposal for a change to Flutter customer: crowd Affects or could affect many people, though not necessarily a specific customer. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.

Comments

@tomasbaran
Copy link

tomasbaran commented Oct 29, 2020

I just finished studying the Navigator 2.0 documentation and I have to say it is REALLY complex. I'm not saying it's badly done but it's very very complex, difficult to grasp and use. It's far from being simple and easy to implement so much so that I no longer have a feeling I'm working with Flutter. I fell in love with Flutter for its simplicity. I don't know how but I have a feeling that for the end developer, the implementation can be done a lot simpler. Only my 2 cents.

Disclaimer: I may be wrong and I'm rather representing junior-level Flutter devs. I have been developing in Flutter for 8 months I have created a few fully funcional apps (iOS/Android/web), some of them are published on my github.

@darshankawar
Copy link
Member

@tomasbaran
Is there any specific part of the documentation that you think is complex to understand and implement ? Would you look for more examples and samples to help it make simpler ?

@darshankawar darshankawar added in triage Presently being triaged by the triage team waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds labels Oct 30, 2020
@tomasbaran
Copy link
Author

tomasbaran commented Oct 30, 2020

Not anything specific, just overall it's very complex. Reading https://medium.com/flutter/learning-flutters-new-navigation-and-routing-system-7c9068155ade Navigation 1.0 is so simple. Navigator.push() and Navigator.pop() and that's basically it. The whole example has 90 lines in that document which most of it are stuff that are NOT related to the Navigator

Comparing that with Navigator 2.0 in the doc, the full example has 256 lines which mostly are Navigator 2.0 related. There has got to be a way to make it a lot simpler—just as there has been a way to make Flutter simple.

More concrete examples to help it make simpler ALWAYS helps. Definitely!!

Thanks for your time @darshankawar

@no-response no-response bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Oct 30, 2020
@darshankawar darshankawar added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. c: proposal A detailed proposal for a change to Flutter and removed in triage Presently being triaged by the triage team labels Oct 30, 2020
@jacobaraujo7
Copy link
Contributor

@tomasbaran I believe that API 2.0 is beneficial for those who need to create a highly customised Route API (Packages).
So the new navigator will not replace what you already have, it adds this functionality to make the routes customisable and to do many things that onGenerateRoute could not do.

@tomasbaran
Copy link
Author

tomasbaran commented Nov 2, 2020

@jacobaraujo7 thanks for your comment. Could you please elaborate on some use cases where API 2.0 should be used instead of API 1.0?

From my understanding API 2.0 provides also some basic functionality that API 1.0 does NOT, i.e. using the back button in the browser, using named routes that are visible in the URL as well as using visible arguments in the URL (e.g. www.url.com/?arg1=123)

I believe this basic functionality should be very easy to implement and should be automatically built in, without doing a detailed implementation that involved delegates and other stuff that adds complexity and frankly in my opinion unnecessary difficulty at least for junior developers like me.

I'm sure I'm not the only one, I believe there are more junior Flutter developers than the senior ones. Flutter was designed to be inviting to junior devs by their very simple way it works. Unfortunately, I think Navigator 2.0 is an exception.

@goderbauer
Copy link
Member

/cc @chunhtai

@chunhtai
Copy link
Contributor

chunhtai commented Nov 4, 2020

@tomasbaran
I can list out several things you can't do with navigator 1.0.

  1. you have full controll over route stack and customize how to transition each route. For example, you cannot remove three routes in the middle with the navigator 1.0 API.
  2. you have complete control over what to do when user press android back button. This become more promising when you have more than one navigator in the widget tree.
  3. Web use case as you mentioned.
  4. you also have full control over how to parse the initial route. In the Navigator 1.0, the initial route is parsed by / and will create a stack of routes, and you cannot update the initial route once it is built.

We are also currently exploring different ways to simplify it, and we are also looking for feedback on how we can make it simpler.

cc @johnpryan

@celesteking
Copy link

celesteking commented Nov 10, 2020

Having 2.5 posts on Medium explaining the use of Nav2 ain't gonna cut it. You should've created proper documentation before releasing into public, including the "start-up" guide, including the tutorial, including tech description. Like the real thing, you know.

I'm just starting to go though that single Medium post explaining the innards, but I think I'm not gonna make it till the end, as reading the first paragraphs is just as daunting as it might get -- delegates, parsers, dispatchers, listeners... oh god, why engage Quantum Mechanics in order to move a piece of cowshit instead of using a shovel? It was way easier for my small brain to just specify a MAP of named routes and just use pushNamed with a humanly readable string.

All this looks more like someone's graduation project than something that was supposed to make things simpler for end-developer. I guess Nav3 is on its way.

@tomasbaran
Copy link
Author

@celesteking thanks, you just verbalized exactly what I was trying to say. These are exactly the things I was daunted by: delegates, parsers, dispatchers, listeners... and I didn't expect that from Flutter, at least from my understanding is that Flutter is supposed to be simple. I DO find it simple, that's why I love it so much... all except Nav 2.0.

@celesteking
Copy link

Several hours later and I have just finished reading and implementing over here (in my 1-legged sample app) that Medium post. I skipped "TransitionDelegate" section as that was way too much for me. I have understood maybe 20% of what's been written there. I still don't understand how those 7 components on the picture interact between each other and whether they're needed at all.

While following the post (and implementing/copying over here), I noticed there's a code smell -- hell lot of duplication.
BookRoutePath was supposed to be the route information holder, so why not make it so and use it for that purpose? Instead, it's being duplicated all over the code.

  • BookRouteInformationParser -- route serialization logic -- should've been put into BookRoutePath
  • BookRouterDelegate #show404 is duplicating BookRoutePath #isUnknown, currentConfiguration is duplicating route serialization logic, setNewRoutePath is duplicating BookRoutePath assignment logic.

I mean, 200 LOC to switch between 2 screens? Really?!

Angular:

    {path: 'crisis-list', component: CrisisListComponent},
    {path: 'heroes-list', component: HeroesListComponent},

Flutter named routes:

    '/crisis-list': (context) => CrisisListScreen(),
    '/heroes-list': (context) => HeroesListScreen(),

I can't even imagine what happens when you need a sub-navigation in Nav 2.0. How many LOCs should be written for that, 2000 probably?

See how it's done @ angular:

const routes: Routes = [{
    path: 'first-component',
    component: FirstComponent,
    children: [
      { path: 'child-a', component: ChildAComponent, },
      { path: 'child-b', component: ChildBComponent, },
    ],
  }];

Fast and easy. Route params parsing is done inside components, if they want that.

Nav 2.0 should really be renamed to Nav "Advanced". It's not an improvement, unless you consider over-engineering, unnecessary complexity and cumbersomeness an improvement.

@carloshwa
Copy link

carloshwa commented Nov 11, 2020

In case it helps anyone, I modified (ie, simplified) the sample in the infamous Medium article to:

  1. Only use the minimal amount of code to demonstrate functionality of Navigator 2.0 by removing all the InnerDelegate stuff.
  2. Make the sample app responsive depending on screen size.
  3. Separate the code into 2 files: main.dart, which includes all the Navigator 2.0-specific implementation, and detail.dart, which includes plain old Flutter code to render the views.

The original sample code is probably a more thorough implementation, but this is a simple implementation that I needed to look at to understand how to start implementing it in my own app. It mostly has the same functionality as the original sample.

https://github.com/carloshwa/flutter-example/tree/navigator_2/

EDIT: I just realized there are 2 samples in the Medium article. I had jumped straight into the Nested Navigator sample, which is way more complex and what I was trying to simplify. Turns out my simplification is basically what the first sample does, but I was able to achieve the behavior of the nested navigator sample with the simplicity of the first sample in my sample, so maybe it is still of use to someone.

@CosmicPangolin
Copy link

CosmicPangolin commented Nov 17, 2020

@carloshwa @chunhtai @johnpryan

I consider my skill level decently advanced (2.5+ yrs Flutter), but it's taken me two days of brainpower parsing the framework, medium article, and design docs to really get my head around this design well enough to figure out how to integrate routing with a typical state management pattern. I 100% concur that this design is unintuitive, for a few important reasons:

Convoluted Delegate/InformationParser Implementations

  1. Why would a Listenable have a build method in its interface....? Most Flutter users have never implemented a Listenable, and it's extremely confusing for a first implementation to extend a class that mixes mental models with Widget because of the need to inject a Navigator tree to the Router. It feels like this alone breaks the single-responsibility mantra that seems to otherwise take precedent in the design of these delegate classes.

  2. Bad naming and unfleshed API docs. parseRouteInformation, restoreRouteInformation, and setNewRoutePath methods don't give enough context between class and method names OR documentation to give users a strong sense for how and why these methods are used. Adding 'Platform' to the method names would make this much more clear...as would fleshing out docs to better explain the flow of route information.

  3. The implicit relationships between these classes are hidden from the user in the Router widget. It's hard to grok that InformationParser methods are intrinsically linked to Delegate's setNewRoutePath and currentConfiguration.

  4. Building and then implicitly passing state objects between these classes through methods. We absolutely don't want to do this - particularly when using provider + builder patterns. It just doesn't fit the state management patterning used across Flutter at all. Imagine I use flutter_bloc + built_value to handle my RouteState - I have to build a new state in InformationParser that then gets returned to Delegate, so I can call a bloc method to rebuild my state given the contents of the new state object floating between methods. I shudder at the thought...I want to be building RouteState ONLY in my bloc/action/whatever.

  5. Scope is not obvious. So the prospect of accessing context in these methods is not clear.

Confusing Documentation

  1. Most of us use some sort of Provider-based state management pattern. The design docs use a ChangeNotifier as a RouteState and the medium article has a weird mix of local Delegate state and RouteState object constructors. It's not at all clear for the average user how to map these examples to more popular or semantically clean patterns.

  2. currentConfiguration is shown in the medium article but not the design docs.

I have to imagine that it's possible and easier to collapse each of these delegate behaviors into distinct Router widget properties - or even extend a Router Widget interface - such that the design is more explicit and clear and there's less repetition and confusion around method flows. If there were a Fluttery borrow checker, I would expect it to put up quite the fight over having the user implement this interlinked delegate interface design :P

@chunhtai
Copy link
Contributor

chunhtai commented Nov 18, 2020

Hi @CosmicPangolin, while I may not be able to reason with the complexity of this API. I am able to answer some of the question you brought up.

We separate the router into four different delegates because the router widget handles too many thing, and we have to provide ways to customize those things as well, we figure it will be a mess the keep everything in one widget, that is why we tried to categorize them into different functional pieces.

This make it possible to customize all the functionality you want while keeping the possibility to write all sorts of different routing framework on top of the API.

  1. Why would a Listenable have a build method in its interface....?

I don't really see this as a problem.

  1. Bad naming and unfleshed API docs

That is something we are still continuing improving on

  1. The implicit relationships between these classes are hidden from the user in the Router widget

all the different delegates has outlined the relationship with other delegates. but I agreed it is hidden too deep in to documentation. We should have a more comprehensive description in the Router widget documentation

  1. Building and then implicitly passing state objects between these classes through methods

Parser returns a configuration for the router to modify the App state. It should be a temporarily data instead of the state because it should not persist.

  1. Scope is not obvious. So the prospect of accessing context in these methods is not clear.

The only place you can access the context should be in build method. Can you elaborate more on this?

  1. Most of us use some sort of Provider-based state management pattern...

the RouteState is only a temporary data to convey the parsed data from the parser to the routerdelegate. The only state in the example is inside the RouterDelegate _selectedBook and show404

  1. currentConfiguration is shown in the medium article but not the design docs.

It was mentioned in Router widget api doc but not in RouterDelegate api doc. we should fix this. thanks for mentioning.

@CosmicPangolin
Copy link

CosmicPangolin commented Nov 18, 2020

@chunhtai ty for quick comments!

It seems very awkward to me to have some proxy configuration object that is only used for passing between delegate methods that set/get a more permanent route state somewhere else. For me this was a huge point of confusion in trying to understand the examples - if the route's state can be fully determined from the configuration object, why would I have both? I feel like it should be possible to construct these classes with one valid representation of routing data for design clarity.

Similarly with the build method in Delegate...it can work (partially, see below), but I think it's an intrinsically confusing design for users that are used to implementing functional state management or stateful widget patterns w/ setState. I'm not even sure that the design allows us to manage state with the same patterning as the rest of our app. For example:

Elaboration on 5 - if my state is stored in an InheritedWidget and I can only access BuildContext from the build method, how do I update that state with setNewRoutePath? I haven't actually checked whether I can access Router's context from the delegate's method scope, but you seem to imply that I can't call BlocProvider.of() to update state. And keeping state local to the delegate doesn't cut it since I want the entire widget tree to have access to the route information.

I do agree that a lot of the documentation issues come down to lots of cross-referencing between classes in the docs. Having each delegate detailed in Router with their relationships will help a lot.

Update: confirmed I can't access Router context to call Provider from setNewRoutePath scope. Unless I'm missing something, this is a really fundamental design flaw...either we're pidgeon-holed into using local delegate state to track route information, or we have to go through the gymnastics adding a post-frame callback to update our provider state with checks on local state in the build method. Both are semantically muddled options.

@johnpryan
Copy link
Contributor

The good news is that Navigator 2.0 is capable enough to build any router API you want. Right now I'm experimenting with a routing package that handles the parsing for you: https://github.com/johnpryan/page_router.

It allows you to map a route path with parameters (/user/123) to a widget builder, which might be enough for most apps. It's implementation is reasonably small.

@tomasbaran
Copy link
Author

Just spreading the word about Simple Nav 2.0 packages that may help us get there at least hopefully in the meantime there is a native Simple Nav 2.0 (or Nav 3.0):

I haven't tested them just passing them along.

@jacobaraujo7
Copy link
Contributor

Just spreading the word about Simple Nav 2.0 packages that may help us get there at least hopefully in the meantime there is a native Simple Nav 2.0 (or Nav 3.0):

I haven't tested them just passing them along.

https://pub.dev/packages/flutter_modular/versions/3.0.0-nullsafety.1
Using Nav 2.0 as well

@venkatd
Copy link

venkatd commented Dec 10, 2020

@chunhtai I would like to override the transitionDelegate property to ensure a few routes aren't animated with the push transition (they are master-detail style). However, the DefaultTransitionDelegate and NoAnimationTransition delegate both look very complex so I am afraid to touch them.

It would be valuable to have some documentation on how this delegate works. Or perhaps the larger function could be split into out into smaller ones to make it easier to override or understand how to implement my own?

Thanks!

@JulianBissekkou
Copy link

@johnpryan Really interesting package that you wrote there. Is this just a playground or are you planning on releasing it at any given time? I have some ideas and potential problems that I need to solve for a mobile, tablet, and web app written in flutter.

@chunhtai I think the problem here is that people don't know how they should restructure the navigation in the app.

I see many people wrapping the declarative API of the Navigator 2.0 in an imperative push and pop API by accessing the written RouterDelgate and just exposing access to the pages of the navigator.

For example:

class MyRouterDelegate extends RouterDelegate<MyRouteInfo>
    with ChangeNotifier, PopNavigatorRouterDelegateMixin<MyRouteInfo> {

  @override
  final navigatorKey = GlobalKey<NavigatorState>();

  List<Page<dynamic>> _pages;

  @override
  Widget build(BuildContext context) {
    return StoreConnector<GlobalAppState, bool>(
      converter: (store) => store.state.isUserLoggedIn,
      builder: (context, isUserLoggedIn) {
        return Navigator(
          key: navigatorKey,
          pages: _pages,
          onPopPage: _onPopPage, //TODO remove poped page from list 
        );
      },
    );
  }
  
  void pushPage(Page<dynamic> page) {
    _pages.add(page);
    notifyListeners();
  }
}

Instead of Navigator.of(context).push(someRoute) someone would do Router.of(context).delegate.pushPage(somePage)

I am not sure if this is the correct way to use this API and it feels wrong tbh.

If you stay declarative routing adds a lot of boilerplate code. Here is my example of doing the same thing using properties of the state:

  @override
  Widget build(BuildContext context) {
    return StoreConnector<GlobalAppState, bool>(
      converter: (store) => store.state.isUserLoggedIn,
      builder: (context, isUserLoggedIn) {
        return Navigator(
          key: navigatorKey,
          pages: [
            AppellaPage(
              key: const ValueKey("Home"),
              child: HomeScreen(onShowLoading: () {
                _isSplashScreenVisible = true;
                notifyListeners();
              }),
            ),
            if (_isSplashScreenVisible) ...[
              FadePage(
                key: const ValueKey("Splash"),
                child: SplashScreen(onAnimationDone: () {
                  _isSplashScreenVisible = false;
                  notifyListeners();
                }),
              ),
            ],
          ],
          onPopPage: _onPopPage,
        );
      },
    );
  }

This introduces complexity in the RouterDelegate and the _pages array can get really big and messy.

Next question: How do I pass data between the pages? final result = Navigator.of(context).push(someRoute) was used before, but I have no idea how to do that in navigator 2.0.
Most examples only cover basic navigation with 2-3 screens.

However, I think the Navigator 2.0 API and all the components around it are very well documented, and after using the API for a day I know how to do everything that I need. I think you guys did a great job with the new API but examples are missing.

To summarize:

  • How to handle the state of the new pages array? Completly Imperative, Declarative, or a mix between both?
  • How to pass data around between pages?
  • How to organize business logic in the RouterDelegate?

@kevlar700
Copy link

It might be worth waiting to see what gets released on 3rd March during the flutter engage event, though.

@orenagiv
Copy link

Of course.
However, please note the discussion here (look at the last two comments):
lulupointu/vrouter#1

And based on everything that is being worked on and discussed here:
https://github.com/flutter/uxr/wiki/Navigator-2.0-API-Usability-Research

My feeling is that it's going to take at least a few months before there will be any major decisions or upgrades from the Flutter team.
Personally, I believe they would prefer to leverage excellent existing packages like they did with the Provider package.

@chunhtai
Copy link
Contributor

We try to be as open as possible, there should not be secret surprises. The current work is publicly discussed in the the https://github.com/flutter/uxr/wiki/Navigator-2.0-API-Usability-Research and https://github.com/flutter/uxr/issues

@Tiska
Copy link

Tiska commented Mar 4, 2021

Hi, I just finished to read the all thread. I also read some medium implementation examples, and tried to implement navigator v2 in my app, which seems great in my case as I need to use a lot of deeplinks. But like everyone one this thread, the v2 is soo much complex than v1, and introduces a lot of concepts, that I'm afraid of a to invest on it in my app if it is in discussion to change it.

I did not see any improvements during flutter engage, or did I missed it? Is the v2 something that we can rely on for the next months or will it change with all the users feedback ?

@slovnicki
Copy link

@Tiska there were some questions about Navigator 2.0, but unfortunately they didn't pick them for answering. I was also kind of expecting it yesterday.

The current information is that "Navigator 2.0 API will not change in the foreseeable future"

@Tiska
Copy link

Tiska commented Mar 4, 2021

thanks 🙂 so we are still at the same point. I actually hesitate between use it pure with the inconvenience of the complexity and boilerplate code, or use the autoroute version of it. I already use autoroute, but did not migrate to the beta with v2 nav yet. One argument that go for pure v2 is that autoroute has not migrate to null safety yet.

@Henzelix
Copy link

Henzelix commented Mar 5, 2021

I have a feeling that Flutter 2.0 didn't fix anything, just pushed Web to stable :/

@kevlar700
Copy link

kevlar700 commented Mar 5, 2021

chunhtai said as much above. They're working on it and want to get it right.

Also 2.0 released more than that with desktop being very close to stable and ffi etc..

@lulupointu
Copy link

Navigator 2.0 allowed way more than just using it with the web! Il allow declarative navigation, handling of pop event and handling of Android back button for example.

You can say that Navigator 2.0 is a poor name or is too much boiler plate, but saying it did not bring anything apart from web support is just showing that you did not look into the new API enough.

@BeliliFahem
Copy link

Very disappointed by this complexity which remains there even after release 2.

@orenagiv
Copy link

orenagiv commented Mar 8, 2021

This "complexity" is not a bad thing.
It gives all of us full and amazing flexibly to do ANYTHING we need.

And if you want simplicity - you can simply use great packages such as VROUTER.DEV.

@xuanswe
Copy link

xuanswe commented Mar 8, 2021

This "complexity" is not a bad thing.
It gives all of us full and amazing flexibly to do ANYTHING we need.

I think this complexity for end developers is a bad thing. It against the philosophy of flutter: simple, fast development. It will give users a chance to think: should I find another framework for my app?

The low level API is only good for 2 kinds of people: flutter team and owner of a advanced, specialized navigation framework.

And if you want simplicity - you can simply use great packages such as VROUTER.DEV.

Navigation is the core function and should be simple, powerful and provided by flutter core, not by a 3rd party. New developers shouldn't be in the war of navigation packages. Choosing a 3rd package is not an easy job. I spent months to try some packages and go to the conclusion: I am writing my own navigation package for now.

@slovnicki
Copy link

@nguyenxndaidev I agree with most what you said, but

Navigation is the core function and should be simple, powerful and provided by flutter core, not by a 3rd party. New developers shouldn't be in the war of navigation packages

Isn't it the same situation for state management?

@orenagiv
Copy link

orenagiv commented Mar 8, 2021

It is @slovnicki.
But the Provider package (for example) is awesome, covers most cases in a very simple approach.
And as Flutter grows and becomes truly the first and only full high-performance Platform Adaptive solution out there, with so many things to cover - I don't think we can escape complexity.
Not in the initial phases of each module anyway.
And let's be realistic - nothing is perfect - there will always be edge cases which simplicity won't cover.

Anyway - look how seriously the Flutter team is taking this:
https://github.com/flutter/uxr/wiki/Navigator-2.0-API-Usability-Research

I've never seen such devotion.
Give them some time!
And meanwhile - use the open source packages - they are doing a great job.

@xuanswe
Copy link

xuanswe commented Mar 8, 2021

@nguyenxndaidev I agree with most what you said, but

Navigation is the core function and should be simple, powerful and provided by flutter core, not by a 3rd party. New developers shouldn't be in the war of navigation packages

Isn't it the same situation for state management?

Not the same, flutter provides simple state management way. Bloc or redux are just advanced approach for that. New developers can easily start and having a good app even without these libs.

Now, if flutter want web as the target, navigator 1 doesn't work at all. That's why flutter team develop Nav 2. But the team should not release it this until they can provide a simple solution for end developers. It makes developers be confused. I don't believe new developers can easily implement an app with current Nav 2.

@xuanswe
Copy link

xuanswe commented Mar 8, 2021

Anyway - look how seriously the Flutter team is taking this:
https://github.com/flutter/uxr/wiki/Navigator-2.0-API-Usability-Research

I've never seen such devotion.
Give them some time!

Yes, flutter team is reacting very well. I love them with that reaction ♥️.

And meanwhile - use the open source packages - they are doing a great job.

They are doing great job, I agree.
But No, these libs also need more time to be usable. Most of them don't work very well now.

@donpaul120
Copy link

I’m new to flutter, I’m coming from Native Android (Java/Kotlin). Reading the whole documentation and got to the Routing part and that’s where I lost it. You mean to tell me that a simple “startActivity(context, Intent)” with flags I have to create all this parsers, delegate, routerPath etc... it’s a lot complex than setting up a DI framework. I was hoping to start with the latest which is Navigator 2.0 since I’m new to it, but I got totally lost as to why I have to do all this.😫

@xuanswe
Copy link

xuanswe commented Apr 25, 2021

Hello everyone, after months of trying multiple packages in details and joined interesting discussions in the uxr research, I couldn't find something I need for my application.

I gather all ideas and problems I found along these months to analyze them seriously.
Finally, Navi 0.2.0 is released today.

The package provides an elegant API, which is simple and easy to learn, yet keeping full power of Navigator 2.0 combine with imperative API on top of the core declarative API.

I am confident to start using it in my application from now on.

I hope you will enjoy Navi as I am.

@iapicca
Copy link
Contributor

iapicca commented Apr 25, 2021

Finally, Navi 0.2.0 is released today.

@nguyenxndaidev I'll be careful with packages with 0% test coverage

@xuanswe
Copy link

xuanswe commented Apr 25, 2021

Edit: 04.05.2021 (91% code coverage) => you shouldn't worry about test coverage of Navi. I am still writing more tests now.

image


Hi @iapicca, thanks for your comment. The package is just in version 0.2 to prove the usage of the API via scenarios and manual testing.

I am starting to write tests now. Please stay tuned. I'm alone in this project and therefore could only concentrate on the API design until now. Everyone is welcome to help me speed up the project. I'm very appreciated for any help.

If you check the milestones section in the documentation, I will make sure at least 90% test coverage before 1.0. My plan is to release 1.0 this year (late Q3 or early Q4) because my application must be online early next year.

For now, you can start checking how it works and use it for not so urgent projects. If you cannot wait for Navi 1.0 until early Q4 this year, I don't suggest to use Navi for your products right now.

@mleonhard
Copy link
Contributor

mleonhard commented May 6, 2021

I'm trying to figure out how to remove a page without animation. This is very difficult. It seems like I must learn and understand Navigator internals (Navigator, TransitionDelegate, DefaultTransitionDelegate.resolve) and write a bunch of extra code to make it work. This solution is 100 times more expensive and risky than the solutions for Navigator 1.0.

I wish it was simple like overriding bool get Page::animatePush and bool get Page::animatePop.

@mleonhard
Copy link
Contributor

Follow-up to my comment directly above:
The TransitionDelegate.resolve method gets RouteTransitionRecord which provides no way to get the Page it operates on. It provides only the Route, which also does not provide the Page.

So I must sub-class the route and make my TransitionDelegate check the type of the route to decide whether or not to show the animation. But CupertinoPage.createRoute returns a private class _PageBasedCupertinoPageRoute which uses multiple-inheritance.

After half an hour of reading, I found a workaround. It turns out that Page implements RouteSettings. And the documentation says that Route.settings is set to the Page. So I can get the Page from RouteTransitionRecord.route.settings.

I will code my own TransitionDelegate. It will call into DefaultTransitionDelegate.resolve and then inspect the resulting transitions. It will modify the appropriate transition from an animated pop to a non-animated remove. Unfortunately, the RouteTransitionRecord class provides no way to inspect it. That prevents me from using DefaultTransitionDelegate.resolve and I must copy that code. This is so disappointing.

@mleonhard
Copy link
Contributor

mleonhard commented May 6, 2021

Here's a delegate that should work but doesn't:

class DontAnimateBottomRoutePop<T> extends TransitionDelegate<T> {
  final TransitionDelegate<T> inner;

  DontAnimateBottomRoutePop({this.inner = const DefaultTransitionDelegate()});

  @override
  Iterable<RouteTransitionRecord> resolve({
    required List<RouteTransitionRecord> newPageRouteHistory,
    required Map<RouteTransitionRecord?, RouteTransitionRecord>
        locationToExitingPageRoute,
    required Map<RouteTransitionRecord?, List<RouteTransitionRecord>>
        pageRouteToPagelessRoutes,
  }) {
    if (locationToExitingPageRoute.containsKey(null)) {
      final RouteTransitionRecord record = locationToExitingPageRoute[null]!;
      if (record.isWaitingForExitingDecision) {
        pageRouteToPagelessRoutes[record]
            ?.where((x) => x.isWaitingForExitingDecision)
            .forEach((x) => x.markForRemove());
        record.markForRemove();
      }
    }
    return inner.resolve(
      newPageRouteHistory: newPageRouteHistory,
      locationToExitingPageRoute: locationToExitingPageRoute,
      pageRouteToPagelessRoutes: pageRouteToPagelessRoutes,
    );
  }
}

Printing RouteTransitionRecord shows "Instance of '_RouteEntry'" which is useless. I will try the debugger.

@mleonhard
Copy link
Contributor

I discovered that calling Navigator.pop() automatically marks routes for animated removal. A TransitionDelegate cannot change the transition type because RouteTransitionRecord contains an assertion preventing it.

I added a popLastPage() method to my AppNavigator stateful widget. It removes the last pages entry and rebuilds the Navigator. This eliminates the animation for the removed bottom-most page. Unfortunately, the page replacing it now does not get an animation. One step forward and one step backward.

@timsneath I've now spent three hours trying get my app to use the appropriate transition from the loading page to the home page, using Navigator 2.0. Am I a bad engineer?

@mleonhard
Copy link
Contributor

I wasted another hour to discover that Navigator treats all CupertinoPage objects as identical unless they have a unique key. All I need to do is add key: UniqueKey() params to every page when adding it to the Navigator pages list. Then Navigator's default behavior meets my needs. I don't even need a transition delegate.

It's a relief that I don't need to maintain all of this extra code that I wrote. I'm pasting it here since it may be useful to others. None of the Navigator-related classes have useful toString() methods. I implemented some to make debugging easier.

Four hours total spent on this. :(

app_navigator.dart
import 'package:flutter/cupertino.dart' show CupertinoPage, CupertinoPageRoute;
import 'package:flutter/widgets.dart'
    show
        BuildContext,
        DefaultTransitionDelegate,
        Navigator,
        Page,
        PageRoute,
        Route,
        RouteTransitionRecord,
        State,
        StatefulWidget,
        TransitionDelegate,
        Widget;
import 'greeting_page.dart' show greetingPage;

String pageToString(dynamic page) => (page is CupertinoPage<dynamic>)
    ? 'CupertinoPage(${page.title!})'
    : page.toString();

String pageListToString(List<Page> pages) =>
    pages.map(pageToString).toList().toString();

String routeClassName(Route route) {
  if (route is CupertinoPageRoute) {
    return 'CupertinoPageRoute';
  } else if (route is PageRoute) {
    return 'PageRoute';
  } else {
    return 'Route';
  }
}

String routeToString(Route route) =>
    '${routeClassName(route)}(${pageToString(route.settings)})';

String recordToString(RouteTransitionRecord record) =>
    'RouteTransitionRecord(${routeToString(record.route)})';

String recordListToString(List<RouteTransitionRecord> records) =>
    records.map(recordToString).toList().toString();

String recordMapToString(
    Map<RouteTransitionRecord?, RouteTransitionRecord> records) {
  final List<String> entries = [];
  records.forEach((RouteTransitionRecord? key, RouteTransitionRecord value) {
    final keyString = key != null ? recordToString(key) : 'null';
    final valueString = recordToString(value);
    entries.add('$keyString: $valueString');
  });
  return '[${entries.join(', ')}]';
}

String recordMapListToString(
    Map<RouteTransitionRecord?, List<RouteTransitionRecord>> records) {
  final List<String> entries = [];
  records
      .forEach((RouteTransitionRecord? key, List<RouteTransitionRecord> value) {
    final keyString = key != null ? recordToString(key) : 'null';
    final valueString = recordListToString(value);
    entries.add('$keyString: $valueString');
  });
  return '[${entries.join(', ')}]';
}

// Do not use this class because it doesn't work.
// Navigator by default does not animate removing the bottom-most page.
// If it's animating for you, you're probably using Navigator.pop() which
// automatically marks the transitions as animated.
// To prevent animating out the bottom-most page, do not use Navigator.pop()
// and instead use your own popPage() method.
class DontAnimateBottomRoutePop<T> extends TransitionDelegate<T> {
  final TransitionDelegate<T> inner;

  DontAnimateBottomRoutePop({this.inner = const DefaultTransitionDelegate()});

  @override
  Iterable<RouteTransitionRecord> resolve({
    required List<RouteTransitionRecord> newPageRouteHistory,
    required Map<RouteTransitionRecord?, RouteTransitionRecord>
        locationToExitingPageRoute,
    required Map<RouteTransitionRecord?, List<RouteTransitionRecord>>
        pageRouteToPagelessRoutes,
  }) {
    print('DontAnimateBottomRoutePop.resolve\n'
        '  newPageRouteHistory = ${recordListToString(newPageRouteHistory)}\n'
        '  locationToExitingPageRoute = ${recordMapToString(locationToExitingPageRoute)}\n'
        '  pageRouteToPagelessRoutes = ${recordMapListToString(pageRouteToPagelessRoutes)}\n');
    if (locationToExitingPageRoute.containsKey(null)) {
      final RouteTransitionRecord record = locationToExitingPageRoute[null]!;
      if (record.isWaitingForExitingDecision) {
        pageRouteToPagelessRoutes[record]
            ?.where((x) => x.isWaitingForExitingDecision)
            .forEach((x) => x.markForRemove());
        record.markForRemove();
      }
    }
    return inner.resolve(
      newPageRouteHistory: newPageRouteHistory,
      locationToExitingPageRoute: locationToExitingPageRoute,
      pageRouteToPagelessRoutes: pageRouteToPagelessRoutes,
    );
  }
}

// Do not use this class because it doesn't work.
// By default, Navigator automatically animates the added page if it notices that it is
// new.  Flutter treats all `CupertinoPage` objects as the same object unless
// they have a unique key.  So to replace the bottom-most page and animate in
// the replacement, just give the new page a unique key.
class AnimateAddedTopPage<T> extends TransitionDelegate<T> {
  final TransitionDelegate<T> inner;

  AnimateAddedTopPage({this.inner = const DefaultTransitionDelegate()});

  @override
  Iterable<RouteTransitionRecord> resolve({
    required List<RouteTransitionRecord> newPageRouteHistory,
    required Map<RouteTransitionRecord?, RouteTransitionRecord>
        locationToExitingPageRoute,
    required Map<RouteTransitionRecord?, List<RouteTransitionRecord>>
        pageRouteToPagelessRoutes,
  }) {
    print('AnimateAddedTopPage.resolve\n'
        '  newPageRouteHistory = ${recordListToString(newPageRouteHistory)}\n'
        '  locationToExitingPageRoute = ${recordMapToString(locationToExitingPageRoute)}\n'
        '  pageRouteToPagelessRoutes = ${recordMapListToString(pageRouteToPagelessRoutes)}\n');
    if (newPageRouteHistory.isNotEmpty &&
        newPageRouteHistory.last.isWaitingForEnteringDecision) {
      pageRouteToPagelessRoutes[newPageRouteHistory.last]
          ?.where((x) => x.isWaitingForEnteringDecision)
          .forEach((x) => x.markForPush());
      newPageRouteHistory.last.markForPush();
    }
    return inner.resolve(
      newPageRouteHistory: newPageRouteHistory,
      locationToExitingPageRoute: locationToExitingPageRoute,
      pageRouteToPagelessRoutes: pageRouteToPagelessRoutes,
    );
  }
}

class _AppNavigatorState extends State<AppNavigator> {
  List<Page> _pages = [greetingPage()];

  void popPage() {
    if (!this.mounted) {
      print('_AppNavigatorState.popPage not mounted');
      return;
    }
    setState(() {
      final before = pageListToString(this._pages);
      _pages.removeLast();
      final after = pageListToString(this._pages);
      print('_AppNavigatorState.popPage $before -> $after');
    });
  }

  void pushPage(Page page) {
    print(
        '_AppNavigatorState.push ${pageListToString(this._pages)} + ${pageToString(page)}');
    if (!this.mounted) {
      print('_AppNavigatorState.push not mounted');
      return;
    }
    setState(() {
      _pages.add(page);
    });
  }

  @override
  Widget build(BuildContext context) {
    if (_pages.isEmpty) {
      final page = greetingPage();
      print(
          '_AppNavigatorState.build ${pageListToString(this._pages)} + ${pageToString(page)}');
      this._pages.add(page);
    }
    return Navigator(
      transitionDelegate: AnimateAddedTopPage(),
      //transitionDelegate: DontAnimateBottomRoutePop(),
      pages: [..._pages] /* Every build must provide a new list object. */,
      onPopPage: (route, result) {
        print('_AppNavigatorState.onPopPage $route');
        if (!route.didPop(result)) {
          return false;
        }
        _pages.removeLast();
        return true;
      },
    );
  }
}

class AppNavigator extends StatefulWidget {
  static _AppNavigatorState of(BuildContext ctx) =>
      ctx.findAncestorStateOfType<_AppNavigatorState>()!;

  @override
  State<StatefulWidget> createState() => _AppNavigatorState();
}
log
flutter: _AppNavigatorState.popPage [CupertinoPage(Greeting)] -> []
flutter: _AppNavigatorState.push [] + CupertinoPage(Dev Home)
flutter: AnimateAddedTopPage.resolve
  newPageRouteHistory = [RouteTransitionRecord(PageRoute(CupertinoPage(Dev Home)))]
  locationToExitingPageRoute = [null: RouteTransitionRecord(PageRoute(CupertinoPage(Greeting)))]
  pageRouteToPagelessRoutes = []

@xuanswe
Copy link

xuanswe commented May 6, 2021

@mleonhard to create a page without animation, you only need to create a custom page like below:

import 'package:flutter/widgets.dart';

class NoAnimationPage extends Page<dynamic> {
  const NoAnimationPage({
    LocalKey? key,
    required this.child,
  }) : super(key: key);

  final Widget child;

  @override
  Route<dynamic> createRoute(BuildContext context) => PageRouteBuilder<dynamic>(
        settings: this,
        pageBuilder: (_, __, ___) => child, // don't wrap in an animation to create a page without animation.
      );
}

Then instead of MaterialPage or CupertinoPage, you use NoAnimationPage.

@github-actions
Copy link

github-actions bot commented Aug 2, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: proposal A detailed proposal for a change to Flutter customer: crowd Affects or could affect many people, though not necessarily a specific customer. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

No branches or pull requests