Repository navigation
#121962 - Add ListenableFutureBuilder - #121963
MelbourneDeveloper wants to merge 14 commits into
Conversation
flutter-dashboard
Bot
added
the
framework
| /// | ||
| /// If your [builder] function contains a subtree that does not depend on the | ||
| /// value of the [Listenable], it's more efficient to build that subtree | ||
| /// once instead of rebuilding it on every animation tick. |
There was a problem hiding this comment.
nit: "animation tick"? AFAICT, this may not be driven by an animation?
| /// value of the [Listenable], it's more efficient to build that subtree | ||
| /// once instead of rebuilding it on every animation tick. | ||
| /// | ||
| /// If you pass the pre-built subtree as the [child] parameter, the |
There was a problem hiding this comment.
here and everywhere: please follow our style guide when it comes to the usage of "you:": https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-the-passive-voice-recommend-do-not-require-never-say-things-are-simple
| /// Using this pre-built child is entirely optional, but can improve | ||
| /// performance significantly in some cases and is therefore a good practice. | ||
| /// | ||
| /// {@tool snippet} |
There was a problem hiding this comment.
This should likely use {@tool dartpad} and include a tested code sample (see other occurrences of this in the code base).
| /// waiting, and trigger rebuild on state changes without the need for setState | ||
| /// | ||
| /// ```dart | ||
| // import 'package:flutter/material.dart'; |
There was a problem hiding this comment.
shouldn't switch from /// to // here
|
|
||
| // @override | ||
| // Widget build(BuildContext context) => MaterialApp( | ||
| // theme: ThemeData( |
| if (_activeCallbackIdentity == callbackIdentity) { | ||
| setState(() { | ||
| _snapshot = AsyncSnapshot<T>.withData(ConnectionState.done, data); | ||
| _snapshot.data!.addListener(_handleChange); |
There was a problem hiding this comment.
this line should probably be outside of the setState since it is not setting state.
|
|
||
| void _subscribe() { | ||
| final Object callbackIdentity = Object(); | ||
| _activeCallbackIdentity = callbackIdentity; |
There was a problem hiding this comment.
What scenario is this guarding against?
| builder: (BuildContext context, Widget? child, | ||
| AsyncSnapshot<ValueNotifier<int>> snapshot) => | ||
| snapshot.connectionState == ConnectionState.done | ||
| ? Text('${snapshot.data!.value}') | ||
| : const CircularProgressIndicator(), | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
(here and elsewhere in this file)
| if (snapshot.connectionState == ConnectionState.done && | ||
| snapshot.error != null) { | ||
| final dynamic error = snapshot.error as dynamic; | ||
| // ignore: avoid_dynamic_calls |
There was a problem hiding this comment.
Please don't ignore these and instead handle the value properly.
| expect(find.text('1'), findsOneWidget); | ||
| }); | ||
|
|
||
| ///Notes: we need to confirm that the builder drops its reference to the |
There was a problem hiding this comment.
Here and elsewhere: add space after /// and //
There was a problem hiding this comment.
Also, these should just be //, not ///
|
Looks like the analyzer is also unhappy with this change. Could you take a look? |
|
Hi @goderbauer I noticed that there were some issues there but was waiting for other feedback before I investigate. Is there any other feedback before I try to figure out what's going wrong here? There is a lot of detail on the issue itself. Thanks |
|
I left a comment on the issue as well about my high-level concerns with this setup: #121962 (comment) |
…Builder' into flutter#121962-ListenableFutureBuilder
|
Hi @goderbauer. Thanks for the detailed responses here. Sorry, I missed the activity here. I didn't realize you had already reviewed this in detail. I'm closing this draft PR because this was more of a demonstration of how the issue could be solved rather than a finished PR. Regarding the issue itself, it doesn't appear that my suggestion is clear. I would hope to leave that open and allow for further discussion. |
2 participants
This PR adds the
ListenableFutureBuilderwidget and tests. Briefly: this widget allows you to composeStatelessWidgets with async work, and without the need to extend StatefulWidget, or use external dependency managers. For a full description of the issue and solution see #121962This PR fixes #121962
See a live sample here
This is a draft PR and I will revisit this, and address issues as they come up. I will go over the criteria here again before full submission
Pre-launch Checklist
///).