felangel / bloc

A predictable state management library that helps implement the BLoC design pattern
https://bloclibrary.dev
MIT License
11.79k stars 3.39k forks source link

pushNamed and new state #2938

Closed eliudio closed 2 years ago

eliudio commented 2 years ago

Hi

I have a situation where I have an event in my bloc which requires:

1) a new state to be set / yielded from the bloc 2) a new page to be pushed through navigation (https://api.flutter.dev/flutter/widgets/Navigator/pushNamed.html)

Today I push the new page then yield the new state. The result is that the new page is opened with the old state then gets a new state. I want these 2 steps combined in 1 so that the new page is opened with the new state. How can this be done?

Thanks

Eliud

narcodico commented 2 years ago

Hi @eliudio 👋

You can add the event, yield the new state and then use a BlocListener to check for that state and pushNamed your route.

Gene-Dana commented 2 years ago

hi ! Just for clarity, it looks something like this !

 @override
  Widget build(BuildContext context) {
    return BlocListener<CategorySelectCubit, CategorySelectState>(
      listener: (context, state) async {
        if (state.status == CategorySelectStatus.created) {
          unawaited(
            Navigator.of(context).push(
              PlayFlow.route(state.game),
            ),
          );
        }
eliudio commented 2 years ago

Thanks

I'm struggling a bit with this. I have implemented this as suggested. However, I seem to have repeated calls to the listener.

For simplifying the explain, let's say I have :

  1. a MaterialApp with onGenerateRoute referring to myOnRouteGenerate
  2. myOnRouteGenerate retrieves uses the arguments from the RouteSettings to return a Page widget
  3. a Bloc which evaluates a "GotoPageEvent" and yields a PageState with some pageId
  4. the widget "Page",
    • which shows the contents of a page with the pageId which it finds from the state, i.e. the widget retrieves this data from a firestore db with the pageId
    • has a next-page button which send a GotoPageEvent(currentPage + 1)
  5. the PageWidget has the BlocListener implemented and Navigator.of(context).pushNamed

When the next page is triggered: 1) the GotoPageEvent is added to the bloc 2) the bloc yields the PageState with the page ID from the GotoPageEvent 3) the bloclistener is triggered with PageState and navigates to a new page using Navigator.of(context).pushNamed(...) 4) the myOnRouteGenerate is triggered 5) the myOnRouteGenerate create a new Page widget and wraps it inside a FadeRoute 6) and then sadly the bloclistener is hit again for 3, 4, and 5 to repeat, a couple of times. It's obviously slow and the fading isn't there neither. Any idea?

eliudio commented 2 years ago

I've noticed that steps 3, 4 and 5 are triggered for all pages in the history.

A. Upon opening the app, the listener is triggered and a first pushNamed is run B. Pressing next page, the listener is run for page where pagestate = page 1, the pushNamed is run and the listener is called ones again for page where pagestate = page 2 C. Pressing next page, the same repeats but now for page 1, page 2 and page 3 D. and so on, the next time page 1, 2, 3 and 4 C...

eliudio commented 2 years ago

Hi,

Recap: the issue is that the listener is called multiple times.

To illustrate this issue in code form, as well as document the pattern, I've created this github repo: https://github.com/eliudio/bloc_nav

Code also copied below.

Put a breakpoint line 79 in main.dart (which is in the listener). Debug the app. Press the '+' button a few times. You will see

This just seems to exponentially increase. Obviously in this app, you don't notice it as there's not much going on in the widget. But in case there's some processing going on, obviously this problem becomes a huge problem.

Any thoughts?

import 'dart:async';

import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:equatable/equatable.dart';

/* Quick and dirty sample code to illustrate how to use navigation in combination of using the bloc pattern */
void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  final GlobalKey _appKey = GlobalKey();
  static GlobalKey<NavigatorState> navigatorKey = GlobalKey<NavigatorState>();
  final GlobalKey<ScaffoldMessengerState> rootScaffoldMessengerKey =
      GlobalKey<ScaffoldMessengerState>();

  MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return BlocProvider<AccessBloc>(
        create: (context) => AccessBloc(navigatorKey)..add(AccessInit(1)),
        child: MaterialApp(
          home: PageComponent()
        ));
  }
}

class PageComponent extends StatefulWidget {
  @override
  State<StatefulWidget> createState() {
    return _PageComponentState();
  }
}

class _PageComponentState extends State<PageComponent> {
  Widget? theBody;

  @override
  Widget build(BuildContext context) {
    return BlocListener(
      bloc: BlocProvider.of<AccessBloc>(context),
      child: BlocBuilder(
          bloc: BlocProvider.of<AccessBloc>(context),
          builder: (BuildContext context, accessState) {
            if (accessState is PageState) {
              return Scaffold(
                appBar: AppBar(
                  title: const Text('Navigation with flutter_bloc'),
                ),
                body: Center(
                  child: Column(
                    mainAxisAlignment: MainAxisAlignment.center,
                    children: <Widget>[
                      const Text(
                        'You have pushed the button this many times:',
                      ),
                      Text(
                        'Page: ' + accessState.pageId.toString(),
                        style: Theme.of(context).textTheme.headline4,
                      ),
                    ],
                  ),
                ),
                floatingActionButton: FloatingActionButton(
                  onPressed: () => BlocProvider.of<AccessBloc>(context).add(GotoPage(accessState.pageId + 1)),
                  tooltip: 'Next page',
                  child: const Icon(Icons.add),
                ),
              );

            } else {
              return const Text('Not PageState');
            }
          }),
      listener: (BuildContext context, accessState) {
        if (accessState is PageState) {
          // PUT BREAKPOINT HERE!
          Navigator.push(
            context,
            MaterialPageRoute(builder: (context) => PageComponent()),
          );
        }
      },
    );

  }
}

class AccessBloc extends Bloc<AccessEvent, AccessState> {
  StreamSubscription? _appSubscription;
  final GlobalKey<NavigatorState> navigatorKey;

  AccessBloc(this.navigatorKey) : super(UndeterminedState());

  @override
  Stream<AccessState> mapEventToState(AccessEvent event) async* {
    if (event is AccessInit) {
      yield PageState(event.initialPageId);
    } else if (event is GotoPage) {
      yield PageState(event.pageId);
    }
  }
}

abstract class AccessEvent extends Equatable {
  @override
  List<Object?> get props => [];
}

class AccessInit extends AccessEvent {
  final int initialPageId;

  AccessInit(this.initialPageId);

  @override
  List<Object?> get props => [ initialPageId ];
}

class GotoPage extends AccessEvent {
  final int pageId;

  GotoPage(this.pageId);

  @override
  List<Object?> get props => [ pageId ];
}

abstract class AccessState extends Equatable {
  const AccessState();

  @override
  List<Object?> get props => [];
}

class UndeterminedState extends AccessState {

}

class PageState extends AccessState {
  final int pageId;

  PageState(this.pageId);

  @override
  List<Object?> get props => [ pageId ];

  @override
  bool operator == (Object other) =>
      identical(this, other) ||
          other is PageState &&
              pageId == other.pageId;
}
eliudio commented 2 years ago

Also see https://gist.github.com/felangel/6bcd4be10c046ceb33eecfeb380135dd#gistcomment-3961544

Basically all examples show that basically navigation using BlocListener doesn't work properly.

Gene-Dana commented 2 years ago

May you share with us the repo of your example?

eliudio commented 2 years ago

Yes, I shared it earlier: https://github.com/eliudio/bloc_nav

And if you feel unconfortable with my example, I think ANY example of using BlocListener will illustrate this problem. But you need to use a slightly more real-world example than just open a page and go back. So I illustrated that with the existing OLD example https://gist.github.com/felangel/6bcd4be10c046ceb33eecfeb380135dd#gistcomment-3961544 which I slightly changed (see that link) to allow to navigate from A(1) > B(2) > A(3) > B(4) > A(5) > B(6). You will see that when the bloclistener is used in ANY navigation implementation than navigatiomn B(6) will trigger all the bloclisteners of widgets A(1), B(2), A(3), B(4), A(5) and A(6) to be triggered, not just B(6)

eliudio commented 2 years ago

@Gene-Dana, any luck?

chonghorizons commented 2 years ago

This is based on about 20 minutes of reading your code, so sorry if I'm not grokking it all. I think the issue is that you have the following nested structure (in this version ) which isn't illegal, but has a lot of debugging headaches:

BlocProvider for AccessState StatefulWidget BlocListener BlocBuilder

any change of the Bloc state may trigger a complete rebuilding of StatefulWidget. That will trigger a complete rebuild of the subtree. So, even when the bloc's state doesn't change, the BlocListener re-reruns because of the rebuild.

See my comments in https://github.com/felangel/bloc/issues/3008#issuecomment-993002127

I'm not sure what's triggering the StatefulWidget in this case, if anything. Or which subtrees are rebuilding. To troubleshoot rebuilding, my standard practice is to print print statements in the build and builders.

This is intended behavior and not a bug. You can maybe avoid it (in a fragile way) using same globalKey to avoid re-building.

You may completely avoid it by removing the StatefulWidget.


tag #BestPractices. #AvoidNestedStatefulWidgetsBecauseOfUnintendedRebuilds

chonghorizons commented 2 years ago

Update: I was mistaken... it isn't because of the nesting, not exactly.

It has to do with interaction of Navigator and the blocListeners.

I think you are intending for only the top-of-Navigator-stack listener to push a new page. But, in fact, every listener is pushing a new page. This is intended behavior. You might have a dashboard that wants to update for every page.

I added some debug print statements to make this clearer.

Screen Shot 2021-12-13 at 6 15 36 PM

TIP: pay close attention to the widget tree. And count the blocListeners.

You can clone my repo or look at the diff: https://github.com/chonghorizons/bloc_nav/commit/aaf43351ef74eacdf15f0d4119ff0ef3677710dc

I think you can put the Navigator.push in the bloc.

CURRENT WAY

button click bloc gets new page event bloc emits new page blocListener (every one) does a Navigator.push blocBuilder (every one) runs one or more times, and updates the number on each page in the stack.

tip: builders can run multiple times so shouldn't have side effects that can't run twice.

NEW WAY

button click bloc gets new page event bloc runs Navigator.push once. blocListener (every one) runs exactly once, but doesn't do anything. blocBuilder (every one) runs one or more times, and updates the number on each page in the stack.


Alternatively, you can use listenWhen to only run Navigator.push for the page that is the top of the stack. You need to then implement logic so that the PageState knows which one is on top. This could be fragile and error prone.

chonghorizons commented 2 years ago

If this addresses things (at least in terms of bloc), please consider closing the issue. If not, please clarify where I misunderstood your expected behavior vs actual behavior.

eliudio commented 2 years ago

@chonghorizons Thanks very much looking into this.

You make it sound as if I'm using bloc in a not illegal yet unusual unintended way. I disagree with that, but let's try not to have the conversation about that, rather about the core of the issue: using listener to navigate. Hence why I've also used an EXISTING example, which I slightly changed:

https://gist.github.com/felangel/6bcd4be10c046ceb33eecfeb380135dd#gistcomment-3961544

See my comments 13 Nov

I've changed it in a way that you explain doesn't work: I've changed the example to NOT ONLY navigate from the top level, but indeed I want to show that navigation isn't working, except from the first page / "top level".

If one claims that the listener can be used to navigate, and it's only usable from the first page in your app, not from any other page, than surely this should be part of the description. To me, you're basically saying: don't use listener to navigate, unless it's in that rare case where you want to use it from the first page in your app. And yeah, I have implemented my app to navigate from the bloc. It's dirty, and it's not good because I have a state change and navigation change that are now handled in 2 steps sadly. But that's how it is.

I can obviously close this case, but shouldn't the documentation not clearly state "DON'T USE THIS MECHANISM UNLESS THIS IS USED FROM THE FIRST PAGE"?

thanks

chonghorizons commented 2 years ago

I think I came off too strong for trying to give lots of details. Didn't mean to. If I came across that way, I apologize.

In Nov 13, you wrote

Recap: the issue is that the listener is called multiple times.

The issue isn't that "the (single) listener" is called multiple times. That would be a bug. Each individual listener is called exactly once. But here there are have multiple listeners. So listeners are called more than once. In other cases, rebuilds of widgets cause things to happen more than once.

If you move the listener so that there is only one, then you will be fine. The navigation will be called only once.

The second post said:

Hi @eliudio 👋

You can add the event, yield the new state and then use a BlocListener to check for that state and pushNamed your route.

And that can be done straightforwardly, like if you had different pages without linking/recursion. Like HomePage, DetailsPage, PreferencesPage.

Try this: Move your blocListener out of PageComponent and as the widget just under MaterialApp. There will be one blocListener. It will run once for each update.

These are tricky things, thinking about where to put the blocListener, the widget tree, and rebuilds. It's a good question. I know this because I've made mistakes like this too. (Not navigation, but things running multiple times).

felangel commented 2 years ago

Closing for now but feel free to comment with any additional questions and I'm happy to continue the conversation 👍

eliudio commented 2 years ago

It is what it is, right? Basically listener pattern can not be used to navigate, unless you use it in your app at the top level and navigate to a page, but that page can never navigate to any other page. That's the only conclusion I can make from the feedback. I've used your existing example (bottom of https://gist.github.com/felangel/6bcd4be10c046ceb33eecfeb380135dd), slightly changed the pattern to actually allow to navigate from page A -> page B -> page C, my points have not been addressed and we seem to be goin in circles about multiple listeners and stuff. Case close: don't use listener to navigate (unless an extreme simple navigation).

narcodico commented 2 years ago

BlocListeners are actually extremely useful for side effects like navigation. You don't necessarily have to place it on top level to be efficient with it. You could also have a look at flow_builder, it's great for controlling feature specific navigation.

felangel commented 2 years ago

@eliudio this is working as expected. When you push multiple pages which each have a BlocListener that is listening to the same bloc state all BlocListener instances will be listening because all pages are still mounted in the widget tree. BlocListener only stops listening when it is no longer mounted.

chonghorizons commented 2 years ago

BlocListeners are actually extremely useful for side effects like navigation. You don't necessarily have to place it on top level to be efficient with it. You could also have a look at flow_builder, it's great for controlling feature specific navigation.

@narcodico

I agree a lot. If I recall correctly, flow_builder only keeps one page mounted in the widget tree at once for the flow. And uses popAndPush to move between pages. So blocListener works well, because only one flow is mounted at once.


I sympathize with @eliudio 's frustration about navigation. There are many ways to do it. And it is a common gotcha for both new bloc users and flutter users.

Tackling navigation has a good understanding of the widget tree as a prerequisite.

There are some nuances about Navigator.push vs Navigator.popAndPush.


Some people have bemoaned why Google has made navigation so complex, especially with Navigator 2.0

My outsider opinion is that it IS complex. Because Google is trying to make the same flutter codebase work for iOS, Android, mobile web (WebView), desktop web, and native. Including with deep links, and interaction with (cached) state/cookies/etc.

narcodico commented 2 years ago

@chonghorizons flow builder keeps a stack of pages.

eliudio commented 2 years ago

@chonghorizons ... to be honest, I've completely given up on the idea of navigating from a widget and sadly instead I'm using navigation from the bloc. It's not ideal, but I can't spend time on this now. It works as it is, actually pretty good. Not sure why you suggest this has much to do with Google. Anyway... Maybe one day I'll have the time to dig deeper into this topic. For now, I've left it as it is.

I'm mostly frustrated in assuming the suggestion of using a BlocListener to be anything that could yield into something useful. It's not. It only works in a rare condition, not in a real-world app. At least not as far as I believe I've proven in my examples. The frustration is therefore more to do with the documentation, rather than the functionality.

chonghorizons commented 2 years ago

@chonghorizons flow builder keeps a stack of pages.

@narcodico

Are they all mounted to the widget tree? I think only one is mounted at a time.

Example

If the stack is [a,b,c]

Widget tree has a when flow starts, Then pop and push to get b, Then pop and push to get c.

But, it also overwrites the default back behavior, so hitting back when at page c does a pop and push b.

(I haven't used flow builder yet, but that is what I noticed when I evaluated it for my use case)

narcodico commented 2 years ago

@chonghorizons you control which pages are mounted with onGeneratePages

erlangparasu commented 1 year ago

@eliudio this is working as expected. When you push multiple pages which each have a BlocListener that is listening to the same bloc state all BlocListener instances will be listening because all pages are still mounted in the widget tree. BlocListener only stops listening when it is no longer mounted.

@felangel this is what i expect, but currently when i try using same bloc object and pass it to other page, then use it for BlocListener, the listener is not invoked after reopen the page.