Add filter to ValueListenableBuilder#91179
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.
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.
|
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
///).