Add a new Theme widget to control color and text color of apps

Add a new Theme widget to control color and text color of apps

R=abarth@chromium.org, abarth, hixie

Committed: https://chromium.googlesource.com/external/mojo/+/e6b4158dbd2cd8aef7c824c7169c982858c15453

[jackson, 2015-06-19 01:12:40]

Question: should we have a default theme for components like checkbox, switch,
radio? Or force the authors to declare a theme to use them?

[abarth-chromium, 2015-06-19 01:50:09]

On 2015/06/19 at 01:12:40, jackson wrote:
> Question: should we have a default theme for components like checkbox, switch,
radio? Or force the authors to declare a theme to use them?

For now, let's force authors to declare a them.  Maybe we should put an assert
in Theme.of on the line you hit if you forget to include a theme and then have a
comment there explaining what to do.

It's easier to add a default in the future than it is to remove a default that
exists already.

[abarth-chromium, 2015-06-19 01:50:17]

LGTM

https://codereview.chromium.org/1194743003/diff/20001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_app.dart (right):

https://codereview.chromium.org/1194743003/diff/20001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_app.dart:55: theme: new ThemeInfo(color:
colors.Purple, text: typography.white)
sorry to nit-pick your names, but I think I liked it better when you called this
parameter info.  That removes the stutter in a few places.

https://codereview.chromium.org/1194743003/diff/20001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_app.dart:55: theme: new ThemeInfo(color:
colors.Purple, text: typography.white)
sorry to nit-pick your names, but I think I liked it better when you called this
parameter info.  That removes the stutter in a few places.

https://codereview.chromium.org/1194743003/diff/20001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_home.dart (right):

https://codereview.chromium.org/1194743003/diff/20001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_home.dart:172: backgroundColor:
Theme.of(this).color[500]
We should make the toolbar get its color from the theme instead of having it
passed in.

https://codereview.chromium.org/1194743003/diff/20001/sky/sdk/lib/widgets/the...
File sky/sdk/lib/widgets/theme.dart (right):

https://codereview.chromium.org/1194743003/diff/20001/sky/sdk/lib/widgets/the...
sky/sdk/lib/widgets/theme.dart:21: return theme.theme;
Then this would be |return theme.info|, which looks prettier.

[abarth-chromium, 2015-06-19 01:55:41]

There's actually a bug in this CL.  Consider the case where a component moves
from one parent to another.  In that case, it will get it's |_inherited| value
updated, but it's children won't.  We probably need to walk the subtree and
update all our children recursively.

It's fine to land this CL with this bug, but we should fix it in a follow up CL.

[Hixie, 2015-06-19 02:23:18]

Another example would be a build() method that only changes the theme, where the
theme's child is stateful and has children of its own.
Or where you hand in a prebuilt subtree that gets inserted into the tree.

Walking all the children recursively seems potentially expensive... in the worst
case (changing the app theme) it means walking the entire app.

[Hixie, 2015-06-19 02:23:23]

https://codereview.chromium.org/1194743003/diff/20001/sky/sdk/lib/widgets/wid...
File sky/sdk/lib/widgets/widget.dart (right):

https://codereview.chromium.org/1194743003/diff/20001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:47: _inherited = null;
Why is this logic on Widget and not on Component?

[abarth-chromium, 2015-06-19 02:31:41]

On 2015/06/19 at 02:23:18, ianh wrote:
> Another example would be a build() method that only changes the theme, where
the theme's child is stateful and has children of its own.

Do we not rebuild stateful components after their parents build?

> Or where you hand in a prebuilt subtree that gets inserted into the tree.

Yeah, a prebuilt subtree will trigger this issue.

> Walking all the children recursively seems potentially expensive... in the
worst case (changing the app theme) it means walking the entire app.

You're already going to rebuild the entire app to change the app's theme because
you're running the app's |build| function.

[jackson, 2015-06-22 16:52:48]

reparenting should work now, I'll do support for inherited text styling in a
separate CL. PTAL

https://codereview.chromium.org/1194743003/diff/20001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_home.dart (left):

https://codereview.chromium.org/1194743003/diff/20001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_home.dart:6: import
'package:sky/theme/colors.dart' as colors;
I'm going to need to bring this import back.

[jackson, 2015-06-22 16:56:15]

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
File sky/sdk/lib/widgets/widget.dart (right):

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:385: void setParent(Widget parent) {
I'll add a TODO here saying that this can be moved to _Heir once we support
super in mixins

[Hixie, 2015-06-22 17:06:40]

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_settings.dart (right):

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_settings.dart:34: center: new Text('Settings',
style: Theme.of(this).text.title)
shouldn't this be automatic now?

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/widgets/se...
File sky/examples/widgets/sector.dart (right):

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/widgets/se...
sky/examples/widgets/sector.dart:83: center: new Text('Sector Layout in a Widget
Tree', style: typography.white.title)),
isn't the style automatic now?

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/widgets/st...
File sky/examples/widgets/styled_text.dart (right):

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/widgets/st...
sky/examples/widgets/styled_text.dart:111: backgroundColor: Blue[500]
aren't the colour and style automatic now?

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
File sky/sdk/lib/widgets/widget.dart (right):

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:31: Widget._withKey(String key) : _key = key {
If this is just to work around a limitation in Dart, make sure you have a TODO
to that effect.

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:222: void set traits(Map<Type, Inherited> value)
{
It's considered bad style to only have a setter. Make this a method instead.

That is, if you use it at all. I don't see a usage anywhere. Can you just delete
it?

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:228: static _Heir _getHeirAncestor(Widget
widget) {
This can return |widget|. If that's intentional, add a comment explaining why
(e.g. "callers are expected to hand in their parent, not themselves").

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:245: void _updateTraitsRecursively(Widget
widget) {
I think this'd be more readable with the inner function hoisted up rather than
nested.

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:267: _updateTraitsFromParent(parent);
Shouldn't we set the parent first?

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:387: super.setParent(parent);
Shouldn't we set the parent first?

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:700: }
You'll also need Scaffold to walkChildren(), no?

[jackson, 2015-06-22 17:45:45]

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/stocks2/li...
File sky/examples/stocks2/lib/stock_settings.dart (right):

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/stocks2/li...
sky/examples/stocks2/lib/stock_settings.dart:34: center: new Text('Settings',
style: Theme.of(this).text.title)
On 2015/06/22 17:06:39, Hixie wrote:
> shouldn't this be automatic now?

I'm going to do the text styling in a separate CL to make it easier to review

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/widgets/se...
File sky/examples/widgets/sector.dart (right):

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/widgets/se...
sky/examples/widgets/sector.dart:83: center: new Text('Sector Layout in a Widget
Tree', style: typography.white.title)),
On 2015/06/22 17:06:39, Hixie wrote:
> isn't the style automatic now?

I'm going to do the text styling in a separate CL to make it easier to review

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/widgets/st...
File sky/examples/widgets/styled_text.dart (right):

https://codereview.chromium.org/1194743003/diff/30001/sky/examples/widgets/st...
sky/examples/widgets/styled_text.dart:111: backgroundColor: Blue[500]
On 2015/06/22 17:06:40, Hixie wrote:
> aren't the colour and style automatic now?

I'll remove the color here and do text in a separate CL

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
File sky/sdk/lib/widgets/widget.dart (right):

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:31: Widget._withKey(String key) : _key = key {
On 2015/06/22 17:06:40, Hixie wrote:
> If this is just to work around a limitation in Dart, make sure you have a TODO
> to that effect.

Will do

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:222: void set traits(Map<Type, Inherited> value)
{
On 2015/06/22 17:06:40, Hixie wrote:
> It's considered bad style to only have a setter. Make this a method instead.
> 
> That is, if you use it at all. I don't see a usage anywhere. Can you just
delete
> it?

This is just a mistake, thanks.

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:228: static _Heir _getHeirAncestor(Widget
widget) {
On 2015/06/22 17:06:40, Hixie wrote:
> This can return |widget|. If that's intentional, add a comment explaining why
> (e.g. "callers are expected to hand in their parent, not themselves").

Done.

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:245: void _updateTraitsRecursively(Widget
widget) {
On 2015/06/22 17:06:40, Hixie wrote:
> I think this'd be more readable with the inner function hoisted up rather than
> nested.

Done.

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:267: _updateTraitsFromParent(parent);
On 2015/06/22 17:06:40, Hixie wrote:
> Shouldn't we set the parent first?

This will trigger callbacks, I'd rather have the traits set correctly first

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:385: void setParent(Widget parent) {
On 2015/06/22 16:56:15, jackson wrote:
> I'll add a TODO here saying that this can be moved to _Heir once we support
> super in mixins

Done.

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:387: super.setParent(parent);
On 2015/06/22 17:06:40, Hixie wrote:
> Shouldn't we set the parent first?

This will trigger callbacks, I'd rather have the traits set correctly first

https://codereview.chromium.org/1194743003/diff/30001/sky/sdk/lib/widgets/wid...
sky/sdk/lib/widgets/widget.dart:700: }
On 2015/06/22 17:06:40, Hixie wrote:
> You'll also need Scaffold to walkChildren(), no?

Done.

[jackson, 2015-06-22 19:10:35]

Committed patchset #6 (id:80001) manually as
e6b4158dbd2cd8aef7c824c7169c982858c15453 (presubmit successful).