-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Windows] Fixed Unstable order of Flyout Items with conditional visibility #29197
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: main
Are you sure you want to change the base?
Conversation
{ | ||
FlyoutItems.Add(item); | ||
if (index < FlyoutItems.Count) |
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.
Store FlyoutItems.Count
in a variable to minimize repeated property accesses.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending snapshots.
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.
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.
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? |
|
||
foreach (var item in newItems) | ||
for (int index = 0; index < newItems.Count; index++) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a HashSet here.
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.
@jsuarezruiz I have used HashSet as per your suggestion
@@ -122,20 +122,33 @@ internal void UpdateMenuItemSource() | |||
{ | |||
_flyoutGrouping = newGrouping; | |||
var newItems = IterateItems(newGrouping).ToList(); | |||
var itemsCount = FlyoutItems.Count; |
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 count becomes stale after Insert operations.
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.
@jsuarezruiz I have used FlyoutItems Count directly.
{ | ||
var item = newItems[index]; | ||
|
||
if (!FlyoutItems.Contains(item)) |
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.
The performance degrades quadratically with collection size, for example, 100 items = 100,000 operations instead of 100.
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.
@jsuarezruiz I've addressed the performance concern by using HashSet. Kindly let me know if you have any suggestions
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