Skip to content

[two_dimensional_scrollables] Add TableSpanPadding #5039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 26, 2023
Merged

[two_dimensional_scrollables] Add TableSpanPadding #5039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d357abd
Add TableSpanPadding
Piinks Sep 29, 2023
c62a209
++
Piinks Oct 10, 2023
0c09049
More edge cases
Piinks Oct 13, 2023
d8040f2
++
Piinks Oct 18, 2023
2245e8c
Capture span decoration consuming padding or not
Piinks Oct 18, 2023
dfbda1c
Nits
Piinks Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/two_dimensional_scrollables/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.0.4

* Adds TableSpanPadding, TableSpan.padding, and TableSpanDecoration.consumeSpanPadding.

## 0.0.3

* Fixes paint issue when axes are reversed and TableView has pinned rows and columns.
Expand Down
114 changes: 83 additions & 31 deletions packages/two_dimensional_scrollables/lib/src/table_view/table.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,13 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
);
}

// TODO(Piinks): Pinned rows/cols do not account for what is visible on the
// screen. Ostensibly, we would not want to have pinned rows/columns that
// 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

@goderbauer goderbauer Oct 24, 2023

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

@Piinks Piinks Oct 26, 2023

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.

// https://github.com/flutter/flutter/issues/136833
int? get _lastPinnedRow =>
delegate.pinnedRowCount > 0 ? delegate.pinnedRowCount - 1 : null;
int? get _lastPinnedColumn =>
Expand Down Expand Up @@ -667,12 +674,17 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
}) {
Copy link
Member

@goderbauer goderbauer Oct 24, 2023

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

@goderbauer goderbauer Oct 24, 2023

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!

// TODO(Piinks): Assert here or somewhere else merged cells cannot span
// pinned and unpinned cells (for merged cell follow-up), https://github.com/flutter/flutter/issues/131224
_Span colSpan, rowSpan;
double yPaintOffset = -offset.dy;
for (int row = start.row; row <= end.row; row += 1) {
double xPaintOffset = -offset.dx;
final double rowHeight = _rowMetrics[row]!.extent;
rowSpan = _rowMetrics[row]!;
final double rowHeight = rowSpan.extent;
yPaintOffset += rowSpan.configuration.padding.leading;
for (int column = start.column; column <= end.column; column += 1) {
final double columnWidth = _columnMetrics[column]!.extent;
colSpan = _columnMetrics[column]!;
final double columnWidth = colSpan.extent;
xPaintOffset += colSpan.configuration.padding.leading;

final TableVicinity vicinity = TableVicinity(column: column, row: row);
// TODO(Piinks): Add back merged cells, https://github.com/flutter/flutter/issues/131224
Expand All @@ -689,9 +701,11 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
cell.layout(cellConstraints);
cellParentData.layoutOffset = Offset(xPaintOffset, yPaintOffset);
}
xPaintOffset += columnWidth;
xPaintOffset += columnWidth +
_columnMetrics[column]!.configuration.padding.trailing;
}
yPaintOffset += rowHeight;
yPaintOffset +=
rowHeight + _rowMetrics[row]!.configuration.padding.trailing;
}
}

Expand Down Expand Up @@ -836,29 +850,45 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
final LinkedHashMap<Rect, TableSpanDecoration> backgroundColumns =
LinkedHashMap<Rect, TableSpanDecoration>();

final TableSpan rowSpan = _rowMetrics[leading.row]!.configuration;
for (int column = leading.column; column <= trailing.column; column++) {
final _Span span = _columnMetrics[column]!;
if (span.configuration.backgroundDecoration != null ||
span.configuration.foregroundDecoration != null) {
final TableSpan columnSpan = _columnMetrics[column]!.configuration;
if (columnSpan.backgroundDecoration != null ||
columnSpan.foregroundDecoration != null) {
final RenderBox leadingCell = getChildFor(
TableVicinity(column: column, row: leading.row),
)!;
final RenderBox trailingCell = getChildFor(
TableVicinity(column: column, row: trailing.row),
)!;

final Rect rect = Rect.fromPoints(
parentDataOf(leadingCell).paintOffset! + offset,
parentDataOf(trailingCell).paintOffset! +
Offset(trailingCell.size.width, trailingCell.size.height) +
offset,
);
Rect getColumnRect(bool consumePadding) {
return Rect.fromPoints(
parentDataOf(leadingCell).paintOffset! +
offset -
Offset(
consumePadding ? columnSpan.padding.leading : 0.0,
rowSpan.padding.leading,
),
parentDataOf(trailingCell).paintOffset! +
offset +
Offset(trailingCell.size.width, trailingCell.size.height) +
Offset(
consumePadding ? columnSpan.padding.trailing : 0.0,
rowSpan.padding.trailing,
),
);
}

if (span.configuration.backgroundDecoration != null) {
backgroundColumns[rect] = span.configuration.backgroundDecoration!;
if (columnSpan.backgroundDecoration != null) {
final Rect rect = getColumnRect(
columnSpan.backgroundDecoration!.consumeSpanPadding);
backgroundColumns[rect] = columnSpan.backgroundDecoration!;
}
if (span.configuration.foregroundDecoration != null) {
foregroundColumns[rect] = span.configuration.foregroundDecoration!;
if (columnSpan.foregroundDecoration != null) {
final Rect rect = getColumnRect(
columnSpan.foregroundDecoration!.consumeSpanPadding);
foregroundColumns[rect] = columnSpan.foregroundDecoration!;
}
}
}
Expand All @@ -869,28 +899,45 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
final LinkedHashMap<Rect, TableSpanDecoration> backgroundRows =
LinkedHashMap<Rect, TableSpanDecoration>();

final TableSpan columnSpan = _columnMetrics[leading.column]!.configuration;
for (int row = leading.row; row <= trailing.row; row++) {
final _Span span = _rowMetrics[row]!;
if (span.configuration.backgroundDecoration != null ||
span.configuration.foregroundDecoration != null) {
final TableSpan rowSpan = _rowMetrics[row]!.configuration;
if (rowSpan.backgroundDecoration != null ||
rowSpan.foregroundDecoration != null) {
final RenderBox leadingCell = getChildFor(
TableVicinity(column: leading.column, row: row),
)!;
final RenderBox trailingCell = getChildFor(
TableVicinity(column: trailing.column, row: row),
)!;

final Rect rect = Rect.fromPoints(
parentDataOf(leadingCell).paintOffset! + offset,
parentDataOf(trailingCell).paintOffset! +
Offset(trailingCell.size.width, trailingCell.size.height) +
offset,
);
if (span.configuration.backgroundDecoration != null) {
backgroundRows[rect] = span.configuration.backgroundDecoration!;
Rect getRowRect(bool consumePadding) {
return Rect.fromPoints(
parentDataOf(leadingCell).paintOffset! +
offset -
Offset(
columnSpan.padding.leading,
consumePadding ? rowSpan.padding.leading : 0.0,
),
parentDataOf(trailingCell).paintOffset! +
offset +
Offset(trailingCell.size.width, trailingCell.size.height) +
Offset(
columnSpan.padding.leading,
consumePadding ? rowSpan.padding.trailing : 0.0,
),
);
}

if (rowSpan.backgroundDecoration != null) {
final Rect rect =
getRowRect(rowSpan.backgroundDecoration!.consumeSpanPadding);
backgroundRows[rect] = rowSpan.backgroundDecoration!;
}
if (span.configuration.foregroundDecoration != null) {
foregroundRows[rect] = span.configuration.foregroundDecoration!;
if (rowSpan.foregroundDecoration != null) {
final Rect rect =
getRowRect(rowSpan.foregroundDecoration!.consumeSpanPadding);
foregroundRows[rect] = rowSpan.foregroundDecoration!;
}
}
}
Expand Down Expand Up @@ -1028,7 +1075,12 @@ class _Span
bool get isPinned => _isPinned;
late bool _isPinned;

double get trailingOffset => leadingOffset + extent;
double get trailingOffset {
return leadingOffset +
extent +
configuration.padding.leading +
configuration.padding.trailing;
}

// ---- Span Management ----

Expand Down
95 changes: 93 additions & 2 deletions packages/two_dimensional_scrollables/lib/src/table_view/table_span.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,42 @@ import 'package:flutter/widgets.dart';

import 'table.dart';

/// Defines the leading and trailing padding values of a [TableSpan].
class TableSpanPadding {
/// Creates a padding configuration for a [TableSpan].
const TableSpanPadding({
this.leading = 0.0,
this.trailing = 0.0,
});

/// Creates padding where both the [leading] and [trailing] are `value`.
const TableSpanPadding.all(double value)
: leading = value,
trailing = value;

/// The leading amount of pixels to pad a [TableSpan] by.
///
/// If the [TableSpan] is a row and the vertical [Axis] is not reversed, this
/// offset will be applied above the row. If the vertical [Axis] is reversed,
/// this will be applied below the row.
///
/// If the [TableSpan] is a column and the horizontal [Axis] is not reversed,
/// this offset will be applied to the left the column. If the horizontal
/// [Axis] is reversed, this will be applied to the right of the column.
final double leading;

/// The trailing amount of pixels to pad a [TableSpan] by.
///
/// If the [TableSpan] is a row and the vertical [Axis] is not reversed, this
/// offset will be applied below the row. If the vertical [Axis] is reversed,
/// this will be applied above the row.
///
/// If the [TableSpan] is a column and the horizontal [Axis] is not reversed,
/// this offset will be applied to the right the column. If the horizontal
/// [Axis] is reversed, this will be applied to the left of the column.
final double trailing;
}

/// Defines the extent, visual appearance, and gesture handling of a row or
/// column in a [TableView].
///
Expand All @@ -20,20 +56,26 @@ class TableSpan {
/// The [extent] argument must be provided.
const TableSpan({
required this.extent,
TableSpanPadding? padding,
Copy link
Member

@goderbauer goderbauer Oct 24, 2023

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

@Piinks Piinks Oct 26, 2023

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,
  // ...
)

this.recognizerFactories = const <Type, GestureRecognizerFactory>{},
this.onEnter,
this.onExit,
this.cursor = MouseCursor.defer,
this.backgroundDecoration,
this.foregroundDecoration,
});
}) : padding = padding ?? const TableSpanPadding();

/// Defines the extent of the span.
///
/// If the span represents a row, this is the height of the row. If it
/// represents a column, this is the width of the column.
final TableSpanExtent extent;

/// Defines the leading and or trailing extent to pad the row or column by.
///
/// Defaults to no padding.
final TableSpanPadding padding;

/// Factory for creating [GestureRecognizer]s that want to compete for
/// gestures within the [extent] of the span.
///
Expand Down Expand Up @@ -251,14 +293,63 @@ class MinTableSpanExtent extends CombiningTableSpanExtent {
/// A decoration for a [TableSpan].
class TableSpanDecoration {
/// Creates a [TableSpanDecoration].
const TableSpanDecoration({this.border, this.color});
const TableSpanDecoration({
this.border,
this.color,
this.consumeSpanPadding = true,
});

/// The border drawn around the span.
final TableSpanBorder? border;

/// The color to fill the bounds of the span with.
final Color? color;

/// Whether or not the decoration should extend to fill the space created by
/// the [TableSpanPadding].
///
/// Defaults to true, meaning if a [TableSpan] is a row, the decoration will
/// apply to the full [TableSpanExtent], including the
/// [TableSpanPadding.leading] and [TableSpanPadding.trailing] for the row.
/// This same row decoration will consume any padding from the column spans so
/// as to decorate the row as one continuous span.
///
/// {@tool snippet}
/// This example illustrates how [consumeSpanPadding] affects
/// [TableSpanDecoration.color]. By default, the color of the decoration
/// consumes the padding, coloring the row fully by including the padding
/// around the row. When [consumeSpanPadding] is false, the padded area of
/// the row is not decorated.
///
/// ```dart
/// TableView.builder(
/// rowCount: 4,
/// columnCount: 4,
/// columnBuilder: (int index) => TableSpan(
/// extent: const FixedTableSpanExtent(150.0),
/// padding: const TableSpanPadding(trailing: 10),
/// ),
/// rowBuilder: (int index) => TableSpan(
/// extent: const FixedTableSpanExtent(150.0),
/// padding: TableSpanPadding(leading: 10, trailing: 10),
/// backgroundDecoration: TableSpanDecoration(
/// color: index.isOdd ? Colors.blue : Colors.green,
/// // The background color will not be applied to the padded area.
/// consumeSpanPadding: false,
/// ),
/// ),
/// cellBuilder: (_, TableVicinity vicinity) {
/// return Container(
/// height: 150,
/// width: 150,
/// child: const Center(child: FlutterLogo()),
/// );
/// },
/// );
/// ```
/// {@end-tool}
Copy link
Member

@goderbauer goderbauer Oct 24, 2023

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.)

final bool consumeSpanPadding;

/// Called to draw the decoration around a span.
///
/// The provided [TableSpanDecorationPaintDetails] describes the bounds and
Expand Down
2 changes: 1 addition & 1 deletion packages/two_dimensional_scrollables/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: two_dimensional_scrollables
description: Widgets that scroll using the two dimensional scrolling foundation.
version: 0.0.3
version: 0.0.4
repository: https://github.com/flutter/packages/tree/main/packages/two_dimensional_scrollables
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+two_dimensional_scrollables%22+

Expand Down
Binary file modified ...nal_scrollables/test/table_view/goldens/tableSpanDecoration.defaultMainAxis.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading