-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
Expose InheritedWidget subscription cancelation #33213
Conversation
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.
This looks like a safe change to me. This is a sensitive part of the implementation; I'd like @Hixie to look at it as well.
/// Called by [dependent] to stop listening to this [InheritedElement]. | ||
/// | ||
/// Subclasses can override this method to cancel potential side-effects | ||
/// related to subscriptions. |
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.
This would be a good place to mention hasDependencies. I assume the use case we're enabling is checking hasDependencies after calling super.removeDependencies().
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.
Indeed, done. Is that what you had in mind?
@@ -4266,6 +4266,7 @@ class InheritedElement extends ProxyElement { | |||
/// created with [inheritFromWidgetOfExactType]. | |||
/// * [setDependencies], which sets dependencies value for a dependent | |||
/// element. | |||
/// * [removeDependencies], which unsets the value for a dependent element. | |||
/// * [notifyDependent], which can be overridden to use a dependent's | |||
/// dependencies value to decide if the dependent needs to be rebuilt. | |||
/// * [InheritedModel], which is an example of a class that uses this method |
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.
these lists of "see also" links is getting out of hand. We don't need to mention every method in each one. We should clean it up so that only the most relevant ones are linked to each time.
This change is scary, but I can't think of a case off-hand that it would break... |
That said, it probably doesn't do what you want. For example, if you have a child that registers with inherited widgets A and B in build 1, then A makes it dirty and the second time it only registers with A, B won't hear about it until the widget goes away entirely, which might be never. So I probably wouldn't recommend going down this path. It's better to just vend the actual stream and let the stream do the logic. |
Indeed, but that's because the dépendent is actually still depending on B, such that if B makes it dirty, the dependent will still rebuild even if it doesn't call So thats excepted |
But if you use the "vend a Stream" solution, B would unregister from the stream and so things would be more efficient. I'm torn on this one. On the one hand, I can see it being useful. On the other hand, it's a few extra cycles even in cases where we don't use it, and you only need a few thousand such "it's only a few extra cycles" before it really adds up. There's also the tradeoff between doing it the efficient way by vending a stream and the reality that doing that is somewhat convoluted and not as clean. Have you considered making a Builder widget that looks up the stream from an InheritedWidget and does the subscribe/unsubscribe? Maybe that would be clean enough? |
Sorry, I don't understand this paragraph.
Yes, but that is not compatible with what One of the key feature behind That both makes it easier to use (a single API), safer (can't forget to subscribe), and malleable (can be used inside An alternative solution is to expose a set of event listeners for the different widgets life-cycle on But I think that would be a lot more dangerous than what this PR does. |
Any update? I'm getting quite a few requests about that. I'm definitely convinced that listening to complex objects on the inherited widget instead of the consumer is a good idea. This allows many powerful things that wouldn't be possible otherwise. AnimatedTheme is one example. |
"I'm torn on this one" is an English idiom meaning that I am struggling to make a decision because I see good arguments both for and against. Maybe The more I think about this the more I think we probably shouldn't do it, because it adds a few cycles to every operation involving inherited widgets, and it isn't strictly adding anything that can't already be done, and it isn't really a complete solution to the problem (since it only handles some "unsubscribes"). Not sure what you mean about AnimatedTheme. Why is it not possible? We do it today, no? |
(I hope that I do not sound harsh or too insisting)
These two quotes are related.
That works great, but one of the requirement is that the "vend a Stream" is not an option (unless I missed something?).
I do not understand what this means.
Is it because of #12992? The use-case behind these changes is to do extra work when all listeners are removed (usually when a And even in the event where it does have an impact, then there's a simple workaround to that specific issue. Instead of: if (foo) {
return Text(Foo.of(context));
} else {
return Text(Bar.of(context));
} we can have: if (foo) {
return Builder(
key: const Key('foo'),
builder: (context) => Text(Foo.of(context)),
);
} else {
return Builder(
key: const Key('bar'),
builder: (context) => Text(Bar.of(context)),
);
} |
At a very minimum it adds a polymorphic call to #12992 is what I was referring to by "not complete", yes. I don't really see why you need this for the AnimatedTheme-equivalent behaviour. AnimatedTheme just builds a new Theme each frame. Why doesn't that work for provider? |
This isn't to implement an AnimatedTheme. |
In case this wasn't clear enough, a concrete example is: With provider, it's not rare to have many Inheritedwidgets above Navigator. There's no problem with the former, but for the latter we do want to both lazily create the object and dispose it when it's not needed anymore. |
For the problem of having a Listenable which behaves differently with zero listeners than non-zero listeners, I would expose the Listenable on the InheritedWidget, and then use an AnimationBuilder to register the callbacks:
Similarly if it's a ValueListenable I'd use a ValueListenableBuilder. You can do similar things with a stream, e.g. using StreamBuilder. I don't see why the inherited widget itself needs to do track if you have callbacks. |
For the same reasons behind Also:
|
But
You can avoid this by using an API where you only get access to the data if you subscribe (e.g.
In the same way that a widget like return ValueListenableWithTransformBuilder<int, Bar>(
valueListenable: Something.of(context),
transform: (int value) => Bar(value),
build: (BuildContext context, Bar value) {
// ...
},
);
I'm not really convinced that hiding details like this is a good thing. It just makes it harder to figure out why things break when they break. Also, it doesn't actually work for everything, as we noted earlier in this thread. It fails to unsubscribe promptly in some cases.
You don't have to use these objects, if you need different properties. They're just existence proofs. |
There's nothing as simple as returning Using Then we'll typically have to use
Why would it break?
While that issue is interesting, it is technically unrelated to these changes. Right now, there's technically still a listener. So it is logical for
That's a lot more complex. And this approach will get stuck when creating our In any case, Changing that would invalidate nearly the entirely library, and reintroduce bugs that were made voluntarily harder. And even then, that is still incompatible with the desired feature that is a For a |
We don't seem to be really convincing each other. I hear what you are saying, it just doesn't seem like a compelling enough reason to change the framework. If you think that I'm missing something, I recommend writing some demo apps using the style you want; we can see what it would look like to do them in styles possible today, and see how compelling the difference is. |
A demo will take a bit of time to make since I have other priorities. Having the InheritedWidget do the listening instead of the consumer of that information absorbs the performance issue of classes such as The documentation is pretty clear:
By having InheritedWidgets do the listening, we'll stay in that "one or two listeners" situations for the entire life of our If |
@Hixie I've been thinking: is |
What about implementing your own In appears that in the Flutter framework itself, the dependents are the keys of a
So I'm not seeing why |
Within a single thread, I can't see any reason for a |
cc @gspencergoog who has been working on fixing the O(N) thing |
@Hixie I think you missed my question here #33213 (comment): Is hasDependencies alone reasonable? With my use-case, I could replace removeDependencies by a post-frame callback
Reimplementing Listenable is outside of the scope of In any case, it's not just about O(N) vs O(N²). |
@Hixie wrote:
@rrousselGit replied:
I responded with a way to listen without using I didn't mean that would create a new public EDIT: Rereading now I see that you would instead have the "consumer" do the listening. My reading was that |
That'd be equivalent to reimplementing The only way we can make this work without that PR is to stop supporting |
I stand corrected. It sounds like the unsubscribe belongs on |
I have a question for you @Hixie. I'm reading this issue as Flutter having a memory leak but the leak apparently not being bad enough to fix, perhaps because the memory would normally free on page change, which presumably happens often enough. QUESTION: Assume |
I can answer that for hixie: It's not the widgets that subscribes to an InheritedWidget, but its
On the other hand, the subscription is not cleared if the widget's |
Thank you @rrousselGit! You've prevented me from expressing a misunderstanding in the article I'm writing. It sounds like a widget need only subscribe on its first build to forever thereafter be subscribed, even if it subsequently calls |
I don't understand what this means. I think we should close this PR, as per #33213 (comment), until we have a stronger reason to make any changes to the framework for this. |
I am going to close this PR as per the previous comment. |
Maybe this PR should be reconsidered:
|
I definitely agree with @szotp. I find it surprising that we have methods for getDependencies & co, which are called a lot. But removeDependencies is rejected, when it would be called only when an Element is deactivated (and it is fairly rare). Currently we may be able to work around it by overriding setDependencies to track everything. But that is a lot of overhead for something that would be very easy to fix on the framework side. |
@Hixie is this completely out of question, or could opening a design doc and making some example potentially convince the team? |
My recommendation would be to file an issue that describes the problem, ideally using sample code that demonstrates the problem. Until there is agreement that there is a real problem worth solving here, there's not much point us talking about what the solutions should be. You can write such an issue on GitHub or as a design doc, whatever is easiest for you. The point is just to separate the "problem" stage from the "solution" stage. It may be that someone can come up with a much better solution to the problem once they understand the problem fully. |
The issue already exists #30062 This is not about a "problem", in the sense that it is impossible to do today. |
A more general problem would be: I will create an issue with better description. |
"I want X" isn't a problem. |
Description
This PR exposes an overridable method to let the
InheritedElement
know that a subscription has been canceled.My plan with this change is to make a custom
InheritedElement
that could be combined with a listenable object (Stream/Listenable) – and subscribe to that object only if there's at least one widget that would be notified bynotifyDependents
.This is something currently not possible because while we know when the listening starts, we currently don't know when it ends.
Related Issues
Tests
removeDependencies
+hasDependencies
: done ininherited_test.dart
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?