mobxjs / mobx.dart

MobX for the Dart language. Hassle-free, reactive state-management for your Dart and Flutter apps.
https://mobx.netlify.app
MIT License
2.4k stars 309 forks source link

Observable `CustomPainter` (willing to contribute code) #908

Open SecondFlight opened 1 year ago

SecondFlight commented 1 year ago

Hi! Thank you for this library. It has helped me increase my productivity and greatly simplify my state and logic code, and it has quickly become my favorite state management solution.

I'm writing an app with a lot of CustomPaint widgets that need to draw things based on MobX model objects. Initially, my strategy was to just unwrap the model items inside an Observer builder, like so:

Observer(
  builder: (context) {
    return CustomPaint(
      painter: MyCustomPainter(
        field1: modelItem.field1,
        field2: modelItem.field2,
        field3: modelItem.field3,
        // ...
      )
    );
  }
);

This works okay, but it doesn't scale. For instance, if modelItem has a list of child model items, then my strategy was to create an immutable proxy object for those child items and pass a list of those, which allowed me to handle changes in shouldRepaint(). This works fine, but it requires a lot of boilerplate, so I've been working on an alternative way to handle this.

I would be interested in contributing code to provide something like a CustomPainterObserver, but I may need some guidance. For my own project I've coded something that accomplishes this: https://github.com/SecondFlight/anthem/blob/fe2d8346d9b046dddcffd0d9c989fd0dd2b27ca4/lib/widgets/basic/mobx_custom_painter.dart

It can be used like so:

class MyPainter extends CustomPainterObserver {
  MyMobxObject someObj;

  MyPainter(this.someObj);

  @override
  void observablePaint(Canvas canvas, Size size) {
    // Paint here. Observables are watched and updates to the watched
    // observables will trigger a repaint.
  }

  @override
  bool shouldRepaint(covariant MyPainter oldDelegate) {
    return oldDelegate.someObj != someObj || super.shouldRepaint(oldDelegate);
  }
}

// ...

// In the widget tree...
CustomPaintObserver(
  painter: MyPainter(
    // Can pass MobX objects here and everything works as expected.
  ),
);

I'd be happy to submit this as-is, but there's a couple issues with it. First, it doesn't take semantics into account. This is pretty fixable, but the second issue is the bigger one in my view: it just feels clunky to me. This is especially true when comparing this implementation with the observer widget base classes, which are literally drop-in replacements, i.e. change the base class and you're good to go.

In contrast, my implementation requires the following:

  1. The CustomPaintObserver widget must be used in the tree.
  2. The CustomPainterObserver base class must be used.
  3. If shouldRepaint() is overridden, it must or (||) its result with super.shouldRepaint().
  4. observablePaint() must be overridden instead of paint().

I'd expect either 1 or 2 to be required but optimally not both, and it feels like I should be able to avoid the other requirements as well. I tried to address these in my initial implementation but kept running into roadblocks, and the implementation above was the first that worked for me.

In addition, there are some things I don't like about how my implementation works internally:

  1. My CustomPaintObserver itself renders a CustomPaint. This means that all constructor arguments need to be forwarded, and any future changes to CustomPaint need to be mirrored in CustomPaintObserver. It would be nice to avoid this maintenance overhead.
  2. In order to trigger a repaint the ReactionImpl triggers a rebuild, which causes the painter to re-evaluate. It feels like this should be more straightforward, something like finding the object in the render tree and marking it as dirty?

Please let me know any thoughts or alternative implementation suggestions. I'm happy to contribute, but I want to make sure I'm contributing high-quality code.

fzyzcjy commented 1 year ago

Just my two cents (I am a maintainer of the repo though not the main one; btw I know you from flutter_rust_bridge ;) )

  1. use delegate instead of inheritance: your CustomPainterObserver do not need to extend CustomPainter. Then there is no problem of "observablePaint", "|| super.shouldRepaint()" etc.
  2. btw try to use final fields