Skip to content

Conversation

@albertodev01
Copy link
Contributor

I have added an example to document how an AnimatedBuilder can be used along with a ChangeNotifier. Follow up for #97256

List which issues are fixed by this PR. You must list at least one issue.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 17, 2022
@albertodev01
Copy link
Contributor Author

@Hixie I have only updated the documentation so no tests are required

@goderbauer
Copy link
Member

haven't looked at this closely yet, but looks like the analyzer is unhappy. Can you take a look?

@goderbauer goderbauer self-requested a review February 17, 2022 23:41
@albertodev01
Copy link
Contributor Author

Thank you @goderbauer for your time, I've created and tested an example as you suggested

@goderbauer
Copy link
Member

Looks like the checks are unhappy with this change. Can you take a look?

@goderbauer
Copy link
Member

Overall I am actually not convinced that this needs a huge example. Maybe a simple sentence in the doc would be enough. Something like: "Oh, by the way, an AnimiatedBuilder accepts any Listenable (not just an Animation) and works great with Listenable subclasses such as ChangeNotifier, ValueNotifier, etc." (of course phrased a little more formal)

That may be easier to understand than having to parse a big block of sample code.

@albertodev01
Copy link
Contributor Author

@goderbauer I have simplified the text and also simplified the example.

I think that having a short example (along with some descriptive text) is a good thing. A person can play with the code, see it in action and we can make sure that our words are misunderstood 🙂

I've followed you valuable comments to improve the code. I've also removed the InheritedWidget which yeah adds more complexity. It's all inside a simple StatefulWidget now and I agree that it looks simpler/better

@goderbauer
Copy link
Member

I have thought about this some more and I still think what I said in #98628 (comment) is valid. A simple doc comment saying that any subclass of a Listenable (e.g. ChangeNotifier and friends) can be used with AnimatedBuilder is clearer and more concise than this example. You have to do a lot more reading to get that point from this example - and may even miss what the example is trying to illustrate altogether.

@albertodev01
Copy link
Contributor Author

Would you suggest to simply drop the example and keep these lines?

/// A [State.setState] method call requires Flutter to entirely rebuild the
/// current widget plus a variable portion of the subtree, depending on the
/// depth and the structure.
///
/// To avoid unnecessary work, the [AnimatedBuilder] class can be used along
/// with a [Listenable] to only rebuild certain portions a widget,
/// leaving other parts and the subtree untouched. For example,
/// [ChangeNotifier] and [ValueNotifier] are subtypes of [Listenable].
///
/// Since a [ChangeNotifier] is a subtype of [Listenable], it can be used in
/// an [AnimatedBuilder] to listen for changes.

I would really like to keep the example but we can remove it if you want 🙂

I think that we could keep the lines I posted above. If we wrote even less contents, maybe it wouldn't give enough context to understand why one should use a ChangeNotifier in an AnimatedBuilder. Also, a very few lines wouldn't add much value and so they wouldn't even need to be there.

What do you think? Do you agree in at least keeping that text?

@goderbauer
Copy link
Member

goderbauer commented Feb 24, 2022

Here's my suggestion for the docs:

## Improve rebuild performance using AnimatedBuilder

Despite the name, [AnimatedBuilder] is not limited to [Animation]s: Any subtype
of [Listenable] (e.g. [ChangeNotifier] and [ValueNotifier]) can be used with an
[AnimatedBuilder] to rebuild only parts of a widget when the [Listenable] notifies its listeners.

If you want to include an example below that, let's use one that is concise and focused on that particular use case. Here's my suggestion:

The following example implements a simple counter, that utilizes an
[AnimatedBuilder] to limit rebuilds to only the [Text] widget showing the current 
count (stored in a [ValueNotifier]) when the value of the counter is changing.
import 'package:flutter/material.dart';

class CounterBody extends StatelessWidget {
  const CounterBody({Key? key, required this.counterValueNotifier}) : super(key: key);

  final ValueNotifier<int> counterValueNotifier;

  @override
  Widget build(BuildContext context) {
    return Center(
      child: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        children: [
          const Text('Current counter value:'),
          // Thanks to the [AnimatedBuilder], only the widget displaying the
          // current count is rebuilt when `counterValueNotifier` notifies its
          // listeners.
          AnimatedBuilder(
            // [AnimatedBuilder] accepts any [Listenable] subtype.
            animation: counterValueNotifier,
            builder: (BuildContext context, Widget? child) {
              return Text('${counterValueNotifier.value}');
            },
          ),
        ],
      ),
    );
  }
}

class MyApp extends StatefulWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  State<MyApp> createState() => _MyAppState();
}

class _MyAppState extends State<MyApp> {
  final ValueNotifier<int> _counter = ValueNotifier<int>(0);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(title: const Text('AnimatedBuilder example')),
        body: CounterBody(counterValueNotifier: _counter),
        floatingActionButton: FloatingActionButton(
          onPressed: () => _counter.value++,
          child: const Icon(Icons.add),
        ),
      ),
    );
  }
}

void main() {
  runApp(const MyApp());
}

@albertodev01
Copy link
Contributor Author

I really like your changes! Thank you for having considered keeping an example, I think it will be very valuable

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Please rebase it to the latest master, though.

@albertodev01
Copy link
Contributor Author

I have rebased but the diff isn't going away...

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flutter_LGTM

@Piinks Piinks added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation labels Feb 28, 2022
@fluttergithubbot fluttergithubbot merged commit b44cbe1 into flutter:master Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
@albertodev01 albertodev01 deleted the change_notifier_doc_update branch February 28, 2022 20:44
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants