-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: Add Justify alignment to WrapPanel #19419
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
base: master
Are you sure you want to change the base?
Conversation
|
I would prefer the word |
Agree |
You can test this PR using the following package version. |
There is a PR for a VirtualizingWrapPanel in Avalonia.Labs. in the best case both controls can share the same spacing enum. I won't argue here which names are better. But we should at least compare them and check naming and functionallity. |
src/Avalonia.Labs.Controls/VirtualizingWrapPanel/SpacingMode.cs |
It looks like you've implemented more precise spacing control. I'll try to implement them if I have time. |
Perhaps we can keep the |
@cla-avalonia agree |
This is another step for |
I have to link to the original discussion when adding alignment to the WrapPanel starting here: #17792 (comment) There are a few things we could talk about based on that discussion but right of the bat: Why not use a |
You can test this PR using the following package version. |
Public API for review: namespace Avalonia.Controls
{
public enum WrapPanelItemsAlignment
{
Start,
Center,
End,
+ Justify
}
} |
You can test this PR using the following package version. |
You can test this PR using the following package version. |
You can test this PR using the following package version. |
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.
Thank you! The feature has a major issue where the justification isn't currently computed per line (see the screenshot below).
bool itemWidthSet = !double.IsNaN(itemWidth); | ||
bool itemHeightSet = !double.IsNaN(itemHeight); | ||
|
||
double GetChildU(Control child) => (isHorizontal ? itemWidthSet : itemHeightSet) |
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.
Consider extracting isHorizontal
as the main condition for readability, to avoid repeating it three times (and saving a branch).
e.g.
double GetChildU(Control child) => isHorizontal
? (itemWidthSet ? itemWidth : child.DesiredSize.Width)
: (itemHeightSet ? itemHeight : child.DesiredSize.Height);
? (isHorizontal ? itemWidth : itemHeight) | ||
: (isHorizontal ? child.DesiredSize.Width : child.DesiredSize.Height); | ||
|
||
double GetChildV(Control child) => (isHorizontal ? itemHeightSet : itemWidthSet) |
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.
Same thing as above.
{ | ||
var layoutSlotU = GetChildU(children[i]); | ||
children[i].Arrange(isHorizontal ? new Rect(u, accumulatedV, layoutSlotU, lineV) : new Rect(accumulatedV, u, lineV, layoutSlotU)); | ||
u += layoutSlotU + spacing; |
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.
This should take the item's visibility into account, like for other alignments.
When that's done, this if
branch can be removed and merged with the otthers below.
{ | ||
var layoutSlotU = GetChildU(children[i]); | ||
children[i].Arrange(isHorizontal ? new Rect(u, accumulatedV, layoutSlotU, lineV) : new Rect(accumulatedV, u, lineV, layoutSlotU)); | ||
u += layoutSlotU + (!children[i].IsVisible ? 0 : itemSpacing); |
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.
spacing
should be used here; I know that's is effectively equal to itemSpacing
at this point, but this will make things clearer (and allow both branches to be merged into one).
/// <summary> | ||
/// Items are laid out with equal spacing between them within each column/row. | ||
/// </summary> | ||
Justify |
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.
It would be nice to adjust the documentation of ItemSpacing
and LineSpacing
to specify that they become minimums in Justify
mode.
} | ||
|
||
var lineBreaks = new List<int>(); | ||
var childrenTotalUByLine = new List<double>(); |
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.
This list has items added to it, but it's never read in any way. Leftover or bug?
} | ||
} | ||
|
||
var lineBreaks = new List<int>(); |
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.
We try to avoid allocations when possible during the Measure
and Arrange
pass. Consider keeping the list in a field once instantiated.
WrapPanelItemsAlignment.Start => 0, | ||
_ => throw new ArgumentOutOfRangeException(nameof(ItemsAlignment), ItemsAlignment, null), | ||
}; | ||
gridSpacing = (uvFinalSize.U - firstRowTotalWidth) / (firstRowChildrenCount - 1); |
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.
This isn't correct: the spacing can't be computed once from the first row. Each line needs to be justified indepentely since items may have different sizes.
What does the pull request do?
This pull request introduces a new
SpaceBetween
Justify
alignment option to theWrapPanel
control. This alignment mode arranges child elements with equal spacing between them within each row, providing a new way to distribute content evenly.What is the current behavior?
The current
WrapPanel
offersStart
,Center
, andEnd
alignments. TheCenter
alignment centers the entire group of children as a single block within each line. This can lead to uneven visual gaps, especially when the items don't fill the entire width of the line.What is the updated/expected behavior with this PR?
With this PR, users can set
ItemsAlignment="Justify"
to achieve a justified layout. In this mode, theWrapPanel
will calculate the remaining horizontal space in each line and distribute it evenly as gaps between the child elements.The expected behavior for
Justify
is:ItemsAlignment="Justify"
: Child items are arranged from the start of the line, with equal spacing injected between each item to fill the remaining space. This creates a visually balanced, "justified" look for each row.To test this, create a
WrapPanel
withItemsAlignment="Justify"
and add several child controls. Observe how the spacing between the children changes as you resize the window.How was the solution implemented (if it's not obvious)?
The solution was implemented by extending the
WrapPanelItemsAlignment
enum with a newJustify
value. The core layout logic was modified within theArrangeLine
local function inArrangeOverride
. A newcase
was added to theItemsAlignment
switch
statement that calculates the spacing required for theJustify
behavior and arranges the children accordingly.Checklist
case
has been added to theLays_Out_With_Items_Alignment
test to verify the behavior ofJustify
.WrapPanelItemsAlignment.Justify
enum value.Breaking changes
No breaking changes. This is an additive change to an existing public API.
Obsoletions / Deprecations
No APIs have been obsoleted or deprecated.
Fixed issues