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

Need mechanism for determining current build target #128461

Closed
2 tasks done
lukehutch opened this issue Jun 7, 2023 · 16 comments
Closed
2 tasks done

Need mechanism for determining current build target #128461

lukehutch opened this issue Jun 7, 2023 · 16 comments
Labels
c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter framework flutter/packages/flutter repository. See also f: labels.

Comments

@lukehutch
Copy link
Contributor

lukehutch commented Jun 7, 2023

Is there an existing issue for this?

Use case

(Moved over from #128384 -- cc @Hixie)

I need a way to determine which widget is currently being built. WidgetsBinding.instance.buildOwner._debugCurrentBuildTarget is not exposed (and is not even available in release builds). (@Hixie noted that it would be expensive to track this in release builds, although the current code that tracks this is just one assignment on the way into a build method, and one assignment on the way back out, so I don't see how that would be expensive...)

The reason I need this information is because I created this library, in which a user subclasses ReactiveWidget (which subclasses StatefulWidget), and then whenever their widget is getting built, any read of the value of a ReactiveValue (which subclasses ValueNotifier) causes a listener to be added to the ReactiveValue that causes setState to be called on the ReactiveWidget whenever the value changes.

I know that build methods should not mutate state -- more strictly, that they should not have side effects. This does not mutate state (at least per se), it just adds a listener. Yes, I know some people will hate this approach for this reason alone. However this is by far the simplest reactive UI update mechanism available so far for flutter.

The big problem I have is that in order to figure out which ReactiveWidget to call from the ReactiveValue listener, I need to know which widget is being built when the ReactiveValue's value field is read while the widget's build method is being run.

The solution that I have, which is horrible, but works (currently, at least!), is to record the current ReactiveWidget in a global variable when entering the build method, then unrecord it on exit from build. (I put these into a stack, although I think there can only ever be zero or one build method(s) running.)
https://github.com/lukehutch/flutter_reactive_widget/blob/main/lib/src/reactive_widget.dart#L43

Proposal

It would be much better if I could just read from WidgetsBinding.instance.buildOwner.currentBuildTarget.

I would like to see some benchmarks to see if always tracking the build target really does impact performance. I highly doubt the impact of setting this field on entry to build and unsetting it on exit would even be measurable.

@Hixie
Copy link
Contributor

Hixie commented Jun 7, 2023

It affects performance because it's an assignment before every build and an assignment after every build. That's thousands of assignments per frame, to a memory location that is using up cache lines, in a way that might prevent inlining, etc. These code paths are super hot and we really want to do as little work as possible.

Your use case is interesting. Generally I would recommend passing the context any time you read a value. The context is an Element which is basically the object backing a widget, and you can call markNeedsBuild on that object to trigger a rebuild. That is the purpose of having the context everywhere.

@lukehutch
Copy link
Contributor Author

That's a fair suggestion; however, I really want to avoid having to change

ReactiveWidget(() => Text('${counter.value}'))

into

ReactiveWidget((context) => Text('${counter.getValue(context)}'))

because I'm trying to avoid as much boilerplate syntax as possible.

I guess the current solution I have, of recording the current build widget in a global variable on entry to ReactiveWidget.build, is not much different than getting access to it in WidgetsBinding.instance.buildOwner._debugCurrentBuildTarget, since that's also basically a global.

I have been told that build never operates recursively. I assume that means that the recursion through the widget tree happens outside of build itself. Will that always be the case, i.e. will there never be more than one build call on the stack? The reason I'm asking is I'm wondering if I should change the stack I am using (referred to in the previous comment) into a single variable assignment.

@Hixie
Copy link
Contributor

Hixie commented Jun 8, 2023

I understand where you're coming from, but the problem is that you're experiencing the tension between making an API look clean and easy to write, and making an API easy to understand. Global variables, getters with side-effects, and the combination of those, code that operates on data it doesn't seem to have (in your case, the caller's identity) are all forms of "magic" that make for very nice-looking code that is hard to understand.

When you see Text('${counter.value}')) it looks like a cheap call. It looks like something that has no side-effects, something that you could easily refactor so that counter.value is read in some different function and then assigned to a variable that's used in the build method instead. It looks like code you could refactor to use TextSpans instead of widgets, with no difference in behavior. It looks like you could substitute a different object for counter and have the same result. It looks like code that O(1). But the reality is quite different.

On the other hand, when you see Text('${counter.subscribe(context)}') or the equivalent, you immediately know something more is happening. You know that you can't use this code without a context. You know that something is being subscribed, which will have huge performance implications. You know that you can't just replace it with a cached value because that would change the semantics. You know that calling this function a hundred times will likely cost a lot more than calling a simple getter a hundred times. You can reason about what exactly will happen in weird situations like complicated build methods with nested Builder widgets and so on, since you know which context you're passing in.

Flutter leans strongly into this second paradigm (the code should look like what it does).

will there never be more than one build call on the stack?

In normal use, that is correct. The build method is called from Element.performRebuild; the recursion is done by the Element logic in widgets/framework.dart.

@lukehutch
Copy link
Contributor Author

In general I hate magic / implicit behavior. It's why I can't stand C++, among other things. So we're on the same page. However I have an even harder time with unnecessary complexity -- and all the other state management solutions out there for Flutter are horrendously complex. This is why I sacrificed on the first of these two problems to try to solve the second one.

My thinking was to treat this as a dataflow problem, where the dependency graph is created by merely reading a value. So the philosophy here is to make Dart look like a reactive dataflow language.

Dart is not a dataflow language though, and you made some very convincing arguments, especially with the suggested method name change to subscribe.

So I'll make the breaking changes you suggested. However, maybe it would be good to figure out a better (much simpler!) state management solution as a provided default for Flutter?

@darshankawar darshankawar added in triage Presently being triaged by the triage team framework flutter/packages/flutter repository. See also f: labels. c: proposal A detailed proposal for a change to Flutter c: new feature Nothing broken; request for a new capability and removed in triage Presently being triaged by the triage team labels Jun 8, 2023
@lukehutch
Copy link
Contributor Author

lukehutch commented Jun 8, 2023

@Hixie OK, I started to make the changes, but I don't understand how to find the nearest ancestor ReactiveWidget (or more precisely, the state of that widget, _ReactiveWidgetState) in the widget hierarchy.

BuildContext.findAncestorStateOfType has this documentation:

This should not be used from build methods, because the build context will not be rebuilt if the value that would be returned by this method changes. In general, dependOnInheritedWidgetOfExactType is more appropriate for such cases. This method is useful for changing the state of an ancestor widget in a one-off manner, for example, to cause an ancestor scrolling list to scroll this build context's widget into view, or to move the focus in response to user interaction.

and

This method should not be called from [State.deactivate] or [State.dispose] because the widget tree is no longer stable at that time. To refer to an ancestor from one of those methods, save a reference to the ancestor by calling findAncestorStateOfType in State.didChangeDependencies.

I'm lost here...

(1) How are you supposed to find ancestor widgets from a BuildContext if you can't determine the ancestor when build is running?

(2) I'm not trying to find the ancestor from a StatefulWidget, so the didChangeDependencies advice doesn't apply...

(3) I need the state object of the ReactiveWidget, not the widget itself.

What I have is this -- but based on the above, I don't think it's correct:

class ReactiveValue<T> extends ChangeNotifier {
  ReactiveValue(this._value);

  T _value;

  T getAndSubscribe(BuildContext context) {
    final parentReactiveWidgetState = context.findAncestorStateOfType<_ReactiveWidgetState>();
    assert(parentReactiveWidgetState != null, 'Could not find parent ReactiveWidget');
    parentReactiveWidgetState?._listenTo(this);
    return _value;
  }

  void set(T newValue) {
    if (_value != newValue) {
      _value = newValue;
      notifyListeners();
    }
  }
}

@Hixie
Copy link
Contributor

Hixie commented Jun 8, 2023

Can you elaborate on why you need a ReactiveWidget?

I was imagining that the subscription mechanism would just remember the BuildContext (aka Element) it was called with, and when the data changes, would just do something like if ((context as Element).mounted) { (context as Element).markNeedsBuild(); } else { /* remove the context from the list of subscribed contexts */ }.

@lukehutch
Copy link
Contributor Author

lukehutch commented Jun 8, 2023

@Hixie Oh, I didn't even know .markNeedsBuild existed! I asked about that once before, #120545. I went looking for exactly this method, and I couldn't find it, presumably because BuildContext doesn't expose that Element method...

(So to answer your question, the reason for ReactiveWidget was originally to allow the current build target to be tracked, so that listeners could be added if the value changed.)

I'll try out the solution you suggested, that's awesome. Thanks.

@lukehutch
Copy link
Contributor Author

@Hixie This is what I have, but there are some issues...

class NewReactiveValue<T> {
  NewReactiveValue(this._value);

  T _value;
  final _subscribedElements = <Element>{};

  T getAndSubscribe(BuildContext context) {
    _subscribedElements.add(context as Element);
    return _value;
  }

  void set(T newValue) {
    if (_value != newValue) {
      _value = newValue;
      notifyListeners();
    }
  }

  void notifyListeners() {
    for (final element in _subscribedElements) {
      if (element.mounted) {
        element.markNeedsBuild();
      }
    }
  }
}

How do I track the lifecycle of the element, so that I can remove the listener when it's no longer needed? Is there a way to subscribe to the dispose method of a StatefulWidget's state, at least, or the equivalent for StatelessWidget? Or should I just remove the listener when !element.mounted? Can an element be re-mounted after it is unmounted?

Sorry for all the basic questions, I find it very hard to figure this stuff out from the documentation...

@Hixie
Copy link
Contributor

Hixie commented Jun 9, 2023

There's no way to be notified when an element goes away.

This would be a good use case for WeakReference, actually. I've never used it myself but I think it would work here. Instead of storing the elements directly, store weak references to the elements. When notifyListeners is called, you can remove all the weak references that are now null, as well as any widgets that aren't mounted.

An element is guaranteed to rebuild if it is ever mounted after being unmounted. (In practice you won't observe an element in its unmounted state because if it's not immediately remounted it'll be disposed. Elements don't survive past the end of the frame if they're not mounted.)

Sorry for all the basic questions, I find it very hard to figure this stuff out from the documentation...

No worries! If you could enumerate all the questions you've asked and where you looked for answers for each one, I can make sure to go and add answers to those places.

@lukehutch
Copy link
Contributor Author

lukehutch commented Jun 9, 2023

@Hixie Sorry, but honestly I have read so much documentation both on the Flutter site and on other sites that I don't even know what to suggest improving. The UI system is very complex in Flutter, and it's hard to get one's mind around all of it. Not many Flutter developers will try to do anything as low-level or out-of-the-ordinary as what I took on for this project, so I don't think my learning curve issues will be representative of what others need anyway.

Thanks for all the pointers. Now what I have is the following:

class ReactiveValue<T> {
  ReactiveValue(this._value);

  T _value;
  final _subscribedElements = <WeakReference<Element>>{};

  /// Get the current value, and cause the [Element] passed as this
  /// [BuildContext] to subscribe to future changes in the value.
  T getAndSubscribe(BuildContext context) {
    _subscribedElements.add(WeakReference(context as Element));
    return _value;
  }

  /// Set the value, and notify subscribers if value changed.
  void set(T newValue) {
    // Don't allow mutating state during `build`
    // https://github.com/flutter/flutter/issues/128384#issuecomment-1580110349
    assert(
        SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks,
        'Do not mutate state (by setting a ReactiveValue\'s value) '
        'during a `build` method. If you need to schedule an update, '
        'use `SchedulerBinding.instance.scheduleTask` or similar.');
    if (_value != newValue) {
      _value = newValue;
      notifyListeners();
    }
  }

  /// Update the value based on current value, and notify subscribers
  /// if value changed.
  /// e.g. to increment: reactiveVal.update((v) => v + 1)
  void update(T Function(T) currValueToNextValue) {
    set(currValueToNextValue(_value));
  }

  /// You should manually invoke this method for deeper changes,
  /// e.g. when items are added to or removed from a set, as in
  /// ReactiveValue<Set<T>>
  void notifyListeners() {
    final liveSubscribedElements = <WeakReference<Element>>[];
    _subscribedElements.forEach((elementWeakRef) {
      // Skip elements that have already been garbage collected
      final element = elementWeakRef.target;
      if (element != null) {
        // For any subscribing elements that are still live, mark element
        // as needing to be rebuilt
        element.markNeedsBuild();
        // Remember elements that are still live
        liveSubscribedElements.add(elementWeakRef);
      }
    });
    // Remove any elements that have been garbage collected
    _subscribedElements.clear();
    _subscribedElements.addAll(liveSubscribedElements);
  }

  @override
  String toString() => '${describeIdentity(this)}($_value)';
}

This finally seems to fully work as expected, and it's a much better solution than what I had before.

Can you please check over the code and let me know if there are any outstanding issues?

Dart needs a single ultra-simple reactive state management solution as a default, to lower the barrier of entry to newcomers (since state management seems to be the hardest part to learn when learning Flutter). What is the likelihood that the above class, or something like it, could be incorporated into the Flutter core libraries?

@lukehutch
Copy link
Contributor Author

lukehutch commented Jun 9, 2023

Actually I just thought of an issue with this -- the set _subscribedElements = <WeakReference<Element>>{} will treat multiple different WeakReference<Element> references to the same Element as distinct, so every time getAndSubscribe is called by an Element's build method, another WeakReference will be added to the set.

I assume then that I have to extend WeakReference and define operator == and hashCode to operate on WeakReference.target rather than the WeakReference itself?

class _HashableWeakReference<T extends Object> {
  late WeakReference<T> _weakReference;
  _HashableWeakReference(T value) {
    _weakReference = WeakReference(value);
  }

  T? get target => _weakReference.target;

  @override
  bool operator ==(Object other) =>
      other is _HashableWeakReference<T> &&
      identical(_weakReference.target, other._weakReference.target);

  @override
  int get hashCode => _weakReference.target.hashCode;
}

@Hixie
Copy link
Contributor

Hixie commented Jun 11, 2023

@Hixie Sorry, but honestly I have read so much documentation both on the Flutter site and on other sites that I don't even know what to suggest improving.

Fair enough. In the future please, any time something is unclear and you don't find the answer right away in our docs, file a bug saying what your question was and where you looked.

Not many Flutter developers will try to do anything as low-level or out-of-the-ordinary as what I took on for this project, so I don't think my learning curve issues will be representative of what others need anyway.

That doesn't matter, we want to make it easy for everyone, not just people doing the basic stuff.

Can you please check over the code and let me know if there are any outstanding issues?

Seems reasonable to me, modulo the issue you pointed out (that I would not have noticed, to be honest).

Your solution to the WeakReference stuff seems reasonable.

Dart needs a single ultra-simple reactive state management solution as a default, to lower the barrier of entry to newcomers (since state management seems to be the hardest part to learn when learning Flutter). What is the likelihood that the above class, or something like it, could be incorporated into the Flutter core libraries?

I think it would make a lot of sense for such a package, once suitably proven, to be listed on https://docs.flutter.dev/data-and-backend/state-mgmt/options. (Currently our recommended solution is package:provider.)

Personally I see state management as the core of writing an application. I don't think it makes that much sense to create packages for it, I just build a bespoke solution for each application based on the shape of its data model, the access patterns, etc. I wouldn't recommend using ReactiveValue for a system where the application is presenting a whole database or spreadsheet of information, for instance. And I wouldn't recommend it for something as simple as the counter app. It probably wouldn't be my first choice for an app where undo/redo is going to be a key mechanic, either. I could see something like it being useful for an application that has a small number of scalar values that it's trying to present, maybe, like an air quality sensor dashboard. Even there, though, I'm likely to just rebuild the whole dashboard when anything changes rather than try to optimize how much gets built; it's simpler. Anyway long story short, I'm the wrong person to try to convince for us to make a particular state management solution be a canonical solution.

@lukehutch
Copy link
Contributor Author

lukehutch commented Jun 11, 2023

Fair enough. In the future please, any time something is unclear and you don't find the answer right away in our docs, file a bug saying what your question was and where you looked.

Thanks, I really appreciate the commitment to producing good documentation.

I filed the first documentation improvement request: #128648

Seems reasonable to me, modulo the issue you pointed out (that I would not have noticed, to be honest).

Your solution to the WeakReference stuff seems reasonable.

Thanks for taking a look.

It occurred to me I could make this even simpler:

class _ListenerWrapper<T> {
  void Function()? listener;
}

extension SubscribableValueNotifier<T> on ValueNotifier<T> {
  T getValueAndSubscribe(BuildContext context) {
    final elementRef = WeakReference(context as Element);
    // Can't refer to listener while it is being declared, so need to add
    // a layer of indirection.
    final listenerWrapper = _ListenerWrapper<void Function()>();
    listenerWrapper.listener = () {
      assert(
          SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks,
          'Do not mutate state (by setting the value of the ValueNotifier '
          'that you are subscribed to) during a `build` method. If you need '
          'to schedule an update after `build` has completed, use '
          '`SchedulerBinding.instance.scheduleTask(updateTask, Priority.idle)` '
          'or similar.');
      // If the element has not been garbage collected, mark the element
      // as needing to be rebuilt
      elementRef.target?.markNeedsBuild();
      // Remove the listener -- only listen to one change per `build`
      removeListener(listenerWrapper.listener!);
    };
    addListener(listenerWrapper.listener!);
    return value;
  }
}

Personally I see state management as the core of writing an application. I don't think it makes that much sense to create packages for it

You are right, it's not a be-all and end-all for state management. It's intended for getting users onboarded to Flutter quickly and easily, for apps with minimal reactivity requirements. I do think there's a solid need for that in the Flutter core libraries.

@lukehutch
Copy link
Contributor Author

lukehutch commented Jun 12, 2023

@Hixie I released this library as:

https://github.com/lukehutch/flutter_reactive_value

Thanks for all the significant feedback you provided to make this happen. I am very happy with the final result, and I couldn't have done it without your guidance!

@Hixie
Copy link
Contributor

Hixie commented Jun 12, 2023

Congratulations!

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

No branches or pull requests

3 participants