[two_dimensional_scrollables] Merged cells for TableView #5917

Piinks · 2024-01-18T01:25:40Z

This adds support for merged cells in TableView.
This contains a breaking change that will require all children of the TableView to be a TableViewCell.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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

…stored/updated

Design doc for flutter/packages#5917

_Description of what this PR is changing or adding, and why:_ Design doc for flutter/packages#5917 _Issues fixed by this PR (if any):_ Related to flutter/flutter#131224 ## Presubmit checklist - [x] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [x] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [x] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer.

goderbauer

Great test coverage!

packages/two_dimensional_scrollables/lib/src/table_view/table_cell.dart

packages/two_dimensional_scrollables/lib/src/table_view/table_delegate.dart

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

packages/two_dimensional_scrollables/test/table_view/table_cell_test.dart

packages/two_dimensional_scrollables/test/table_view/table_delegate_test.dart

…cell.dart Co-authored-by: Michael Goderbauer <[email protected]>

goderbauer · 2024-01-31T00:23:13Z

Somehow the checks are rather unhappy now.

goderbauer · 2024-01-31T00:28:05Z

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

+    ChildVicinity vicinity, {
+    bool allowMerged = true,
+  }) {
+    return super.getChildFor(vicinity) ??


Did you mean to change this to the suggestion in that other comment?

I hadn't because I want to be able to assert that if we requested a child that returned null (using the super getChildFor first), then it is for certain covered by a merged cell.

Or, maybe stated better, I want to see if there is a canonical child first, and if not, need to ensure we have not lost a child so it asserts the vicinity returning null is in fact covered by a merged cell.

I see. That makes sense. Thanks for the explanation!

Piinks · 2024-01-31T15:12:10Z

Somehow the checks are rather unhappy now.

Oh I see I accidentally committed some changes to the example app.

goderbauer

LGTM

goderbauer · 2024-01-31T17:14:34Z

packages/two_dimensional_scrollables/lib/src/table_view/table_delegate.dart

 /// [TableVicinity] in a [TableView], but may return null.
 ///
 /// Used by [TableCellBuilderDelegate.builder] to build cells on demand for the
 /// table.
-typedef TableViewCellBuilder = Widget? Function(
+typedef TableViewCellBuilder = TableViewCell Function(


I didn't notice this before: Is this intentional that null is no longer allowed as a return value? If so, the doc above needs updating. (Remind me: what did it mean when this was previously returning null? What are we no longer allowing?)

This felt like a bug I came across. Typically in Flutter scrolling land, returning null from the child delegate means we have reached the end of the children. This is not currently supported, but is part of supporting infinite children in the table: flutter/flutter#131226.

I think it was here by mistake since if you return null currently, with or without this change, it will crash! :(

goderbauer · 2024-01-31T17:20:33Z

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

-/// represented by its [TableVicinity]. The table supports lazy rendering and
-/// will only instantiate those cells that are currently visible in the table's
-/// viewport and those that extend into the [cacheExtent].
+/// Each child [TableViewCell] can belong to exactly one row and one column as


über-nit: What do you think about inserting an "either" in this sentence between "to" and "exactly"? The first part of the sentence makes it sound like we don't support merged cells at all and then the second part catches you by surprise. With that either we can maybe emphasize a little more that the "exactly one row/column" is not the whole story.

Makes excellent sense to me!

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

goderbauer · 2024-01-31T17:23:32Z

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

+    ChildVicinity vicinity, {
+    bool allowMerged = true,
+  }) {
+    return super.getChildFor(vicinity) ??


I see. That makes sense. Thanks for the explanation!

goderbauer · 2024-01-31T17:25:47Z

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

+    // vicinity.
+    assert(_mergedVicinities.keys.contains(vicinity));
+    final TableVicinity mergedVicinity = _mergedVicinities[vicinity]!;
+    return getChildFor(mergedVicinity)!;


For my own sanity, should this call specify mapMergedVicinityToCanonicalChild: false to make it super clear, that we do not expect this call to recurse? It is a bug if this call to getChildFor would call _getMergedChildFor again, right? I think specifying this expectation here clearly would make this code a little easier to understand and follow.

Oh wow I totally missed that recursion. Ha! Gotcha, nice catch. 🙃

…tter/packages#5917)

flutter/packages@5b48c44...d37fb0a 2024-02-02 [email protected] Add a link the different possible Android virtual device configs (flutter/packages#6033) 2024-02-01 [email protected] Update the emulator versions and expose cipd. (flutter/packages#6025) 2024-02-01 [email protected] [tool] Add details to missing gradle coverage error (flutter/packages#6029) 2024-02-01 [email protected] [file_selector] Fix comment typo (flutter/packages#6027) 2024-02-01 [email protected] [camerax] Change `buildPreview` to return `Texture` versus `FutureBuilder` (flutter/packages#6021) 2024-02-01 [email protected] Manual roll Flutter from c65ab4d to e02e207 (38 revisions) (flutter/packages#6028) 2024-02-01 [email protected] Manual roll Flutter from 75a2e5b to c65ab4d (22 revisions) (flutter/packages#6026) 2024-02-01 [email protected] Roll Flutter from ace9181 to 75a2e5b (16 revisions) (flutter/packages#6017) 2024-02-01 [email protected] [webview_flutter] Support for handling basic authentication requests (flutter/packages#5727) 2024-01-31 [email protected] [tool] Extend `flutter test` workaround to other desktops (flutter/packages#6024) 2024-01-31 [email protected] [two_dimensional_scrollables] Merged cells for TableView (flutter/packages#5917) 2024-01-31 [email protected] [rfw] Restore RFW to 100% coverage after `ButtonBar` update (flutter/packages#6020) 2024-01-31 [email protected] [in_app_purchase] Convert storefront(), transactions(), canMakePayment(), and addPayment() to pigeon (flutter/packages#5910) 2024-01-31 [email protected] [in_app_purchase] Add play country code api (flutter/packages#5941) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

ahmednfwela · 2024-02-02T17:54:28Z

@Piinks It seems that the package wasn't uploaded on pub.dev after this PR, is there a reason for this ?

_Description of what this PR is changing or adding, and why:_ Design doc for flutter/packages#5917 _Issues fixed by this PR (if any):_ Related to flutter/flutter#131224 - [x] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [x] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [x] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer.

_Description of what this PR is changing or adding, and why:_ Design doc for flutter/packages#5917 _Issues fixed by this PR (if any):_ Related to flutter/flutter#131224 ## Presubmit checklist - [x] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [x] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [x] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer.

Fixes flutter/flutter#131224 â�¡ï¸� [Design Doc](https://docs.google.com/document/d/1UekXjG_VKmWYbsxDEzMqTb7F-6oUr05v998n5IqtVWs/edit?usp=sharing) â¬�ï¸� This adds support for merged cells in TableView. This contains a breaking change that will require all children of the TableView to be a TableViewCell. ![Screenshot 2024-01-25 at 4 38 49â�¯PM](https://github.com/flutter/packages/assets/16964204/02f4c158-23e9-401e-ac84-b6303d999095)

Piinks added the p: two_dimensional_scrollables Issues pertaining to the two_dimensional_scrollables package label Jan 18, 2024

ahmednfwela reviewed Jan 20, 2024

View reviewed changes

packages/two_dimensional_scrollables/lib/src/table_view/table.dart Outdated Show resolved Hide resolved

Piinks added 13 commits January 23, 2024 11:32

Need to account for decorations.

161bfd8

Rows work with resolution for decorations, example app needs to be re…

7245dcd

…stored/updated

Stop before rework

0d461c7

Got it. Need clean up and tests

3dd9658

Optimize build calls

4960a25

++

dd9f4c3

++

ab812f7

Update docs

629eb7f

Found bug, ugh

bd84020

Fixed it

9e9abaa

Found deco bug, stop to figure

ae22375

++

5993e7d

Fisnished tests

b8fbc86

Piinks force-pushed the mergedTableCells branch from 7cb9e1a to b8fbc86 Compare January 23, 2024 17:45

Piinks added 3 commits January 23, 2024 12:15

Clean up records in _paintCells

923ea8b

++

693db59

Finish self review

f88dd70

Piinks added a commit to flutter/website that referenced this pull request Jan 25, 2024

Add design doc for merged cells

eaeb5c3

Design doc for flutter/packages#5917

Piinks mentioned this pull request Jan 25, 2024

Add design doc for merged cells flutter/website#10086

Merged

3 tasks

Piinks changed the title ~~[WIP] [two_dimensional_scrollables] Merged cells for TableView~~ [two_dimensional_scrollables] Merged cells for TableView Jan 25, 2024

Piinks requested a review from goderbauer January 25, 2024 22:47

goderbauer reviewed Jan 27, 2024

View reviewed changes

Piinks and others added 3 commits January 30, 2024 12:08

Update packages/two_dimensional_scrollables/lib/src/table_view/table_…

03838f1

…cell.dart Co-authored-by: Michael Goderbauer <[email protected]>

Review feedback

6a3f661

Merge branch 'main' of github.com:flutter/packages into mergedTableCells

0aba463

github-actions bot added the platform-macos label Jan 30, 2024

Piinks requested a review from goderbauer January 30, 2024 18:30

goderbauer reviewed Jan 31, 2024

View reviewed changes

Piinks added 2 commits January 31, 2024 09:20

Review feedback, clean up example error

fab93be

Analyzer therapy

c51bab9

github-actions bot removed the platform-macos label Jan 31, 2024

goderbauer approved these changes Jan 31, 2024

View reviewed changes

Feedback

ac28304

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2024

auto-submit bot merged commit 2d0f24f into flutter:main Jan 31, 2024
78 checks passed

engine-flutter-autoroll mentioned this pull request Feb 1, 2024

Roll Packages from 5b48c446976f to 013763dd574e (6 revisions) flutter/flutter#142710

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 1, 2024

2d0f24f [two_dimensional_scrollables] Merged cells for TableView (flu…

8c54e25

…tter/packages#5917)

engine-flutter-autoroll mentioned this pull request Feb 1, 2024

Roll Packages from 5b48c446976f to be4dd938acc3 (7 revisions) flutter/flutter#142715

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 1, 2024

2d0f24f [two_dimensional_scrollables] Merged cells for TableView (flu…

d7bc623

…tter/packages#5917)

engine-flutter-autoroll mentioned this pull request Feb 1, 2024

Roll Packages from 5b48c446976f to aee1980899fc (8 revisions) flutter/flutter#142718

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 1, 2024

2d0f24f [two_dimensional_scrollables] Merged cells for TableView (flu…

0b81955

…tter/packages#5917)

engine-flutter-autoroll mentioned this pull request Feb 2, 2024

Roll Packages from 5b48c446976f to 0f677d370a1f (13 revisions) flutter/flutter#142805

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 2, 2024

2d0f24f [two_dimensional_scrollables] Merged cells for TableView (flu…

fae5848

…tter/packages#5917)

engine-flutter-autoroll mentioned this pull request Feb 2, 2024

Roll Packages from 5b48c446976f to d37fb0a716e7 (14 revisions) flutter/flutter#142812

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 2, 2024

2d0f24f [two_dimensional_scrollables] Merged cells for TableView (flu…

f7867b8

…tter/packages#5917)

ahmednfwela mentioned this pull request Feb 18, 2024

[two_dimensional_scrollables] consistency issues when scrolling with merged cells in TableView flutter/flutter#143667

Closed

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

Piinks mentioned this pull request Feb 22, 2024

[two_dimensional_scrollables] Fix layout offset of merged pinned cells #6178

Merged

11 tasks

Gustl22 mentioned this pull request Sep 20, 2024

feat: Table cell spans DavBfr/dart_pdf#1736

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[two_dimensional_scrollables] Merged cells for TableView #5917

[two_dimensional_scrollables] Merged cells for TableView #5917

Piinks commented Jan 18, 2024 •

edited

Loading

goderbauer left a comment

goderbauer commented Jan 31, 2024

goderbauer Jan 31, 2024

Piinks Jan 31, 2024

Piinks Jan 31, 2024

goderbauer Jan 31, 2024

Piinks commented Jan 31, 2024

goderbauer left a comment

goderbauer Jan 31, 2024

Piinks Jan 31, 2024

goderbauer Jan 31, 2024

Piinks Jan 31, 2024

goderbauer Jan 31, 2024

goderbauer Jan 31, 2024

Piinks Jan 31, 2024

ahmednfwela commented Feb 2, 2024

[two_dimensional_scrollables] Merged cells for TableView #5917

[two_dimensional_scrollables] Merged cells for TableView #5917

Conversation

Piinks commented Jan 18, 2024 • edited Loading

Pre-launch Checklist

goderbauer left a comment

Choose a reason for hiding this comment

goderbauer commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks commented Jan 31, 2024

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmednfwela commented Feb 2, 2024

Piinks commented Jan 18, 2024 •

edited

Loading