[Windows] Fixed Unstable order of Flyout Items with conditional visibility #29197
[Windows] Fixed Unstable order of Flyout Items with conditional visibility #29197jfversluis merged 6 commits intodotnet:inflight/currentfrom
Conversation
| if (flyoutItemIndex == -1) | ||
| { | ||
| FlyoutItems.Add(item); | ||
| if (index < FlyoutItems.Count) |
There was a problem hiding this comment.
Store FlyoutItems.Count in a variable to minimize repeated property accesses.
There was a problem hiding this comment.
I have updated the changes per your suggestion.
| var item = newItems[index]; | ||
| var flyoutItemIndex = FlyoutItems.IndexOf(item); | ||
|
|
||
| if (flyoutItemIndex == -1) |
There was a problem hiding this comment.
Not big performance differences, but can simplify this using Contains:
if (!FlyoutItems.Contains(item))
{
// Use Insert when within bounds, otherwise Add
if (index < count)
{
FlyoutItems.Insert(index, item);
}
else
{
FlyoutItems.Add(item);
}
}
NOTE: count is a variable with the FlyoutItems.Count value.
There was a problem hiding this comment.
I have updated the changes per your suggestion.
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| App.WaitForElement("button"); | ||
| App.Tap("button"); | ||
| App.ShowFlyout(); | ||
| VerifyScreenshot(); |
There was a problem hiding this comment.
Snapshots already available in the latest build:
For example:
There was a problem hiding this comment.
I have committed the pending snapshots
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@jsuarezruiz friendly ping, is it possible to merge in the next inflight round this pr? It seems ready. |
|
@jsuarezruiz friendly ping, is it possible to merge this pr in the next inflight round? |
| var itemsCount = FlyoutItems.Count; | ||
|
|
||
| foreach (var item in newItems) | ||
| for (int index = 0; index < newItems.Count; index++) |
There was a problem hiding this comment.
Usually cannot expect to have a big collection here, but use the Contains with the two nested for loops have a big performance impact. Ideally we could kept it simple, but it should have the best possible behavior in all situations.
There was a problem hiding this comment.
Let's use a HashSet here.
There was a problem hiding this comment.
@jsuarezruiz I have used HashSet as per your suggestion
| { | ||
| _flyoutGrouping = newGrouping; | ||
| var newItems = IterateItems(newGrouping).ToList(); | ||
| var itemsCount = FlyoutItems.Count; |
There was a problem hiding this comment.
This count becomes stale after Insert operations.
There was a problem hiding this comment.
@jsuarezruiz I have used FlyoutItems Count directly.
| { | ||
| var item = newItems[index]; | ||
|
|
||
| if (!FlyoutItems.Contains(item)) |
There was a problem hiding this comment.
The performance degrades quadratically with collection size, for example, 100 items = 100,000 operations instead of 100.
There was a problem hiding this comment.
@jsuarezruiz I've addressed the performance concern by using HashSet. Kindly let me know if you have any suggestions
|
@jsuarezruiz friendly ping |
|
@jsuarezruiz @PureWeen friendly ping |
|
@PureWeen @jfversluis friendly ping, this should be ready to merge |
|
/rebase |
|
@bronteq did you download the artifacts and verified this fixes your issue? |
66cd5b0 to
663ec0e
Compare
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@jfversluis i followed this https://github.com/dotnet/maui/wiki/Testing-PR-Builds but i do not find nuget folder in the artifacts, probably because MAUI-public was not executed I also tried this https://github.com/dotnet/maui/wiki/Nightly-Builds but i think that nightlies do not contain not merged PRs, this has the wrong behavior |
|
/azp run MAUI-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@bronteq you're totally right, my bad! Triggered that one now, please let me know :) |
|
@jfversluis thank you so much, i can confirm that this PR solves the issue |
Updated concerns Updated review concern Added snapshots and requested changes Modified the sample used HashSet Used FlyoutItems Count directly
Root Cause:
When the visibility of a Flyout item is changed, the UpdateMenuItemSource method simply adds the item to the end, resulting in the original order of the Flyout items being mismatched.
Description of change:
Instead of adding the item, I have retrieved its original position and inserted it back into the Flyout items accordingly, which results the original order being maintained.
Tested the behavior in the following platforms:
Issue Fixed:
Fixes #23834
Issue23834_BeforeFix.mp4
Issue23834_AfterFix.mp4