Improve DataTable control#781
Conversation
|
@dotnet-policy-service agree |
|
Thanks @9rnsr for this PR, DataTable certainly needs a bit of love, so appreciate you diving in! There's a lot of different pieces changed here (including some formatting and variable renames) which makes it a bit tricky to review. Do you think you could do some of the following to boost this PR/process up a bit for us?
Thanks! |
|
@michael-hawker Thanks for your quick review. To make the review process more speedy, I'll split each commits into separate PR. |
|
@michael-hawker I opened independent smaller PRs #782, #783, and #784. |
|
@michael-hawker I opened two more PRs, #786 and #787. |
|
Thanks @9rnsr! I have a few other items I'm looking at currently, but the small break-outs will make it easier to start reviewing/discussing these bits independently and start merging them in over the next week! Just to confirm, we can close this PR now as it'll be superseded by the others? |
@michael-hawker No problem. |
|
I maintain and improve the core logic update. Also I rebase the commits: five preparation changes from the PRs (still opened), two core update commits. |
|
I fixed a small layout logic bug, that I found while dogfooding this improved control. @michael-hawker Do you have any time to review this (or separated PRs)? |
|
Thanks @9rnsr for breaking this up, it certainly helped look at smaller improvements in the other PRs. I did come back to this one to try and better understand the changes. Behavior wise, it seems to fix a few things, though there's a regression with the I think that's one of my concerns here with this large a change is that there seems to be a lot of extra work done to re-measure/arrange in the layout passes. This fixes some issues like the column jitter and keyboard resizing, but is also pushing a lot of extra layout cycles. I wonder if there's a better happy medium (though there is certainly some of that always going to occur with a column resize from the drag event, but outside that is the main concern)? I wonder if we can have some tests that either help validate the minimum number of passes required, but also behavior for some of these scenarios could help convey the changes. Also, maybe a few more comments or at least maybe some more annotations here in the PR around why changes were made and what scenarios they're handling. |
… Grid or DataTable The removed logic would have been useless or more harmful, because of the reasons: 1) DataTable control is designed to represent 'header' of a table, not for containing the list of DataRows. 2) Grid control is often used to lay out other controls rather than constructing a table, so there was possibilities that completely unrelated Grids would have been caught as a parent.
…o the child elements This added behavior just covers a corner case. If a user fails to place corresponding DataTable at correct position, displaying somethings is better than nothing. If there's no main controller to remember the column widths, each DataRow uses the full width of its parent element to provide equal widths to its child elements.
That case doesn't work well for the Width="Auto" column. I'd like to make DataTable control simple rather than leave the pitfall.
For `IsAuto` and `IsStar` columns, additional attribute `IsFixed` is considered.
- `IsAuto && IsFixed`: have a manually resized width.
- `IsAuto && !IsFixed`: have a calculated width that fits to each column content of visualized row.
- `IsStar && IsFixed`: have a manually resized width.
- `IsStar && !IsFixed`: have a calculated width, from the proportion of remained spaces.
If `IsAbsolute == true`, `IsFixed` is also `true` always.
If `IsStar` has zero ratio `0*`, `IsFixed` is specially set to `true` and the column has zero width.
`DataTable` assigns the width space first to the fixed columns, then unfixed ones.
For the IsFixed == true columns, there's nothing to difficult.
For the column that is IsAuto && !IsFixed, the column size is determined with the following steps:
- [DataTable.MeasureOverride]
- The column's `CurerntWidth` is set to best-fit width of its header content.
- For each visualized `DataRow`s, `InvalidateMeasure()` is invoked.
- [DataRow.MeasureOverride]
- Calculates the best-fit width of the column content and increase the column's `CurrentWidth` to get the maximun width.
- Then, call `DataTable.InvalidateMeasure()`.
- [DataTable.MeasureOverride]
- In the last, gets the best-fit width for the column.
- The mutual `InvalidateMeasure()` call between `DataTable` and `DataRow`s are stopped by the layout system when all element sizes are stable.
For the column that is IsStar && !IsFixed, the column size is determined with the following steps:
- [DataTable.MeasureOverride]
- The column's `CurerntWidth` is set to best-fit width of its header content.
- For each visualized `DataRow`s, InvalidateArrange() is invoked.
- [DataRow.ArrangeOverride]
- Remaining space of finalSize.Width is supplied to the star proportion column.
The child elements in non-fixed Start columns should get always double.PositiveInfinity width on its Measure call, because the column.ActualCurrentWidth at the Measure phase may be narrow than the finalRect.Width in Arrange phase.
Oops, it's my fault. When I updated the logic once after starting this PR, I was accidentally added the bug. Now it's fixed. |
|
The core part of my improvement for this Let's try to explain the the layout steps for the calculation width of an "auto column" (means
Indeed the invalidation calls are increased. However I expect the layout system will elide the actual repeated calculation on the elements, with the cache of previous measurement result to Actually, we can see it with From an another perspective, the steps |
2 participants
DataColumn.IsTabStopfalse(an issue in here)DataColumn.Visibilityin the layout logic.And I removed the hybrid-case support (
DataTableHybridSample). As I commented here, it would not work onAutocolumns as expected, so we should use the pair ofDataTableandDataRowalways.