Fix Flex layout child ordering with correct LINQ stable sort#34368
Fix Flex layout child ordering with correct LINQ stable sort#34368kubaflo wants to merge 1 commit intodotnet:mainfrom
Conversation
Replace manual insertion sort with Enumerable.Range().OrderBy(), which correctly sorts original child indices by their Order property using a stable sort. The previous PR attempt (item.OrderBy().Select((v,i)=>i)) was broken — it always produced [0,1,2,...] because Select's index parameter is the position in the sorted sequence, not the original index. Also add comprehensive tests covering reverse ordering, stable sort preservation for equal Order values, negative Order values, and row direction ordering. Fixes the correctness bug in PR dotnet#21961. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34368Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34368" |
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness bug introduced by PR #21961 in the Flex layout child ordering logic. PR #21961 replaced a manual insertion sort with LINQ but introduced a bug where .Select((value, index) => index) after OrderBy always produced an identity mapping [0, 1, 2, ...], effectively making the sort a no-op. The fix correctly uses Enumerable.Range(0, item.Count).OrderBy(i => item[i].Order).ToArray() to sort original child indices by their Order property, leveraging LINQ's guaranteed stable sort.
Changes:
- Replaced the broken LINQ sort (and the original manual insertion sort) in
Flex.cswith a correct, readable LINQ-based stable sort of child indices byOrdervalues - Added 4 new unit tests covering reverse ordering, stable sort preservation, negative order values, and row direction ordering
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Core/src/Layouts/Flex.cs |
Added using System.Linq and replaced the child ordering logic with a correct LINQ Enumerable.Range(...).OrderBy(...).ToArray() expression |
src/Controls/tests/Core.UnitTests/FlexOrderTests.cs |
Added 4 new test methods for flex order scenarios: reverse ordering, stable sort, negative values, and row direction |
| [Fact] | ||
| public void TestReverseOrderingElements() | ||
| { | ||
| // Children inserted in order 0..3, but Order values are reversed (3, 2, 1, 0) | ||
| // so layout should place label0 last and label3 first | ||
| var label0 = MockPlatformSizeService.Sub<Label>(); | ||
| var label1 = MockPlatformSizeService.Sub<Label>(); | ||
| var label2 = MockPlatformSizeService.Sub<Label>(); | ||
| var label3 = MockPlatformSizeService.Sub<Label>(); | ||
|
|
||
| FlexLayout.SetOrder(label0, 3); | ||
| FlexLayout.SetOrder(label1, 2); | ||
| FlexLayout.SetOrder(label2, 1); | ||
| FlexLayout.SetOrder(label3, 0); | ||
|
|
||
| var layout = new FlexLayout | ||
| { | ||
| IsPlatformEnabled = true, | ||
| Direction = FlexDirection.Column, | ||
| Children = { label0, label1, label2, label3 } | ||
| }; | ||
|
|
||
| layout.Layout(new Rect(0, 0, 912, 912)); | ||
|
|
||
| // label3 (Order=0), label2 (Order=1), label1 (Order=2), label0 (Order=3) | ||
| Assert.Equal(new Rect(0, 0, 912, 20), label3.Bounds); | ||
| Assert.Equal(new Rect(0, 20, 912, 20), label2.Bounds); | ||
| Assert.Equal(new Rect(0, 40, 912, 20), label1.Bounds); | ||
| Assert.Equal(new Rect(0, 60, 912, 20), label0.Bounds); | ||
| } |
There was a problem hiding this comment.
TestReverseOrderingElements is functionally identical to the existing TestOrderingElements test. Both assign the exact same Order values to the same labels (label0=3, label1=2, label2=1, label3=0), use the same insertion order, and assert the same expected positions. The only difference is the order in which FlexLayout.SetOrder calls are written, which has no effect on behavior.
Consider replacing this with a test that exercises a genuinely different scenario — for example, a non-monotonic ordering (e.g., Order values [2, 0, 3, 1]) or a scenario where only some children have non-default Order values while others keep the default (0).
Summary
Replaces the manual insertion sort in
Flex.cswithEnumerable.Range(0, item.Count).OrderBy(i => item[i].Order)— a correct, readable, and stable LINQ sort.Problem
PR #21961 attempted this optimization but introduced a correctness bug:
After
OrderBysorts the items,.Select((value, index) => index)returns the position in the sorted sequence (always0, 1, 2, …), not the original child indices. This makesordered_indicesa no-op identity mapping.Fix
This creates indices
[0, 1, 2, …]then reorders them by theOrderproperty of the corresponding child. LINQ'sOrderByis guaranteed stable in .NET.Tests Added
4 new tests in
FlexOrderTests.cs:All 5 FlexOrder tests pass ✅
Performance
This change provides the same algorithmic improvement as #21961 (O(n log n) vs O(n²) insertion sort) while being correct. The allocation profile is similar — one
int[]array viaToArray().Relates to #21961
/cc @symbiogenesis @mattleibow @jonathanpeppers