-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Revert "Replace deprecated onReportTimings w/ frameTimings" #39778
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
This reverts commit 6ef80b4.
|
I'm not clear what the issues with the Stream API are. Do we have a proposal for what would replace the API? The issues with the previous API where that clients of the API had to manage ownership of a field, and would likely mostly do that unsuccessfully. |
|
@devoncarew : yes, I'm thinking about storing the callbacks as a list like addPostFrameCallback. @Hixie : does that sound good to you? |
|
I'm also curious about the problems with streams? it doesn't seem too long ago that BLoC + stream was the go-to solution.. is there some major difference between framework use of streams, vs. framework users? I tend to follow the lead of the framework to have some uniform solution vs. having everything doing things differently (callbacks vs Listenable/ChangeNotifier vs stream). |
goderbauer
left a comment
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.
LGTM
|
@Hixie created another section in the style guide for this here: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-of-streams-in-flutter-framework-code |
|
Streams are fine in general, they're just not the best solution to this problem. |
| WidgetsBinding.instance.window.onReportTimings = oldCallback; | ||
| _firstFrameCompleter.complete(); | ||
| }); | ||
| }; |
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 all probably belongs in the SchedulerBinding, fwiw.
Reverts #38861
Reason for revert: Stream is considered a bad API: flutter/engine#11041 (comment)