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

Navigation Observer not being called in tabs router #450

Closed
rugemo opened this issue Apr 4, 2021 · 29 comments
Closed

Navigation Observer not being called in tabs router #450

rugemo opened this issue Apr 4, 2021 · 29 comments

Comments

@rugemo
Copy link

rugemo commented Apr 4, 2021

Hi,

I've been trying to observe routes using a NavigationObserver. It works fine for root level pages, but when setting usesTabsRouter: true it seems child elements are not observed. Is there a way to this, or a way to set another observer for the child routes? I've had a good look through the documentation but couldn't see anything obvious to make this happen.

Here are some screenshots showing the code. It's a basic set up, with some print statements in the NavigationObserver callbacks.

  1. MaterialAutoRouter configuration

Screenshot 2021-04-04 at 22 53 06

  1. Main build function and NavObserver class

Screenshot 2021-04-04 at 22 53 51

Screenshot 2021-04-04 at 22 54 06

  1. Class to handle bottom navigation

Screenshot 2021-04-04 at 22 54 45

Screenshot 2021-04-04 at 22 54 57

  1. Logs from NavObserver callbacks (emulator on the right)

As you can see there are callbacks to the nav observer (simple print statements) for the splash > launch > login > AuthenticatedNavigation routes. However there aren't any logs for routes within the 'AuthenticatedNavigationRoute'. See the second image below, navigated from favourites tab to contacts tab, but nothing registered in the observer.

Screenshot 2021-04-04 at 22 56 27

Screenshot 2021-04-04 at 22 57 04

Is there something specific we have to do to observer child routes?

Thanks for all your work on the library, and apologies if there is something obvious I am missing!

@Milad-Akarie
Copy link
Owner

@rugemo Because tabsRotuer does not use a navigator, the route it self does not have the same lifecycle as stacked routes, tab routes are pushed at the same time and only put in focus depending on the active index, if your tab is a nested navigator you could observe it.

@rugemo
Copy link
Author

rugemo commented Apr 5, 2021

@Milad-Akarie - Thanks for the explanation, I didn't realise the distinction between stacked routes and tab routes. I'll look into using a nested navigator.

@Milad-Akarie
Copy link
Owner

@rugemo If we're to mimic the life cycle of stacked routes inside of TabsRouter I guess we could use didPush when the tab first pushed then use didReplace for when a tab is switched.
I guess what I'm asking is how do you expect the logs to look like when observing a TabsRouter?

@rugemo
Copy link
Author

rugemo commented Apr 9, 2021

I reckon didReplace for the tabs register in a similar way to the logs we see now. So after this log

Did push route: _PageBasedMaterialPageRoute(MaterialPageX("AuthenticatedNavigationRoute", null, null), animation: AnimationController#2b7d0(▶ 0.000; for _PageBasedMaterialPageRoute(AuthenticatedNavigationRoute)))

We'd see the first tab registered:

Did replace new route: _PageBasedMaterialPageRoute(MaterialPageX("FavouritesRoute", null, null), animation: AnimationController#2b7d0(▶ 0.000; for _PageBasedMaterialPageRoute(AuthenticatedNavigationRoute)))

then switching tab to Contacts we'd see the same:

Did replace new route: _PageBasedMaterialPageRoute(MaterialPageX("ContactsRoute", null, null), animation: AnimationController#2b7d0(▶ 0.000; for _PageBasedMaterialPageRoute(AuthenticatedNavigationRoute)))

Then lets say contacts has a child page contact_details, we'd see

Did push route: _PageBasedMaterialPageRoute(MaterialPageX("ContactDetailsRoute", null, null), animation: AnimationController#2b7d0(▶ 0.000; for _PageBasedMaterialPageRoute(AuthenticatedNavigationRoute)))

@Milad-Akarie
Copy link
Owner

Milad-Akarie commented Apr 9, 2021

@rugemo @mrgnhnt96 on a second thought tab routes don't have an associated Route object, I guess we're forced to extend the native observer with our own like follows

abstract class TabsAwareNavigatorObserver extends NavigatorObserver {
  void didInitTabRoute(PageRouteInfo route) {}
  void didChangeTabRoute(PageRouteInfo route) {}
} 

it would be simple to add this to an existing observer for example FirebaseAnalyticsObserver.

abstract class TabsAwareFirebaseAnalyticsObserver extends FirebaseAnalyticsObserver implements  TabsAwareNavigatorObserver {
  void didInitTabRoute(PageRouteInfo route) {}
  void didChangeTabRoute(PageRouteInfo route) {}
} 

@mrgnhnt96 suggested that we should include indexes so it would look something like this

abstract class TabsAwareNavigatorObserver extends NavigatorObserver {
  void didInitTabRoute(PageRouteInfo route, int index) {}
  void didChangeTabRoute(PageRouteInfo route, int previousIndex , int index) {}
} 

@Milad-Akarie
Copy link
Owner

@rugemo @mrgnhnt96 sounds good?

@Milad-Akarie
Copy link
Owner

Milad-Akarie commented Apr 9, 2021

are indexes really important though? I mean you have the route name/path

how about just including a previous route like the native observer does

abstract class TabsAwareNavigatorObserver extends NavigatorObserver {
  void didInitTabRoute(PageRouteInfo route, PageRouteInfo? previousRoute) {}
  void didChangeTabRoute(PageRouteInfo route, PageRouteInfo previousRoute) {}
} 

@mrgnhnt96
Copy link

What if we created an AutoRouteObserver which would extend NavigatorObserver And TabObserver?
Thoughts behind this feature,
It would only be 1 observer that would need to be created and maintained, it would be a catch all. And it would also be specific to AutoRoute which would help understandability.
“to observe auto_routes, you can use AutoObserver

@Milad-Akarie
Copy link
Owner

Milad-Akarie commented Apr 9, 2021

TabsAwareNavigatorObserver extends NavigatorObserver so it should work both navigators, not sure if I know what you're getting at.
you're only going to need one observer for both router types.

@mrgnhnt96
Copy link

are indexes really important though? I mean you have the route name/path

how about just including a previous route like the native observer does

abstract class TabsAwareNavigatorObserver extends NavigatorObserver {
  void didInitTabRoute(PageRouteInfo route, PageRouteInfo? previousRoute) {}
  void didChangeTabRoute(PageRouteInfo route, PageRouteInfo previousRoute) {}
} 

It never hurts to keep a feature open, if you make it available, people will use it.
I’m sure it would be helpful for analytical tools as well, you know which tab is being favored.

just throwing this out there, what if we used Object instead of int? int would still be accepted, and if you wanted to specify which tab you were navigating you could also easily provide _tabs[index].
just realizing now that you need the index for the tab controller, so this might not work, it would be a cool feature though

@mrgnhnt96
Copy link

mrgnhnt96 commented Apr 9, 2021

TabsAwareNavigatorObserver extends NavigatorObserver so it should work both navigators, not sure if I know what you're getting at.
you're only going to need one observer for both router types.

So technically you could just use TabsAwareNavigatorObserver for both routes and tabs? The name doesn’t really fit the feature. If you have the ability to observe routes and tabs, use AutoObserver or something. It would make it more understandable

@Milad-Akarie
Copy link
Owner

using a TabRouteData object sounds like a good idea.
regarding the first point, you need to create a single observer to be used by all routers right?

class MyObserver extends TabsAwareNavigatorObserver{
 	
 	// implement tab router method
 	@override
  void didChangeTabRoute(TabRouteData route, TabRouteData previousRoute) {
    // TODO: implement didChangeTabRoute
  }
  
  // implement native navigator observer methods
  @override
  void didPush(Route route, Route? previousRoute) {
    // TODO: implement didPush
  }
  
  // implement other methods
}

@Milad-Akarie
Copy link
Owner

Milad-Akarie commented Apr 9, 2021

oh I can understand the confusion, but it's a NavigatorObserver that's aware of Tabs, FeatureAwareClass convention seems to be used alot in flutter so I'm not really sure

e.g this class [RouteAware] is used to make a widget aware of routes

@mrgnhnt96
Copy link

using a TabRouteData object sounds like a good idea.
regarding the first point, you need to create a single observer to be used by all routers right?

class MyObserver extends TabsAwareNavigatorObserver{
 	
 	// implement tab router method
 	@override
  void didChangeTabRoute(TabRouteData route, TabRouteData previousRoute) {
    // TODO: implement didChangeTabRoute
  }
  
  // implement native navigator observer methods
  @override
  void didPush(Route route, Route? previousRoute) {
    // TODO: implement didPush
  }
  
  // implement other methods
}

Yeah, this is perfect. The name is the only thing catching me haha

Totally get the FeatureAwareClass, I can just imagine people getting confused that they are using a “Tabs Aware” class to observe “Routes”. Flutter uses the FeatureAwareClass convention, they also have NavigatorObserver (Which extends RouteObserver Which extends RouteAware). To me, it would total sense to name it after auto_route, as that’s what you’re trying to do essentially. The AutoObserver, or even AutoAware, can extend tabs and routes, both are available, and neither are required, thus making it possible fit your apps needs specifically.

@mrgnhnt96
Copy link

Ah, I see your point too, it’s a Tabs Aware, NavigatorObserver, from the first look it sounds like it is only aware of “tab navigation” not “tabs AND navigation”

@Milad-Akarie
Copy link
Owner

Milad-Akarie commented Apr 9, 2021

so AutoRouteNavigatorObserver makes sense? or just AutoRouterObserver?

@mrgnhnt96
Copy link

so AutoRouteNavigatorObserver makes sense? or just AutoRouterObserver?

I would go with AutoRouterObserver. It makes it a little more vague, but they both work 👌

@amritk
Copy link

amritk commented Apr 9, 2021

@rugemo Because tabsRotuer does not use a navigator, the route it self does not have the same lifecycle as stacked routes, tab routes are pushed at the same time and only put in focus depending on the active index, if your tab is a nested navigator you could observe it.

Sorry to hijack, but how do you go about observing nested routes? I have added an observer like the code above which works on root routes but doesn't seem to work on nested routes.

MaterialApp.router(
   ...
  routerDelegate: widget.appRouter.delegate(
     initialRoutes: [Splash()],
     navigatorObservers: [AppRouteObserver()],
  ),
)
@MaterialAutoRouter(
  replaceInRouteName: 'Page,',
  routes: <AutoRoute>[
    AutoRoute(
      page: HomePage,
      maintainState: true,
      path: '/',
      children: <AutoRoute>[
        // Observer does not work on these
        AutoRoute(
          page: DashboardPage,
          maintainState: true,
          path: '',
        ),
        AutoRoute(
          page: ProfilePage,
          maintainState: true,
          path: 'profile',
        ),
      ],
    ),
    // Observer works on these
    AutoRoute(
      page: LoginPage,
      maintainState: false,
      path: '/login',
    ),
    AutoRoute(
      page: SplashPage,
      maintainState: false,
      path: '/splash',
    ),
  ],
)
class $AppRouter {}

@Milad-Akarie
Copy link
Owner

Milad-Akarie commented Apr 10, 2021

@amritk You'll need to pass an observer for each navigator, as I dug more into this we can't actually use the same observer for different navigators ( restricted by native Navigator) so in the next build navigatorObservers will be a create-method that returns a list of observers so that every child navigator can call it to create a copy of the provided observes.
but for now you can just pass your observer to the nested AutoRouter you're using inside of HomePage

@amritk
Copy link

amritk commented Apr 10, 2021

Hey @Milad-Akarie would you happen to know the syntax? I was poking around trying to figure it out but I could not.

@Milad-Akarie
Copy link
Owner

@amritk The syntax of what exactly?

@amritk
Copy link

amritk commented Apr 10, 2021

@Milad-Akarie passing the observer into the nested autorouter

@Milad-Akarie
Copy link
Owner

Please share your home page setup

@amritk
Copy link

amritk commented Apr 11, 2021

hey @Milad-Akarie sure thing:

import 'package:flutter/material.dart';

import 'package:auto_route/auto_route.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:flutter_svg/flutter_svg.dart';

import 'package:operto_member_portals/housekeeping/presentation/router/app_router.gr.dart';
import 'package:operto_member_portals/owner/logic/access/access_cubit.dart';
import 'package:operto_member_portals/owner/logic/activity/activity_cubit.dart';
import 'package:operto_member_portals/owner/logic/property/property_cubit.dart';
import 'package:operto_member_portals/owner/logic/reservation/reservation_cubit.dart';
import 'package:operto_member_portals/owner/presentation/style/theme.dart';
import 'package:operto_member_portals/owner/presentation/widgets/access_code_bottomsheet.dart';
import 'package:operto_member_portals/owner/presentation/widgets/home_drawer.dart';
import 'package:operto_member_portals/owner/presentation/widgets/loading.dart';
import 'package:operto_member_portals/shared/localization/app_localizations.dart';
import 'package:operto_member_portals/shared/logic/blocs/authentication/authentication_bloc.dart';
import 'package:operto_member_portals/shared/presentation/widgets/o_empty_state.dart';
import 'add_access.dart';

class HomePage extends StatefulWidget {
  @override
  _HomePageState createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  Future<dynamic> _openAddAccess(BuildContext context) {
    return showModalBottomSheet(
      isScrollControlled: true,
      context: context,
      builder: (context) => AddAccessPage(),
    );
  }

  @override
  Widget build(BuildContext context) {
    final _scaffoldKey = GlobalKey<ScaffoldState>();
    return MultiBlocProvider(
      providers: [
        BlocProvider(
          create: (BuildContext context) => AccessCubit(
            authenticationBloc: BlocProvider.of<AuthenticationBloc>(context),
          ),
        ),
        BlocProvider(
          create: (BuildContext context) => ActivityCubit(
            authenticationBloc: BlocProvider.of<AuthenticationBloc>(context),
          ),
        ),
        BlocProvider(
          create: (BuildContext context) => PropertyCubit(
            authenticationBloc: BlocProvider.of<AuthenticationBloc>(context),
          )..getProperties(),
        ),
        BlocProvider(
          create: (BuildContext context) => ReservationCubit(
            authenticationBloc: BlocProvider.of<AuthenticationBloc>(context),
          ),
        ),
      ],
      // Trigger streams once we have the propertyId
      child: BlocListener<PropertyCubit, PropertyState>(
        listener: (context, state) {
          // Use the standard error states on each tab, emit errors to each cubit
          if (state is PropertyError) {
            context.read<AccessCubit>().emit(AccessError(error: state.error));
            context
                .read<ActivityCubit>()
                .emit(ActivityError(error: state.error));
            context
                .read<ReservationCubit>()
                .emit(ReservationError(error: state.error));
          } else if (state is PropertyLoaded && state.selectedPropertyId != 0) {
            context
                .read<AccessCubit>()
                .startStream(propertyId: state.selectedPropertyId);
            context
                .read<ActivityCubit>()
                .startStream(propertyId: state.selectedPropertyId);
            context
                .read<ReservationCubit>()
                .startStream(propertyId: state.selectedPropertyId);
          } else {
            throw 'Something has gone terribly wrong';
          }
        },
        child: Builder(
          builder: (context) {
            final accessState = context.watch<AccessCubit>().state;
            final activityState = context.watch<ActivityCubit>().state;
            final propertyState = context.watch<PropertyCubit>().state;
            final reservationState = context.watch<ReservationCubit>().state;

            if (isLoading(
                accessState, activityState, propertyState, reservationState)) {
              return Loading();
            } else if (propertyState is PropertyError) {
              return OEmptyState(
                title: AppLocalizations.of(context).translate(
                  'error_status_code',
                  interpolations: {
                    'status': propertyState.error.status.toString()
                  },
                ),
                imagePath: 'assets/images/hk_error.svg',
                message: propertyState.error.message,
              );
            } else {
              return Scaffold(
                key: _scaffoldKey,
                appBar: AppBar(
                  elevation: 0,
                  leading: Builder(
                    builder: (BuildContext context) {
                      return IconButton(
                        icon: const Icon(Icons.menu),
                        onPressed: () {
                          Scaffold.of(context).openDrawer();
                        },
                        tooltip: MaterialLocalizations.of(context)
                            .openAppDrawerTooltip,
                      );
                    },
                  ),
                  title: SvgPicture.asset(
                    'assets/images/operto_white_official.svg',
                    width: 100,
                  ),
                  centerTitle: true,
                ),
                drawer: HomeDrawer(),
                body: AutoRouter(),
                floatingActionButton: FloatingActionButton(
                  backgroundColor: OPTheme.primary,
                  foregroundColor: Colors.white,
                  onPressed: () {
                    AccessCodeBottomSheet.showAccessCode(context);
                  },
                  child: Icon(
                    Icons.lock_outline,
                  ),
                ),
                floatingActionButtonLocation:
                    FloatingActionButtonLocation.endDocked,
                bottomNavigationBar: Builder(builder: (BuildContext context) {
                  return Container(
                    decoration: BoxDecoration(
                      border: Border(
                        top: BorderSide(color: Colors.grey.shade300, width: 0),
                      ),
                    ),
                    // Here we are faking a bottom navigation bar
                    child: SizedBox(
                      height: 64,
                      child: Row(
                        mainAxisAlignment: MainAxisAlignment.center,
                        children: [
                          SizedBox.fromSize(
                            size: Size(64, 64),
                            child: ClipOval(
                              child: Material(
                                child: InkWell(
                                  onTap: () {
                                    context.router
                                        .innerRouterOf<StackRouter>('Home')
                                        ?.push(Dashboard());
                                  },
                                  child: Column(
                                    mainAxisAlignment: MainAxisAlignment.center,
                                    children: <Widget>[
                                      Icon(
                                        Icons.home_outlined,
                                        color: homeColor(context),
                                      ),
                                      Text(
                                        AppLocalizations.of(context).translate(
                                          'home',
                                        ),
                                        style: TextStyle(
                                          color: homeColor(context),
                                        ),
                                      ),
                                    ],
                                  ),
                                ),
                              ),
                            ),
                          ),
                          SizedBox(width: 128),
                          SizedBox.fromSize(
                            size: Size(64, 64),
                            child: ClipOval(
                              child: Material(
                                child: InkWell(
                                  onTap: () {
                                    _openAddAccess(context);
                                  },
                                  child: Column(
                                    mainAxisAlignment: MainAxisAlignment.center,
                                    children: <Widget>[
                                      Icon(
                                        Icons.group_add_sharp,
                                        color: Colors.black54,
                                      ),
                                      Text(
                                        AppLocalizations.of(context).translate(
                                          'add_user',
                                        ),
                                        style: TextStyle(
                                          color: Colors.black54,
                                        ),
                                      ),
                                    ],
                                  ),
                                ),
                              ),
                            ),
                          ),
                        ],
                      ),
                    ),
                  );
                }),
              );
            }
          },
        ),
      ),
    );
  }
}

@Milad-Akarie
Copy link
Owner

@amritk I just wanted to make sure you get the idea of nesting scoped navigators.
all you gotta do is pass your observer to the inner router scope AutoRouter(navigatorObservers:[]),.
in the upcoming build navigatorObservers will be a callback function used to create the observers so observers will be inherited automatically. and by inherited I don't mean using the same instance because that is out of hand,
so just make sure you pass a fresh instance of your observer.

@amritk
Copy link

amritk commented Apr 12, 2021

@Milad-Akarie oooo that's where it goes. Thanks a lot and nice work!

@AAverin
Copy link

AAverin commented Feb 19, 2022

I've been playing with didChangeTabRoute and looks like it's not invoked neither when you click on the tab in the bottom navigation, nor when you push a new route that is supposed to show a new tab selected in the bottom navigation.

How can I get an event that tab is supposed to change? I need to make a custom implementation changing selected tab.

@FernandoAndrade83
Copy link

@Milad-Akarie, any updates about this?

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants