It's a bit ambiguous what's the difference between this and _kCupertinoDialogFrontFillDecoration. Feel free to add more words to the variable name.

Do we need this? It'll end up getting clipped by the outermost cliprrect anyway right? (Just because there's a cost to these clips)

we shouldn't have any clips. Just draw the right shape.

Might be tricky with BackdropFilter

the top corners of the content, and all corners of the backdrop filter have been given corner radii by way of a container decoration instead of ClipRRect. There is still one remaining ClipRRect on the bottom because the bottom actions are arbitrary developer-provided widgets.

We can define a protocol for telling those widgets which corner they're in so that they can do the right thing, then we can make the widgets we provide implement it without a clip.

We want to get rid of all clips. They are prohibitively expensive.

ditto

Maybe you can just wrap the container at 239 with the contents of this function instead of having it have its own layout id.

The container is actually the maximum possible height for a dialog. Remember that issue where the dialog was always the height of the screen because the parent size can't depend on the children? The way I'm getting around that is by manually laying out all the individual pieces of the dialog, including the blur area.

We need a better solution. Why can't the parent size depend on the children?

I don't really have an opinion but the rest of the todos have the format // TODO(mattcarroll):. Worth keeping with convention.

the exact syntax is in the style guide: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo

Ya, I might be missing something but from reading this, I still think we don't need this

spoke offline. the blur is needed in the custom layout because the parent layout is the full height of the screen. therefore moving the blur to the parent would blur most of the screen.

This will only work on 3x density devices right? Might have to read the MediaQuery

yeah you probably want this to be 1/dpr.
If we were using strokeWidth, we could do it by setting the strokeWidth to 0.0 (hairline), but I don't see how that could work here.

@Hixie might have more opinions. I'd just let this be nullable and let the default come from Paint so we don't add cross-package defaulting and to avoid the setter for every decoration construction.

yeah, @xster's comment seems reasonable

nit: 2 indent

constructor first.

why false? what if the actionsCount changes?

I'll change it to check action count. This invalidation will need some thought to get it right. Because it's not just action count, it's text length and stacking. I'll add a todo for that.

we should make sure to get this right before landing.

i don't see the two things you mentioned in the list of constructor arguments, though, so how can they affect the algorithm?

Yeah, this was a rough approach for the common case because there are a few issues that come together here. First, I'm not sure that the CupertinoDialogAction handles text wrapping correctly. Maybe it does, but I need to check. Second, we're not stacking buttons correctly. Currently we're only stacking when 3+ buttons are used. But, actually, based on the desired width of the buttons, there are times when we should stack 2. That as-of-yet-unimplemented stacking behavior impacts this invalidation. There is also an open ended question as to what the button layout logic means with regard to arbitrary Widgets that a developer chooses to pass instead of CupertinoDialogActions.

Due to those TODOs and unknowns I figured we should take the implest approach possible here that handles the typical cases. Which I think this does.

2.0

prefer i += 1

here's a crazy trick for _buildVerticalButtons:

Iterable<Widget> _buildVerticalButtons() sync* {
  for (Widget child in children) {
    yield _buildHorizontalDivider();
    yield _buildDialogButton(child);
  }
}

...then at the callsite use it as _buildVerticalButtons().toList().

if you know there's only one entry, prefer children.single

do you handle this todo?

Nope. I just put the TODO back in.

you should assert that it's null in that case, i think, to help people who try to use it without understanding it

extraneous blank line

Mini-nit: you can just say BorderRadius.circular

nit: seems like we're always importing 'dart:math' as math in a global search.

Would still either make this variable name longer or add a comment above it saying it's a color that's put on top of _kCupertinoDialogBlurOverlayDecoration

you can just say BorderRadius.circular

Had to read this twice to understand this one. Might be clearer if we made this a margin instead of padding

What's a margin? Do you just mean create a Padding widget outside the container?

Ya. Though Container has a specific argument that does that for you inside the Container's build

nit: align with ContainerRenderObjectMixin (+1 space)

constraints.biggest probably gives you the same thing

ah, you might not need to convert the constraints to a size and back.

You can probably just do constraints.deflate(new EdgeInsets(top: minActionSpace))

Looks like we probably can do that. But should we? Does that code better express the intention of the calculation than what is there now?

I don't think you need to do these 2 steps. You're giving back to the child the size that it itself just gave you.

yep, you're right.

@override here and below

make private

Actually is this right? For its max intrinsic height, you're reporting the smaller of the content or the actions's height?

Reading the doc for this method, since your width doesn't depend on your height, I think your max and min intrinsic heights should be the same.

I guess that should actually be contentHeight+actionsHeight, right?

This is another reason why a render object seems like a strange place for this. We compute all relevant sizing in the layout method, but we also have to implement these other methods....and it seems they're so unrelated that even a bad implementation doesn't effect the dialog....

probably doesn't need to inherit from MultiChildLayoutParentData

Changed to extend ParentData

Actually I think I do need it here. The action buttons are used in a RenderObject that has mixins that depend on MultiChildLayoutParentData

if it doesn't contain a space it should wrap?

nit: only one TODO per TODO, otherwise you'll be overly-inflating the technical debt benchmark :-)

Fixed the TODOs. And yes, if the text does not contain a space then it wraps, whereas if even a single space exists in the text, the text ellipsizes.