Repository navigation
refactor: Centralize reuseable bindings and brushes - #448
Merged
Merged
Conversation
w-ahmad
changed the title
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The shared name-based bindings are incompatible with the Native AOT changes explicitly assumed from PR #445.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Centralizes reusable WinUI bindings and brushes to reduce allocations during table virtualization.
Changes:
- Adds thread-local shared bindings and a transparent brush.
- Updates rows, cells, and headers to consume shared resources.
- However, shared name-based bindings conflict with PR #445’s Native AOT approach.
| File | Description |
|---|---|
src/Helpers/SharedResources.cs |
Defines shared thread-local resources. |
src/TableView.cs |
Reuses row font bindings. |
src/TableViewRow.cs |
Reuses cell height bindings. |
src/TableViewRowPresenter.cs |
Reuses row-header bindings and brush. |
src/TableViewHeaderRow.cs |
Reuses header binding and brush. |
src/TableViewColumnHeader.cs |
Reuses the transparent brush. |
src/TableViewCell.cs |
Reuses the transparent brush. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// Gets a binding to <see cref="RowHeight"/>, shared by every cell and row header. | ||
| /// </summary> | ||
| [field: ThreadStatic] | ||
| internal static Binding RowHeightBinding => field ??= new Binding { Path = new("TableView.RowHeight"), RelativeSource = new() { Mode = RelativeSourceMode.Self } }; |
| /// Gets a binding to the column header, shared by every column header. | ||
| /// </summary> | ||
| [field: ThreadStatic] | ||
| internal static Binding HeaderBinding => field ??= new Binding { Path = new(nameof(TableViewColumn.Header)) }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Cells, row headers, rows and column headers were each creating their own identical
BindingandSolidColorBrushobjects. That is a lot of redundant allocation when scrolling or virtualizing large tables.This PR adds an internal
SharedResourceshelper (src/Helpers/SharedResources.cs) that creates each of these objects once and reuses it:TransparentBrush, for hidden grid lines inTableViewCell,TableViewColumnHeader,TableViewHeaderRowandTableViewRowPresenter.FontFamilyBindingandFontSizeBinding, forTableView, where they are set on each row.RowHeightBinding,RowMinHeightBindingandRowMaxHeightBinding, for cells (TableViewRow) and row headers (TableViewRowPresenter).HeaderBinding, for column header content (TableViewHeaderRow).The row-height bindings now point at
TableView.RowHeightthroughRelativeSource.Self, where the old ones went throughTableViewCell.TableVieworTableViewRowHeader.TableView. Both types expose the sameTableViewproperty, so the resolved values are the same.Each shared object is
[ThreadStatic], because XAML objects can't be used from a thread other than the one that created them.There is no intended behavior change.
Related Issue
Closes #
Type of Change
Checklist
mainbranchScreenshots / Recordings
N/A, there are no visual changes.
Additional Notes
GeneratedBindableCustomPropertyfrom library types #445 already removes them.