-
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
Changes from all commits
57f506d
50f7f34
285b570
606ba21
c53dfc8
663ec0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,20 +122,34 @@ internal void UpdateMenuItemSource() | |
| { | ||
| _flyoutGrouping = newGrouping; | ||
| var newItems = IterateItems(newGrouping).ToList(); | ||
| var newItemsSet = new HashSet<object>(newItems); | ||
| var flyoutItemsSet = new HashSet<object>(FlyoutItems); | ||
|
|
||
| foreach (var item in newItems) | ||
| for (int index = 0; index < newItems.Count; index++) | ||
| { | ||
| if (!FlyoutItems.Contains(item)) | ||
| var item = newItems[index]; | ||
|
|
||
| if (!flyoutItemsSet.Contains(item)) | ||
| { | ||
| FlyoutItems.Add(item); | ||
| // Use Insert when within bounds, otherwise Add | ||
| if (index < FlyoutItems.Count) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Store There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated the changes per your suggestion. |
||
| { | ||
| FlyoutItems.Insert(index, item); | ||
| } | ||
| else | ||
| { | ||
| FlyoutItems.Add(item); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (var i = FlyoutItems.Count - 1; i >= 0; i--) | ||
| { | ||
| var item = FlyoutItems[i]; | ||
| if (!newItems.Contains(item)) | ||
| if (!newItemsSet.Contains(item)) | ||
| { | ||
| FlyoutItems.RemoveAt(i); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 23834, "Flyout Item misbehavior", PlatformAffected.UWP)] | ||
| public class Issue23834 : TestShell | ||
| { | ||
| protected override void Init() | ||
| { | ||
| var shellContent = new ShellContent() | ||
| { | ||
| ContentTemplate = new DataTemplate(typeof(Issue23834SamplePage)) | ||
| }; | ||
|
|
||
| this.Items.Add(new FlyoutItem() | ||
| { | ||
| Title = "Flyout Item 1", | ||
| IsVisible = false, | ||
| Items = | ||
| { | ||
| shellContent | ||
| } | ||
| }); | ||
|
|
||
| this.Items.Add(new FlyoutItem() | ||
| { | ||
| Title = "Flyout Item 2", | ||
| Items = | ||
| { | ||
| shellContent | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| public class Issue23834SamplePage : ContentPage | ||
| { | ||
| public Issue23834SamplePage() | ||
| { | ||
| var layout = new StackLayout(); | ||
| var changeButton = new Button | ||
| { | ||
| Text = "Change First Item's IsVisible", | ||
| AutomationId = "button" | ||
| }; | ||
|
|
||
| changeButton.Clicked += (s, e) => | ||
| { | ||
| Shell.Current.Items[0].IsVisible = true; | ||
| }; | ||
|
|
||
| layout.Children.Add(changeButton); | ||
|
|
||
| Content = layout; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
| public class Issue23834 : _IssuesUITest | ||
| { | ||
| public Issue23834(TestDevice device) : base(device) { } | ||
|
|
||
| public override string Issue => "Flyout Item misbehavior"; | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.FlyoutPage)] | ||
| public void Issue23834FlyoutMisbehavior() | ||
| { | ||
| App.WaitForElement("button"); | ||
| App.Tap("button"); | ||
| App.ShowFlyout(); | ||
| VerifyScreenshot(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I have committed the 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.
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