rodydavis / signals.dart

Reactive programming made simple for Dart and Flutter
http://dartsignals.dev
Apache License 2.0
438 stars 50 forks source link

Unsure about dispose #191

Closed guygit closed 6 months ago

guygit commented 7 months ago

In Flutter I have created this function outside of a widget: `

textEditingController() {

 final ctrl = TextEditingController();

 final s = signal(ctrl, autoDispose: true);

 s.onDispose(() {

   debugPrint("s.onDispose");

   ctrl.dispose();

 });

 return s;

}`

USAGE in Stateless Widget: return Watch((context) { final keyboardApi = KeyboardApi(textEditingController: textEditingController().value);

Question: When the Widget goes out of scope, I cannot see the "s.onDispose" message. Does that mean, the TextEditingController is not getting disposed?

Thank you!

rodydavis commented 7 months ago

I can write some tests for this use case to see if this is desired behavior.

Can you make a complete example to test?

jinyus commented 7 months ago

This line is the issue:

final keyboardApi = KeyboardApi(textEditingController: textEditingController().value);

You're creating a new instance on every rebuild so it's a memory leak. Call this method outside of the build method.

guygit commented 7 months ago

@jinyus & @rodydavis thank you for getting back on this!

@jinyus: Now, in hindsight - man, yes - that is absolutely the case :-)

I think I was driven away by the dream to be able to use it like with Hooks (useTextEditingController(...)) where the controller can be used in a Stateless Widget without having to bother to manually dispose it.

Do you guys think a similar (handy) way would be possible with Signals?

I am asking this, because Signals - even without that possibility - makes the Flutter code so much more readable and clean.

Anyways - thanks for the the feedback - appreciate !

jinyus commented 7 months ago

The only ways I'm currently seeing is to create a mutable global signal (bad practice) or using a stateful widget (kinda defeats purpose).

[!WARNING] This is just a proof of concept; I don't recommend writing code like this. The code isn't tested either.

This is how it'd be done...basically, it only creates a new instance if the old one was disposed.

// global variable
Signal<TextEditingController>? s;

class SomeWidget extends StatelessWidget{

    @override
    Widget build(BuildContext context){
          s = s == null || s!.disposed
          ? (signal(TextEditingController(), autoDispose: true)
            ..onDispose(() {
              print('disposing');
              s?.value.dispose();
            }))
          : s;
    }
}
rodydavis commented 7 months ago

You could also do the same with an Expando:

https://api.flutter.dev/flutter/dart-core/Expando-class.html

But would use the garbage collector. You would need a finalized for triggering the dispose if you didn't use a watch in the build method

jinyus commented 7 months ago

Update: This actually works, not sure how I feel about it. @rodydavis

Downside: It can only create 1 TextEditingController per widget. This could be remedied by requiring an id as well.

final useTextController = signalContainer(
  (BuildContext context) {
    late final Signal<TextEditingController>? s;

    s = signal(TextEditingController(), autoDispose: true)
      ..onDispose(() {
        print('disposing text controller');
        s?.value.dispose();
      });

    return s;
  },
  cache: true,
);
class TestScreen extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final controller = useTextController(context);
    return Watch.builder(
      builder: (context) {
        return Column(
          children: [
            Center(
              child: TextField(
                controller: controller(),
              ),
            ),
            TextButton(
              onPressed: () {
                print(controller().text);
              },
              child: const Text('show text'),
            ),
          ],
        );
      },
    );
  }
}

PS: It doesn't work when using 'mysignal.watch(context)' or when I call the controller within the Watch builder function.

guygit commented 7 months ago

@jinyus @rodydavis - man, thanks a lot for your feedback on this - I didn't expect such high quality answers!

guygit commented 7 months ago

@jinyus & @rodydavis I just tested it with 2 Widgets - works ... mindblowing!

I wondered why this "late " & nullable definition, but I guess that is needed when "dispose" kicks in?!

Very smart code indeed

jinyus commented 7 months ago

It actually doesn't have to be nullable. It's done so I can access the value in the dispose callback.

guygit commented 7 months ago

ah - thx - still learning Dart :-) ... i can see now, written in one go, you wouldn't have had access to "s".

For me, this little snippet shows how much power there is in this "signals" lib - absolutely crazy.

rodydavis commented 7 months ago

Honestly I think it is a good use case for signal container. And I think I can fix it some to work correctly.

rodydavis commented 7 months ago

Also related: https://github.com/rodydavis/signals.dart/issues/192

jinyus commented 7 months ago

Yea, container can be used to implement useSignal(https://github.com/rodydavis/signals.dart/pull/146) That's what I've been trying to do with my pkg but the 1 type per widget is limiting, that's the main problem I'm tackling right now.

rodydavis commented 7 months ago

Also to rework your code a bit:

final useTextController = signalContainer(
  (BuildContext context) {
    final s = signal(TextEditingController(), autoDispose: true);
    s.onDispose(() {
      print('disposing text controller');
      s.value.dispose();
    });
    return s;
  },
  cache: true,
);
rodydavis commented 7 months ago

Got an example working with the Finalizer which does not need autoDispose to be true:

class CounterExample extends StatelessWidget {
  const CounterExample({super.key});

  @override
  Widget build(BuildContext context) {
    final counter = createCounter(this);
    return Scaffold(
      appBar: AppBar(
        title: const Text('Counter Example'),
      ),
      body: Column(
        children: [
          Watch.builder(builder: (context) {
            return Text('Count: $counter');
          }),
          ElevatedButton(
            onPressed: () {
              counter.value++;
            },
            child: const Text('Increment'),
          ),
        ],
      ),
    );
  }
}

final createCounter = signalContainer((Widget e) {
  final inner = signal(0);
  print('creating signal: ${inner.globalId}');
  inner.onDispose(() {
    print('disposing signal: ${inner.globalId}');
  });
  final s = CounterSignal(signal(0), _finalizer);
  _finalizer.attach(s, inner, detach: s);
  return s;
});

final _finalizer = Finalizer<ReadonlySignal>((s) => s.dispose());

class CounterSignal implements Signal<int> {
  CounterSignal(this._signal, this._finalizer);
  final Finalizer<ReadonlySignal> _finalizer;
  final Signal<int> _signal;

  @override
  int get globalId => _signal.globalId;

  @override
  int get value => _signal.value;

  @override
  set value(int v) => _signal.value = v;

  @override
  bool get disposed => _signal.disposed;

  @override
  set disposed(bool d) => _signal.disposed = d;

  @override
  bool get autoDispose => _signal.autoDispose;

  @override
  int call() => _signal();

  @override
  String? get debugLabel => _signal.debugLabel;

  @override
  void dispose() {
    _finalizer.detach(this);
    _signal.dispose();
  }

  @override
  int get() {
    return _signal.get();
  }

  @override
  int get initialValue => _signal.initialValue;

  @override
  void onDispose(void Function() cleanup) {
    _signal.onDispose(cleanup);
  }

  @override
  int peek() {
    return _signal.peek();
  }

  @override
  int get previousValue => _signal.previousValue;

  @override
  ReadonlySignal<int> readonly() {
    return _signal.readonly();
  }

  @override
  void set(int value, {bool force = false}) {
    _signal.set(value, force: force);
  }

  @override
  EffectCleanup subscribe(void Function(int value) fn) {
    return _signal.subscribe(fn);
  }

  @override
  int toJson() {
    return _signal.toJson();
  }

  @override
  String toString() {
    return _signal.toString();
  }
}
Kypsis commented 7 months ago

Slightly OT but Finalizer has got to have the least confidence inspiring description ever whether it actually fires ("maybe").

rodydavis commented 7 months ago

In my tests it was firing a lot, but correct it is not a guarantee

jinyus commented 7 months ago

It's not guaranteed because there are cases where the VM isn't obligated to free the memory (force close by user or OS). In the context of flutter, it will be called if/when the object is GC'ed. You just have to make sure the object is collectable (not accessible by any other objects). The only issue is that it's not deterministic so it's hard to get coverage in tests.