Breaking change. brightness is no longer used anywhere and would love to get rid of it in the near future.

per our style guide: Please add a comment to all // ignore: explaining why the ignore is there and when (if at all) it can be removed.

I feel like this sentence needs a conjunction or to be split into 2 sentences, but this is obviously a nit.

rephrased the paragraph a bit.

"against its own"

Tell me if my understanding is correct. Natively, systemBlue changes slightly depending on dark/light mode (and maybe other things like high contrast mode). In Flutter CupertinoColors.systemBlue will always return some static default systemBlue color. Using CupertinoDynamicColor.resolve returns the right systemBlue color for the current dark/light mode etc. Also, if passing CupertinoColors.systemBlue to a Cupertino widget, it will automatically resolve the color to the right systemBlue color as well.

Yes exactly. As of now all Cupertino widgets (with 1 or 2 exceptions I think, some PRs are blocked or still waiting to be merged) resolve the colors given to them in their build or createRenderObject/updateRenderObject methods. But if a Cupertino widget takes a Border things get complicated because it can be a subclass of Border. I feel this is going to be a big gotcha but not sure how I can better document it.

What happens if you do pass an unresolved CupertinoDynamicColor to Border, does it use the light mode color as default? Would it be possible to log a warning in that case, or does it happen too commonly in other situations?

Now that I understand the problem the examples above are pretty clear, though. Maybe bring this up to the team and see if anyone has any ideas if you're still worried about it.

Good idea! It sounds wrong to me to use a CupertinoDynamicColor that has never been resolved directly. Maybe putting some asserts in the CupertinoDynamicColor.value override to ensure no one does something like Paint()..color = CupertinoColors.systemBlue will be a viable approach. I'll try this out in a different PR to see if that breaks anything.

This might as well be a single dot, right? Or am I missing something... Same goes for below.

I just realized this is not working because Paint is not storing the color as is:

  set color(Color value) {
    assert(value != null);
    final int encoded = value.value ^ _kColorDefault;
    _data.setInt32(_kColorOffset, encoded, _kFakeHostEndian);
  }

I'll get rid of foreground and background.

Oh nice, sounds good!

This sentence leaves me wondering: How do I as a developer know when to manually resolve and when not?

The next few sections are intended to answer that question. Should I replace the . at the end with : ?

This should be enclosed in [] and not in`?

Why best-effort? What are the failure cases?

If we're given an instance of a class that may contain CupertinoDynamicColors (e.g. Border or TextStyle), we can't reliably determine what needs to be resolved in that class since we don't know beforehand what its runtime type is.

Does having two ```dart sections in the same {@tool ...} render correctly in dartdocs?

it ended up looking like this:

Builder(builder: (BuildContext context) {
  final Color resolvedColor = CupertinoDynamicColor.resolve(CupertinoColors.systemBlue, context);
  return CupertinoNavigationBar(
    // CupertinoNavigationBar does not know how to resolve colors used in
    // a Border class.
    border: Border(bottom: BorderSide(color: resolvedColor)),
  );
})

// ...

Builder(builder: (BuildContext context) {
  final Color resolvedColor = CupertinoDynamicColor.resolve(CupertinoColors.systemBlue, context);
  // Container is not a Cupertino widget.
  return Container(color: resolvedColor);
})

Thanks for pointing this out!

nit: removed this blank line

per our style guide: Please add a comment to all // ignore: explaining why the ignore is there and when (if at all) it can be removed.

Document this requirement on the public constructor?

(I know this was already missing before you made your change, but:): It would be cool to document what the nullOk parameters does.

Move the annotation to the same line as the property that is deprecated?

Should this method be private?

The class is private and since the implementation of it depends on the TextStyle constant defaults, it should probably never be made public so I guess its fine?

We've made a habit of making private class methods public, I think this is OK.

We don't want developers to think that they must do this to support dark mode. Maybe briefly explain that CupertinoButton's color is a dynamic color by default...

How does

By default a [CupertinoButton] has no background color. The following sample
code shows how to build a [CupertinoButton] that appears white in light mode,
and changes automatically to black in dark mode.

Sound?

Please reserve the => notation for one-liners whose return type is not void.

onPressed: () { },

Since code fragments like this are really just about the Builder's return value, I think it would be better to leave out the Builder. I realize that there's a small (acceptable) risk that the developer will put the fragment in a context that doesn't inherit what they expect. Here and elsewhere.

Maybe describe this situation as creating a custom Cupertino component that configures ordinary widgets with dynamic colors.

Changed to

When used to configure a non-Cupertino widget, or wrapped in an object opaque
to the receiving Cupertino component, a [CupertinoDynamicColor] may need to be
manually resolved using [CupertinoDynamicColor.resolve], before it can used
to paint. For example, to use a custom [Border] in a [CupertinoNavigationBar],
the colors used in the [Border] have to be resolved manually before being passed
to [CupertinoNavigationBar]'s constructor.

after retrieved => after it has been retrieved

Why is this a breaking change? I don't think we call adding a method to a public class a breaking change (even though it could cause a name collision).

Could we code-generate all of these values with an iOS app?

if no [CupertinoTheme] ancestors exist. Resolve all of the colors defined ...

It would be helpful if we explained here (and probably elsewhere) that the CupertinoTheme can be overriden and that the default one is provided by CupertinoApp. Along the same lines: explain that MaterialApp includes a MaterialBasedCupertinoTheme, it will be visible to Cupertino widgets created within the MaterialApp, and it can be overridden with a CupertinoTheme.

CupertinoApp and Theme are briefly mentioned in the see also section. Expanded it a little bit:

See also:

• [CupertinoThemeData], specifies the theme's visual styling.
• [CupertinoApp], which will automatically add a [CupertinoTheme] based on the
  value of [CupertinoApp.theme].
• [Theme], a Material theme which will automatically add a [CupertinoTheme]
  with a [CupertinoThemeData] derived from the Material [ThemeData].

with explicit => with an explicit

We should probably describe these values as "text styles" not typography. If for no other reason than https://api.flutter.dev/flutter/material/Typography-class.html

We've made a habit of making private class methods public, I think this is OK.

It would help to refer to MaterialBasedCupertinoThemeData here; assuming that class has a good explanation for why it exists, how it's created, etc.

A color that must be easy to see when rendered on a [primaryColor] background.

This is difficult to parse. Is the text color for all of the textTheme TextStyles the [primaryColor]?

Not all of them, just the ones that used for interactive elements (e.g., the back button on an ios navigation bar).

[BuildContext] doesn't have a brightness.

Changed to

Defaults to a light gray in light mode, or a dark translucent gray color in
dark mode.

I think you mean that this method is called by [CupertinoTheme.of]?

This is a bit confusing. What does it mean to imply the primaryColor in the context of copyWith? This comment seems to imply that myTheme.copyWith(primaryColor: foo) will create a copy of myTheme with the textTheme's color changed to foo? Most of our copyWith methods don't change dependent values and this one doesn't seem to?

Yeah sorry I should've updated the example