Skip to content

Commit 049cc84

Browse files
PiinksPiinks
authored
[two_dimensional_scrollables] Add debug check for pinning out of bounds (#11366)
Fixes flutter/flutter#136833 This cleans up an old todo from when we added support for pinned rows and columns to TableView. It is possible for the pinned extents to exceed the viewport bounds, which would make scrolling impossible, and unpinned rows/columns inaccessible. This adds debug check during layout to catch this now. ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
1 parent e627503 commit 049cc84

4 files changed

Lines changed: 290 additions & 8 deletions

File tree

‎packages/two_dimensional_scrollables/CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
## 0.4.1
2+
3+
* Adds warnings for TableView pinned rows and columns that exceed the viewport dimensions.
4+
15
## 0.4.0
26

37
* Added `alignment` property to `TableView` and `TreeView` to align content within the viewport when it is smaller than the viewport extent.

‎packages/two_dimensional_scrollables/lib/src/table_view/table.dart‎

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -429,13 +429,6 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
429429
);
430430
}
431431

432-
// TODO(Piinks): Pinned rows/cols do not account for what is visible on the
433-
// screen. Ostensibly, we would not want to have pinned rows/columns that
434-
// extend beyond the viewport, we would never see them as they would never
435-
// scroll into view. So this currently implementation is fairly assuming
436-
// we will never have rows/cols that are outside of the viewport. We should
437-
// maybe add an assertion for this during layout.
438-
// https://github.com/flutter/flutter/issues/136833
439432
int? get _lastPinnedRow =>
440433
delegate.pinnedRowCount > 0 ? delegate.pinnedRowCount - 1 : null;
441434
int? get _lastPinnedColumn =>
@@ -448,6 +441,49 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
448441
? _columnMetrics[_lastPinnedColumn]!.trailingOffset
449442
: 0.0;
450443

444+
void _debugCheckPinnedExtent() {
445+
assert(() {
446+
if (_pinnedColumnsExtent > viewportDimension.width) {
447+
debugPrint(
448+
'TableView has pinned columns with a total width of '
449+
'$_pinnedColumnsExtent, which exceeds the viewport width of '
450+
'${viewportDimension.width}. This will prevent unpinned columns '
451+
'from being visible.',
452+
);
453+
} else if (_pinnedColumnsExtent == viewportDimension.width) {
454+
final bool hasUnpinnedColumns =
455+
delegate.columnCount == null ||
456+
delegate.columnCount! > delegate.pinnedColumnCount;
457+
if (hasUnpinnedColumns) {
458+
debugPrint(
459+
'TableView has pinned columns that fully consume the viewport width. '
460+
'Unpinned columns will not be visible.',
461+
);
462+
}
463+
}
464+
465+
if (_pinnedRowsExtent > viewportDimension.height) {
466+
debugPrint(
467+
'TableView has pinned rows with a total height of '
468+
'$_pinnedRowsExtent, which exceeds the viewport height of '
469+
'${viewportDimension.height}. This will prevent unpinned rows '
470+
'from being visible.',
471+
);
472+
} else if (_pinnedRowsExtent == viewportDimension.height) {
473+
final bool hasUnpinnedRows =
474+
delegate.rowCount == null ||
475+
delegate.rowCount! > delegate.pinnedRowCount;
476+
if (hasUnpinnedRows) {
477+
debugPrint(
478+
'TableView has pinned rows that fully consume the viewport height. '
479+
'Unpinned rows will not be visible.',
480+
);
481+
}
482+
}
483+
return true;
484+
}());
485+
}
486+
451487
@override
452488
TableViewParentData parentDataOf(RenderBox child) =>
453489
super.parentDataOf(child) as TableViewParentData;
@@ -892,6 +928,7 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
892928
_updateColumnMetrics();
893929
_updateRowMetrics();
894930
_updateScrollBounds();
931+
_debugCheckPinnedExtent();
895932
} else {
896933
// Updates the visible cells based on cached table metrics.
897934
_updateFirstAndLastVisibleCell();

‎packages/two_dimensional_scrollables/pubspec.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
name: two_dimensional_scrollables
22
description: Widgets that scroll using the two dimensional scrolling foundation.
3-
version: 0.4.0
3+
version: 0.4.1
44
repository: https://github.com/flutter/packages/tree/main/packages/two_dimensional_scrollables
55
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+two_dimensional_scrollables%22+
66

‎packages/two_dimensional_scrollables/test/table_view/pinned_extent_warning_test.dart‎

Lines changed: 241 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,241 @@
1+
// Copyright 2013 The Flutter Authors
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:flutter/foundation.dart';
6+
import 'package:flutter/material.dart';
7+
import 'package:flutter_test/flutter_test.dart';
8+
import 'package:two_dimensional_scrollables/two_dimensional_scrollables.dart';
9+
10+
void main() {
11+
group('TableView pinned extent warnings', () {
12+
testWidgets('Warns when pinned columns exceed viewport width', (
13+
WidgetTester tester,
14+
) async {
15+
// Regression test for https://github.com/flutter/flutter/issues/136833
16+
final log = <String>[];
17+
final DebugPrintCallback oldDebugPrint = debugPrint;
18+
debugPrint = (String? message, {int? wrapWidth}) {
19+
log.add(message!);
20+
};
21+
22+
await tester.pumpWidget(
23+
MaterialApp(
24+
home: Scaffold(
25+
body: SizedBox(
26+
width: 200,
27+
height: 400,
28+
child: TableView.builder(
29+
columnCount: 5,
30+
rowCount: 5,
31+
pinnedColumnCount: 3,
32+
columnBuilder: (int index) =>
33+
const TableSpan(extent: FixedTableSpanExtent(100)),
34+
rowBuilder: (int index) =>
35+
const TableSpan(extent: FixedTableSpanExtent(100)),
36+
cellBuilder: (BuildContext context, TableVicinity vicinity) =>
37+
const TableViewCell(child: SizedBox.shrink()),
38+
),
39+
),
40+
),
41+
),
42+
);
43+
44+
// Pinned columns extent = 300 (3 * 100), viewport width = 200.
45+
// A warning is expected because the pinned columns are wider than the
46+
// viewport, meaning even the pinned content cannot be fully displayed.
47+
expect(
48+
log,
49+
contains(
50+
matches(
51+
r'TableView has pinned columns with a total width of 300(\.0)?, which exceeds the viewport width of 200(\.0)?',
52+
),
53+
),
54+
);
55+
debugPrint = oldDebugPrint;
56+
});
57+
58+
testWidgets('Warns when pinned rows exceed viewport height', (
59+
WidgetTester tester,
60+
) async {
61+
// Regression test for https://github.com/flutter/flutter/issues/136833
62+
final log = <String>[];
63+
final DebugPrintCallback oldDebugPrint = debugPrint;
64+
debugPrint = (String? message, {int? wrapWidth}) {
65+
log.add(message!);
66+
};
67+
68+
await tester.pumpWidget(
69+
MaterialApp(
70+
home: Scaffold(
71+
body: SizedBox(
72+
width: 400,
73+
height: 200,
74+
child: TableView.builder(
75+
columnCount: 5,
76+
rowCount: 5,
77+
pinnedRowCount: 3,
78+
columnBuilder: (int index) =>
79+
const TableSpan(extent: FixedTableSpanExtent(100)),
80+
rowBuilder: (int index) =>
81+
const TableSpan(extent: FixedTableSpanExtent(100)),
82+
cellBuilder: (BuildContext context, TableVicinity vicinity) =>
83+
const TableViewCell(child: SizedBox.shrink()),
84+
),
85+
),
86+
),
87+
),
88+
);
89+
90+
// Pinned rows extent = 300 (3 * 100), viewport height = 200.
91+
// A warning is expected because the pinned rows are taller than the
92+
// viewport, meaning even the pinned content cannot be fully displayed.
93+
expect(
94+
log,
95+
contains(
96+
matches(
97+
r'TableView has pinned rows with a total height of 300(\.0)?, which exceeds the viewport height of 200(\.0)?',
98+
),
99+
),
100+
);
101+
debugPrint = oldDebugPrint;
102+
});
103+
104+
testWidgets(
105+
'Warns when pinned columns fully consume viewport width and there are unpinned columns',
106+
(WidgetTester tester) async {
107+
// Regression test for https://github.com/flutter/flutter/issues/136833
108+
final log = <String>[];
109+
final DebugPrintCallback oldDebugPrint = debugPrint;
110+
debugPrint = (String? message, {int? wrapWidth}) {
111+
log.add(message!);
112+
};
113+
114+
await tester.pumpWidget(
115+
MaterialApp(
116+
home: Scaffold(
117+
body: SizedBox(
118+
width: 200,
119+
height: 400,
120+
child: TableView.builder(
121+
columnCount: 3,
122+
rowCount: 5,
123+
pinnedColumnCount: 2,
124+
columnBuilder: (int index) =>
125+
const TableSpan(extent: FixedTableSpanExtent(100)),
126+
rowBuilder: (int index) =>
127+
const TableSpan(extent: FixedTableSpanExtent(100)),
128+
cellBuilder: (BuildContext context, TableVicinity vicinity) =>
129+
const TableViewCell(child: SizedBox.shrink()),
130+
),
131+
),
132+
),
133+
),
134+
);
135+
136+
// Pinned columns extent = 200 (2 * 100), viewport width = 200.
137+
// There is 1 unpinned column (columnCount: 3, pinnedColumnCount: 2).
138+
// Since the pinned columns take up the entire viewport width, the
139+
// unpinned column will never be visible during scrolling.
140+
expect(
141+
log,
142+
contains(
143+
'TableView has pinned columns that fully consume the viewport width. Unpinned columns will not be visible.',
144+
),
145+
);
146+
debugPrint = oldDebugPrint;
147+
},
148+
);
149+
150+
testWidgets(
151+
'Warns when pinned rows fully consume viewport height and there are unpinned rows',
152+
(WidgetTester tester) async {
153+
// Regression test for https://github.com/flutter/flutter/issues/136833
154+
final log = <String>[];
155+
final DebugPrintCallback oldDebugPrint = debugPrint;
156+
debugPrint = (String? message, {int? wrapWidth}) {
157+
log.add(message!);
158+
};
159+
160+
await tester.pumpWidget(
161+
MaterialApp(
162+
home: Scaffold(
163+
body: SizedBox(
164+
width: 400,
165+
height: 200,
166+
child: TableView.builder(
167+
columnCount: 5,
168+
rowCount: 3,
169+
pinnedRowCount: 2,
170+
columnBuilder: (int index) =>
171+
const TableSpan(extent: FixedTableSpanExtent(100)),
172+
rowBuilder: (int index) =>
173+
const TableSpan(extent: FixedTableSpanExtent(100)),
174+
cellBuilder: (BuildContext context, TableVicinity vicinity) =>
175+
const TableViewCell(child: SizedBox.shrink()),
176+
),
177+
),
178+
),
179+
),
180+
);
181+
182+
// Pinned rows extent = 200 (2 * 100), viewport height = 200.
183+
// There is 1 unpinned row (rowCount: 3, pinnedRowCount: 2).
184+
// Since the pinned rows take up the entire viewport height, the
185+
// unpinned row will never be visible during scrolling.
186+
expect(
187+
log,
188+
contains(
189+
'TableView has pinned rows that fully consume the viewport height. Unpinned rows will not be visible.',
190+
),
191+
);
192+
debugPrint = oldDebugPrint;
193+
},
194+
);
195+
196+
testWidgets(
197+
'Does not warn when all columns are pinned even if they consume viewport',
198+
(WidgetTester tester) async {
199+
// Regression test for https://github.com/flutter/flutter/issues/136833
200+
final log = <String>[];
201+
final DebugPrintCallback oldDebugPrint = debugPrint;
202+
debugPrint = (String? message, {int? wrapWidth}) {
203+
log.add(message!);
204+
};
205+
206+
await tester.pumpWidget(
207+
MaterialApp(
208+
home: Scaffold(
209+
body: SizedBox(
210+
width: 200,
211+
height: 400,
212+
child: TableView.builder(
213+
columnCount: 2,
214+
rowCount: 5,
215+
pinnedColumnCount: 2,
216+
columnBuilder: (int index) =>
217+
const TableSpan(extent: FixedTableSpanExtent(100)),
218+
rowBuilder: (int index) =>
219+
const TableSpan(extent: FixedTableSpanExtent(100)),
220+
cellBuilder: (BuildContext context, TableVicinity vicinity) =>
221+
const TableViewCell(child: SizedBox.shrink()),
222+
),
223+
),
224+
),
225+
),
226+
);
227+
228+
// Pinned columns extent = 200 (2 * 100), viewport width = 200.
229+
// Although the pinned columns fully consume the viewport width,
230+
// ALL columns are pinned (columnCount: 2, pinnedColumnCount: 2).
231+
// Since there are no unpinned columns, no warning is issued about
232+
// unpinned columns being hidden.
233+
expect(
234+
log,
235+
isNot(contains(contains('Unpinned columns will not be visible'))),
236+
);
237+
debugPrint = oldDebugPrint;
238+
},
239+
);
240+
});
241+
}

0 commit comments

Comments
 (0)