Skip to content

Conversation

@Piinks
Copy link
Contributor

@Piinks Piinks commented Sep 29, 2023


Fixes flutter/flutter#134453
Also related: flutter/flutter#134655

This adds padding to TableSpans with TableSpanPadding. This affords folks a leading and trailing offset to pad rows and columns by.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@Quijx
Copy link

Quijx commented Oct 6, 2023

The paddings for the TableSpans are useful for sure the way they are implemented here, but I think having a normal padding on the TableView that only applies to the outer sides of the table in addition would also be useful. For example in the app I showed in flutter/flutter#134453, I have a global padding defined in a theme extension that needs to be applied to every screen filling scrollable on each page. Or I could imagine a case where someone wants to use MediaQuery.paddingOf(context) to make room for system UI while still seeing the cells scroll below it. Integrating such a padding into the TableSpans would be a bit verbose:

@override
  Widget build(BuildContext context) {
    final padding = MediaQuery.paddingOf(context);
    const columnCount = 100;
    const rowCount = 100;
    return TableView.builder(
      cellBuilder: (context, vicinity) {
        return const Text('foo');
      },
      columnCount: columnCount,
      columnBuilder: (int column) => TableSpan(
        extent: const FixedTableSpanExtent(64),
        padding: TableSpanPadding(
          leading: column == 0 ? padding.left : 0.0,
          trailing: column == columnCount - 1 ? padding.right : 0.0,
        ),
      ),
      rowCount: rowCount,
      rowBuilder: (int row) => TableSpan(
        extent: FixedTableSpanExtent(64),
        padding: TableSpanPadding(
          leading: row == 0 ? padding.top : 0.0,
          trailing: row == rowCount - 1 ? padding.bottom : 0.0,
        ),
      ),
    );
  }

Also the TableSpanExtent should probably clarify in its doc that the colums/rows width/height that it describes does not include the padding.

@Piinks
Copy link
Contributor Author

Piinks commented Oct 10, 2023

a normal padding on the TableView that only applies to the outer sides of the table in addition would also be useful

That can be achieved here by applying the appropriate padding to the first/last columns/rows. Additionally, you can always wrap the TableView with a padding widget. In general, we avoid enumerating padding properties on widgets when we have a Padding widget itself. :)

I could imagine a case where someone wants to use MediaQuery.paddingOf(context) to make room for system UI while still seeing the cells scroll below it

This sounds like a case for SafeArea!

Also the TableSpanExtent should probably clarify in its doc that the columns/rows width/height that it describes does not include the padding.

Good point! Thanks for the feedback!

@Quijx
Copy link

Quijx commented Oct 10, 2023

That can be achieved here by applying the appropriate padding to the first/last columns/rows.

Sure. This wouldn't make anything possible that wasn't possible before. It would just be a convenience feature and would make it consistent with other scrollable widgets such as ListView or GridView which also have a padding.

Additionally, you can always wrap the TableView with a padding widget.
This sounds like a case for SafeArea!

But wouldn't both a SafeArea or a wrapping Padding widget behave different from a build in padding the same way it is different in for example a ListView?

Padding widget around ListView:

screen-20231010-211917.2.mp4

padding attribute on ListView:

screen-20231010-212041.2.mp4

@Piinks
Copy link
Contributor Author

Piinks commented Oct 10, 2023

Sure! I think we are talking about different things, I may have misunderstood. :)

@Piinks
Copy link
Contributor Author

Piinks commented Oct 18, 2023

FYI @goderbauer, not ready for review yet.

// TODO(Piinks): Rewrite this to remove golden files from this repo when
// mock_canvas is public - https://github.com/flutter/flutter/pull/131631
// foreground, background, and precedence per mainAxis
// * foreground, background, and precedence per mainAxis
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize I am packing a lot into this single golden file test, but I don't want to add more images to the repo when very soon I will be able to rewrite this into tests that use the mock canvas API when it reaches stable.

@Piinks Piinks changed the title [WIP][two_dimensional_scrollables] Add TableSpanPadding [two_dimensional_scrollables] Add TableSpanPadding Oct 18, 2023
@Piinks Piinks marked this pull request as ready for review October 18, 2023 21:09
@Piinks Piinks requested a review from goderbauer October 18, 2023 21:09
// extend beyond the viewport, we would never see them as they would never
// scroll into view. So this currently implementation is fairly assuming
// we will never have rows/cols that are outside of the viewport. We should
// maybe add an assertion for this during layout.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertion sounds dangerous. For me as a developer on my MegaPhone everything my look fine, but when my users run it on their TinyPhone things will be broken for them - and I never saw the assertion during development to warn me about this.

To really fix this, we probably need to do more than just an assertion. What do other implementations do if there isn't enough screen real estate to display all frozen rows/cols? Spreadsheet apps come to mind that we could check for their behavior (both on web and in their native app).

(This is of course not required to be resolved for this PR, having a TODO for this use case seems fine for now.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this offline and agreed we will pursue a non fatal warning here in the same way we do the render overflow error.

/// The [extent] argument must be provided.
const TableSpan({
required this.extent,
TableSpanPadding? padding,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if this could/should be

Suggested change
TableSpanPadding? padding,
this.padding = const TableSpanPadding(),

and then remove the padding initializer below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it this way originally, then when writing the sample code, I found it unfriendly. For example:

// Non-nullable
TableSpan(
  padding: someCondition ? TableSpanPadding(leading: 20) : TableSpanPadding(),
  // ...
)

versus

// Nullable, resolves to non nullable in field member assignment
TableSpan(
  padding: someCondition ? TableSpanPadding(leading: 20) : null,
  // ...
)

/// },
/// );
/// ```
/// {@end-tool}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I know we cannot do this here (yet) but I really wish this would be an interactive example so I can play with enabling/disabling consumeSpanPadding and see what it does.)

required TableVicinity start,
required TableVicinity end,
required Offset offset,
}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain to me how the padding is taken into account when calculating the start and end (input to this method) cells?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_Span.trailingOffset is used to calculate the start and end, which includes the padding. There are even tests for this case. Awesome!

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 26, 2023
@auto-submit auto-submit bot merged commit 2af6954 into flutter:main Oct 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 27, 2023
flutter/packages@fea24c5...2af6954

2023-10-26 katelovett@google.com [two_dimensional_scrollables] Add TableSpanPadding (flutter/packages#5039)
2023-10-26 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 6c4930c to d211f42 (1 revision) (flutter/packages#5238)
2023-10-26 chrislangham@rightnow.org [url_launcher] migrating objc plugin to swift (flutter/packages#4753)
2023-10-26 ybz975218925@live.com [go_router_builder]Avoid losing NullabilitySuffix for typeArguments (flutter/packages#5215)
2023-10-26 engine-flutter-autoroll@skia.org Roll Flutter from 5dd2a4e to c555599 (14 revisions) (flutter/packages#5237)

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 flutter-ecosystem@google.com,rmistry@google.com 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
@reidbaker reidbaker mentioned this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: two_dimensional_scrollables Issues pertaining to the two_dimensional_scrollables package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[two_dimensional_scrollables] - TableView is missing padding attribute

3 participants