Added widgets/AppModel#92297
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
flutter-dashboard
bot
added
the
framework
|
Thanks |
|
The Record/tuple proposal could help here |
|
@cedvdb - could you expand a little on what you mean by #92297 (comment) |
|
@HansMuller I meant this dart-lang specification https://github.com/dart-lang/language/blob/master/working/0546-patterns/records-feature-specification.md but looking at the commit again it does not necessarily applies here. |
flutter-dashboard
bot
added
d: api docs
craiglabenz
left a comment
craiglabenz
left a comment
There was a problem hiding this comment.
This feature seems well designed, and is definitely a common use-case for Flutter developers (as evidenced by the popularity of Provider, which, as far as I can tell, is the closest analog that already exists). I'll have to play with this a bit myself before I have more sophisticated thoughts, but at a glance, this seems to add directly to the framework some/most/all (not sure which of these yet) is currently available in Provider.
Which I suppose makes my first question - in a framework looking to shed responsibilities, do we want to absorb functionality currently well-implemented and understood (by the community) by a third-party package?
|
@craiglabenz - AppModel is not intended to be a substitute for Provider or any of the other general purpose application state systems. AppModel is for situations where a package's custom widgets need to share one or a handful of immutable data objects, and where those objects can be lazily initialized. It exists so that packages like that can deliver custom widgets without requiring the developer to add an umbrella widget to their application. |
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
How does this shake out with state restoration? Does the AppModel need to serialize out the stored data somehow? Or is it reasonable to assume that the entities putting values into the AppModel are responsible for putting them there again on restore?
|
How would we remove/dispose a key? Also what concrete use-case is it designed for? The issue mentions a problem, but in which situation is this problem encountered exactly? |
|
About dispose, could we add a tear-off logic? As in, a callback that is executed when all consumers are removed. |
There was a problem hiding this comment.
What about keys that are removed/added?
There was a problem hiding this comment.
Actually since we can't remove keys atm this probably wouldn't be an issue I think
There was a problem hiding this comment.
Won't this try to notify all consumers when a key changes, even when the key they are listening too didn't change?
This would make notification really expensive if there's a large number of keys
There was a problem hiding this comment.
Only the consumers that depend on a key (because they used that key to lookup a value) are rebuilt.
There was a problem hiding this comment.
Only the consumers where the value changed are indeed rebuilt.
But as far as I understand, the rebuild trigger would involve calling == on all values/keys.
In a Riverpod application, which solves a similar problem (no umbrella widget), the application could have thousands of such keys. Including values/keys which override == to have a deep collection comparison.
There was a problem hiding this comment.
For comparison, Riverpod's notification mechanism will only call == on the changed key.
And it won't need to call == on values at all. So whether there's only one key or thousands of keys, the notification process takes the same amount of time.
Only adding consumers increase the time it takes to notify listeners
There was a problem hiding this comment.
Why do you think that rebuilding an AppModel would cause calling == on all values/keys?
There was a problem hiding this comment.
It creates a new Map, which will call updateShouldNotify followed by updateShouldNotifyDependents, which iterates over all the aspects that a consumer depends on
So my understanding is that this would end up calling == on all keys/values with active listeners (possibly multiple times if a key has multiple consumers)
There was a problem hiding this comment.
It's true that calling setValue() once will cause operator== on the value twice: once at the call site and once in updateShouldNotifyDependents. Since setValue() could be called more than once before dependent widgets are rebuilt, the second == check covers the possibility that the new value is no longer different than the original one. It's also true that when multiple widgets depend on the same value, the == check is repeated unnecessarily. If these proves to be a problem in practice we could complicate the implementation a little.
|
What if we want to optionally locally override a key? As in, there's no umbrella widget, but we optionally allow specifying such widget to change the behavior of one (but not all) key for a specific screen Which means the key that is overridden is only for a subtree. But those that aren't use the ancestor's key value |
|
Considering how the API seem to be focused on immutable objects and how expensive the notification process would be if there's a large number of keys, does exposing a "setValue" really make sense? If we fused getValue/setValue/initValue into a "putIfAbsent" api, we would never have to notify listeners, while still allowing to implement a global immutable value |
|
@goderbauer per #92297 (comment) Although I was disappointed to add a required initialize callback to AppModel.get(), it eliminates the need for AppModel.init() and, as you pointed out, is less likely to lead to a use-before-init error. Since AppModel is likely to be wrapped up in a package or app-specific helpers anyway, this doesn't seem like too much of a compromise. |
|
@goderbauer per #92297 (review) I've updated the API docs to emphasize the fact that AppModel values are expected to be immutable. State restoration of initial values will not require additional support. State restoration of values that have been updated with AppModel.set() are the app's responsibility. |
There's no support for that although we could certainly add it.
The goal is to make it possible for a package to deliver custom widgets that depend on a shared value without requiring the developer to include an application scoped "umbrella" widget. The setValue method exists because the shared value might need to be updated; it was not included for a concrete use case.
InheritedModel supports that and we could extend the support to AppModel. It seemed simpler to explain the semantics of AppModel without including this feature for now. |
| /// or a handful of immutable data objects that can be lazily | ||
| /// initialized. It exists so that packages like that can deliver | ||
| /// custom widgets without requiring the developer to add a | ||
| /// package-specific umbrella widget to their application. |
There was a problem hiding this comment.
nit: the sentence about the umbrella widget appears to be a repetition of the last sentence in the previous paragraph.
| /// an immutable value because intrinsic changes to the value will | ||
| /// not cause dependent widgets to be rebuilt. | ||
| /// | ||
| /// A Widget that depends on the app model's value for `key` should use |
This reverts commit 389a12f.
8 participants
Enables sharing key/value data with the all of the widgets below a child. Values must be immutable.
AppModel.setValue(context, key, value)changes the value of an entry in the shared data table and forces widgets that depend on that entry to be rebuilt.AppModel.getValue(context, key, initCallback)creates a dependency on the key and returns the value for the key from the shared data table. If not value was defined for the key then the init callback is used to create one.A widget whose build method uses AppModel.get(context, keyword, init) creates a dependency on the AppModel: when the value of keyword changes with AppModel.set(), the widget will be rebuilt.
An instance of this widget is created automatically by
WidgetsApp.AppModel is not intended to be a substitute for Provider or any of the other general purpose application state systems. AppModel is for situations where a package's custom widgets need to share one or a handful of immutable data objects, and where those objects can be lazily initialized. It exists so that packages like that can deliver custom widgets without requiring the developer to add an umbrella widget to their application.