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

No support for nested Navigators? #23

Closed
esDotDev opened this issue Mar 14, 2020 · 6 comments
Closed

No support for nested Navigators? #23

esDotDev opened this issue Mar 14, 2020 · 6 comments

Comments

@esDotDev
Copy link

esDotDev commented Mar 14, 2020

Just curious how you see this working in apps where I have multiple views containing tab bars, or a desktop style app that has sidemenu that pushes views to the content area in the right, but still wants to show fullscreen dialogs over the entire app.

Seems like this would be more flexible if it had the ability to have the root navigator, but also a context based lookup.

NestedNavigator(key: Get.getKey(context))

Get.to(SomeNestedPage(), context)
@esDotDev esDotDev changed the title No support for nested widgets? No support for nested Navigators? Mar 14, 2020
@jonataslaw
Copy link
Owner

This is not difficult to implement, I would do it in a few minutes, but my concern is that it can have side effects, and major performance problems.
I centralized everything that depends on context in a single navigatorKey, which causes less memory consumption compared to the standard framework.
In order to have NestedNavigators, it is necessary to create a Map of globalKeys, and the documentation is clear that globalKeys are expensive and should be avoided (and this is remarkable, as I mentioned above, only the fact of centralizing half of the GlobalKeys of the framework in one, already caused a noticeable difference in ram consumption).
It is a function that I understand necessary, but that will clearly bring performance problems.
Hypothetical situation:
The user enters a tab, navigates through 3 more tabs (in Flutter the navigation is superimposed, so one screen is literally on top of the other). Then he opens another tab, navigates through 5 more tabs, and does it successively. 15, 20, 30 screens that are not being used will be kept in memory and will not be discarded properly. I understand the need for this function, but I can't think of a single situation that would be worth having nestedNavigators at the expense of performance.
I could implement NestedNavigators now, but at least in my applications it did not have a good result, however if this is something urgent and more people have the same need, I can add it experimentally on the master, but for now, thinking about great applications, I don't really like that idea.
I will in the future use GetObserver to save the application's routes and global state, and thus, simulate NestedNavigators. The screens would not be open, but when the route that was already saved was clicked, Get would take you directly to the page where you left off. But I only have plans to do this for version 2.0, where I will include the permanent navigation feature. I'm working on it in my spare time.

@esDotDev
Copy link
Author

esDotDev commented Mar 14, 2020

Do you think these are really practical concerns though? Dart is very fast, Flutter has low memory usage in general, is also very quick at inflating/deflating widgets.

I think this case of many navigators, with many pages all stacked up, is quite outside the main path of like 1 root navigator, and 1 or 2 sub navigators that I keep running into anytime I want to have some persistent chrome around my content pages.

Without this, I wonder how you would recommend doing anything like this currently? I'm thinking desktop apps specifically, where this sort of persistent menu/nav bar is always present:
http://screens.gskinner.com/shawn/Photoshop_2020-03-14_02-04-57.png

Also, as I understand page routes, unless you set them to be non-opaque, the widgets are removed, and the page route deflated, as soon as the new route has finished transitioning in.
https://api.flutter.dev/flutter/widgets/PageRoute/opaque.html

@jonataslaw
Copy link
Owner

Do you think these are really practical concerns though? Dart is very fast, Flutter has low memory usage in general, is also very quick at inflating/deflating widgets.

I think this case of many navigators, with many pages all stacked up, is quite outside the main path of like 1 root navigator, and 1 or 2 sub navigators that I keep running into anytime I want to have some persistent chrome around my content pages.

Without this, I wonder how you would recommend doing anything like this currently? I'm thinking desktop apps specifically, where this sort of persistent menu/nav bar is always present:
http://screens.gskinner.com/shawn/Photoshop_2020-03-14_02-04-57.png

Also, as I understand page routes, unless you set them to be non-opaque, the widgets are removed, and the page route deflated, as soon as the new route has finished transitioning in.
https://api.flutter.dev/flutter/widgets/PageRoute/opaque.html

If there is no abuse, as in your case, there would be no problems.
However, as for opaque routes, this does not correspond to reality.
Opaque routes should work as in the documentation, but this does not actually happen, because the GlobalKey used in the Overlay that builds the base of navigation routes is not a const. Every time a new route is pushed, a new GlobalKey is created, and this causes the entire MaterialApp rebuild. So setting opaque = true has the exact opposite behavior to that of the documentation.
Currently, in the stable and beta version, opaque route is not a performance optimization, on the contrary, it is a complete bug, but necessary for the operation of the Cupertino Route, in fact it is not so necessary, but to make this work, I I would have to rebuild the Framework from the Widget Overlay for it to work properly, and it would inflate my library a lot.

If you want to see in practice how this happens, reproduce the example below and you will see that when pushing a new route, all the others below the opaque route are rebuild.

import 'package:flutter/material.dart';

void main() {
  runApp(new MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return new MaterialApp(
      title: 'Flutter Demo',
      theme: new ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: new MyHomePage(id: 1),
    );
  }
}

class MyHomePage extends StatefulWidget {
  MyHomePage({Key key, this.id}) : super(key: key);

  final int id;

  @override
  _MyHomePageState createState() => new _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {

  @override
  Widget build(BuildContext context) {
    print('Page ${widget.id} built!');
    return new Scaffold(
      appBar: new AppBar(
        title: new Text(widget.id.toString()),
      ),
      body: new Center(
        child: new RaisedButton(
          child: new Text('Open next page'),
          onPressed: () {
            Navigator.of(context).push(
              new MaterialPageRoute(
                builder: (_) => new MyHomePage(id: widget.id + 1),
                maintainState: true
              )
            );
          }
        )
      ),
    );
  }
}

The widgets are not removed, they remain in memory, the only thing that changes is that the app will be bottlenecked by a setState in the entire materialApp.

Here is an example of the memory usage between Get with opaque = false and MaterialPageRoute with opaque = true:
https://www.youtube.com/watch?v=yVBqT6hg4VA&t=14s

However, goderbauer is solving this already. He did a recent pull on the master that took the GlobalKey from the overlay off the stage, preventing a new one from being created with each navigation, but this was reversed 2 days later because it caused instability in the snackbar, which needs a new GlobalKey to stack a OverlayRoute over the normal route. Numerous changes were made, which even caused this lib to be incompatible with the master, so I stopped following until the changes went to Dev.
If this is corrected, everything will be fine.

@esDotDev
Copy link
Author

Wow, good to know! Thx for the overview.

@jlubeck
Copy link

jlubeck commented Apr 3, 2020

I supposed this is the place to be for the nested navigators that you mentioned on the other ticket.

Looking forward to it!

@jonataslaw
Copy link
Owner

Added on branch dev 1.19.0-dev.

kamazoun added a commit to kamazoundevz/getx that referenced this issue Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants