Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1205953002:
Version 0 of TabLabel, Tab, and TabBar components (Closed)
|
Created:
5 years, 6 months ago by hansmuller1 Modified:
5 years, 5 months ago CC:
gregsimon, jackson_old, mojo-reviews_chromium.org, qsr+mojo_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionVersion 0 of TabLabel, Tab, and TabBar components
There's is no support for animating the selected tab indicator, there isn't a TabNavigator container yet, overflow layout (tabs don't fit) isn't supported yet, etc.
R=abarth@chromium.org, ianh@google.com
Committed: https://chromium.googlesource.com/external/mojo/+/c314cfbf8e8de2651d2b8bbad15cb01128d11791
Patch Set 1 #Patch Set 2 : Added selection indcator #
Total comments: 55
Patch Set 3 : just the minor corrections #Patch Set 4 : Remaining updates per Adam's feedback - except stateless TabBar #
Total comments: 24
Patch Set 5 : Updates per Ian's feedback #
Total comments: 2
Patch Set 6 : More updates; including making TabBar stateless #
Total comments: 20
Patch Set 7 : Fixed the constraint.minWidth problem Adam pointed out #Patch Set 8 : Fix TabBar instrinsic width methods, refactor child walks #
Total comments: 1
Patch Set 9 : #
Messages
Total messages: 22 (3 generated)
hansmuller@chromium.org changed reviewers: + ianh@google.com
hansmuller@chromium.org changed reviewers: + hansmuller@chromium.org
PTAL
abarth@chromium.org changed reviewers: + abarth@chromium.org
https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:45: } Should this be a function on |BoxConstraints| ? https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:54: } This functions isn't correct. Consider if you were asked to size yourself to what you return here. Unless all your children were exactly the same size, you're not going to have enough space for at least one of them. Instead what you want here is to take the |max| over the getMinIntrinsicWidth of your children and then multiply that by the number of children. That way when you go through layout and divide that amount by the number of children, each child will have enough space. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:63: } You want to do a similar thing here. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:65: double getMinIntrinsicHeight(BoxConstraints constraints) => _kTabBarHeight; constraints.constrainHeight(_kTabBarHeight) https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:67: double getMaxIntrinsicHeight(BoxConstraints constraints) => _kTabBarHeight; constraints.constrainHeight(_kTabBarHeight) https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:72: childCount += 1; ++childCount https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:89: if (childCount == 0) return; This early return isn't correct. You'll fail to set |size|, which means you'll have a stale value. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:91: double tabWidth = constraints.maxWidth / childCount.toDouble(); constraints.constrainWidth(constraints.maxWidth) https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:93: new BoxConstraints.tightFor(width: tabWidth, height: _kTabBarHeight); This height constraint isn't correct in the case where _kTabBarHeight isn't allowed by |constraints|. It might be easier to structure this function by setting |size| right away: size = constraints.constrain(new Size(constraints.maxWidth, _kTabBarHeight)); Then you can return early when childCount == 0 and you can write this line as: BoxConstraints tabConstraints = new BoxConstraints.tightFor(width: tabWidth, size.height); https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:96: child.layout(tabConstraints, parentUsesSize: false); "parentUsesSize: false" is the default. You don't need to specify it. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:99: x += child.size.width; Isn't this always tabWidth? I'd just += tabWidth here and assert(child.size.width == tabWidth); https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:101: size = new Size(constraints.maxWidth, _kTabBarHeight); What if _kTabBarHeight is not allowed by |constraints| ? You probably want: size = constraints.constrain(new Size(constraints.maxWidth, _kTabBarHeight)); https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:113: paint.setStyle(sky.PaintingStyle.fill); ..setStyle https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:125: paint.setStyle(sky.PaintingStyle.fill); ditto https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:137: TabBarWrapper(List<Tab> children, int this.selectedIndex, { String key }) I'd make selectedIndex a named argument with a default value of 0 https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:238: } Why not make |TabBar| stateless and just call |onChanged| here? That would mimic how |CheckBox| works. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:255: List<Tab> tabs = labels.map((label) => _toTab(label, tabIndex++)).toList(); IMHO, we should just use a regular loop here. It's more efficient.
https://codereview.chromium.org/1205953002/diff/20001/sky/examples/widgets/ta... File sky/examples/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/20001/sky/examples/widgets/ta... sky/examples/widgets/tabs.dart:21: .toList(); why toList()? https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:45: } On 2015/06/25 at 00:19:36, abarth wrote: > Should this be a function on |BoxConstraints| ? Yeah this could be boxConstraints.unconstrainHeight() or some such. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:69: int _childCount() { If your data is in a linked list, you should avoid needing to know how many children you have. In practice you only use this to determine if you have any children at all, so you can just remove it. See comment below. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:72: childCount += 1; On 2015/06/25 at 00:19:37, abarth wrote: > ++childCount FWIW personally I much prefer += 1 to the ++ prefix operator. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:77: RenderBox _childAt(int index) { Given that you have to walk a list to get the child at an index, I would try really really hard to structure your code in such a way as to not need this function. Or, use an array instead of a linked list to store the children. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:89: if (childCount == 0) return; This is a very expensive way to find out if you have no children. You are walking the entire linked list just to find if the first link in the chain is null or not. Instead, just check firstChild == null. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:99: x += child.size.width; If you access child.size, then you're using the size, so parentUsesSize should be true. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:111: // TODO(hansmuller): background color should be based on the theme. Have the colours be configurable in this class, so you can set them accordingly in the RenderObjectWrapper. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:113: paint.setStyle(sky.PaintingStyle.fill); Either use ".." here, or have the color and setStyle calls on separate lines. I'd prefer the second. There's not really a good reason to use ".." here. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:119: RenderBox selectedTab = _childAt(selectedIndex); Instead of using _childAt and thus walking the list twice (once to paint, once just to find the child), I'd just walk the list once, and at each point, paint the indicator if you need to, then paint the child. This avoids having a _childAt() function hanging around making people think it's cheap to call it. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:137: TabBarWrapper(List<Tab> children, int this.selectedIndex, { String key }) You don't have to give the type if you're using the "this.foo" syntax. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:201: opacity: selected ? 1.0 : 0.7 Using opacity will mean you can see what's behind it. Is that the right behaviour? https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:221: List<TabLabel> labels; These should be in the same order as in the constructor and in syncFields().
Updates per Adam's feedback. I think I've addressed them all, except for the one about making TabBar stateless. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:45: } On 2015/06/25 00:19:36, abarth wrote: > Should this be a function on |BoxConstraints| ? Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:54: } On 2015/06/25 00:19:37, abarth wrote: > This functions isn't correct. Consider if you were asked to size yourself to > what you return here. Unless all your children were exactly the same size, > you're not going to have enough space for at least one of them. > > Instead what you want here is to take the |max| over the getMinIntrinsicWidth of > your children and then multiply that by the number of children. That way when > you go through layout and divide that amount by the number of children, each > child will have enough space. You're right. I was thinking about scrollable tabs, but there's no support for that yet. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:63: } On 2015/06/25 00:19:37, abarth wrote: > You want to do a similar thing here. Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:65: double getMinIntrinsicHeight(BoxConstraints constraints) => _kTabBarHeight; On 2015/06/25 00:19:37, abarth wrote: > constraints.constrainHeight(_kTabBarHeight) Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:67: double getMaxIntrinsicHeight(BoxConstraints constraints) => _kTabBarHeight; On 2015/06/25 00:19:37, abarth wrote: > constraints.constrainHeight(_kTabBarHeight) Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:72: childCount += 1; On 2015/06/25 00:19:37, abarth wrote: > ++childCount Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:72: childCount += 1; On 2015/06/25 17:03:10, Hixie wrote: > On 2015/06/25 at 00:19:37, abarth wrote: > > ++childCount > > FWIW personally I much prefer += 1 to the ++ prefix operator. I do too, but I've already bowed to ++ :-). https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:89: if (childCount == 0) return; On 2015/06/25 00:19:37, abarth wrote: > This early return isn't correct. You'll fail to set |size|, which means you'll > have a stale value. Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:91: double tabWidth = constraints.maxWidth / childCount.toDouble(); On 2015/06/25 00:19:36, abarth wrote: > constraints.constrainWidth(constraints.maxWidth) Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:93: new BoxConstraints.tightFor(width: tabWidth, height: _kTabBarHeight); On 2015/06/25 00:19:36, abarth wrote: > This height constraint isn't correct in the case where _kTabBarHeight isn't > allowed by |constraints|. > > It might be easier to structure this function by setting |size| right away: > > size = constraints.constrain(new Size(constraints.maxWidth, _kTabBarHeight)); > > Then you can return early when childCount == 0 and you can write this line as: > > BoxConstraints tabConstraints = new BoxConstraints.tightFor(width: tabWidth, > size.height); Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:96: child.layout(tabConstraints, parentUsesSize: false); On 2015/06/25 00:19:37, abarth wrote: > "parentUsesSize: false" is the default. You don't need to specify it. Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:99: x += child.size.width; On 2015/06/25 00:19:36, abarth wrote: > Isn't this always tabWidth? I'd just += tabWidth here and > assert(child.size.width == tabWidth As written written the code works for both the scrollable and fixed tabs cases. Plus: what if the child doesn't respect tabConstraints? Isn't this like your comment per lines 92,93? https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:101: size = new Size(constraints.maxWidth, _kTabBarHeight); On 2015/06/25 00:19:37, abarth wrote: > What if _kTabBarHeight is not allowed by |constraints| ? > > You probably want: > > size = constraints.constrain(new Size(constraints.maxWidth, _kTabBarHeight)); Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:113: paint.setStyle(sky.PaintingStyle.fill); On 2015/06/25 00:19:37, abarth wrote: > ..setStyle Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:125: paint.setStyle(sky.PaintingStyle.fill); On 2015/06/25 00:19:37, abarth wrote: > ditto Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:238: } On 2015/06/25 00:19:37, abarth wrote: > Why not make |TabBar| stateless and just call |onChanged| here? That would > mimic how |CheckBox| works. I'd neglected to call onChanged() at all. I've fixed that. Not entirely sure how to make TabBar stateless, it needs to rebuild the list tabs, since they reflect the selection. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:255: List<Tab> tabs = labels.map((label) => _toTab(label, tabIndex++)).toList(); On 2015/06/25 00:19:37, abarth wrote: > IMHO, we should just use a regular loop here. It's more efficient. Done.
On 2015/06/25 at 17:32:26, hansmuller wrote: > https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... > sky/sdk/lib/widgets/tabs.dart:99: x += child.size.width; > On 2015/06/25 00:19:36, abarth wrote: > > Isn't this always tabWidth? I'd just += tabWidth here and > > assert(child.size.width == tabWidth > > As written written the code works for both the scrollable and fixed tabs cases. IMHO, we should worry about the scrollable case when we implement it. > Plus: what if the child doesn't respect tabConstraints? Isn't this like your comment per lines 92,93? Subclasses of RenderBox need to respect the constraints their given. If they don't respect the constraints, they're buggy and need to be fixed. We should add asserts to enforce that invariant whenever it's easy. > https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... > sky/sdk/lib/widgets/tabs.dart:238: } > On 2015/06/25 00:19:37, abarth wrote: > > Why not make |TabBar| stateless and just call |onChanged| here? That would > > mimic how |CheckBox| works. > > I'd neglected to call onChanged() at all. I've fixed that. > > Not entirely sure how to make TabBar stateless, it needs to rebuild the list tabs, since they reflect the selection. The idea is that you just call onChanged() and it's your parent's job to rebuild the TabBar with a new selectedIndex. As the TabBar, your job is just to build widgets that reflect the parameters handed to you by your parent and then to notify your parent when something interesting happens. It's your parent's job to respond to your notification and change your parameters if that's what it wants. https://github.com/domokit/mojo/blob/master/sky/sdk/lib/widgets/README.md#state
This is looking pretty good. We just need to make TabBar stateless. https://codereview.chromium.org/1205953002/diff/60001/sky/examples/widgets/ta... File sky/examples/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/60001/sky/examples/widgets/ta... sky/examples/widgets/tabs.dart:59: }; I'd just remove lines 56-58. If someone wants to dump the render tree, they can added this code themselves. We don't need to clutter up all the examples with it. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:47: BoxConstraints widthConstraints = constraints.widthConstraints(); Can you update RenderBlock:: getMinIntrinsicWidth to use widthConstraints too? https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:54: return maxWidth * childCount.toDouble(); No need to call toDouble() here. Also, you need to apply |constraints| here. There's no reason to believe maxWidth * childCount will be feasible within |constraints|: return constraints.constrainWidth(maxWidth * childCount); https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:65: return maxWidth * childCount.toDouble(); ditto https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:100: double tabWidth = constraints.constrainWidth(constraints.maxWidth) / childCount.toDouble(); s/constraints.constrainWidth(constraints.maxWidth)/size.width/ There's no need to recompute this value. It's also clearer semantically if just read the value of size.width that you just set. Also, no need for toDouble(). https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:108: x += child.size.width; x += tabWidth; Using the local variable tabWidth rather than reading the child's size makes it clear that we don't need to pass parentUsesSize: true to layout() https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:116: void _paintBackground(RenderObjectDisplayList canvas) { s/RenderObjectDisplayList/RenderCanvas/ This class has been renamed. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:126: RenderBox selectedTab = _childAt(selectedIndex); I agree with Ian that we shouldn't re-walk the child list to get the selectedTab. When we're walking the child list to paint the children, we should keep count and then remember the selected tab. That way we can remove _childAt. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:127: var size = new Size(selectedTab.size.width, 2.0); 2.0 ? Is that _kTabIndicatorHeight? https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:128: var point = new Point(selectedTab.parentData.position.x, 46.0); 46.0 ? Is that _kTabHeight? https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:133: ..setStyle(sky.PaintingStyle.fill); Isn't |fill| the default? https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:140: _paintIndicator(canvas); Should we handle the case of _selectedIndex being null? Maybe we should skip drawing the indicator if _selectedIndex is null or is greater than the number of children we actually have.
Updates per Hixie feedback. https://codereview.chromium.org/1205953002/diff/20001/sky/examples/widgets/ta... File sky/examples/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/20001/sky/examples/widgets/ta... sky/examples/widgets/tabs.dart:21: .toList(); On 2015/06/25 17:03:10, Hixie wrote: > why toList()? Sorry, just a nervous tic. I switched the types of these variables to Iterable and removed the .toList()s. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:89: if (childCount == 0) return; On 2015/06/25 17:03:10, Hixie wrote: > This is a very expensive way to find out if you have no children. You are > walking the entire linked list just to find if the first link in the chain is > null or not. Instead, just check firstChild == null. I'm also using childCount to compute the (fixed) tabWidth. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:99: x += child.size.width; On 2015/06/25 17:03:10, Hixie wrote: > If you access child.size, then you're using the size, so parentUsesSize should > be true. Done. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:111: // TODO(hansmuller): background color should be based on the theme. On 2015/06/25 17:03:10, Hixie wrote: > Have the colours be configurable in this class, so you can set them accordingly > in the RenderObjectWrapper. If it's OK with you, I'd like to deal with styles, colors, and themes, in a separate CL (next). https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:113: paint.setStyle(sky.PaintingStyle.fill); On 2015/06/25 17:03:10, Hixie wrote: > Either use ".." here, or have the color and setStyle calls on separate lines. > I'd prefer the second. There's not really a good reason to use ".." here. Acknowledged. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:119: RenderBox selectedTab = _childAt(selectedIndex); On 2015/06/25 17:03:10, Hixie wrote: > Instead of using _childAt and thus walking the list twice (once to paint, once > just to find the child), I'd just walk the list once, and at each point, paint > the indicator if you need to, then paint the child. > > This avoids having a _childAt() function hanging around making people think it's > cheap to call it. This makes a nice improvement; thanks! https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:201: opacity: selected ? 1.0 : 0.7 On 2015/06/25 17:03:10, Hixie wrote: > Using opacity will mean you can see what's behind it. Is that the right > behaviour? Yes. The opaque TabBar background is behind the tab. https://codereview.chromium.org/1205953002/diff/20001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:221: List<TabLabel> labels; On 2015/06/25 17:03:10, Hixie wrote: > These should be in the same order as in the constructor and in syncFields(). Done.
https://codereview.chromium.org/1205953002/diff/80001/sky/sdk/lib/widgets/tab... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/80001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:97: child.layout(tabConstraints, parentUsesSize: true); Rather than passing |parentUsesSize: true| here, we should just change line 100 to x += tabWidth; Passing |parentUsesSize: true| is expensive because it means whenever the child goes through layout, this object and potentially other objects need to go through layout as well. When you pass |false|, that creates a layout boundary in the render tree so that the subtree can change internally independently. In this case, you don't actually need to use the child's size because you already know enough to layout all the tabs without it.
https://codereview.chromium.org/1205953002/diff/60001/sky/examples/widgets/ta... File sky/examples/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/60001/sky/examples/widgets/ta... sky/examples/widgets/tabs.dart:59: }; On 2015/06/25 17:59:20, abarth wrote: > I'd just remove lines 56-58. If someone wants to dump the render tree, they can > added this code themselves. We don't need to clutter up all the examples with > it. Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:47: BoxConstraints widthConstraints = constraints.widthConstraints(); On 2015/06/25 17:59:20, abarth wrote: > Can you update RenderBlock:: getMinIntrinsicWidth to use widthConstraints too? Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:54: return maxWidth * childCount.toDouble(); On 2015/06/25 17:59:20, abarth wrote: > No need to call toDouble() here. > > Also, you need to apply |constraints| here. There's no reason to believe > maxWidth * childCount will be feasible within |constraints|: > > return constraints.constrainWidth(maxWidth * childCount); Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:65: return maxWidth * childCount.toDouble(); On 2015/06/25 17:59:20, abarth wrote: > ditto Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:100: double tabWidth = constraints.constrainWidth(constraints.maxWidth) / childCount.toDouble(); On 2015/06/25 17:59:20, abarth wrote: > s/constraints.constrainWidth(constraints.maxWidth)/size.width/ > > There's no need to recompute this value. It's also clearer semantically if just > read the value of size.width that you just set. > > Also, no need for toDouble(). Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:108: x += child.size.width; On 2015/06/25 17:59:20, abarth wrote: > x += tabWidth; > > Using the local variable tabWidth rather than reading the child's size makes it > clear that we don't need to pass parentUsesSize: true to layout() Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:116: void _paintBackground(RenderObjectDisplayList canvas) { On 2015/06/25 17:59:20, abarth wrote: > s/RenderObjectDisplayList/RenderCanvas/ > > This class has been renamed. Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:126: RenderBox selectedTab = _childAt(selectedIndex); On 2015/06/25 17:59:20, abarth wrote: > I agree with Ian that we shouldn't re-walk the child list to get the > selectedTab. When we're walking the child list to paint the children, we should > keep count and then remember the selected tab. That way we can remove _childAt. Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:127: var size = new Size(selectedTab.size.width, 2.0); On 2015/06/25 17:59:20, abarth wrote: > 2.0 ? Is that _kTabIndicatorHeight? Oops. Yes. Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:128: var point = new Point(selectedTab.parentData.position.x, 46.0); On 2015/06/25 17:59:20, abarth wrote: > 46.0 ? Is that _kTabHeight? Done. https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:133: ..setStyle(sky.PaintingStyle.fill); On 2015/06/25 17:59:20, abarth wrote: > Isn't |fill| the default? It appears to be. I changed this to: canvas.drawRect(rect, new Paint()..color = White); https://codereview.chromium.org/1205953002/diff/60001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:140: _paintIndicator(canvas); On 2015/06/25 17:59:20, abarth wrote: > Should we handle the case of _selectedIndex being null? Maybe we should skip > drawing the indicator if _selectedIndex is null or is greater than the number of > children we actually have. Yes, that works out in the new version of the paint() method: int index = 0; for (RenderBox child = firstChild; ...) { ... if (index++ == selectedIndex) _paintIndicator(canvas, child); } https://codereview.chromium.org/1205953002/diff/80001/sky/sdk/lib/widgets/tab... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/80001/sky/sdk/lib/widgets/tab... sky/sdk/lib/widgets/tabs.dart:97: child.layout(tabConstraints, parentUsesSize: true); On 2015/06/25 18:25:49, abarth wrote: > Rather than passing |parentUsesSize: true| here, we should just change line 100 > to > > x += tabWidth; > > Passing |parentUsesSize: true| is expensive because it means whenever the child > goes through layout, this object and potentially other objects need to go > through layout as well. When you pass |false|, that creates a layout boundary > in the render tree so that the subtree can change internally independently. > > In this case, you don't actually need to use the child's size because you > already know enough to layout all the tabs without it. Sorry, I wasn't disagreeing with this feedback; It's just that I went to lunch before uploading this change (really!) :-).
I think I've addressed all of the feedback. Please take another look.
On 2015/06/25 at 19:45:51, hansmuller wrote: > Sorry, I wasn't disagreeing with this feedback; It's just that I went > to lunch before uploading this change (really!) :-). :)
LGTM A thing of beauty is a joy forever.
https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:47: BoxConstraints widthConstraints = constraints.widthConstraints(); Sigh. There's actually a bug on this line (and on line 58). Consider the case where |constraints| has a minWidth. The minWidth applies to the RenderTabBar, but there's no reason the tabs inside the tab bar need to have a certain minimum width. Instead, I think you want this line to be: BoxConstraints widthConstraints = new BoxConstraints(maxWidth: constraints.maxWidth); (and the same thing on line 58).
https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:47: BoxConstraints widthConstraints = constraints.widthConstraints(); On 2015/06/25 19:58:00, abarth wrote: > Sigh. There's actually a bug on this line (and on line 58). > > Consider the case where |constraints| has a minWidth. The minWidth applies to > the RenderTabBar, but there's no reason the tabs inside the tab bar need to have > a certain minimum width. > > Instead, I think you want this line to be: > > BoxConstraints widthConstraints = new BoxConstraints(maxWidth: > constraints.maxWidth); > > (and the same thing on line 58). Thanks for catching this. It's due to my being sloppy/lazy in using block.dart as a template.
https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:41: if (child.parentData is! TabBarParentData) { remove the {}s, they just make this function an extra line longer without improving readability. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:47: BoxConstraints widthConstraints = constraints.widthConstraints(); You actually need the height constraint to be tight, too, because otherwise the child won't know that you're always going to be laying it out tight. Imagine a child that does vertical text layout. Its width will depend on its height. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:50: for (RenderBox child = firstChild; child != null; child = child.parentData.nextSibling) { you can't use a for loop here because you need to assert(child.parentData is TabBarParentData) before you access it. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:57: double getMaxIntrinsicWidth(BoxConstraints constraints) { comments from previous function apply here also https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:74: int _childCount() { Seeing this function makes me nervous. I'm paranoid that people are going to copy-pasta this code. If we need to know the number of children, we really should find a better way to store them in the first place. Please put a comment, TODO, or file a bug or something to track this. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:121: for (RenderBox child = firstChild; child != null; child = child.parentData.nextSibling) { see earlier comment about parentData. You can't access it until you've asserted that it's the right type, otherwise once we make it not be "dynamic" in the declaration, it'll throw warnings on access. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:124: if (index++ == selectedIndex) _paintIndicator(canvas, child); Please put the statement on a different line than the condition, otherwise it all blends together and if we ever add another line it'll look like that's the statement. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:130: TabBarWrapper(List<Widget> children, int this.selectedIndex, { String key }) see earlier comment about type when using "this.foo". https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:219: if (tabIndex != selectedIndex && onChanged != null) { no {} around one-line blocks
https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:41: if (child.parentData is! TabBarParentData) { On 2015/06/25 20:26:05, Hixie wrote: > remove the {}s, they just make this function an extra line longer without > improving readability. Done. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:47: BoxConstraints widthConstraints = constraints.widthConstraints(); On 2015/06/25 20:26:05, Hixie wrote: > You actually need the height constraint to be tight, too, because otherwise the > child won't know that you're always going to be laying it out tight. > > Imagine a child that does vertical text layout. Its width will depend on its > height. Done. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:50: for (RenderBox child = firstChild; child != null; child = child.parentData.nextSibling) { On 2015/06/25 20:26:05, Hixie wrote: > you can't use a for loop here because you need to assert(child.parentData is > TabBarParentData) before you access it. OK. I fixed the loops in block.dart as well. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:57: double getMaxIntrinsicWidth(BoxConstraints constraints) { On 2015/06/25 20:26:05, Hixie wrote: > comments from previous function apply here also Done. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:74: int _childCount() { On 2015/06/25 20:26:05, Hixie wrote: > Seeing this function makes me nervous. I'm paranoid that people are going to > copy-pasta this code. If we need to know the number of children, we really > should find a better way to store them in the first place. Please put a comment, > TODO, or file a bug or something to track this. Done. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:121: for (RenderBox child = firstChild; child != null; child = child.parentData.nextSibling) { On 2015/06/25 20:26:05, Hixie wrote: > see earlier comment about parentData. You can't access it until you've asserted > that it's the right type, otherwise once we make it not be "dynamic" in the > declaration, it'll throw warnings on access. Done. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:124: if (index++ == selectedIndex) _paintIndicator(canvas, child); On 2015/06/25 20:26:05, Hixie wrote: > Please put the statement on a different line than the condition, otherwise it > all blends together and if we ever add another line it'll look like that's the > statement. Done. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:130: TabBarWrapper(List<Widget> children, int this.selectedIndex, { String key }) On 2015/06/25 20:26:05, Hixie wrote: > see earlier comment about type when using "this.foo". Sorry to have overlooked that. Done. https://codereview.chromium.org/1205953002/diff/100001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:219: if (tabIndex != selectedIndex && onChanged != null) { On 2015/06/25 20:26:05, Hixie wrote: > no {} around one-line blocks Done.
lgtm https://codereview.chromium.org/1205953002/diff/140001/sky/sdk/lib/widgets/ta... File sky/sdk/lib/widgets/tabs.dart (right): https://codereview.chromium.org/1205953002/diff/140001/sky/sdk/lib/widgets/ta... sky/sdk/lib/widgets/tabs.dart:81: // TODO(hansmuller): track this value in ParentData rather than computing it. I don't really understand how tracking it in ParentData would work. Wouldn't it just be a field of the parent?
On 2015/06/25 21:03:56, Hixie wrote: > lgtm > > https://codereview.chromium.org/1205953002/diff/140001/sky/sdk/lib/widgets/ta... > File sky/sdk/lib/widgets/tabs.dart (right): > > https://codereview.chromium.org/1205953002/diff/140001/sky/sdk/lib/widgets/ta... > sky/sdk/lib/widgets/tabs.dart:81: // TODO(hansmuller): track this value in > ParentData rather than computing it. > I don't really understand how tracking it in ParentData would work. Wouldn't it > just be a field of the parent? Yes, I think that's right. I'm going to try and do it.
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as c314cfbf8e8de2651d2b8bbad15cb01128d11791 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
