From 471a7f241c25db63b55a516ec18269cd83e67272 Mon Sep 17 00:00:00 2001 From: Hixie Date: Wed, 17 Jun 2015 15:10:42 -0700 Subject: [PATCH] Make the popup menu work again. This removes the requirement that things with the same type things have unique keys. Now, anything without a key is assumed to be interchangeable. R=abarth@chromium.org Review URL: https://codereview.chromium.org/1178723010. --- examples/stocks2/lib/stock_home.dart | 14 +++------- examples/stocks2/lib/stock_row.dart | 29 +++++++++++++------- examples/stocks2/lib/stock_settings.dart | 3 +- sdk/lib/widgets/basic.dart | 4 +-- sdk/lib/widgets/drawer_header.dart | 3 +- sdk/lib/widgets/fixed_height_scrollable.dart | 5 +++- sdk/lib/widgets/popup_menu.dart | 4 +-- sdk/lib/widgets/widget.dart | 19 +++++++------ 8 files changed, 42 insertions(+), 39 deletions(-) diff --git a/examples/stocks2/lib/stock_home.dart b/examples/stocks2/lib/stock_home.dart index e7b58d979..c7f2ba800 100644 --- a/examples/stocks2/lib/stock_home.dart +++ b/examples/stocks2/lib/stock_home.dart @@ -114,38 +114,32 @@ class StockHome extends Component { children: [ new DrawerHeader(children: [new Text('Stocks')]), new MenuItem( - key: 'Stock list', icon: 'action/assessment', children: [new Text('Stock List')]), new MenuItem( - key: 'Account Balance', icon: 'action/account_balance', children: [new Text('Account Balance')]), - new MenuDivider(key: 'div1'), + new MenuDivider(), new MenuItem( - key: 'Optimistic Menu Item', icon: 'action/thumb_up', onGestureTap: (event) => _handleStockModeChange(StockMode.optimistic), children: [ new Flexible(child: new Text('Optimistic')), - new Radio(key: 'optimistic-radio', value: StockMode.optimistic, groupValue: _stockMode, onChanged: _handleStockModeChange) + new Radio(value: StockMode.optimistic, groupValue: _stockMode, onChanged: _handleStockModeChange) ]), new MenuItem( - key: 'Pessimistic Menu Item', icon: 'action/thumb_down', onGestureTap: (event) => _handleStockModeChange(StockMode.pessimistic), children: [ new Flexible(child: new Text('Pessimistic')), - new Radio(key: 'pessimistic-radio', value: StockMode.pessimistic, groupValue: _stockMode, onChanged: _handleStockModeChange) + new Radio(value: StockMode.pessimistic, groupValue: _stockMode, onChanged: _handleStockModeChange) ]), - new MenuDivider(key: 'div2'), + new MenuDivider(), new MenuItem( - key: 'Settings', icon: 'action/settings', onGestureTap: (event) => _navigator.pushNamed('/settings'), children: [new Text('Settings')]), new MenuItem( - key: 'Help & Feedback', icon: 'action/help', children: [new Text('Help & Feedback')]) ] diff --git a/examples/stocks2/lib/stock_row.dart b/examples/stocks2/lib/stock_row.dart index 7e3ee7d16..bd48c5f72 100644 --- a/examples/stocks2/lib/stock_row.dart +++ b/examples/stocks2/lib/stock_row.dart @@ -27,16 +27,25 @@ class StockRow extends Component { List children = [ new Container( - child: new StockArrow(percentChange: stock.percentChange), - margin: const EdgeDims.only(right: 5.0)), - new Flexible(child: new Text(stock.symbol), flex: 2, key: "symbol"), - // TODO(hansmuller): text-align: right - new Flexible(child: new Text(lastSale, - style: const TextStyle(textAlign: TextAlign.right)), - key: "lastSale"), - new Flexible(child: new Text(changeInPrice, - style: typography.black.caption.copyWith(textAlign: TextAlign.right)), - key: "changeInPrice") + child: new StockArrow(percentChange: stock.percentChange), + margin: const EdgeDims.only(right: 5.0) + ), + new Flexible( + child: new Text(stock.symbol), + flex: 2 + ), + new Flexible( + child: new Text( + lastSale, + style: const TextStyle(textAlign: TextAlign.right) + ) + ), + new Flexible( + child: new Text( + changeInPrice, + style: typography.black.caption.copyWith(textAlign: TextAlign.right) + ) + ) ]; // TODO(hansmuller): An explicit |height| shouldn't be needed diff --git a/examples/stocks2/lib/stock_settings.dart b/examples/stocks2/lib/stock_settings.dart index 1fdfdf8d2..3ca3aef5f 100644 --- a/examples/stocks2/lib/stock_settings.dart +++ b/examples/stocks2/lib/stock_settings.dart @@ -41,12 +41,11 @@ class StockSettings extends Component { decoration: new BoxDecoration(backgroundColor: colors.Grey[50]), child: new Block([ new MenuItem( - key: 'Optimistic Setting', icon: 'action/thumb_up', onGestureTap: (event) => _handleAwesomeChanged(!_awesome), children: [ new Flexible(child: new Text('Everything is awesome')), - new Checkbox(key: 'awesome', value: _awesome, onChanged: _handleAwesomeChanged) + new Checkbox(value: _awesome, onChanged: _handleAwesomeChanged) ] ), ]) diff --git a/sdk/lib/widgets/basic.dart b/sdk/lib/widgets/basic.dart index 453098b46..ed2402478 100644 --- a/sdk/lib/widgets/basic.dart +++ b/sdk/lib/widgets/basic.dart @@ -346,7 +346,7 @@ class Flexible extends ParentDataNode { } class Inline extends RenderObjectWrapper { - Inline({ Object key, this.text }) : super(key: key); + Inline({ String key, this.text }) : super(key: key); RenderParagraph get root => super.root; RenderParagraph createNode() => new RenderParagraph(text); @@ -366,7 +366,7 @@ class Inline extends RenderObjectWrapper { } class Text extends Component { - Text(this.data, { TextStyle this.style }) : super(key: '*text*'); + Text(data, { String key, TextStyle this.style }) : data = data, super(key: key); final String data; final TextStyle style; bool get interchangeable => true; diff --git a/sdk/lib/widgets/drawer_header.dart b/sdk/lib/widgets/drawer_header.dart index 879995289..5a8f4ecb3 100644 --- a/sdk/lib/widgets/drawer_header.dart +++ b/sdk/lib/widgets/drawer_header.dart @@ -27,9 +27,8 @@ class DrawerHeader extends Component { padding: const EdgeDims.only(bottom: 7.0), margin: const EdgeDims.only(bottom: 8.0), child: new Flex([ - new Flexible(child: new Container(key: 'drawer-header-spacer')), + new Flexible(child: new Container()), new Container( - key: 'drawer-header-label', padding: const EdgeDims.symmetric(horizontal: 16.0), child: new Flex(children, direction: FlexDirection.horizontal) )], diff --git a/sdk/lib/widgets/fixed_height_scrollable.dart b/sdk/lib/widgets/fixed_height_scrollable.dart index 4905336d8..3ae0d0474 100644 --- a/sdk/lib/widgets/fixed_height_scrollable.dart +++ b/sdk/lib/widgets/fixed_height_scrollable.dart @@ -68,12 +68,15 @@ abstract class FixedHeightScrollable extends Scrollable { } } + List items = buildItems(itemShowIndex, itemShowCount); + assert(items.every((item) => item.key != null)); + return new SizeObserver( callback: _handleSizeChanged, child: new ClipRect( child: new Transform( transform: transform, - child: new Block(buildItems(itemShowIndex, itemShowCount)) + child: new Block(items) ) ) ); diff --git a/sdk/lib/widgets/popup_menu.dart b/sdk/lib/widgets/popup_menu.dart index 6ac1be4b9..88d08f54c 100644 --- a/sdk/lib/widgets/popup_menu.dart +++ b/sdk/lib/widgets/popup_menu.dart @@ -94,9 +94,7 @@ class PopupMenu extends AnimatedComponent { int i = 0; List children = new List.from(items.map((Widget item) { double opacity = _opacityFor(i); - return new PopupMenuItem(key: '${key}-${item.key}', - child: item, - opacity: opacity); + return new PopupMenuItem(child: item, opacity: opacity); })); return new Opacity( diff --git a/sdk/lib/widgets/widget.dart b/sdk/lib/widgets/widget.dart index 1154d73a1..a83b2c77c 100644 --- a/sdk/lib/widgets/widget.dart +++ b/sdk/lib/widgets/widget.dart @@ -21,8 +21,7 @@ final bool _shouldLogRenderDuration = false; // can be sync'd. abstract class Widget { - Widget({ String key }) { - _key = key != null ? key : runtimeType.toString(); + Widget({ String key }) : _key = key { assert(this is AbstractWidgetRoot || this is App || _inRenderDirtyComponents); // you should not build the UI tree ahead of time, build it only during build() } @@ -84,8 +83,6 @@ abstract class Widget { // Component._retainStatefulNodeIfPossible() calls syncFields(). bool _retainStatefulNodeIfPossible(Widget old) => false; - bool get interchangeable => false; // if true, then keys can be duplicated - void _sync(Widget old, dynamic slot); // 'slot' is the identifier that the parent RenderObjectWrapper uses to know // where to put this descendant @@ -154,7 +151,11 @@ abstract class Widget { return node; } - String toString() => '$runtimeType($key)'; + String toString() { + if (key == null) + return '$runtimeType(unkeyed)'; + return '$runtimeType("$key")'; + } } @@ -628,11 +629,11 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper { var idSet = new HashSet(); for (var child in children) { assert(child != null); - if (child.interchangeable) + if (child.key == null) continue; // when these nodes are reordered, we just reassign the data if (!idSet.add(child.key)) { - throw '''If multiple non-interchangeable nodes exist as children of another node, they must have unique keys. Duplicate: "${child.key}"'''; + throw '''If multiple keyed nodes exist as children of another node, they must have unique keys. $this has duplicate child key "${child.key}".'''; } } return false; @@ -700,13 +701,13 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper { oldNodeIdMap = new HashMap(); for (int i = oldStartIndex; i < oldEndIndex; i++) { var node = oldChildren[i]; - if (!node.interchangeable) + if (node.key != null) oldNodeIdMap.putIfAbsent(node.key, () => node); } } bool searchForOldNode() { - if (currentNode.interchangeable) + if (currentNode.key == null) return false; // never re-order these nodes ensureOldIdMap(); -- GitLab