Table widget refactors#1776
Conversation
|
@DavBfr I think my formatter is different to yours, are you at 80 chars? |
| bottom: side, | ||
| left: side, | ||
| horizontalInside: side, | ||
| verticalInside: side, |
There was a problem hiding this comment.
Added trailing comma to force formatting
|
|
||
| final double? width; | ||
| final double? flex; | ||
| final double width; |
There was a problem hiding this comment.
Removing the null here makes the amount the work later on easier. I couldn't find a case where they would be.
| final columnWidth = columnWidths != null && columnWidths![n] != null | ||
| ? columnWidths![n]! | ||
| : defaultColumnWidth; | ||
| for (final entry in row.children.asMap().entries) { |
There was a problem hiding this comment.
Hopefully this is a bit more legible and no longer has to worry about null as much
Yes I am using the default remanded dart formatting. |
|
I'll update with 80 chars. It'll be the same code. |
grahamsmith
force-pushed
the
table_multi_page
branch
from
7c7b34c to
1a6272e
Compare
|
@DavBfr - sorted - line lengths are 80 so the diff is simpler now Unsure about the CI failures from the last run |
grahamsmith
force-pushed
the
table_multi_page
branch
from
8d37cb9 to
ee9d075
Compare
|
@DavBfr - in my next PR I have extracted the width calculation to its own function, and added it what I hope would be a decent performance enhancement to reuse the calculations if all the column widths are fixed. It would mean we wouldn't have to calculate for every multipage page the width again. |
|
@DavBfr - sorry not relevant to this PR but do not know where else to post as too excited before my changes for a demo pdf with 10,000 items into a table it took 0:00:57.012834 Nothing too major either! |
|
Congrats! |
|
I noticed the checks have passed, do I need to do anything else @DavBfr ? |
|
Just need some time to review. |
|
@grahamsmith this will potentially conflicting with #1736, where some changes to the algorithm were made. Hope we find a good solution to make it perform good also with cell spans enabled (haven't tested the performance yet, though). |
|
I imagine it will be terrible, although a potential useful feature. I have more changes in the background that greatly enhance the performance of the layout as its quite slow as other's have mentioned. Wonder if the spanning could be in a specific set of widgets. In a perfect world would really love to know how many users need cell spanning (clearly you did). |
|
@Gustl22 - do you know when your changes will land? I am not particularly precious about this PR. One of the biggest gains is in MultiPage so I can do a seperate PR for that and hopefully it will not clash. |
No I don't, open source takes time, just wanted to make aware of this conflict. I am also lacking the time to review my stuff in my repos.
At least 14 people created or upvoted an issue regarding that. And I think the most people are working around this by creating nested tables or column and row layouts, but that's not the clean way. I don't mind which one is merged first, I also have to create performance tests. There surely can made also optimizations, especially if no cell span is present. |
|
Thanks! |
This reverts commit c49c37d.
|
@grahamsmith Can you share your performance test? I executed following: test('Table Widget multi-pages with header', () {
pdf.addPage(MultiPage(
maxPages: 10000,
build: (Context context) => <Widget>[
Table(
children: buildTable(
context: context, count: 10000, repeatHeader: true),
border: TableBorder.all(),
tableWidth: TableWidth.max,
),
]));
});Before your changes: With your changes:
So I only got a difference from about 4-10 seconds improvements, but not 46 (?) |
|
I did not keep a copy sorry! From memory I inserted a string with the item count e.g. 'item $index' into a cell on each row of 10000 rows. Performance tests vary per machine setup too so are usually subjective. |
|
Thank you, ya just wanted to make this more objective. I repeated the tests multiple times. |
3 participants
Started looking to see if the Multipage and Table performance can be enhanced with larger datasets.
Thought a small PR to start with would be easier.
Refactored to eliminate nullable doubles and reduce force unboxing.