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

Observable CustomPainter (willing to contribute code) #908

Open
SecondFlight opened this issue Mar 24, 2023 · 1 comment
Open

Observable CustomPainter (willing to contribute code) #908

SecondFlight opened this issue Mar 24, 2023 · 1 comment

Comments

@SecondFlight
Copy link

SecondFlight commented Mar 24, 2023

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.

@SecondFlight SecondFlight changed the title Observable CustomPainter Observable CustomPainter (willing to contribute code) Mar 24, 2023
@fzyzcjy
Copy link
Collaborator

fzyzcjy commented Mar 24, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants