From 78ff7707e16eff98f766c467aca8f77b1567bcc0 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Tue, 19 Dec 2017 12:39:48 -0800 Subject: [PATCH] Fix niggling PaginatedDataTable bugs (#13556) Prevent header from thinking it can wrap and then overflowing. Fix default footer string which lost its colon (localized values are fine). Make the "rows per page" drop-down include at least one value even when the table lacks many items. (Previously it would assert if your table was too short.) Make the footer scrollable. Fix some todos and improve some debug output. Tests for much of the above. --- .../flutter_gallery/lib/gallery/item.dart | 7 ++ .../flutter/lib/src/material/data_table.dart | 5 +- .../src/material/material_localizations.dart | 2 +- .../src/material/paginated_data_table.dart | 46 ++++--- .../lib/src/painting/text_painter.dart | 4 +- packages/flutter/lib/src/rendering/table.dart | 3 + packages/flutter/lib/src/widgets/text.dart | 4 + .../test/material/data_table_test.dart | 112 ++++++++++++++++++ .../material/paginated_data_table_test.dart | 86 ++++++++++++++ .../flutter/test/rendering/table_test.dart | 2 +- 10 files changed, 247 insertions(+), 24 deletions(-) diff --git a/examples/flutter_gallery/lib/gallery/item.dart b/examples/flutter_gallery/lib/gallery/item.dart index 2a66be7739f..eef79d39d41 100644 --- a/examples/flutter_gallery/lib/gallery/item.dart +++ b/examples/flutter_gallery/lib/gallery/item.dart @@ -109,6 +109,13 @@ List _buildGalleryItems() { routeName: ChipDemo.routeName, buildRoute: (BuildContext context) => new ChipDemo(), ), + new GalleryItem( + title: 'Data tables', + subtitle: 'Data tables', + category: 'Material Components', + routeName: DataTableDemo.routeName, + buildRoute: (BuildContext context) => new DataTableDemo(), + ), new GalleryItem( title: 'Date and time pickers', subtitle: 'Date and time selection widgets', diff --git a/packages/flutter/lib/src/material/data_table.dart b/packages/flutter/lib/src/material/data_table.dart index 8d0f8922d35..06a9a7fa3c3 100644 --- a/packages/flutter/lib/src/material/data_table.dart +++ b/packages/flutter/lib/src/material/data_table.dart @@ -411,14 +411,15 @@ class DataTable extends StatelessWidget { alignment: numeric ? Alignment.centerRight : AlignmentDirectional.centerStart, child: new AnimatedDefaultTextStyle( style: new TextStyle( - // TODO(ianh): font family should be Roboto; see https://github.com/flutter/flutter/issues/3116 + // TODO(ianh): font family should match Theme; see https://github.com/flutter/flutter/issues/3116 fontWeight: FontWeight.w500, fontSize: _kHeadingFontSize, - height: _kHeadingRowHeight / _kHeadingFontSize, + height: math.min(1.0, _kHeadingRowHeight / _kHeadingFontSize), color: (Theme.of(context).brightness == Brightness.light) ? ((onSort != null && sorted) ? Colors.black87 : Colors.black54) : ((onSort != null && sorted) ? Colors.white : Colors.white70), ), + softWrap: false, duration: _kSortArrowAnimationDuration, child: label, ), diff --git a/packages/flutter/lib/src/material/material_localizations.dart b/packages/flutter/lib/src/material/material_localizations.dart index 7b809f48954..481fae87a5b 100644 --- a/packages/flutter/lib/src/material/material_localizations.dart +++ b/packages/flutter/lib/src/material/material_localizations.dart @@ -503,7 +503,7 @@ class DefaultMaterialLocalizations implements MaterialLocalizations { } @override - String get rowsPerPageTitle => 'Rows per page'; + String get rowsPerPageTitle => 'Rows per page:'; @override String selectedRowCountTitle(int selectedRowCount) { diff --git a/packages/flutter/lib/src/material/paginated_data_table.dart b/packages/flutter/lib/src/material/paginated_data_table.dart index 3dcda74063b..370d96eb687 100644 --- a/packages/flutter/lib/src/material/paginated_data_table.dart +++ b/packages/flutter/lib/src/material/paginated_data_table.dart @@ -324,7 +324,7 @@ class PaginatedDataTableState extends State { final List footerWidgets = []; if (widget.onRowsPerPageChanged != null) { final List availableRowsPerPage = widget.availableRowsPerPage - .where((int value) => value <= _rowCount) + .where((int value) => (value <= _rowCount || value == widget.rowsPerPage)) .map>((int value) { return new DropdownMenuItem( value: value, @@ -333,15 +333,22 @@ class PaginatedDataTableState extends State { }) .toList(); footerWidgets.addAll([ + new Container(width: 14.0), // to match trailing padding in case we overflow and end up scrolling new Text(localizations.rowsPerPageTitle), - new DropdownButtonHideUnderline( - child: new DropdownButton( - items: availableRowsPerPage, - value: widget.rowsPerPage, - onChanged: widget.onRowsPerPageChanged, - style: footerTextStyle, - iconSize: 24.0 - ) + new ConstrainedBox( + constraints: const BoxConstraints(minWidth: 64.0), // 40.0 for the text, 24.0 for the icon + child: new Align( + alignment: AlignmentDirectional.centerEnd, + child: new DropdownButtonHideUnderline( + child: new DropdownButton( + items: availableRowsPerPage, + value: widget.rowsPerPage, + onChanged: widget.onRowsPerPageChanged, + style: footerTextStyle, + iconSize: 24.0, + ), + ), + ), ), ]); } @@ -422,15 +429,18 @@ class PaginatedDataTableState extends State { ), child: new Container( height: 56.0, - child: new Row( - mainAxisAlignment: MainAxisAlignment.end, - children: footerWidgets - ) - ) - ) - ) - ] - ) + child: new SingleChildScrollView( + scrollDirection: Axis.horizontal, + reverse: true, + child: new Row( + children: footerWidgets, + ), + ), + ), + ), + ), + ], + ), ); } } diff --git a/packages/flutter/lib/src/painting/text_painter.dart b/packages/flutter/lib/src/painting/text_painter.dart index f3134034e9a..d80c2102683 100644 --- a/packages/flutter/lib/src/painting/text_painter.dart +++ b/packages/flutter/lib/src/painting/text_painter.dart @@ -407,7 +407,7 @@ class TextPainter { final int nextCodeUnit = _text.codeUnitAt(offset); if (nextCodeUnit == null) return null; - // TODO(goderbauer): doesn't handle flag emojis (https://github.com/flutter/flutter/issues/13404). + // TODO(goderbauer): doesn't handle extended grapheme clusters with more than one Unicode scalar value (https://github.com/flutter/flutter/issues/13404). return _isUtf16Surrogate(nextCodeUnit) ? offset + 2 : offset + 1; } @@ -417,7 +417,7 @@ class TextPainter { final int prevCodeUnit = _text.codeUnitAt(offset - 1); if (prevCodeUnit == null) return null; - // TODO(goderbauer): doesn't handle flag emojis (https://github.com/flutter/flutter/issues/13404). + // TODO(goderbauer): doesn't handle extended grapheme clusters with more than one Unicode scalar value (https://github.com/flutter/flutter/issues/13404). return _isUtf16Surrogate(prevCodeUnit) ? offset - 2 : offset - 1; } diff --git a/packages/flutter/lib/src/rendering/table.dart b/packages/flutter/lib/src/rendering/table.dart index 1effa3f2b3a..239049b6836 100644 --- a/packages/flutter/lib/src/rendering/table.dart +++ b/packages/flutter/lib/src/rendering/table.dart @@ -113,6 +113,9 @@ class IntrinsicColumnWidth extends TableColumnWidth { @override double flex(Iterable cells) => _flex; + + @override + String toString() => '$runtimeType(flex: ${_flex?.toStringAsFixed(1)})'; } /// Sizes the column to a specific number of pixels. diff --git a/packages/flutter/lib/src/widgets/text.dart b/packages/flutter/lib/src/widgets/text.dart index 9f1202b3dd7..d63f438de32 100644 --- a/packages/flutter/lib/src/widgets/text.dart +++ b/packages/flutter/lib/src/widgets/text.dart @@ -144,6 +144,10 @@ class DefaultTextStyle extends InheritedWidget { void debugFillProperties(DiagnosticPropertiesBuilder description) { super.debugFillProperties(description); style?.debugFillProperties(description); + description.add(new EnumProperty('textAlign', textAlign, defaultValue: null)); + description.add(new FlagProperty('softWrap', value: softWrap, ifTrue: 'wrapping at box width', ifFalse: 'no wrapping except at line break characters', showName: true)); + description.add(new EnumProperty('overflow', overflow, defaultValue: null)); + description.add(new IntProperty('maxLines', maxLines, defaultValue: null)); } } diff --git a/packages/flutter/test/material/data_table_test.dart b/packages/flutter/test/material/data_table_test.dart index 08cdfd70f22..456c35a95a9 100644 --- a/packages/flutter/test/material/data_table_test.dart +++ b/packages/flutter/test/material/data_table_test.dart @@ -98,4 +98,116 @@ void main() { expect(log, ['row-selected: KitKat']); log.clear(); }); + + testWidgets('DataTable overflow test - header', (WidgetTester tester) async { + await tester.pumpWidget( + new MaterialApp( + home: new Material( + child: new DataTable( + columns: [ + new DataColumn( + label: new Text('X' * 2000), + ), + ], + rows: const [ + const DataRow( + cells: const [ + const DataCell( + const Text('X'), + ), + ], + ), + ], + ), + ), + ), + ); + expect(tester.renderObject(find.byType(Text).first).size.width, greaterThan(800.0)); + expect(tester.renderObject(find.byType(Row).first).size.width, greaterThan(800.0)); + expect(tester.takeException(), isNull); // column overflows table, but text doesn't overflow cell + }); + + testWidgets('DataTable overflow test - header with spaces', (WidgetTester tester) async { + await tester.pumpWidget( + new MaterialApp( + home: new Material( + child: new DataTable( + columns: [ + new DataColumn( + label: new Text('X ' * 2000), // has soft wrap points, but they should be ignored + ), + ], + rows: const [ + const DataRow( + cells: const [ + const DataCell( + const Text('X'), + ), + ], + ), + ], + ), + ), + ), + ); + expect(tester.renderObject(find.byType(Text).first).size.width, greaterThan(800.0)); + expect(tester.renderObject(find.byType(Row).first).size.width, greaterThan(800.0)); + expect(tester.takeException(), isNull); // column overflows table, but text doesn't overflow cell + }, skip: true); // https://github.com/flutter/flutter/issues/13512 + + testWidgets('DataTable overflow test', (WidgetTester tester) async { + await tester.pumpWidget( + new MaterialApp( + home: new Material( + child: new DataTable( + columns: const [ + const DataColumn( + label: const Text('X'), + ), + ], + rows: [ + new DataRow( + cells: [ + new DataCell( + new Text('X' * 2000), + ), + ], + ), + ], + ), + ), + ), + ); + expect(tester.renderObject(find.byType(Text).first).size.width, lessThan(800.0)); + expect(tester.renderObject(find.byType(Row).first).size.width, greaterThan(800.0)); + expect(tester.takeException(), isNull); // cell overflows table, but text doesn't overflow cell + }); + + testWidgets('DataTable overflow test', (WidgetTester tester) async { + await tester.pumpWidget( + new MaterialApp( + home: new Material( + child: new DataTable( + columns: const [ + const DataColumn( + label: const Text('X'), + ), + ], + rows: [ + new DataRow( + cells: [ + new DataCell( + new Text('X ' * 2000), // wraps + ), + ], + ), + ], + ), + ), + ), + ); + expect(tester.renderObject(find.byType(Text).first).size.width, lessThan(800.0)); + expect(tester.renderObject(find.byType(Row).first).size.width, lessThan(800.0)); + expect(tester.takeException(), isNull); + }); } diff --git a/packages/flutter/test/material/paginated_data_table_test.dart b/packages/flutter/test/material/paginated_data_table_test.dart index 8592801e1cb..19c9e21119b 100644 --- a/packages/flutter/test/material/paginated_data_table_test.dart +++ b/packages/flutter/test/material/paginated_data_table_test.dart @@ -192,4 +192,90 @@ void main() { expect(log, ['action: adjust']); log.clear(); }); + + testWidgets('PaginatedDataTable text alignment', (WidgetTester tester) async { + await tester.pumpWidget(new MaterialApp( + home: new PaginatedDataTable( + header: const Text('HEADER'), + source: new TestDataSource(), + rowsPerPage: 8, + availableRowsPerPage: [ + 8, 9, + ], + onRowsPerPageChanged: (int rowsPerPage) { }, + columns: [ + const DataColumn(label: const Text('COL1')), + const DataColumn(label: const Text('COL2')), + const DataColumn(label: const Text('COL3')), + ], + ), + )); + expect(find.text('Rows per page:'), findsOneWidget); + expect(find.text('8'), findsOneWidget); + expect(tester.getTopRight(find.text('8')).dx, tester.getTopRight(find.text('Rows per page:')).dx + 40.0); // per spec + }); + + testWidgets('PaginatedDataTable with large text', (WidgetTester tester) async { + final TestDataSource source = new TestDataSource(); + await tester.pumpWidget(new MaterialApp( + home: new MediaQuery( + data: const MediaQueryData( + textScaleFactor: 20.0, + ), + child: new PaginatedDataTable( + header: const Text('HEADER'), + source: source, + rowsPerPage: 501, + availableRowsPerPage: [ 501 ], + onRowsPerPageChanged: (int rowsPerPage) { }, + columns: [ + const DataColumn(label: const Text('COL1')), + const DataColumn(label: const Text('COL2')), + const DataColumn(label: const Text('COL3')), + ], + ), + ), + )); + // the column overflows because we're forcing it to 600 pixels high + expect(tester.takeException(), contains('A RenderFlex overflowed by')); + expect(find.text('Rows per page:'), findsOneWidget); + // Test that we will show some options in the drop down even if the lowest option is bigger than the source: + assert(501 > source.rowCount); + expect(find.text('501'), findsOneWidget); + // Test that it fits: + expect(tester.getTopRight(find.text('501')).dx, greaterThanOrEqualTo(tester.getTopRight(find.text('Rows per page:')).dx + 40.0)); + }); + + testWidgets('PaginatedDataTable footer scrolls', (WidgetTester tester) async { + final TestDataSource source = new TestDataSource(); + await tester.pumpWidget(new MaterialApp( + home: new Align( + alignment: Alignment.topLeft, + child: new SizedBox( + width: 100.0, + child: new PaginatedDataTable( + header: const Text('HEADER'), + source: source, + rowsPerPage: 5, + availableRowsPerPage: [ 5 ], + onRowsPerPageChanged: (int rowsPerPage) { }, + columns: [ + const DataColumn(label: const Text('COL1')), + const DataColumn(label: const Text('COL2')), + const DataColumn(label: const Text('COL3')), + ], + ), + ), + ), + )); + expect(find.text('Rows per page:'), findsOneWidget); + expect(tester.getTopLeft(find.text('Rows per page:')).dx, lessThan(0.0)); // off screen + await tester.dragFrom( + new Offset(50.0, tester.getTopLeft(find.text('Rows per page:')).dy), + const Offset(1000.0, 0.0), + ); + await tester.pump(); + expect(find.text('Rows per page:'), findsOneWidget); + expect(tester.getTopLeft(find.text('Rows per page:')).dx, 18.0); // 14 padding in the footer row, 4 padding from the card + }); } diff --git a/packages/flutter/test/rendering/table_test.dart b/packages/flutter/test/rendering/table_test.dart index f564d3224fb..4681912c6a7 100644 --- a/packages/flutter/test/rendering/table_test.dart +++ b/packages/flutter/test/rendering/table_test.dart @@ -82,7 +82,7 @@ void main() { ' │ parentData: offset=Offset(335.0, 185.0) (can use size)\n' ' │ constraints: BoxConstraints(0.0<=w<=800.0, 0.0<=h<=600.0)\n' ' │ size: Size(130.0, 230.0)\n' - ' │ default column width: IntrinsicColumnWidth\n' + ' │ default column width: IntrinsicColumnWidth(flex: null)\n' ' │ table size: 5×5\n' ' │ column offsets: 0.0, 10.0, 30.0, 130.0, 130.0\n' ' │ row offsets: 0.0, 30.0, 30.0, 30.0, 30.0, 230.0\n'