-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add filter to ValueListenableBuilder #91179
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
Conversation
d788838 to
d4bb6ba
Compare
| if (filter == null || filter(value, widget.valueListenable.value)) { | ||
| setState(() {}); | ||
| } | ||
| value = widget.valueListenable.value; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
|
Some checks appear to be failing. Can you take a look at those? |
|
Done! |
1fba41f to
db75e41
Compare
|
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 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 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
) |
|
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. |
Resolves #91178.
ValueListenableBuilderwas lacking a way to evaluate the value before a rebuild. The newfilterproperty resolves it by allowing to check the previous/new values to decide whether to trigger a rebuild.Pre-launch Checklist
///).