binder icon indicating copy to clipboard operation
binder copied to clipboard

Widget watching on Computed is sometimes not rebuilt

Open Delgan opened this issue 3 years ago • 7 comments

Hey @letsar!

So, I was doing some tests using your excellent package, and I noticed a very weird bug. Basically, I have one widget watching a Computed reference which itself is based on a State reference. Each time the button is pressed, the state is updated and the other reference is re-computed. So far so good.

However, the widget is not systematically rebuilt. It only works half the time, randomly. Yet, the value returned by the Computed is definitely different!

https://user-images.githubusercontent.com/4193924/120108355-1f8b1b80-c165-11eb-8f40-6865a460d624.mp4

import 'package:binder/binder.dart';
import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

void main() => runApp(BinderScope(child: AppView()));

class AppView extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        children: [
          RaisedButton(
            child: Text("Press me"),
            onPressed: () => context.use(testLogicRef).click(),
          ),
          Builder(
            builder: (context) {
              final val = context.watch(computedValRef);
              print("Rebuilt with: $val");
              return Text("$val");
            },
          )
        ],
      ),
    );
  }
}

final testLogicRef = LogicRef((scope) => TestLogic(scope));

final testStateRef = StateRef(0);

final computedValRef = Computed<int>((watch) {
  final val = watch(testStateRef);
  final computedVal = DateTime.now().millisecondsSinceEpoch;
  print("Computed value: $computedVal");
  return computedVal;
});

class TestLogic with Logic {
  const TestLogic(this.scope);

  @override
  final Scope scope;

  void click() => update(testStateRef, (int val) => val + 1);
}

It drives me crazy. Am I doing something wrong?

Also, note that when I press the button once, the Computed is called twice or thrice. Is that expected?

I'm using Binder 0.4.0 with Flutter 2.0.6 and Dart 2.12.3.

Delgan avatar May 30 '21 14:05 Delgan

Hi,

I took a look at your example as i'm using Binder now in a live app, so this made me a bit concerned.

The behavior of your example does seem a bit strange.

If i simplify your example and only return val (the counter value) in computedValRef, the Text is always rebuilt. However, i do notice some strange behavior here. The old value is first returned in Computed, before the new value is returned twice.

If i instead return the computedVal (the time value), i noticed when it is generated twice with the same value, the widget is not rebuilt. Even if it is different than the previous clicked value.

Se my example below and try to comment out val in computedVal.

So to summarize, there seem to be two problems with Computed:

  1. The value is generated too often. First once with the previous value, then twice with the new. I would expect only the latest value.
  2. If two successive equal values are returned from Computed, the widget is not rebuilt.
import 'package:binder/binder.dart';
import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

void main() => runApp(const BinderScope(child: AppView()));

class AppView extends StatelessWidget {
  const AppView({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        children: [
          ElevatedButton(
            child: const Text("Press me"),
            onPressed: () => context.use(testLogicRef).click(),
          ),
          Builder(
            builder: (context) {
              final val = context.watch(computedValRef);
              debugPrint("Rebuilt with: $val");
              return Text("$val");
            },
          )
        ],
      ),
    );
  }
}

final testLogicRef = LogicRef((scope) => TestLogic(scope));

final testStateRef = StateRef(0);

final computedValRef = Computed<int>((watch) {
  final val = watch(testStateRef);
  debugPrint('Computed watch val $val');
  final int timeValue = DateTime.now().millisecondsSinceEpoch;
  debugPrint("Computed watch timeValue: $timeValue");
  return val;
  return timeValue;
});

class TestLogic with Logic {
  const TestLogic(this.scope);

  @override
  final Scope scope;

  void click() => update(testStateRef, (int val) {
        final int newVal = val + 1;
        debugPrint('click');
        debugPrint('TestLogic update with newVal $newVal');
        return newVal;
      });
}

erf avatar Jun 12 '21 12:06 erf

Thanks @erf for taking a closer look at this!

I came to the same conclusion. The behavior depends whether or not the same value is returned twice. I have no idea what could cause this in the Computed implementation.

Delgan avatar Jun 13 '21 16:06 Delgan

@Delgan we could try to write some tests for this

erf avatar Jun 13 '21 17:06 erf

Hi, sorry for the delay, I'm very busy nowadays.

For the moment, the state of Computed is not cached, this is why the function inside the Computed is executed 3 times:

  • For getting the old value.
  • For getting the new value (so that we can compare them an know if we have to update the widget watching it).
  • The last time for getting the value when the widget is rebuilt.

I didn't looked how to cache the values for the moment, but this is the next thing I want to achieve, so that it will be more efficient.

The purpose of Computed is to create a new data derived from one or more other values. In the example mentioned in the issue, the result of the Computed is not derived from the watched value, that's why you observe strange behaviors. It's not intended to be used like that.

In the example, the old and the new values will be too close from each other, since we don't cache the values but getting them with the old context and then with the new context. Since the two calls are next to each others I suspect that millisecondsSinceEpoch returns the same value for the old and the new values sometimes. Binder don't see any changes, so the widget is not rebuilt.

I hope this give you enough details to understand the situation.

letsar avatar Jun 14 '21 20:06 letsar

Hi, no worries. Caching sounds great - if it also will work for complex objects.

erf avatar Jun 14 '21 20:06 erf

As long as objects are still immutable, it should also work

letsar avatar Jun 15 '21 06:06 letsar

Hi @letsar, thanks for the update!

To clarify, I came to the millisecondsSinceEpoch example by narrowing down the problem step by step. However, in my code, the value which initially triggered the issue looks as follows:

class SomeClass {
    final List<int> someValues;
    SomeClass(this.someValues);
}

final baseRef = StateRef(Map<DateTime, SomeClass>());

final derivedRef = Computed<Map<DateTime, int>>((watch) {
  final base = watch(baseRef);
  final derived = Map<DateTime, int>();
  base.forEach((key, value) {
    derived[key] = value.someValues.length;
  });
  return derived;
});

final computedValRef = Computed<int>((watch) {
  final derived = watch(derivedRef);
  final now = DateTime.now();
  var date = DateTime(now.year, now.month, now.day);
  var count = 0;
  while (derived.containsKey(date) && derived[date] > 0) {
    date = date.subtract(Duration(days: 1));
    count++;
  }
  return count;
});

The computedValRef is sort of derived from the others. I also make sure to create a new object each time baseRef is updated.

I guess the reported problem may occur if baseRef is updated several times during a short interval? Perhaps it is related to the parent context.

Anyway, I may have abused Computed values due to how convenient they are. :slightly_smiling_face: I will try to keep things simple and favor StateRef.

Delgan avatar Jun 20 '21 13:06 Delgan