Skip to content

Conversation

@kaboc
Copy link
Contributor

@kaboc kaboc commented Oct 3, 2021

Resolves #91178.

ValueListenableBuilder was lacking a way to evaluate the value before a rebuild. The new filter property resolves it by allowing to check the previous/new values to decide whether to trigger a rebuild.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 3, 2021
@google-cla google-cla bot added the cla: yes label Oct 3, 2021
@kaboc kaboc force-pushed the valuelistenablebuilder branch from d788838 to d4bb6ba Compare October 3, 2021 21:19
if (filter == null || filter(value, widget.valueListenable.value)) {
setState(() {});
}
value = widget.valueListenable.value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modifying the state of a widget outside of setState is prone to hard to debug bugs. In this case it is very much possible that the builder will be called with a value that the filter filtered out. For example, when a build is triggered by an updated dependency or when this widget is moved to a different part of the tree. That will be confusing and surprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@goderbauer
Copy link
Member

Some checks appear to be failing. Can you take a look at those?

@kaboc
Copy link
Contributor Author

kaboc commented Oct 14, 2021

Done!
Thank you for making me realise that the failures were due my mistake. I was thinking they had been caused by some infrastructure issues, not knowing such a tiny error could affect that much.

@kaboc kaboc force-pushed the valuelistenablebuilder branch from 1fba41f to db75e41 Compare October 14, 2021 10:03
@goderbauer
Copy link
Member

I don't think we should add this to the framework. It's kinda confusing. The first time the widget is build, it just builds with whatever the value is completely ignoring the filter. Even if we would apply the filter to the initial value - what would we build with if the value is filtered out.

If you need this behavior, I would suggest creating a FliteringValueListenable, which takes in a ValueListenable and only notifies its own listeners if it passes the filter.

@goderbauer goderbauer closed this Oct 14, 2021
@kaboc
Copy link
Contributor Author

kaboc commented Oct 15, 2021

@goderbauer
The behaviour of the filter not being applied to the initial value is described in the document to be less confusing. If it is insufficient, I think just changing the parameter name from filter to rebuildFilter will remove the confusion completely and resolve the concern. What do you think about it?

If a ValueListenableBuilder has to be used with another widget like a FilteringValueListenable for filtering, it adds the new issue of discoverability where developers need a chance to learn the fact that the combination is possible, while the rebuildFilter seems more obvious.

I only have a vague image of the FilteringValueLitenable you mentioned, but assuming that it is something like below, it looks like it's very likely to add even more issues: verbosity, worse readability, deeper indentation, etc.

FilteringValueListenable(
  valueListenable: listenable,
  filter: (oldValue, newValue) { ... },
  notifier: (context, newListenable) {
    return ValueListenableBuilder(
      valueListenable: newListenable,
      builder: (context, value, _) {
        ...
      },
    );
  },
  placeholder: const SizedBox.shrink(),  // a widget used while no value is notified yet
)

@kaboc
Copy link
Contributor Author

kaboc commented Oct 26, 2021

Hey @goderbauer, can you just have a look at my idea above and give your opinion please? Renaming the parameter eliminates the confusion you mentioned, and so I believe there's nothing preventing the feature from being merged any more.

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

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Proposal] Add filter to ValueListenableBuilder for controlling rebuilds

2 participants