-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Android] Removing outdated menu items as Android can delete them after switching activities #28767
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
[Android] Removing outdated menu items as Android can delete them after switching activities #28767
Conversation
…can delete them after switching activities
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.
Could we include a test? Can move App to background etc using Appium using App.BackgroundApp()
.
var clonedPreviousMenuItems = previousMenuItems.ToArray(); | ||
foreach (var previousMenuItem in clonedPreviousMenuItems) | ||
{ | ||
var item = menu.FindItem(previousMenuItem.ItemId); |
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.
Can directly: if (menu.FindItem(previousMenuItem.ItemId) == null)
// menu items can be deleted by Android after switching activities, removing outdated menu items first | ||
if (menu != null) | ||
{ | ||
var clonedPreviousMenuItems = previousMenuItems.ToArray(); |
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.
Can reduce the memory allocation and copying overhead of ToArray()
iterating backwards through the collection.
for (int i = previousMenuItems.Count - 1; i >= 0; i--)
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.
Hi @jsuarezruiz, thank you for the suggestions, updated PR.
I have never added tests like this and creating test environment will take forever, not sure I can do that.
/azp run MAUI-UITests-public |
Azure Pipelines successfully started running 1 pipeline(s). |
if (menu.FindItem(previousMenuItem.ItemId) == null) | ||
{ | ||
previousMenuItem.Dispose(); | ||
previousMenuItems.Remove(previousMenuItem); |
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.
previousMenuItems.RemoveAt(j);
Use RemoveAt(j)
instead of Remove(previousMenuItem)
. Remove() which has to search the collection for the item while RemoveAt(j) directly removes at the index we already know, eliminating the search operation.
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.
Thanks, done
/azp run MAUI-UITests-public |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Could we include a test here? Can create an UITest based on the issue sample and use the methods
public static void BackgroundApp(this IApp app) |
Let me know if can help with something!
@VitalyKnyazev Would you like to include the tests yourself? If you don’t have time, just let me know, I’d be happy to add them for you! |
Hi @jsuarezruiz, I have tried App.BackgroundApp but still getting the toolbar item via App.WaitForElement(AppiumQuery.ByXPath("//android.widget.Button[@content-desc='ToolbarItem1']")). |
Description of Change
When updating toolbar items first removing outdated menu items as Android can sometimes delete them after switching activities (releasing resources under memory pressure?).
Issues Fixed
Fixes #24357