Fix Radio's painters - #170321

I don't get why this won't record correctly if reuse paint, looking a the code, it just call renderObject.paint on the TestRecordingPaintingContext which will record all the push layer. Can you explain a bit?

Sure!

The previous implementation was reusing the same Paint and was something similar to:

// Draw background.
final Paint paint = Paint()..color = Colors.transparent;
canvas.drawCircle(backgroundOffset, backgroundRadius, paint);

// Draw border.
paint..color = Colors.red; // Red for simplicity here
canvas.drawCircle(borderOffset, borderRadius, paint);

I wanted to verify the background and the border were painted, for that, I was using mock canvas with:

expect(
  radioFinder,
  paints
    ..circle(color: Colors.transparent)
    ..circle(color: Colors.red),
);

But it was failing, it couldn't "find" the ..circle(color: Colors.transparent) part and the reason was because the reuse of the reference of Paint paint.

TestRecordingPaintingContext records the invocations and here is what was happening during a test:

/*
There are no invocations recorded yet:
invocations: []
*/

// Draw background.
final Paint paint = Paint()..color = Colors.transparent; 
canvas.drawCircle(backgroundOffset, backgroundRadius, paint);

/*
drawCircle is recorded with the paint reference (#123 here) and the color transparent.
invocations: [MethodInvocation(#drawCircle, Paint#123(Colors.transparent))] 
*/

// Draw border.
paint..color = Colors.red; // Red for simplicity here

/*
The paint object is mutated, which ends up modifying the invocation list:
invocations: [MethodInvocation(#drawCircle, Paint#123(Colors.red))] 
*/

canvas.drawCircle(borderOffset, borderRadius, paint);

/*
drawCircle is recorded with the same paint reference (#123 here) and the color red.
invocations: [
  MethodInvocation(#drawCircle, Paint#123(Colors.red)),
  MethodInvocation(#drawCircle, Paint#123(Colors.red)),
] 
*/

So the test:

expect(
  radioFinder,
  paints
    ..circle(color: Colors.transparent)
    ..circle(color: Colors.red),
);

fails and

expect(
  radioFinder,
  paints
    ..circle(color: Colors.red)
    ..circle(color: Colors.red),
);

passes.

That's why I changed it to not reuse the same Paint

Having said this, do my changes still make sense? Or was there a better thing to do I missed?

Thanks for the thorough explanation here! Is the reason why the invocation list changes simply because it is holding a weak reference to the Paint or something like that? I'm just trying to understand the details here. Maybe we can explain this in the docs somewhere.

Yes, if I apply the same example (using a transparent circle for the background and a red circle for the border) the recorded invocation list was:

// invocations: [
  MethodInvocation(#drawCircle, Paint#123(Colors.red)),
  MethodInvocation(#drawCircle, Paint#123(Colors.red)),
] 

The tests

expect(
  radioFinder,
  paints.circle(color: Colors.red),
);

successfully passed.

However, after my modification, the invocation list changed to:

// invocations: [
  MethodInvocation(#drawCircle, Paint#123(Colors.transparent)),
  MethodInvocation(#drawCircle, Paint#123(Colors.red)),
] 

Consequently, the tests failed until I updated them to:

expect(
  radioFinder,
  paints
    ..circle(color: Colors.transparent)
    ..circle(color: Colors.red),
);

Does that clear your question?

Maybe we can explain this in the docs somewhere.

Sure, where did you picture the doc to be? A comment in paint method in _RadioPainter?

Hello everyone,

Does my explanation address your questions? How do you envision the documentation?

It seems a bit odd to me to include comments about the tests in the paint method of _RadioPainter. What are your thoughts?

Sorry for the delay here. I think this PR is fine as-is. I've made a note about this in the docs for paints here: #171180

I think this pr is fine as-is, but I think we should fix the mock canvas to be able to handle this. we can continue the discussion in other pr

nIcE 👍

Thanks for the thorough explanation here! Is the reason why the invocation list changes simply because it is holding a weak reference to the Paint or something like that? I'm just trying to understand the details here. Maybe we can explain this in the docs somewhere.

Sorry for the delay here. I think this PR is fine as-is. I've made a note about this in the docs for paints here: #171180