Skip to content
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

Avoid reconstructing handler when it is already available #5591

Merged
merged 2 commits into from
Mar 26, 2022

Conversation

cshung
Copy link
Member

@cshung cshung commented Mar 25, 2022

Description of Change

The root cause of this bugs is this seemly harmless call to AlertRequestHelper.PageIsInThisContext accidentally disposed the GestureManager through this stack.

...
Microsoft.Maui.Controls.Platform.GestureManager.Dispose
Microsoft.Maui.Controls.View.OnHandlerChangingCore
Microsoft.Maui.Controls.Element.SetHandler
Microsoft.Maui.Controls.Element.set_Handler
Microsoft.Maui.Controls.VisualElement.Microsoft.Maui.IElement.set_Handler
Microsoft.Maui.Platform.ElementExtensions.ToHandler
Microsoft.Maui.Platform.ElementExtensions.ToPlatform
Microsoft.Maui.Handlers.ContentViewHandler.UpdateContent
Microsoft.Maui.Handlers.ContentViewHandler.MapContent
Microsoft.Maui.PropertyMapper<Microsoft.Maui.IContentView,Microsoft.Maui.Handlers.IContentViewHandler>.
Microsoft.Maui.PropertyMapper.UpdatePropertyCore
Microsoft.Maui.PropertyMapper.UpdateProperties
Microsoft.Maui.Handlers.ElementHandler.SetVirtualView
Microsoft.Maui.Handlers.ViewHandler.SetVirtualView
Microsoft.Maui.Handlers.ViewHandler<Microsoft.Maui.IContentView,Microsoft.Maui.Platform.ContentViewGroup>.SetVirtualView
Microsoft.Maui.Handlers.ContentViewHandler.SetVirtualView
Microsoft.Maui.Handlers.ViewHandler<Microsoft.Maui.IContentView,Microsoft.Maui.Platform.ContentViewGroup>.SetVirtualView
Microsoft.Maui.Controls.Element.SetHandler
Microsoft.Maui.Controls.Element.set_Handler
Microsoft.Maui.Controls.VisualElement.Microsoft.Maui.IElement.set_Handler
Microsoft.Maui.Platform.ElementExtensions.ToHandler
Microsoft.Maui.Platform.ElementExtensions.ToPlatform
Microsoft.Maui.Controls.Platform.AlertManager.AlertRequestHelper.PageIsInThisContext
...

When the GestureManager is disposed of, the associated OnTouchListener is unregistered. Later on, it will register the OnTouchListener for a label again. But that label is not the same instance as the one that is currently showing, that is why touching the label for the second time won't lead to the event listener.

My observation is that all this heavy lifting just to check if the page is in the current activity is a lot of waste. We could have just reused the existing Handler instance, that is what inspired the fix.

Issues Fixed

Fixes #5425

@cshung cshung requested a review from PureWeen March 25, 2022 22:56
@Redth Redth added this to the 6.0.300-rc.1 milestone Mar 25, 2022
@mattleibow
Copy link
Member

@PureWeen Is this supposed to happen? Should ToHandler be checking to see if the maui context is the same and returning the same instance?

Or is that designed to always create and this was a bad use of it?

@PureWeen
Copy link
Member

PureWeen commented Mar 25, 2022

@mattleibow @cshung

ToPlatform should be checking this and not creating a new handler already if the IMauiContext has changed.

if (handler?.MauiContext != null && handler.MauiContext != context)

If the IMauiContext has changed then the handler needs to be recreated.

This is the mechanism that makes it work if the Activity gets changed out from underneath you.

@Redth Redth added the do-not-merge Don't merge this PR label Mar 26, 2022
@PureWeen
Copy link
Member

@cshung I'm pretty sure the fix here is to just change the call to this

bool PageIsInThisContext(IView page)
{
	var platformView = page.ToPlatform();

It's passing the wrong MauiContext into ToPlatform there which causes the Page to recreate the handler. That page already has a handler so ToPlatform will just return it if you don't pass in a MauiContext

I'm not too sure why this broke in p14. The MauiContext has been passed into the code since we created AlertManager. I'm guessing the work we did to rewrite these extensions made it so two wrongs no longer were making a right

@cshung
Copy link
Member Author

cshung commented Mar 26, 2022

@cshung I'm pretty sure the fix here is to just change the call to this

I verified that this alternative fix worked just as well, and is much safer.

@PureWeen PureWeen merged commit eee205e into dotnet:main Mar 26, 2022
@cshung cshung deleted the public/avoid-reconstruct-handler branch March 26, 2022 16:11
@cshung cshung removed the do-not-merge Don't merge this PR label Mar 26, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@samhouts samhouts added the fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DisplayAlert only fires once on TapGesture in ShellItem Page
5 participants