jb3rndt / PersistentBottomNavBarV2

A highly customizable persistent bottom navigation bar for Flutter
https://pub.dev/packages/persistent_bottom_nav_bar_v2
BSD 3-Clause "New" or "Revised" License
47 stars 48 forks source link

[Bug]: navigation to initialRoute #146

Closed lucasfcardozo closed 2 months ago

lucasfcardozo commented 2 months ago

Version

5.2.2

What platforms are you seeing the problem on?

Android, iOS

What happened?

When we start the 'PersistentTabView', each tab starts with a "/" route, even setting a 'NavigatorConfig.initialRoute'. This is not good, because all tabs start with the "/" route which is not right. Even if it were, in my example, the Home screen already appears with the back button, indicating that there was a navigation to a second page. The proof of this is the 'navigatorObservers' logs.

Omitting the 'initialRoute', it seems to work, but the route on the first screen is as "/"

Additional info: Flutter 3.16.5 • channel stable • https://github.com/flutter/flutter.git Framework • revision 78666c8dc5 (4 months ago) • 2023-12-19 16:14:14 -0800 Engine • revision 3f3e560236 Tools • Dart 3.2.3 • DevTools 2.28.4

Steps to reproduce

  1. Start the app. You will see the first logs.
  2. Click on each tab.

Code to reproduce the problem

import 'package:flutter/material.dart';
import 'package:persistent_bottom_nav_bar_v2/persistent_bottom_nav_bar_v2.dart';

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

class MyApp extends StatelessWidget {
  const MyApp({super.key});
  @override
  Widget build(BuildContext context) => MaterialApp(
    title: 'Flutter Demo',
    debugShowCheckedModeBanner: false,
    routes: {
      "/home": (context) => const ScreenPage(title: "Home", contentMessage: "Home"),
      "/home/second": (context) => const ScreenPage(title: "Home 2nd", contentMessage: "Home 2nd"),

      "/messages": (context) => const ScreenPage(title: "Messages", contentMessage: "Messages"),
      "/messages/second": (context) => const ScreenPage(title: "Messages 2nd", contentMessage: "Messages 2nd"),

      "/settings": (context) => const ScreenPage(title: "Settings", contentMessage: "Settings"),
      "/settings/second": (context) => const ScreenPage(title: "Settings 2nd", contentMessage: "Settings 2nd"),
    },
    home: const MyHomePage(),
  );
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key});

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) => PopScope(
    canPop: false,
    child: PersistentTabView(
      navBarHeight: 48,
      tabs: [
        PersistentTabConfig(
          screen: ScreenPage(key: GlobalKey(), title: "Home", contentMessage: "Home 1"),
          item: ItemConfig(
            icon: Icon(Icons.home),
            title: "Home",
          ),

          navigatorConfig: NavigatorConfig(
            initialRoute: "/home",
            navigatorObservers: [AnalyticsNavigatorObserver()]
          ),
        ),
        PersistentTabConfig(
          screen: ScreenPage(key: GlobalKey(), title: "Messages", contentMessage: "Messages"),
          item: ItemConfig(
            icon: Icon(Icons.message),
            title: "Messages",
          ),
          navigatorConfig: NavigatorConfig(
            initialRoute: "/messages",
            navigatorObservers: [AnalyticsNavigatorObserver()]
          ),
        ),
        PersistentTabConfig(
          screen: ScreenPage(key: GlobalKey(), title: "Settings", contentMessage: "Settings"),
          item: ItemConfig(
            icon: Icon(Icons.settings),
            title: "Settings",
          ),

          navigatorConfig: NavigatorConfig(
            initialRoute: "/settings",
            navigatorObservers: [AnalyticsNavigatorObserver()]
          ),
        ),
      ],
      handleAndroidBackButtonPress: true,
      popActionScreens: PopActionScreensType.all,
      backgroundColor: Theme.of(context).colorScheme.background,
      navBarBuilder: (config) => Style2BottomNavBar(navBarConfig: config),
    ),
  );
}

class ScreenPage extends StatelessWidget {
  final String title;
  final String contentMessage;

  const ScreenPage({super.key, required this.title, required this.contentMessage});

  @override
  Widget build(BuildContext context) => Scaffold(
    appBar: AppBar(
      title: Text(title),
    ),
    body: Container(
      width: double.infinity,
      child: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        crossAxisAlignment: CrossAxisAlignment.center,
        children: [
          Text(contentMessage),
          if (!title.endsWith("2nd"))
            TextButton(
              onPressed: () {
                Navigator.of(context).pushNamed("/${title.toLowerCase()}/second");
              },
              child: Text("Go to 2nd screen"),
            )
        ],
      ),
    ),
  );
}

class AnalyticsNavigatorObserver extends NavigatorObserver {

  @override
  void didPush(Route<dynamic> route, Route<dynamic>? previousRoute) {
    print("didPush");
    print("new route: ${route.settings.name}");
    print("previous route: ${previousRoute?.settings.name}");
    print("------------------------------------------------");
  }

  @override
  void didPop(Route<dynamic> route, Route<dynamic>? previousRoute) {
    print("didPop");
    print("new route: ${route.settings.name}");
    print("previous route: ${previousRoute?.settings.name}");
    print("------------------------------------------------");
  }

  @override
  void didRemove(Route<dynamic> route, Route<dynamic>? previousRoute) {
    print("didRemove");
    print("new route: ${route.settings.name}");
    print("previous route: ${previousRoute?.settings.name}");
    print("------------------------------------------------");
  }

  @override
  void didReplace({Route<dynamic>? newRoute, Route<dynamic>? oldRoute}) {
    print("didReplace");
    print("new route: ${newRoute?.settings.name}");
    print("previous route: ${oldRoute?.settings.name}");
    print("------------------------------------------------");
  }
}

Relevant log output

Launching lib\main.dart on sdk gphone64 x86 64 in debug mode...
√  Built build\app\outputs\flutter-apk\app-debug.apk.
Connecting to VM Service at ws://127.0.0.1:59309/V6jizf7HFH0=/ws
I/flutter (15273): didPush
I/flutter (15273): new route: /
I/flutter (15273): previous route: null
I/flutter (15273): ------------------------------------------------
I/flutter (15273): didPush
I/flutter (15273): new route: /home
I/flutter (15273): previous route: /
I/flutter (15273): ------------------------------------------------

if you click on "messenges" tab
I/flutter (15273): didPush
I/flutter (15273): new route: /
I/flutter (15273): previous route: null
I/flutter (15273): ------------------------------------------------
I/flutter (15273): didPush
I/flutter (15273): new route: /messages
I/flutter (15273): previous route: /
I/flutter (15273): ------------------------------------------------

now go to settings tab
I/flutter (15273): didPush
I/flutter (15273): new route: /
I/flutter (15273): previous route: null
I/flutter (15273): ------------------------------------------------
I/flutter (15273): didPush
I/flutter (15273): new route: /settings
I/flutter (15273): previous route: /
I/flutter (15273): ------------------------------------------------

Screenshots

image

jb3rndt commented 2 months ago

Hi, if I understand correctly, you are wondering, why there is a back arrow in the app when you set e.g. /home as the initialRoute? In that case, this is by design by the flutter team because /home is a deeplink consisting of the two destinations / and home. Thus, the router will push a page for each route in the correct order to the navigation stack initially. You can read more about it here: https://api.flutter.dev/flutter/material/MaterialApp/initialRoute.html

lucasfcardozo commented 2 months ago

It just doesn't seem right to tab open 2 screens on the first "click of the tab". If you name initialRoute for the home tab, this will happen when you open the app whithout navigate. Since I'm required to pass a widget in the PersistentTabConfig.screen parameter, it shouldn't create a new page/route, as demonstrated in the AnalyticsNavigatorObserver logs, if I want to name this "screen" with a name other than "/".

PR #148 solved the problem for me. See if it's interesting.

jb3rndt commented 2 months ago

The PersistentTabConfig.screen parameter is treated as the screen for the route /, so it behaves like the home property of MaterialApp. That is why there are two pages. If you want the initial page to have a name other than /, try setting initialRoute to something that does not contain a / and including that route in the routes map.

Deviating from the convention does not sound like a good idea to me, since every part of the framework (and every developer) expect two routes when seeing /home or something like that. So changing that behavior will probably break many other parts. For example, in #148 you are removing the initialRoute from being passed to the navigator again, which would break the behavior that just got fixed according to #142.

lucasfcardozo commented 2 months ago

Yeah, but "Messages" tab and "Settings" tab algo are with "/" route name; That is the problem. I mean .... we have 3 different screens with same route name "/". And if you name it (using v5.2.2 / v5.2.3) you will have a navigation for each this tabs to the same screen.

In #148 I moved initialRoute to _onGenerateRoute method. With this, we have the first screen of each tab with a route name that I defined.

Maybe the best solution is creating a PersistentTabConfig factory that's screen isn't required. In this case, initialRoute will be used correctly, calling a route defined

lucasfcardozo commented 2 months ago

Yeah, but "Messages" tab and "Settings" tab algo are with "/" route name; That is the problem. I mean .... we have 3 different screens with same route name "/". And if you name it (using v5.2.2 / v5.2.3) you will have a navigation for each this tabs to the same screen.

In #148 I moved initialRoute to _onGenerateRoute method. With this, we have the first screen of each tab with a route name that I defined.

But I tried to understand your suggestion. You mean something like this:

PersistentTabConfig(
  screen: ScreenPage(key: globalKeys[0], title: "Home", contentMessage: "Home 1"),
  item: ItemConfig(
    icon: Icon(Icons.home),
    title: "Home",
  ),
  navigatorConfig: NavigatorConfig(
    initialRoute: "/home",
    routes: {
      "/home": (context) => const ScreenPage(title: "Home", contentMessage: "Home 1"),
    },
    navigatorObservers: [AnalyticsNavigatorObserver()]
  ),
  onSelectedTabPressWhenNoScreensPushed: () {
    (globalKeys[0].currentState as IScrollToTop).scrollToTop();
  },
)

If yes, i got same problem :

Connecting to VM Service at ws://127.0.0.1:56805/eXtyMeI0Lk4=/ws
I/flutter ( 9434): didPush
I/flutter ( 9434): new route: /
I/flutter ( 9434): previous route: null
I/flutter ( 9434): ------------------------------------------------
I/flutter ( 9434): didPush
I/flutter ( 9434): new route: /home
I/flutter ( 9434): previous route: /

But if you not saying this, sorry but I didn't get it 😔

Anyway, perhaps another solution is creating a PersistentTabConfig factory that's screen isn't required. In this case, initialRoute will be used correctly, calling a route defined in MaterialApp.

PR #148 works for me, mainly because I record the user's navigation. As it was, ghost browsing was being recorded.

By the way, thank you very much for trying to understand and help me. (my English in on in progress, so ... thanks for understanding)

jb3rndt commented 2 months ago

Hi, no worries, I try my best :) My suggestion is one of these options:

Option 1

PersistentTabConfig(
  // This is ignored in this option, so you could also just place an empty Container() here
  screen: ScreenPage(key: globalKeys[0], title: "Home", contentMessage: "Home 1"),
  item: ItemConfig(
    icon: Icon(Icons.home),
    title: "Home",
  ),
  navigatorConfig: NavigatorConfig(
    initialRoute: "home",
    routes: {
      "home": (context) => const ScreenPage(title: "Home", contentMessage: "Home 1"),
      "home/second": (context) => const ScreenPage(title: "Second", contentMessage: "Second Screen"),
    },
    navigatorObservers: [AnalyticsNavigatorObserver()]
  ),
  onSelectedTabPressWhenNoScreensPushed: () {
    (globalKeys[0].currentState as IScrollToTop).scrollToTop();
  },
)

Option 2

PersistentTabConfig(
  screen: ScreenPage(key: globalKeys[0], title: "Home", contentMessage: "Home 1"),
  item: ItemConfig(
    icon: Icon(Icons.home),
    title: "Home",
  ),
  navigatorConfig: NavigatorConfig(
    initialRoute: "/", // This means "home". This is the default value, so you could just remove that line if you want
    routes: {
      // This "/" route is redundant to the "screen" attribute above. This route will be ignored so it can be removed
      "/": (context) => const ScreenPage(title: "Home", contentMessage: "Home 1"),
      "/second": (context) => const ScreenPage(title: "Second", contentMessage: "Second Screen"),
    },
    navigatorObservers: [AnalyticsNavigatorObserver()]
  ),
  onSelectedTabPressWhenNoScreensPushed: () {
    (globalKeys[0].currentState as IScrollToTop).scrollToTop();
  },
)

I hope that helps :) I'm thinking if it would make sense to have the exact same behavior here as in the MaterialApp regarding the specification of MaterialApp.home and MaterialApp.initialRoute.

lucasfcardozo commented 2 months ago

I liked "Option 1" :D But in my case, I had to set the GlobalKey twice:

PersistentTabConfig(
  screen: ScreenPage(key: globalKeys[0], title: "Home", contentMessage: "Home 1"),
  item: ItemConfig(
    icon: Icon(Icons.home),
    title: "Home",
  ),
  navigatorConfig: NavigatorConfig(
    initialRoute: "home",
    routes: {
      "home": (context) => ScreenPage(key: globalKeys[0], title: "Home", contentMessage: "Home"),
    },
    navigatorObservers: [AnalyticsNavigatorObserver()]
  ),
  onSelectedTabPressWhenNoScreensPushed: () {
    (globalKeys[0].currentState as IScrollToTop).scrollToTop();
  },
),

Seeing this, it makes me wonder why the widget is set in the screen parameter. Will the idea I gave to create a new a PersistentTabConfig factory that's screen isn't required and initialRoute + routes will be used to show de 1st screen of a tab, is a good one? Just to get you thinking.

Well, thank you so much for this. It helped me a lot.