Fix menu item command binding (#13848)

[ooikengsiang]

Description of Change

Fix menu item command binding is removed after navigation. The reference to parent should not be remove when navigating between pages. Page level already handle removal of parent on menu item if the menu item is removed / no longer needed.

Issues Fixed

Fixes #13848

[ghost]

Hey there @ooikengsiang! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[jfversluis]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[PureWeen]

Looks like we might need to adjust this test?

Error message
Assert.Null() Failure
Expected: (null)
Actual:   ContentPage { AnchorX = 0.5, AnchorY = 0.5, AutomationId = null, Background = ImmutableBrush { AutomationId = null, BindingContext = { Name = Matthew }, ClassId = null, Color = null, Dispatcher = DispatcherStub { ... }, ... }, BackgroundColor = null, ... }
Stack trace
at Microsoft.Maui.Controls.Core.UnitTests.Menu.MenuTestBase`4.ClearUpdatesParent() in /Users/builder/azdo/_work/1/s/src/Controls/tests/Core.UnitTests/Menu/MenuTestBase.cs:line 111
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

[ooikengsiang]

Looks like we might need to adjust this test?

Error message
Assert.Null() Failure
Expected: (null)
Actual:   ContentPage { AnchorX = 0.5, AnchorY = 0.5, AutomationId = null, Background = ImmutableBrush { AutomationId = null, BindingContext = { Name = Matthew }, ClassId = null, Color = null, Dispatcher = DispatcherStub { ... }, ... }, BackgroundColor = null, ... }
Stack trace
at Microsoft.Maui.Controls.Core.UnitTests.Menu.MenuTestBase`4.ClearUpdatesParent() in /Users/builder/azdo/_work/1/s/src/Controls/tests/Core.UnitTests/Menu/MenuTestBase.cs:line 111
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

New commit should fix the unit test, but it also mean calling clear directly on menuitems won't have their parent set to null. This is because on collection changed event doesn't provide removed item in olditems for clear event.

[PureWeen]

I think this one needs to be updated to user the newer AddLogical and RemoveLogical APIs

• removing the setting of parent to null still makes sense
• Page should add items as logical children inside collectionchanged
• can we make Clear work? If we're removing the code that sets the parent to null we'll need to fix clear.

[ghost]

Hi @ooikengsiang. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time.

[ooikengsiang]

Sorry, I currently don't have time for this. Since the bot already closed this, I will do it in another branch when I free.
Hey bot, how you count 14 days?