-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: DockPanel measure fix #618
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contributions @Poker-sang! These are very helpful improvements.
I've added a unit test for the empty DockPanel + LastChildFill problem that was fixed here.
I've also carefully reviewed the other changes and clarified the missing rational behind each change:
What was changed or removed, and what is the rationale for each change?
- The custom GetPositiveOrZero() helper method was removed and replaced with direct calls to Math.Max(0, value)
- Clarification: Is parameter passing consistent in the replaced version?
- Rationale: Yes, the parameter passing is consistent in all replacements. However, there's an important note: the original implementation used
Math.Max(value, 0)
inside the helper method, while the direct replacements useMath.Max(0, value)
. This is a reversal of parameter order but functionally equivalent, and the replacement is applied consistently throughout the code.
- Changed from four separate boolean flags (leftSpacing, rightSpacing, topSpacing, bottomSpacing) to just two (horizontalSpacing, verticalSpacing)
- Left and Right docking switch cases were combined, Top and Bottom docking switch cases were combined.
- Originally suggested in the Avalonia PR [DockPanel] Add Spacing properties AvaloniaUI/Avalonia#17977 (comment)
- Spacing is now added based on visibility (
child.Visibility is Visibility.Visible
) rather than constraint dimensions (childConstraint.Width is not 0
)- Clarification: Why Visibility based instead of Width-based in WinUI XAML? Doesn't setting Visibility to Collapsed also set Width to 0?
- Rationale: In WinUI XAML, setting Visibility to Collapsed doesn't literally set the Width to 0. Instead, it takes the element completely out of the layout flow. The difference is important because:
- An element with Width=0 but Visibility=Visible still participates in layout calculations, can receive events, and may have margins or other properties that affect layout.
- An element with Visibility=Collapsed is excluded from layout entirely regardless of its Width.
- The final parentWidth and parentHeight calculations no longer use
Math.Min(availableSize, Math.Max(...))
and usesMath.Max(...)
directly.- Clarification: Why might this change have been made?
- Investigation: According to docs for FrameworkElement.MeasureOverride(Size), "During this process, child objects might return a larger DesiredSize size than the initial availableSize to indicate that the child object wants more space."
- Rationale: In WinUI's layout system, the
MeasureOverride
method should return the panel's natural desired size without constraining to the available size. The original code withMath.Min(availableSize.Width, ...)
was artificially limiting the reported size to the available space. This change allows the panel to properly report its true content size, which is important for correct behavior in scrolling scenarios and for parent layouts to make proper sizing decisions.
- Padding.Left + Padding.Right are no longer used as initial values for accumulatedWidth, but are still used in the initial currentBounds calculation in the ArrangeOverride pass.
- Padding.Top + Padding.Bottom are no longer used as initial values for accumulatedHeight, but are still used in the initial currentBounds calculation in the ArrangeOverride pass.
- Whether or not to have padding was discussed in the linked Avalonia thread [DockPanel] Add Spacing properties AvaloniaUI/Avalonia#17977.
- It appears Padding was initially removed then considered for adding back due to feedback, but ended up being out of scope for the PR.
- It's unclear whether this change is intentional. This may be an artifact leftover from prior changes.
DesiredSize
= Content Size + Padding + Border, so presumably the Measure pass would include the Padding in the returned value.- If the child automatically sizes itself, adding padding does push its external footprint larger in the parent’s layout (because it increases the child’s DesiredSize).
- If, however, the child has a fixed width/height, the external size is capped, so the new padding simply shrinks the available space for its content instead.
- Clarification: Is DockPanel expected to handle items that take up available space, or are all items expected to be a fixed size?
- A "Stretch" child doesn't appear to stretch when added. This also appears to be a bug with the sample. Upon fixing it, a stretch child adds properly.
- However, removing the Stretch child results in a visual bug in other docked panels that have been added to the visual tree. This should be filed in a separate ticket if it isn't already.
- Since this behavior reproduces on
main
, for now we'll assume that DockPanel isn't fully equipped to handle items that take up available space.
- After testing both versions (new and old) locally to see the effects of different Padding values around the control and its items, there are no behavioral differences.
- Given all of the above, we'll keep and merge this change.
The above change evaluation should provide sufficient clarification of rationale to move forward with confidence. PR approved 🚀
Fixes
Optimized the logic of the DockPanel based on suggestions from the avalonia team, which broadly include:
see original AvaloniaUI/Avalonia#17977
PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Other information