Is it possible to somehow convert Path..addOval() to drawOval instead of drawPath? This would solve the tests.

We could consider adding void paintFill(Canvas, Rect, Paint, {TextDirection}) to ShapeBorder, whose default implementation is just canvas.drawPath(getOuterPath(rect, textDirection: textDirection))... of course then we would lose the precaching of _outerPath so that might actually be worse...

The biggest question is how to go from _RenderChip:<_RenderChip#c0abb relayoutBoundary=up10> to drawCircle or something like that. If only there were a universal solution for tests. There are not many broken tests (15, only 3 are not related to switch), I just don't know how to 'improve them'.

To fix the tests, you'll want to find the expectations that say paints..circle()..rect() or whatever, and adjust them to match what we paint now.

cc @goderbauer who owns the framework and may have opinions on whether this is worth doing.

Luckily there seem to be only a few of these cases, but I still have no idea how to go from a RenderThing to paint. Yes, we should adjust, but how do I "print" what a Render is painting so I can compare (and possibly adjust) the test? I don't know how to do this.

Comparing BoxDecoration with ShapeDecoration, I found that ShapeDecoration ALWAYS adds Padding to Container (usually zero). Therefore, I made it null when the decorator is used + padding zero (f4088d1). Container(padding: zero) will still render as Padding(Container) (I don't know why this is wanted, but I'm not changing this behavior).

who owns the framework and may have opinions on whether this is worth doing.

Seems like a nice cleanup. If it does not have any performance impact, I am for this change.

Luckily there seem to be only a few of these cases, but I still have no idea how to go from a RenderThing to paint.

Ignore the Actual: line, it's just that we can't put the right thing there. It's the last bit that's relevant. It should tell you what is being rendered, and you can change the paints... expectations to call the right methods based on what it lists. So far example, when it says:

Expected: Object or closure painting:
          'a rounded rectangle with Color(0x52000000), RRect: RRect.fromLTRBR(13.0, 17.0, 46.0,
31.0, 7.0)'
          'a circle with Color(0x33000000)'
          'a circle with Color(0x24000000)'
          'a circle with Color(0x1f000000)'
          'a circle with Color(0xfffafafa)'
  Actual: _RenderInkFeatures:<_RenderInkFeatures#61c17>
   Which: did not match the pattern.
          It called 5 other methods on the canvas, the first of which was drawPath(Instance of
'Path', Paint(Color(0x33000000))), but did not call drawCircle() at the time where a circle with
Color(0x33000000) was expected.

...you want to find where we expect(..., paints..drawCircle()) and replace it with expect(..., paints..drawPath()) (because Which: comment says it calls drawPath). Does that make sense? The more arguments you can add to these calls the better, too, since it'll test more. Look around for other tests that use paints to see what I mean.

Thanks. I'm doing this right now, already got many tests passing. Check 3cd5c23. Running flutter test locally to see what else is missing.

Edit: it seems there is only one test remaining, expansion tile. I'll check later.

Tests now pass 😀

Next up: get some benchmark.

Benchmarks

We have benchmarks!!!! I made a Switch (the widget that's by far the most affected by this change) many times, applied an animation, and executed the benchmark: 10x, 100x, 1000x, many times.

@Hixie is right. ShapeDecoration can be 20% slower than BoxDecoration. But I'm not giving up. I compared ShapeDecoration with BoxDecoration and found the biggest difference. It is not preCache. It is path! DrawPath is slow. DrawCircle is fast. Optimizing _paintInterior gave a bit of performance, but it was still much slower. paintShadows gave the boost we needed. I'm not killing pre-cache, I'm just changing it to pre-cache the bounds instead of the path (when possible, else do everything else as always).

While optimizing Circle, I optimized Oval and while optimizing Oval I also optimized RoundedRectangle. You know what, I'm changing everything anyway.

First optimized benchmark with Switch (now uses CircleBorder):

Then RoundedRectangleBorder:

Implementation details:

I added a function and a getter to ShapeBorder (Hixie is going to kill me, twice): hasOptimizedPaintBox and paintBox. I'm too mentally exhausted to think in better names.
paintBox calls canvas.drawRect, canvas.drawCircle, etc. It "replaces" getOuterPath for 80% of the borders. They don't need it anymore (but it is still there, of course).

This is how it works:

• ShapeDecoration wants to draw a circle, paintBox is called.
• ShapeDecoration wants to draw a path, same cache as always.
  To control this flow, there is hasOptimizedPaintBox. When true, ShapeBorder doesn't cache the Path and calls paintBox (which, if it has drawPath, might be slower, I didn't test, but will work fine). I wish paintBox were an optional function that we could check if it exists before caching or not. I don't think Dart allows this, so the second variable is doing this.

As a side effect from my last change, I'm reverting the tests and fixing others.

Instead of changing circle to path, now we are going from path to rrect! It seems a great change.

MaterialApp(
      home: Scaffold(
        body: Wrap(children: [
          for (int i = 0; i < 100; i++)
            SizedBox(
                width: 100,
                height: 100,
                child: Center(
                  child: _RotatingWidget(
                    child: Container(
                      width: 50,
                      height: 50,
                      decoration: BoxDecoration(
                        color: Colors.red,
                        // shape: RoundedRectangleBorder(
                        borderRadius: BorderRadius.circular(25),
                      // )
                      ),

                    ),
                  ),
                )),
        ]),
      ),
    );

// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
import 'package:flutter/material.dart';

import 'recorder.dart';

/// Measures expense of constructing Image widgets.
class BenchBuildCircle extends WidgetRecorder {
  BenchBuildCircle() : super(name: benchmarkName);

  static const String benchmarkName = 'build_circle';

  @override
  Widget createWidget() {
    return MaterialApp(
      home: Scaffold(
        body: Wrap(children: [
          for (int i = 0; i < 100; i++)
            SizedBox(
                width: 100,
                height: 100,
                child: Center(
                  child: _RotatingWidget(
                    child: Switch(value: true, onChanged: (value) {}),
                  ),
                )),
        ]),
      ),
    );
  }
}

class _RotatingWidget extends StatefulWidget {
  const _RotatingWidget({required this.child});

  final Widget child;

  @override
  _RotatingWidgetState createState() => _RotatingWidgetState();
}

class _RotatingWidgetState extends State<_RotatingWidget>
    with SingleTickerProviderStateMixin {
  late AnimationController controller;

  @override
  void initState() {
    super.initState();
    controller = AnimationController(
      duration: const Duration(milliseconds: 200),
      vsync: this,
    )..repeat();
  }

  @override
  void dispose() {
    controller.dispose();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return AnimatedBuilder(
      animation: controller,
      builder: (BuildContext context, Widget? child) {
        return Transform.rotate(
          angle: 2 * 3.14 * controller.value,
          child: widget.child,
        );
      },
    );
  }
}

I added a default (empty) implementation for paintBox function so it doesn't break existing code (and probably most external usage will be limited, no one will re-implement RoundedRectangleBorder).

That's it, for now.

This sounds fantastic and I'll definitely look closer.

Thinking out loud, one option might be to have paintBox (or whatever we call it) return a boolean, default implementation returns false, and returning true means you did the work and don't need to use getOuterPath et al. That way you just always call it and don't need to have the decoupled boolean getter.

Sure, that's easy, but can you look the Google testing now? I'll be at home in a few hours, the failure will still be the same.

I think you have to resync first. The old test results are no longer available.

Thank you @bernaferrari for syncing this up. I can see the test results now. I'll take a look and let you know what I find.

Hey @bernaferrari just wanted to check back in to let you know I have been looking at the failing tests, but have not been able to create a minimal reproducible example yet. I'll let you know as soon as I figure it out.

Ok! Thanks. Looking forward to.

After exactly 8 months, I think I'll close this. Even though I think shape: BoxShape is extreeeemly confusing (since there is the shape: RoundedRectangleBorder) and I wish we could rename it for something better (material primary -> foreground was a lot less bad). I don't think we are not going to remove it (for now). Can we go with better names?

Meanwhile I am, against my will, giving BoxDecoration a reason to exist; I'm adding non-uniform borders there, since we can't have it in ShapeBorder (natively, at least, for now, at least. My package does it: https://github.com/bernaferrari/non_uniform_border). I still like the 'remove boxShape' idea, I'm just thinking on alternatives since it's been a while and we can't move forward.
Ref: #121921

Do you agree with the non-removal and request for a better name? I'm also fine if we remove it, ShapeDecoration is better, if we remove we free the user from a bunch of possible exceptions... but the PR has hit a roadblock for a few months now and no one knows how to solve the internal google bug. If you could also solve that bug, it would be great...

Thinking smaller, I wish:

• BoxDecoration(boxShadow: ) became BoxDecoration(shadows: ) (so it matches ShapeDecoration
• BoxDecoration(shape: ) became BoxDecoration(boxShape: ) (wouldn't conflict with shape from everywhere else)

We could make the migration with Dart fix, life would go on.. What do you think?

Yikes! this is taking ages. I was looking forward to this. :(

Ops, wrong button. I'm just thinking on alternative ways to have the issue "out of the way" and be "less confusing" for many people.. BoxDecoration.circle() could also be a possibility.

But if anyone figures out the internal Google problem, I'm all in.. I hope you can figure out. But so far, nothing.

@bernaferrari I just kicked off a rerun of all the tests, if any of them except Google Testing are failing, we should probably fix those first because otherwise the Google Testing shard will definitely fail anyway (by hitting the same problems).

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

👀

(triage): @bernaferrari looks like one of the customer tests failed. Do you still have plans to look into that and fix those up?

I could fix the customer tests, but Google testing is failing on avatar image and no one knows why.

@bernaferrari Do you want to update this PR to the latest master so we can do another google testing run to see what the current status is? Or have you decided to no longer pursue this and we should close it for now?

Sure, I'll. I just need someone brave enough to be able to see what is happening with AvatarImage lol

Hey @bernaferrari, we came across this PR during triage today. The problem with AvatarImage has eluded all of us, in spite of multiple attempts to diagnose the issue (over almost 2 years!). Regrettably, I don't know that we will be able to land this PR. Do you think we can close this? Or do you think there is another way we can approach this solution?

I truly appreciate all of the work you have put into this. Unfortunately we have not been able to find a way to land it.

Yeah, let's close it :( I can't do anything without external help and external help can't figure out.