[go: nahoru, domu]

Skip to content
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

Add ComputedNotifier streaming class #150143

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Prev Previous commit
Next Next commit
doc: apply doc fixing for ComputedNotifier class
  • Loading branch information
HenriqueNas committed Jun 14, 2024
commit 9ce230a2467749a8d94b11be1abd0c65489fa95a
3 changes: 1 addition & 2 deletions packages/flutter/lib/src/foundation/change_notifier.dart
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ class ValueNotifier<T> extends ChangeNotifier implements ValueListenable<T> {
/// final ValueNotifier<int> b = ValueNotifier<int>(2);
///
/// final ComputedNotifier<int> sum = ComputedNotifier<int>(
/// [a, b, c, d],
/// [a, b],
/// () => a.value + b.value,
Copy link
Member

Choose a reason for hiding this comment

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

What worries me about this API design: It's gonna be very easy to use a listenable in the compute function and accidentally forget to include it in the listenable list...

Copy link
Contributor Author
@HenriqueNas HenriqueNas Jun 14, 2024

Choose a reason for hiding this comment

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

You're absolutely right!! But although I think it might happen, I am not very worried about that.. It's a basic programming skill issue, not very related to the API signature.
Also, in some cases, you might not want to include all values ​​used in the computed callback in your listenable list, right?

But, what do you think we can do to improve it? Besides a clear documentation..

Copy link
Contributor Author
@HenriqueNas HenriqueNas Jun 14, 2024

Choose a reason for hiding this comment

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

Maybe we can return the listenable list as a parameter in our callback, so it can reinforces the user to add the listenable to the list:

final ComputedNotifier<String> sum = ComputedNotifier<int>(
  <ValueListenable<int>>[a, b],
  (List<ValueListenable<dynamic>> list) => ((list.first.value as int) + (list[1].value as int)).toString(),
);

To be honest, I do not found this a good solution. Maybe because I don't see it as a ruge problem.

Copy link
Contributor Author
@HenriqueNas HenriqueNas Jun 15, 2024

Choose a reason for hiding this comment

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

You made me intriguer about a solution for this... what do you think about write the API as this?

  final ValueNotifier<int> a = ValueNotifier<int>(1);
  final ValueNotifier<int> b = ValueNotifier<int>(2);

  final ComputedNotifier<int> sum = ComputedNotifier<int>(
      (ValueGetter getter) => getter(a) + getter(b),
  );

  int sumValue = sum.value;
  sum.addListener(() => sumValue = sum.value);

  a.value = 2;
  print(sumValue); // 4

  b.value = 3;
  print(sumValue); // 5

/// );
///
Expand All @@ -595,7 +595,6 @@ class ValueNotifier<T> extends ChangeNotifier implements ValueListenable<T> {
///
/// Additionally, it is important to dispose of the listeners that are passed
/// to the constructor when they are no longer needed.
///
class ComputedNotifier<T> extends ChangeNotifier implements ValueListenable<T> {
/// Creates a [ComputedNotifier] that computes its value from the given listeners.
ComputedNotifier(this._listenableList, this._compute) {
Expand Down