Fix DataCell overflows with column with multiple text widgets, row height doesn't adjust based on content height #70510 #102886
Conversation
flutter-dashboard
bot
added
f: material design
…70510 # Conflicts: # packages/flutter/test/material/data_table_theme_test.dart
|
@chunhtai please review again. |
|
@chunhtai can you please check again. |
# Conflicts: # packages/flutter/lib/src/material/data_table_theme.dart
|
Any news about this PR? This fix is really needed. |
|
This fix is very essential. Please review it |
|
Unless I'm misunderstanding (definitely a strong possibility), this PR seems like a pretty fundamental change to the DataTable API. Currently, DataTable forces its rows to have a fixed height (cells have a fixed size) and that keeps RenderTable's extravagant layout costs somewhat under control. Although this PR's description focuses on the addition of a vertical padding parameter, the fundamental change here is support for content-sizing rows. For starters, what's needed is a comprehensive description of the proposed API changes and the rationale for those changes. If we're going to deprecate API, that should be clearly stated. If this proposal is going to have significant performance implications, that should be clearly stated and measured. Support for rows whose height matches their content, plus minimum, maximum height constraints, plus vertical padding, is certainly a reasonable addition to the table API. CC @Piinks who has been thinking about how to support general table layout and scrolling at scale. |
There was a problem hiding this comment.
Hey @inouiw thanks for the contribution. I see it is your first - welcome! Have you considered writing a design document? It is helpful to discuss the solution, what the motivations are, as well as various options. We commonly use it to give and receive feedback on designs and to better understand the proposal for review. Our wiki has more information along with our standard template: https://github.com/flutter/flutter/wiki/Design-Documents
| 'This feature was deprecated after v3.1.0-0.0.pre.', | ||
| ) | ||
| double? dataRowHeight, | ||
| DataRowHeightStyle? dataRowHeightStyle, | ||
| this.dataTextStyle, | ||
| this.headingRowColor, | ||
| this.headingRowHeight, |
There was a problem hiding this comment.
Is it reasonable to assume users would want the same for the header row height?
There was a problem hiding this comment.
The auto height for the data row is needed if you have some rows that are higher than other rows for example for multiline text. Then you do not want all rows to be e.g. twice as high but only the rows which need it.
For the header row, even though not as convenient as an auto height, it should be enough to be able to set a fixed height.
There was a problem hiding this comment.
it should be enough to be able to set a fixed height.
How would that logic not apply to the general row height? :)
|
|
||
| /// Creates height settings where the row height adjusts to the content. | ||
| const DataRowHeightStyle.auto({ | ||
| required this.verticalPadding, |
There was a problem hiding this comment.
This seems a bit unnecessary. Could the user not just wrap the cell content in Padding? The framework tries to avoid enumerating padding as a property on widgets, deferring instead to the actual padding widget.
It seems like the padding is what makes the difference here. Is there not a way we can determine the row needs to be bigger without changing the user facing code? Like if the height is not set, and the child is larger than the default height?
Again, this is very expensive because you will need to determine the height of all of the children in the row to determine the ultimate height. I do not know that users will want the trade off in performance in exchange for an auto-size row.
There was a problem hiding this comment.
This seems a bit unnecessary. Could the user not just wrap the cell content in Padding?
Yes. I like the suggestion.
Is there not a way we can determine the row needs to be bigger without changing the user facing code?
The first commit used a different approach than the current code, it changed the meaning of the dataRowHeight parameter to minDataRowHeight and used it in BoxConstraints as constraints: BoxConstraints(minHeight: minDataRowHeight), (see 77c06b0)
The first commit also had an additional parameter topBottomRowPadding that seemed necessary because rows with one line appeared to have padding because the row height was much larger than the content but for rows with multiple lines the BoxConstraints(minHeight: minDataRowHeight) would apply and there was no padding. Without needing to support padding the original solution might be simplified.
So one solution would be to use the BoxConstraints approach again but without the additional topBottomRowPadding parameter. However, then I would suggest to rename dataRowHeight to minDataRowHeight because that is what it would represent. Using BoxConstraints instead of a fixed height would probably be more expensive to render but it can be tested if that really makes a difference.
One note about your concerns for changing user facing code. I suppose, for most users there is no depreciation warning because most users use the default height and do not explicitly set dataRowHeight. The users that have set dataRowHeight will get a depreciation warning. However I suppose most apps do not use a lot of DataTable widgets, so there are not many places that need to be adjusted.
|
@HansMuller Thanks for your feedback. The change will only increase the layout costs if you use the auto size. I will try to measure the performance impact as you suggested. |
|
Here is the performance analysis. I tested on my iPhone 13 in profile mode using the flutter version of this branch. The used test project is at: https://github.com/inouiw/fix70510_perf_test Overall the data seems to suggest that there is no clear performance difference between fixed-size, auto-size and minHeight constraint. Screenshot of the test app
|
|
@Piinks what would be your preferred solution? |
|
Definitely would love to have this feature. Or at the very least, if we could move the row height to the individual row, rather than table level, we could create our own method for setting row height (based on text length, for example). For now, because rows are all set to the same height, I cut text off after a certain length and add '...'; can use the onTap to show full text/more details. A little tricky since characters have different widths, but not many other workarounds available afaik. |
There was a problem hiding this comment.
@inouiw it looks like there are a lot of proposed solutions and options listed among the comments here. It is a bit difficult to understand and evaluate each approach in this format. Can you write a design document so we can better consider your ideas for resolving this issue? We have a guide on our wiki: https://github.com/flutter/flutter/wiki/Design-Documents
|
@Piinks, now I created a design document with a number of proposed solutions and a bit of background information about the problem. The design document is at https://docs.google.com/document/d/1qr1IgaU2z9eaM6zCa4u-Q8kJD5Zq7IcdxVyrgR7CwZc/edit?resourcekey=0-1bNp0ocF4AwGBj6NvRfrEA# the nice go link will probably be |
|
I left some comments on the doc, thanks for writing it! |
|
Hey @inouiw, I just noticed I haven't been receiving notifications on the design doc comments, and this fell off my review queue. Are you still interested in proceeding with this change? I read through the updates in the doc and it looks good! |
|
Hey @Piinks, yes I would like to finish it. I will modify this pull request or create a new one in the next 7 days. |
Great to hear, thank you! I will be happy to take a look, just let me know when. :) |
|
Closed, because I created a new pull request that uses a better solution. See #114338 |
Fixes DataTable and PaginatedDataTable DataCell overflow when the cell content height is larger than dataRowHeight.
There are no breaking changes. Existing code using DataTable will display the same as before. Only code that had the overflow error will now have the row height adjusted to the content.
There is an additional property on DataTable called topBottomRowPadding with a default value of 0.0. It is needed because rows with normal height seem to the user as having top and bottom padding because the content is centered in the container with a minimum height (dataRowHeight). Rows with cells that exceed dataRowHeight seem to the user as not having padding. topBottomRowPadding allows to specify a padding. If dataRowHeight is changed and there are cells that exceed this height then usually topBottomRowPadding also must be adjusted because both impact the space above and below the cell content (padding).
Before
After
List which issues are fixed by this PR. You must list at least one issue.
Fixes #70510
Pre-launch Checklist
///).