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. |
…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.
From the design concept, DataColumn control simply provides a space for the header content, so it should not interact with keyboard operations by default. If focusable something is actually needed, DataTable users can place any controls like a button there.
… not present 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. Each rows layout their columns independent, because there's no main controller to remember the column widths, and then it looks like a horizontal StackPanel.
|
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. |
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.
|
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. |
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.