[chore] Correct `Computed`'s `previousValue` getter

[manuel-plavsic]

At the moment calling previousValue sets a previous value, which is unwanted. By removing this getter, the Signal one will be used.

I tested this manually.

[manuel-plavsic]

I probably didn't correctly hot-reload/hot-restart after removing the getter while testing manually. The getter is actually needed, otherwise — i.e. when there is no getter in Computed — ReadSignal's previousValue getter is used (instead of Signal's previousValue).

[manuel-plavsic]

The failing test will pass once also the other pull request (https://github.com/nank1ro/solidart/pull/43) is accepted.

[manuel-plavsic]

Actually, the test will not pass even if the other pull request is accepted. It seems like derived is simply not reactive in the test (and more in general Computed are not reactive in Dart projects). I think this pull request goes in the right direction, however, some additional changes are needed for the test to pass successfully (do know why instances created through Computed are not reactive in Dart projects/tests?).

For some reason, even though the test fails, these changes work in a Flutter project: doubleCount is reactive when manually testing solidart/packages/flutter_solidart/example/lib/pages/derived_signal.dart.

[nank1ro]

Can you provide a sample code with the issue? I don't understand where the issue is.

[manuel-plavsic]

Try running this code in the current dev branch (note: this is an edited version of flutter_solidart/examples/lib/pages/derived_signal.dart, so temporarily replace the code in the file)

import 'package:flutter/material.dart';
import 'package:flutter_solidart/flutter_solidart.dart';

const nullOrIntSignalStyle = TextStyle(
  color: Colors.yellow,
  backgroundColor: Colors.black,
);

class DerivedSignalsPage extends StatefulWidget {
  const DerivedSignalsPage({super.key});

  @override
  State<DerivedSignalsPage> createState() => _DerivedSignalsPageState();
}

class _DerivedSignalsPageState extends State<DerivedSignalsPage> {
  late final Signal<int?> nullOrIntSignal = createSignal(null);
  late final count = createSignal(0);
  late final doubleCount = createComputed(() {
    final value = count.call();
    if ((value) <= 5) {
      return null;
    } else {
      return value * 2;
    }
  });

  @override
  void dispose() {
    count.dispose();
    // No need to dispose a SignalSelector because it disposes automatically when the parent disposes
    // doubleCount.dispose();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Derived Signals'),
        actions: [
          SignalBuilder(
              signal: nullOrIntSignal,
              builder: (context, value, child) {
                return Text(
                  nullOrIntSignal.hasPreviousValue ? 'true' : 'false',
                  style: nullOrIntSignalStyle,
                );
              }),
          const SizedBox(width: 20),
          SignalBuilder(
              signal: count,
              builder: (context, value, child) {
                return Text(count.hasPreviousValue ? 'true' : 'false');
              }),
          SignalBuilder(
              signal: doubleCount,
              builder: (context, value, child) {
                return Text(doubleCount.hasPreviousValue ? 'true' : 'false');
              }),
        ],
      ),
      body: Center(
        child: Column(
          mainAxisSize: MainAxisSize.min,
          children: [
            SignalBuilder(
                signal: nullOrIntSignal,
                builder: (_, value, __) {
                  return Column(
                    children: [
                      ElevatedButton(
                          onPressed: () {
                            nullOrIntSignal.value = (9);
                          },
                          child: const Text(
                            'change',
                            style: nullOrIntSignalStyle,
                          )),
                      Text(
                        'NullOrIntvalue: $value',
                        style: nullOrIntSignalStyle,
                      ),
                      Text(
                        'Previous NullOrIntvalue: ${nullOrIntSignal.previousValue}',
                        style: nullOrIntSignalStyle,
                      )
                    ],
                  );
                }),
            const SizedBox(height: 32),
            SignalBuilder(
                signal: count,
                builder: (_, value, __) {
                  return Column(
                    children: [
                      Text('Count: $value'),
                      Text('Previous: ${count.previousValue}')
                    ],
                  );
                }),
            SignalBuilder(
                signal: doubleCount,
                builder: (_, value, __) {
                  return Column(
                    children: [
                      const Text('----------------'),
                      Text('Double Count: $value'),
                      Text('Previous: ${doubleCount.previousValue}')
                    ],
                  );
                }),
            const SizedBox(height: 16),
          ],
        ),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: () {
          count.value++;
        },
        child: const Icon(Icons.add),
      ),
    );
  }
}

The nullOrIntSignal and count signals works as expected, but the doubleCount signal does not.

In the following screenshot

the last element in the app bar is the hasPreviousValue value of doubleCount. It is supposed to be initially false (and its value should become true only after doubleCount value gets changed, i.e., when count becomes 6). This can be fixed with the other PR.

However, what this PR fixes is the following:

Previous should be 10 and not 12. This happens because the current Computed.previousValue getter does not only get the previous value, but it also computes it.

Your implementation of previousValue seems to me to update the _previousValue (or maybe it does something else? I don't fully follow the logic...):

  /// The previous value, if any.
  @override
  T? get previousValue {
    final prevVal = _value;
    // get the actual value to cause observation
    final _ = value;
    return _previousValue = prevVal;
  }

If you run the code i shared above with the changes made with this and the other PR, you will see that everything is displayed correctly.

[manuel-plavsic]

By the way, you can ignore the nullOrIntSignal entirely. I was manually testing Signal with null as well, that is why it is there.

[manuel-plavsic]

I think I understand what is causing hasPreviousValue to start with true. It is SignalBuilder. So, if in the app bar code, you replace:

SignalBuilder(
    signal: doubleCount,
    builder: (context, value, child) {
      return Text(doubleCount.hasPreviousValue ? 'true' : 'false');
    }),

with:

Text(doubleCount.hasPreviousValue ? 'true' : 'false'),

you will see that it starts correctly with false (however, without the builder it is not possible to rebuild on update).

So, consuming the Computed with a builder causes at least one initial update.

[nank1ro]

Thanks for your detailed report 👏. I'm on it. There's surely something that doesn't work correctly. Will update you when I have news

[nank1ro]

Hi @manuel-plavsic can you check and try this PR ? I did all the fixes and now seems working as expected. I also simplified the logic so it should work fine also with the IDE.

[manuel-plavsic]

I really like your PR, especially the simplified logic. I am closing here.