Clean up TableBorder#paint API #12226
Conversation
b51261c
to
658314b
Compare
|
We still paint the borders for the columns from left-to-right in RTL, but I think that's ok. |
We paint them as a single path, all at once. |
| @@ -1008,7 +1008,7 @@ class RenderTable extends RenderBox { | |||
| positions[columns - 1] = 0.0; | |||
| for (int x = columns - 2; x >= 0; x -= 1) | |||
| positions[x] = positions[x+1] + widths[x+1]; | |||
| _columnLefts = positions.reversed.toList(); | |||
| _columnLefts = positions.reversed; | |||
There was a problem hiding this comment.
I guess this is fine in Dart, but in C++ this would be crazy...
|
The path internally has an order. I don't think its a big deal. |
| /// columns, relative to the given rectangle. For example, if the table | ||
| /// contained two columns of height 100.0 each, then `columns` would contain a | ||
| /// single value, 100.0, which is the vertical position between the two | ||
| /// columns. |
There was a problem hiding this comment.
this should talk about the origin
| assert(columns.isNotEmpty); | ||
| assert(columns.first == 0.0); | ||
| assert(columns.last < rect.width); | ||
| assert(columns.isEmpty || (columns.first > 0.0 && columns.last < rect.width)); |
There was a problem hiding this comment.
it's debug mode so it probably doesn't matter but note that these asserts are now more expensive (they apply the range and skips multiple times now)
| break; | ||
| case BorderStyle.none: | ||
| break; | ||
| if (columns.isNotEmpty) { |
There was a problem hiding this comment.
similarly here, checking isNotEmpty means you walk two steps into the original list (first looking at the last entry, then the second to last entry, via multiple objects, the original list, the reversed iterator, and the skip iterator) before you can determine if it's empty.
There was a problem hiding this comment.
I can remove this check. We'll just end up drawing an empty path (which is what we used to do anyway).
There was a problem hiding this comment.
I think it's fine with the check, I just wanted to make sure the implications of using an Iterable (vs reifying the list) were clear.
There was a problem hiding this comment.
I'm going to land it without the checks. It's conceptually a separate change (a performance optimization rather than an API cleanup), and likely a premature optimization at that.
| break; | ||
| case BorderStyle.none: | ||
| break; | ||
| if (rows.isNotEmpty) { |
There was a problem hiding this comment.
and here you run the logic of the getRange iterator to check if the _rowTops list is empty, then you run it again for the for loop below.
|
|
Even if the border widths are wide enough that the inside borders overlap each other, and the color is semi-transparent, won't it all be painted as one solid shape? Or in that particular edge case will the different subsegments of the path be painted in order and composited against each other? |
Yes, I think you're right. The path painting shouldn't depend on the order the path in specified (even for self-intersections). |
Previously, the rows and columns arguments had different semantics. Now they have the same semantics. The new API also uses Iterable rather than List to give clients more flexiblity in how they construct these arguments. For example, RenderTable no longer needs to reify the reversed list of column positions.
658314b
to
2f58723
Compare
Previously, the rows and columns arguments had different semantics. Now
they have the same semantics. The new API also uses Iterable rather than
List to give clients more flexiblity in how they construct these
arguments. For example, RenderTable no longer needs to reify the
reversed list of column positions.