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

BlocBuilder with Optional Build Condition #315

Closed
felangel opened this issue May 24, 2019 · 10 comments

Comments

@felangel
Copy link
Owner

commented May 24, 2019

Is your feature request related to a problem? Please describe.
Proposal to address #174

Describe the solution you'd like

Proposing to add an optional condition property to BlocBuilder which takes the previousState and currentState as arguments and must return a bool which determines whether or not to rebuild.

Usage

@override
Widget build(BuildContext context) {
  final TimerBloc _timerBloc = BlocProvider.of<TimerBloc>(context);
  return Scaffold(
    appBar: AppBar(title: Text('Flutter Timer')),
    body: Column(
      children: <Widget>[
        BlocBuilder(
          bloc: _timerBloc,
          builder: (context, state) => TimerText(duration: state.duration),
        ),
        BlocBuilder(
          condition: (prevState, currState) =>
              currState.runtimeType != prevState.runtimeType,
          bloc: _timerBloc,
          builder: (context, state) => Actions(),
        ),
      ],
    ),
  );
}
import 'package:equatable/equatable.dart';
import 'package:meta/meta.dart';

@immutable
abstract class TimerState extends Equatable {
  final int duration;

  TimerState(this.duration, [List props = const []])
      : super([duration]..addAll(props));
}

class Ready extends TimerState {
  Ready(int duration) : super(duration);
}

class Paused extends TimerState {
  Paused(int duration) : super(duration);
}

class Running extends TimerState {
  Running(int duration) : super(duration);
}

class Finished extends TimerState {
  Finished() : super(0);
}

In the above example, the Text widget with the remaining time will be rebuilt every time a new state is received (every second) whereas the Actions widget will only be rebuilt if the state runtimeType changes (Ready -> Running, Running -> Paused, etc...).

ezgif com-optimize

Describe alternatives you've considered
Lots of alternatives described in #174.

@felangel felangel changed the title BlocBuilder with Condition BlocBuilder with Optional Build Condition May 24, 2019

@felangel felangel self-assigned this May 24, 2019

@felangel felangel added this to In progress in bloc May 24, 2019

@ThinkDigitalSoftware

This comment has been minimized.

Copy link

commented May 24, 2019

Yeah. Like the rebuild on change feature. Sure. This is also an alternative to putting a where condition on a stream to filter each event. I used to do this right in my StreamBuilder.

StreamBuilder(
              stream: myStream.where((transition) =>
              transition.oldState != transition.newState),
              builder: (BuildContext context, AsyncSnapshot snapshot) {
                return Container();
              },
            )

To build the where into the backend of the BlocBuilder widget and drop any conditions into it with a default function return true. I think that would be the simplest solution. I think the rebuildOnChange is a better name. or shouldRebuild

@jorgecoca

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

I think there's a few things to consider here before making a final decision:

  • You run into the case of having your previousState not being the same state rendered in the UI. I think it all depends on what point of view you are using: is the previousState the last state that a bloc emitted, or is it the last state that was rendered on the UI? Should we track also the last rendered state (that is, the last time the condition was true)?
  • This case seems to be only useful if you share a bloc in more than one widget, right?
  • This might be conflict a bit with the idea of having all your business logic contained in the bloc, since now BlocBuilders have extra power to decide what states will make it to the UI... I think this will have an affect on logs, since you might states emitted by bloc that won't make to the UI... should this be captured in logs, or not?

There could be other ways to solve this, like introducing just a BlocBuilderWithCondition, or maybe finding a different/better way of composing blocs, so rather than sharing, you just have a second bloc doing the filtering/condition check...

I'll try to think about it more over the weekend

@ThinkDigitalSoftware

This comment has been minimized.

Copy link

commented May 24, 2019

@jorgecoca previous state would always be last emitted. For tracking last rendered, the builder can implement onDidChangeDependencies as for logs, it should always log all bloc outputs. The builder just affects the subscriber and offers filtering capabilities that were otherwise unavailable unless you accessed the stream directly using a StreamBuilder. I personally wouldn't hold this feature back for any issues you mentioned.

@felangel

This comment has been minimized.

Copy link
Owner Author

commented May 25, 2019

@jorgecoca thanks for the feedback here's what I'm thinking:

  • imo previousState and currentState should always reflect the bloc state and not the UI state. I would prefer not to include a last rendered state unless there's a compelling use-case for it.
  • yeah it would mainly come in handy if you have multiple UI elements that respond to the same bloc state (like the timer or complex forms)
  • I personally don't consider the filtering of states business logic and would consider it to be presentation logic because it is just determining whether to re-render the UI. You cannot modify the state or have business logic in here. I think you make an interesting point regarding the logs but, similarly to the first point, I always think of the logs as reflecting the state of the blocs and not necessarily tied to any specific UI.

I considered a separate widget but I really like the fact that there's just a single widget used to render UI in response to state changes. I also agree that this can be accomplished using multiple blocs but it's not as intuitive and easy to manage imo. Definitely think about it over the weekend 👍

@axellebot

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

Seems good ! 👌
What will be the default condition ? I guess it will be :

condition: (prevState, currState) =>
              currState != prevState,

and this way we also avoid breaking changes, right ?

I guess it will be also implemented for the BlocListener ?

@jorgecoca

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

@felangel the suggestion sounds good to me. I think it makes sense; I just wanted to bring those points to our attention, so we do not oversee them.

I think there might be value, though, in tracking/logging when a BlocBuilder filters a state and does not render it in the UI.
That could avoid confusion in the long run, specially in large codebases where you might have to debug part of the code that you have not written, and you might need to debug/find an issue only with access to the logs. I think this idea would fall under the category of being predictable: in most of the cases, when a bloc emits a state, the expectation is that the emitted state will be rendered in the UI, so in those cases where that state might not be rendered, having a flag/message in the logs might save you some time and confusion.

What do you think?

PS. And now thinking about this, and given the history of issues that some other devs have opened in the past, I wonder if this kind of extra logging should be added when the bloc does not emit a state because currentState == previousState.

@jorgecoca

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

Also, in case we would want to leave this API for experimentation/trial period, we could use the experimental annotation from the meta package:

https://pub.dev/documentation/meta/latest/meta/experimental-constant.html

@ThinkDigitalSoftware

This comment has been minimized.

Copy link

commented May 26, 2019

@jorgecoca how would the bloc know? He would have to wire in a message back to it when the rebuild is ignored. If it's anything like the principle of simply notify listeners and let them do what they want with the information, where the sender is not concerned with what the reciever does with the information, then it's not the bloc's job to log that.

@felangel felangel referenced this issue May 26, 2019

Merged

Add optional condition to BlocBuilder #319

3 of 3 tasks complete
@felangel

This comment has been minimized.

Copy link
Owner Author

commented May 26, 2019

@axellebot the condition would default to always return true and it would continue to work as expected when no condition is provided.

condition: (previous, current) => true

The case you're describing is already handled by the bloc itself.

PR is open (#319)

@felangel

This comment has been minimized.

Copy link
Owner Author

commented May 28, 2019

This feature is now available in flutter_bloc v0.15.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.