Skip to content

Conversation

@xster
Copy link
Member

@xster xster commented Oct 31, 2018

Fixes #18037

  • Add CupertinoTheme using InheritedModel
  • Remove material from cupertino gallery demos and use cupertino scaffolds
  • Collect fonts and styles from various Cupertino widgets to the theme
  • Refer to the them from various Cupertino widgets
  • Move CupertinoThemeData to a model where derivation of dependent properties are done at read time rather than construction time
  • Let the CupertinoThemeData's copyWith etc allow unspecified downstream properties be re-derived when upstream overrides change
  • Add a new constructor to CupertinoButton to use the color of the theme instead of any particular color or null which means no color
  • Add a dark mode to most Cupertino widgets
  • Let Cupertino widgets demos in the gallery also respect the top level Material dark mode toggle
  • There are no AnimatedCupertinoTheme or lerping because iOS is very non-visual-mutating-animation styled
  • Create a MaterialBasedCupertinoThemeData that derives upwards from Material ThemeData rather than iOS defaults when upstream is unspecified
  • MaterialBasedCupertinoThemeData is also based on InheritedModels where dependency is only created on individual properties read
  • Material Theme also creates a CupertinoTheme and all Cupertino widgets only have dependencies on CupertinoTheme
  • MaterialBasedCupertinoThemeData can also be copiedWith (and reused in a CupertinoTheme) and only specified properties rather than Material or iOS based derivations
  • Let InheritedModel.inheritFrom not crash when no InheritedModels exist in the ancestry tree
  • There's a single input API (CupertinoThemeData constructor) and a single output API regardless of if it's constructed and read right away (in which case no dependencies get created) or read via inheritance from CupertinoTheme or Material Theme. This is probably the most debatable point.

Upgrade Flow

Starting this PR, Cupertino widgets under a Material Theme will somewhat adapt to that Material Theme (such as the primaryColor, brightness etc).

To avoid Material adaptations and to let Cupertino widgets use a default blank CupertinoTheme, insert a CupertinoTheme under the Material Theme such as via

Material(
  data: ...,
  child: CupertinoTheme(
    data: ...,
    child: child,
  ),
);

or

MaterialApp(
  theme: ...,
  builder: (BuildContext context, Widget child) {
    return CupertinoTheme(
      data: ...,
      child: child,
    );
  },
  home: ...,
);

in case the Material Theme comes from the MaterialApp.

@xster
Copy link
Member Author

xster commented Oct 31, 2018

cc @HansMuller this is what I have in mind so far. The InheritedModel stuff and the deferring to Material Theme parts still need work.

@xster xster force-pushed the cupertino-theme branch 2 times, most recently from 208ef47 to 6cda682 Compare November 2, 2018 22:23
@xster xster changed the title [WIP] Adds CupertinoTheme Adds CupertinoTheme Nov 2, 2018
@xster xster requested a review from HansMuller November 2, 2018 23:47
@xster
Copy link
Member Author

xster commented Nov 2, 2018

@HansMuller sorry, this thing is a bit long but it's ready for a look. Should be based on the same ideas as your PR.

I think we're going to want something different than the _CupertinoThemeInheritedData thing but the rest should be mostly unaffected.

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a little feedback based on reading some of the tests (as you suggested, I've started by reading the tests).

The tests all check the properties of themed components against constant values, like:

expect(decoration.color, CupertinoColors.activeOrange);

This kind of test could pass even if the component didn't consult the CupertinoTheme at all. Component tests that check a component's styles and colors have the expected default values seem worthwhile. Theme tests could could configure a theme with non-default values and verify that the components picked up those values:

expect(decoration.color, darkTestTheme.primaryColor);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of this file seems to consistently put the test name on the first line, the test closure on the next

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still reading through everything, just sending an update.

Most of the feedback is just doc nitpickery. And on that topic:

Although the words 'descendent' and 'descendant' are probably interchangeable, in the Flutter sources we use the latter almost exclusively.

@xster
Copy link
Member Author

xster commented Nov 8, 2018

@xster
Copy link
Member Author

xster commented Nov 9, 2018

Since it seems to be getting closer, adding Matt as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there were a CupertinoThemeData.merge() method, roughly like TextStyle's merge then you could just say that the default MaterialBasedCupertinoThemeData will be merged with the cupertinoOverrideTheme.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried it might create more confusion on having to explain the difference between a CupertinoThemeData.merge, a CupertinoThemeData.copyWith, a MaterialBasedCupertinoThemeData.merge and a MaterialBasedCupertinoThemeData.copyWith.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi: having a subclass here makes CupertinoThemeData less efficient to access (every access now has to have a potential dynamic dispatch). If we could just create a regular CupertinoThemeData directly with the fallback logic in the caller, rather than having a subclass, it might be better for performance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I was debating about inheritance too. The compromise is basically using inheritance or interface (and composition here) but the base CupertinoThemeData can't be const constructed.

I don't have a strong opinion either way. Do you feel strongly one way? Material ThemeData is factory constructed with all the fields initializing logic but it sounds like Hans is moving away from that too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we actually need this? we only need it in ThemeData because the default constructor does all kinds of magic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just needed 2 different constructors for optional arguments and required arguments. But you point below about the raw constructor and method overlap is a good point. I'll rename this (since it's @protected and less important).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this explanation really doesn't help anyone who doesn't understand the subtleties of the class hierarchy here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added more text

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's very confusing that CupertinoThemeData.raw() doesn't return the same thing as CupertinoThemeData().raw()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the non-constructor method to noDefault() instead since it's more explicit

@xster xster force-pushed the cupertino-theme branch 2 times, most recently from d0850d7 to 6416084 Compare November 13, 2018 01:48
@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. f: cupertino flutter/packages/flutter/cupertino repository labels Nov 28, 2018
xster added a commit to flutter/goldens that referenced this pull request Dec 19, 2018
@xster xster merged commit b8a035a into flutter:master Dec 19, 2018
@xster xster deleted the cupertino-theme branch December 19, 2018 04:36
liyuqian added a commit that referenced this pull request Dec 19, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create a CupertinoApp and a CupertinoTheme

6 participants