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

docs: BlocConsumer distinction between single time events and screen related events #4248

Open
bobekos opened this issue Sep 13, 2024 · 4 comments
Assignees
Labels
needs triage This issue requires triage question Further information is requested

Comments

@bobekos
Copy link

bobekos commented Sep 13, 2024

Description

We often use the BlocConsumer or BlocListener to deliver single-based events to the UI (such as: showing snackbar or dialog). On the BlocConsumer we uses the listenWhen and buildWhen methods to differentiate between such events. In the following example I show a Cubit and the corresponding implementation that we normally use:

//State class
abstract class NewsState {}

//screen based states
sealed class NewsScreenState extends NewsState {}

class NewsScreenLoadingState extends NewsScreenState {}

class NewsScreenDataState extends NewsScreenState {
  final List<String> items;

  NewsScreenDataState(this.items);
}

//single event based states
sealed class NewsEventState extends NewsState {}

class NewsEventLoadingError extends NewsEventState {}

class NewsEventShowInfo extends NewsEventState {
  final String info;

  NewsEventShowInfo(this.info);
}
//Cubit class

class NewsCubit extends Cubit<NewsState> {
  NewsCubit(this._repo) : super(NewsScreenLoadingState()) {
    _loadData();
  }

  final NewsRepository _repo;

  void _loadData() async {
    final data =  await _repo.getNews();
    emit(NewsScreenDataState(data));
  }

  void showInfo(String id) async {
    final data = await _repo.getInfo();

    if (data.isSuccess) {
      emit(NewsEventShowInfo('some data'));
    } else {
      emit(NewsEventLoadingError());
    }
  }
}
//Widget implementation
class NewsWidget extends StatelessWidget {
  const NewsWidget({super.key});

  @override
  Widget build(BuildContext context) {
    return BlocConsumer<NewsCubit, NewsState>(
      listenWhen: (previous, current) => current is NewsEventState,
      buildWhen: (previous, current) => current is NewsScreenState,
      listener: (context, state) => switch (state as NewsEventState) {
        NewsEventLoadingError() => context.showSnackbar('something went wrong'),
        NewsEventShowInfo(info: final info) => showDialog(
            context: context,
            builder: (context) => AlertDialog(title: Text(info)),
          ),
      },
      builder: (context, state) => switch (state as NewsScreenState) {
        NewsScreenLoadingState() => const Center(
            child: CircularProgressIndicator(),
          ),
        NewsScreenDataState(items: final items) => ListView.builder(
            itemBuilder: (context, index) => ElevatedButton(
              onPressed: () => context.read<NewsCubit>().showInfo(items[index]),
              child: Text(
                '$index click me!',
              ),
            ),
            itemCount: items.length,
          ),
      },
    );
  }
}

This works fine when the bloc/cubit is responsible for the current emitted state. But if the user clicks the item and for example the "NewsEventShowInfo" state is emitted and then flutter decide to rebuild the widget we got an error because the "buildWhen" method is ignored and the switch expression of the builder method fails because the last emitted state "NewsEventShowInfo" is not a instance of the sealed NewsScreenState class.

What would be the correct procedure here? The best approach would be if the BlocConsumer implementation would emit (on rebuild) not the last emitted state but the last emitted state which passed the buildWhen method.

@bobekos bobekos added the documentation Documentation requested label Sep 13, 2024
@felangel
Copy link
Owner

felangel commented Oct 4, 2024

Hi @bobekos 👋
Thanks for opening an issue!

Are you able to share a link to a complete, minimal reproduction sample that illustrates the problem you're facing? I'm not sure I fully understand the problem since the error SnackBar is shown as part of the listener, not the builder and it should only ever be called on state changes (independent of rebuilds). Thanks!

@felangel felangel added question Further information is requested waiting for response Waiting for follow up needs repro info The issue is missing a reproduction sample and/or steps and removed documentation Documentation requested labels Oct 4, 2024
@felangel felangel self-assigned this Oct 4, 2024
@bobekos
Copy link
Author

bobekos commented Oct 5, 2024

Hi @felangel glad you found time to answer me. I prepare a full example of exactly what i mean and my view of things.

Here is a "full" example to reproduce what i mean (i would suggest to run this on web, mac or windows to better trigger the rebuild):

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

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

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        body: BlocProvider(
          create: (context) => NewsCubit(),
          child: LayoutBuilder(
            builder: (context, constraints) {
              if (constraints.maxWidth >= 600) {
                return Row(
                  children: [
                    const Expanded(
                      child: NewsWidget(),
                    ),
                    Expanded(
                      child: Container(
                        color: Colors.red,
                      ),
                    ),
                  ],
                );
              } else {
                return const NewsWidget();
              }
            },
          ),
        ),
      ),
    );
  }
}

//State class
abstract class NewsState {}

//screen based states
sealed class NewsScreenState extends NewsState {}

class NewsScreenLoadingState extends NewsScreenState {}

class NewsScreenDataState extends NewsScreenState {
  final List<String> items;

  NewsScreenDataState(this.items);
}

//single event based states
sealed class NewsEventState extends NewsState {}

class NewsEventLoadingError extends NewsEventState {}

class NewsEventShowInfo extends NewsEventState {
  final String info;

  NewsEventShowInfo(this.info);
}

//Cubit class
class NewsCubit extends Cubit<NewsState> {
  NewsCubit() : super(NewsScreenLoadingState()) {
    _loadData();
  }

  void _loadData() async {
    //mock data fetch
    await Future.delayed(const Duration(seconds: 1));
    emit(NewsScreenDataState(['1', '2', '3']));
  }

  void showInfo(String id) {
    //mock of something to check if it success
    const isSuccess = false;

    if (isSuccess) {
      emit(NewsEventShowInfo('some data'));
    } else {
      emit(NewsEventLoadingError());
    }
  }
}

//Widget implementation
class NewsWidget extends StatelessWidget {
  const NewsWidget({super.key});

  @override
  Widget build(BuildContext context) {
    return BlocConsumer<NewsCubit, NewsState>(
      listenWhen: (previous, current) => current is NewsEventState,
      buildWhen: (previous, current) => current is NewsScreenState,
      listener: (context, state) => switch (state as NewsEventState) {
        NewsEventLoadingError() => ScaffoldMessenger.of(context).showSnackBar(
            const SnackBar(
              content: Text('something went wrong'),
            ),
          ),
        NewsEventShowInfo(info: final info) => showDialog(
            context: context,
            builder: (context) => AlertDialog(title: Text(info)),
          ),
      },
      builder: (context, state) => switch (state as NewsScreenState) {
        NewsScreenLoadingState() => const Center(
            child: CircularProgressIndicator(),
          ),
        NewsScreenDataState(items: final items) => ListView.builder(
            itemBuilder: (context, index) => ElevatedButton(
              onPressed: () => context.read<NewsCubit>().showInfo(items[index]),
              child: Text(
                '$index click me!',
              ),
            ),
            itemCount: items.length,
          ),
      },
    );
  }
}

If you now start this example and resize the window back and forth in width, everything works. But if you click on a button and then trigger the rebuild (by modifying the width), the cast in the builder method of the BlocConsumer widget fails.

And of course I know what happens here and that it is not an error of bloc in that particular sense. The last known state is of the type NewsEventState. So this state is passed to the BlocConsumer and since it has no other state, it must of course be able to render something, ignores the "buildWhen" method and the cast fails.

My solution now, I have changed the structure and such “One Time Events” (like NewsEventState in the example) are now emitted to the UI independently of the main “bloc stream”. For this I use the package bloc_presentation which provides an additional stream for such type of events.

But I would like to know how you would approach such a problem and what the structure of your state class would look like. I'm not too happy with the options I've come up with like:

  • Keep the instance of the NewsScreenState in the cubit and after each NewsEventState just emit the last "screenState" after it. (this is just dirty)
  • New Cubit only for the NewsEventState class and handle it over the BlocListener Widget (yeah this would work but i like the idea behind the Consumer Widget where you can keep the listener and builder together and of course you don't need extra classes)
  • Change the structure of the StateClasses and move the "events" into the screen classes to have always the last known screen state (I don't know, I like the separation between Screen and Events State. I would mix it up unnecessarily that way)
  • Something like this (where the baseCubit/Bloc is not storing the states which are emitted over the "emitEvent" method)
abstract class BaseCubit<State> extends Cubit<State> {
  State _defaultState;

  BaseCubit(this._defaultState) : super(_defaultState);

  @override
  State get state => _defaultState;

  @override
  void emit(State state) {
    _defaultState = state;
    super.emit(_defaultState);
  }

  void emitEvent(State state) {
    super.emit(state);
  }
}

In my opinion also not a satisfying solution and it breaks the idea behind the bloc implementation and the "state" keeping. On the other hand it solves the problem with the one time events.

I would like to know how you would handle such things like this?

@xoliq0v
Copy link

xoliq0v commented Nov 17, 2024

Hi! 👋

Thanks for highlighting this issue. I see how this can lead to errors when the buildWhen condition is ignored on a rebuild, especially if the last emitted state is an instance of NewsEventState, which isn’t intended for building the UI.

Suggested Workaround:

Until a more robust solution is implemented, you could consider an approach like maintaining two different streams or creating a “placeholder” screen state after each event is emitted. Here’s an idea to ensure that the last screen-based state persists, even after an event-based state:

void showInfo(String id) async {
  final data = await _repo.getInfo();

  if (data.isSuccess) {
    emit(NewsEventShowInfo('some data'));
    emit(lastScreenState); // Re-emit the last screen-based state
  } else {
    emit(NewsEventLoadingError());
    emit(lastScreenState); // Re-emit the last screen-based state
  }
}

In this case, lastScreenState would hold the most recent screen state, ensuring that when an event state occurs, it won’t interfere with the builder logic upon a rebuild.

Proposed Enhancement:

It would indeed be helpful if BlocConsumer could track the last emitted state that passed the buildWhen condition and only rebuild based on that state. This would allow BlocConsumer to separate the responsibilities of listener and builder more effectively, avoiding this kind of rebuild conflict.

Would be glad to assist with an implementation or further discussion if needed. Thanks again for bringing this up!

@felangel felangel added needs triage This issue requires triage and removed needs repro info The issue is missing a reproduction sample and/or steps waiting for response Waiting for follow up labels Nov 19, 2024
@bobekos
Copy link
Author

bobekos commented Nov 20, 2024

Hi @xoliq0v

thank you for your response. I mentioned the same workaround that you suggested in the first point of my comment above. But like you, I don't like this solution at all. Firstly, you have to think about it and secondly, this implementation leads to unnecessary rebuilds of the widget.

I'm glad that you see it the way I do. If there is still a need here, I would be happy to help as far as my expertise allows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue requires triage question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants