Refactor StrokeAlign to allow double values. by bernaferrari · Pull Request #108339 · flutter/flutter

bernaferrari · 2022-07-26T01:38:55Z

This is radical, this is breaking, this is awesome. cc @gspencergoog

Edit: technically it is not breaking, I only break a single test (that is dumb and might or might not be fixed by another PR). But StrokeAlign change is really big.

Screen.Recording.2022-07-26.at.00.59.44.mov

Also, I made a completely new sample:

Click to see the sample code

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData.dark(),
      home: const Scaffold(body: HomePage()),
    );
  }
}

class HomePage extends StatefulWidget {
  const HomePage({Key? key}) : super(key: key);

  @override
  State<HomePage> createState() => _HomePageState();
}

enum Shapes {
  roundedRectangleBorder,
  roundedinside20,
  roundedcenter30,
  roundedOutside40,
  circleBorder,
  ovalBorder,
  stadiumBorder
}

class _HomePageState extends State<HomePage> {
  double sliderValue = 0;
  Shapes from = Shapes.roundedinside20;
  Shapes to = Shapes.roundedOutside40;

  OutlinedBorder getBorder(Shapes shape) {
    switch (shape) {
      case Shapes.roundedRectangleBorder:
        return RoundedRectangleBorder(
          borderRadius: BorderRadius.circular(20),
          side: BorderSide(
              color: Colors.green.withOpacity(0.5),
              width: 20,
              strokeAlign: StrokeAlign.center),
        );
      case Shapes.roundedinside20:
        return RoundedRectangleBorder(
          borderRadius: BorderRadius.circular(5),
          side: BorderSide(
              color: Colors.purple.withOpacity(0.5),
              width: 20,
              strokeAlign: StrokeAlign.inside),
        );
      case Shapes.roundedcenter30:
        return RoundedRectangleBorder(
          borderRadius: BorderRadius.circular(10),
          side: BorderSide(
              color: Colors.blue.withOpacity(0.5),
              width: 30,
              strokeAlign: StrokeAlign.center),
        );
      case Shapes.roundedOutside40:
        return RoundedRectangleBorder(
          borderRadius: BorderRadius.circular(15),
          side: BorderSide(
              color: Colors.red.withOpacity(0.5),
              width: 40,
              strokeAlign: StrokeAlign.outside),
        );
      case Shapes.circleBorder:
        return CircleBorder(
          side: BorderSide(
            color: Colors.green.withOpacity(0.5),
            width: 20,
            strokeAlign: StrokeAlign.center,
          ),
        );
      case Shapes.ovalBorder:
        return OvalBorder(
          side: BorderSide(
            color: Colors.yellow.withOpacity(0.5),
            width: 20,
            strokeAlign: StrokeAlign.center,
          ),
        );
      case Shapes.stadiumBorder:
        return StadiumBorder(
          side: BorderSide(
              color: Colors.yellow.withOpacity(0.5),
              width: 20,
              strokeAlign: StrokeAlign.center),
        );
    }
  }

  @override
  Widget build(BuildContext context) {
    return Column(
      crossAxisAlignment: CrossAxisAlignment.center,
      mainAxisAlignment: MainAxisAlignment.center,
      children: [
        Container(
          width: 400,
          height: 200,
          decoration: ShapeDecoration(
            shape: getBorder(from).lerpTo(getBorder(to), sliderValue) ??
                RoundedRectangleBorder(),
            color: Colors.white,
          ),
        ),
        const SizedBox(height: 40),
        Slider(
            // divisions: 10,
            value: sliderValue,
            onChanged: (value) {
              setState(() {
                sliderValue = value;
              });
            }),
        Row(
          crossAxisAlignment: CrossAxisAlignment.center,
          mainAxisAlignment: MainAxisAlignment.center,
          children: [
            Text("From"),
            SizedBox(width: 10),
            DropdownButton<Shapes>(
                value: from,
                items: [
                  for (final shape in Shapes.values)
                    DropdownMenuItem(
                      value: shape,
                      child: Text(shape.toString()),
                    ),
                ],
                onChanged: (value) {
                  setState(() {
                    from = value!;
                  });
                }),
            SizedBox(width: 20),
            Text("To"),
            SizedBox(width: 10),
            DropdownButton<Shapes>(
                value: to,
                items: [
                  for (final shape in Shapes.values)
                    DropdownMenuItem(
                      value: shape,
                      child: Text(shape.toString()),
                    ),
                ],
                onChanged: (value) {
                  setState(() {
                    to = value!;
                  });
                }),
            SizedBox(width: 10),
            Text("${(sliderValue * 100).toStringAsFixed(0)}%"),
          ],
        ),
      ],
    );
  }
}

gspencergoog · 2022-07-26T21:36:39Z

This is great. It's not technically breaking if it doesn't break any tests in customer_testing or in Google internal tests. Since it's a new addition, it probably won't.

packages/flutter/lib/src/painting/borders.dart

bernaferrari · 2022-07-26T23:01:33Z

I just made some (many) changes based on your comments. I love the double strokeAlign, but I'm still not sure I want to get rid of StrokeAlign. Can't we have a global StrokeAlign.inside = 0.0? I think BorderSide.strokeAlignInside is too weird.

gspencergoog · 2022-07-26T23:19:46Z

I don't think BorderSide.strokeAlignInside is that weird, we have lots of places where there are const defaults for double values, and it's a descriptive name.

And it seems like waaay overkill to have an entire class that effectively is a namespace for three doubles. It makes it more complicated to assign values to the strokeAlign value too:

BorderSide(strokeAlign: StrokeAlign(value));

vs

BorderSide(strokeAlign: value);

Seems more complicated for no benefit.

bernaferrari · 2022-07-26T23:21:35Z

I simplified further, now: BorderSide(strokeAlign: StrokeAlign.inside) or BorderSide(strokeAlign: 0.5). I removed StrokeAlign(0.5). The opposite, strokeAlign: BorderSide.strokeAlignInside feels like a secret API.

But yeah, I might be able to remove it :( but will hurt.

gspencergoog · 2022-07-27T00:09:25Z

One other thing to consider is that you might want to allow values outside of -1 and 1, so that you can animate the border with a bounce animation or other animation that goes slightly outside the bounds. The equations I gave you should mostly just work for that, although if people go too crazy, it'll result in some odd behavior (rects that get deflated to zero size, for instance).

bernaferrari · 2022-07-27T00:10:08Z

The math for -1, 0, 1 is very hard to understand, but I think I got it working. Need to test if getInnerPath is working correctly later.

bernaferrari · 2022-07-27T00:11:39Z

I thought about that. I think we should set loose the constraints and wait to see what shows up. Technically there isn't a difference from changing that to changing the width, it is just augmenting it.

But, if people want to have fun... committing.

gspencergoog · 2022-07-27T03:48:52Z

Hey, @bernaferrari this looked like so much fun, I started playing with your code, and made my own branch with some changes. There's also some example code you might want to grab.

Here's my branch: https://github.com/gspencergoog/flutter/tree/stroke_align (and the diff if you'd rather look at that).

The sample looks like this when it runs:

ThrobWithBoundaries.mov

Throb.mov

Not intending it as a replacement for yours, just some ideas for you, feel free to appropriate any/all/none of it. Particularly useful, I think, were the new getters I added on BorderSide for strokeInset, strokeOutset, and strokeOffset.

bernaferrari · 2022-07-27T03:58:40Z

I'll check soon (need to sleep), but congrats, that's awesome! I told you it is fun.. lol this is the most fun I've had in a while.

BTW, did you get any progress in the milter idea? I'm still wondering what that was, lol.

I still think you should rewrite ContinousRectangleBorder into something useful. It is the worst border by far and doesn't support any StrokeAlign 👀. Your Polygon code + this PR = a border everybody will love.

I opened the link and I'm already crying here. I wanted the squared border from Figma, you wanted the strokeAlign as double, I got the PR, you got the -1, 0, 1. Now we need to decide who is going to get the StrokeAlign and who is going to give up. lol

bernaferrari · 2022-07-27T16:36:26Z

If you do this in your test, you will see some small weirdness:

static final Color borderColor = Colors.red.shade500.withOpacity(0.50);

gspencergoog · 2022-07-27T16:42:47Z

If you do this in your test, you will see some small weirdness

Yes, perhaps it needs some more work.

bernaferrari · 2022-07-27T17:54:49Z

That bug also happened in my branch, forget it. I think it is a bug in drawPath rounding almost-square to square (or something like this). I've seen it happening in other places 😢.

Ok, so fantastic work!

strokeInset and strokeOutset are exactly what I wanted when I made the similar but bad named lerpStrokeAlignDecreasing lol.

I'm merging all of your branch here (* except for something I'm still too connected to let it go, today 🤣).

Something I still think we need to fix is make this max 0. Negative EdgeInset isn't valid, right?

bernaferrari · 2022-07-27T19:01:11Z

The Diagnosticable seems reasonably big. Do we expect a random number after BorderSide now?

gspencergoog · 2022-07-27T19:09:50Z

The Diagnosticable seems reasonably big. Do we expect a random number after BorderSide now?

We use Diagnosticable on all the widgets, and many other classes, to handle formatting of the debug strings in a consistent way. The "random number" is part of the object hash, so that you can tell if two instances of the object are the same instance or not. It's not big in the runtime sense: it doesn't have any data members, and only costs something when toString is called, which is only used in debug builds anyhow.

bernaferrari · 2022-07-27T19:10:54Z

Ok, I'll fix our tests, but I feel you will need to patch Google. That will probably be our breaking change.

BTW, how do we get the merge working? You told me we forgot in StrokeAlign. Should I take a mean of the StrokeAligns? Trying.

bernaferrari · 2022-07-27T19:47:16Z

I don't know how to hash things, shouldn't the hash value be always the same?
Due to this, Diagnosticable result is random.
If I do Object.hash(1, 1) in a test and execute from terminal twice, the results are different.

gspencergoog · 2022-07-27T22:02:30Z

The Object hash includes a component for the base object instance's location in memory, and since the object has a new instance each time, it appears random.

bernaferrari · 2022-07-27T22:06:19Z

Is it possible to test that? Because there some tests that expect toString().

gspencergoog · 2022-07-27T22:08:05Z

Yes, use expect(foo.toString(), equalsIgnoringHashCodes('Expected#00000()'));

bernaferrari · 2022-07-27T22:11:37Z

How would that work here?

gspencergoog · 2022-07-27T22:22:21Z

Ahh. It won't. OK, that's annoying. Instead, just add this to BorderSide:

  @override
  String toStringShort() => 'BorderSide';

bernaferrari · 2022-07-28T03:31:05Z

Thanks. Aaaaalmost there!

So, I changed these to be the same default as BorderSide:

But now it produces BorderSide (instead of BorderSide()) and BorderSide(width: 0.0, style: none) (instead of style: BorderStyle.none. Any ideas on how to solve it?

Remove `width == 0.0` so it follows the same pattern as others.

HansMuller

LGTM

This reverts commit dd8573b.

CaseyHillers · 2022-08-16T06:16:00Z

FYI this broke many tests, I've uploaded a revert in #109591. @gspencergoog and @HansMuller can you help with the reland to prevent these breakages?

bernaferrari · 2022-08-16T13:37:04Z

How bad is it? Is it a golden, a bug from the code, the StrokeAlign or something else?

…#108339)

HansMuller · 2022-08-16T15:27:13Z

This PR removed the StrokeAlign enum. That's a breaking change and it caused many internal test failures due to a StrokeAlign dependency (and see https://docs.flutter.dev/resources/compatibility). In this case a small G3Fix would probably enable this PR to land.

Failures reported in Google-internal issue b/242668688

gspencergoog · 2022-08-16T16:18:43Z

OK, I'll take care of re-landing it.

…#108339)

…0234)

flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 26, 2022

bernaferrari force-pushed the StrokeAlign-v2 branch from c762ffe to 862b931 Compare July 26, 2022 04:47

bernaferrari mentioned this pull request Jul 26, 2022

Improve StrokeAlign on RoundedRectangleBorder #106368

Closed

gspencergoog reviewed Jul 26, 2022

View reviewed changes

packages/flutter/lib/src/painting/borders.dart Outdated Show resolved Hide resolved

bernaferrari force-pushed the StrokeAlign-v2 branch from 862b931 to 9744387 Compare July 26, 2022 22:59

bernaferrari force-pushed the StrokeAlign-v2 branch from 36ca9ef to f82f51a Compare July 27, 2022 18:00

bernaferrari added 13 commits August 15, 2022 15:45

Rename strokeOffset -> halfStrokeOffset

d562415

Imprrove documentation.

7e97fd0

Fix doc.

97a396d

Fix BoxDecoration with borderRadius.

aba0366

Re-add StrokeAlign class. It is back. For now.

30b917f

Remove `width == 0.0` so it follows the same pattern as others.

Fix PR comments.

d1f571e

Fix analyzer.

acb25ac

Rename description

1cad490

Add "for example".

e2e877d

Fix typo.

172ab57

Bring back BorderSide.strokeAlignInside.

8106006

Fix test

7aa2408

Fix PR comments.

40f96e5

HansMuller approved these changes Aug 15, 2022

View reviewed changes

bernaferrari force-pushed the StrokeAlign-v2 branch from 9166b32 to 40f96e5 Compare August 15, 2022 18:57

bernaferrari added 2 commits August 15, 2022 16:12

Fix redundant import.

c6f42aa

Fix test (ops).

4eda7a3

gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 15, 2022

auto-submit bot merged commit dd8573b into flutter:master Aug 15, 2022

CaseyHillers added a commit that referenced this pull request Aug 16, 2022

Revert "Refactor StrokeAlign to allow double values. (#108339)"

f3db3a1

This reverts commit dd8573b.

CaseyHillers mentioned this pull request Aug 16, 2022

Revert "Refactor StrokeAlign to allow double values." #109591

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2022

dd8573b Refactor StrokeAlign to allow double values. (flutter/flutter…

146653c

…#108339)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 17, 2022

dd8573b Refactor StrokeAlign to allow double values. (flutter/flutter…

a01c986

…#108339)

bernaferrari mentioned this pull request Aug 22, 2022

I don't think Google Testing is working #110031

Closed

gspencergoog mentioned this pull request Aug 25, 2022

Re-Land "Refactor StrokeAlign to allow double values." (#108339) #110234

Merged

auto-submit bot pushed a commit that referenced this pull request Aug 25, 2022

Re-Land "Refactor StrokeAlign to allow double values." (#108339) (#11…

fefb2b0

…0234)

Conversation

bernaferrari commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog commented Jul 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bernaferrari commented Jul 26, 2022

Uh oh!

gspencergoog commented Jul 26, 2022

Uh oh!

bernaferrari commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog commented Jul 27, 2022

Uh oh!

bernaferrari commented Jul 27, 2022

Uh oh!

bernaferrari commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bernaferrari commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bernaferrari commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog commented Jul 27, 2022

Uh oh!

bernaferrari commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bernaferrari commented Jul 27, 2022

Uh oh!

gspencergoog commented Jul 27, 2022

Uh oh!

bernaferrari commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bernaferrari commented Jul 27, 2022

Uh oh!

gspencergoog commented Jul 27, 2022

Uh oh!

bernaferrari commented Jul 27, 2022

Uh oh!

gspencergoog commented Jul 27, 2022

Uh oh!

bernaferrari commented Jul 27, 2022

Uh oh!

gspencergoog commented Jul 27, 2022

Uh oh!

bernaferrari commented Jul 28, 2022

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

CaseyHillers commented Aug 16, 2022

Uh oh!

bernaferrari commented Aug 16, 2022

Uh oh!

HansMuller commented Aug 16, 2022

Uh oh!

gspencergoog commented Aug 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bernaferrari commented Jul 26, 2022 •

edited

Loading

bernaferrari commented Jul 26, 2022 •

edited

Loading

bernaferrari commented Jul 27, 2022 •

edited

Loading

gspencergoog commented Jul 27, 2022 •

edited

Loading

bernaferrari commented Jul 27, 2022 •

edited

Loading

bernaferrari commented Jul 27, 2022 •

edited

Loading

bernaferrari commented Jul 27, 2022 •

edited

Loading

bernaferrari commented Jul 27, 2022 •

edited

Loading