Xamarin.forms: [Android] Effects don't detach and OnElementchanged(null) is never called

[PureWeen]

Description

On Android when the Renderer disposes it doesn't call SetElement(null) it only sets the property so it doesn't propagate anywhere. This behavior isn't consistent with how it works on iOS and UWP. In the documentation it typically talks about using OnDetached and if(e.OldElement != null) to unhooks your code. As it stands on Android it's probably best to write a general method that unhooks everything and then call that from both Dispose and if(e.OldElement != null) as that's the only reliable way your custom bits will be disposed of.

I tested calling SetElement(null) but it didn't "just work"

Steps to Reproduce

• run the sample
• the sample has a custom effect and it's own custom button renderer
• click the button on the first page to navigate forward
• click the button on the second page to navigate back
• notice on UWP/iOS that OnDetached and OnElementChanged are called when the page is popped off the stack but Android that does not happen

Expected Behavior

• OnElementChanged gets called when the Element is set to null
• Effects get Detached when the Element is set to null

Actual Behavior

• OnElementChanged never gets called when the Element is set to null
• Effects don't Detached when the Element is set to null

Basic Information

• Version with issue: 3.0

Reproduction Link

EffectsAndRenderers.zip

[samhouts]

TODO: Clean up test added in #2857 when this is resolved.

[kvpt]

I don't know if it requires a separate issue, but the same apply to behavior.

[velocitysystems]

As it stands on Android it's probably best to write a general method that unhooks everything and then call that from both Dispose and if(e.OldElement != null) as that's the only reliable way your custom bits will be disposed of.

Exactly, this is what we are doing right now.
@samhouts Is the XF team planning on working on this, or would you accept a PR?

[samhouts]

This was partially resolved for legacy renderers, but fast renderers still suffer from it.

[chaosifier]

Any updates on the issue?

[dersia]

Is anyone working on this?

[dersia]

I started working on this issue...
From what I can see so far, the fix would go through all Heads. There are places in the GTK head that are also involved.
@PureWeen do you think this should be fixed in all Heads?
What bothers me, is that when it comes to Effects, other Heads seem to use other methods to attach Effects, this makes it hard to refactor and make sure it works everywhere as intended.

Or should I just provide a fix for Android and we'll open issues for the other Heads?

[PureWeen]

What bothers me, is that when it comes to Effects, other Heads seem to use other methods to attach Effects, this makes it hard to refactor and make sure it works everywhere as intended.

can you give examples?

Or should I just provide a fix for Android and we'll open issues for the other Heads?

We'll probably do this but depending on the examples from the above question maybe we can rope it all together!!

[dersia]

can you give examples?

Sure, but first let me summarize what I came across today, while working on the fix.

TL;TR
There seems to be no consistency in VisualElementRenderer implementations.

I spent the day figuring out what changes are necessary and what needs to be done. For that I went through all Heads and looked for VisualElementRenderer implementations and other places, that are using either SetElement() or OnElementChanged().
And it really does not look good.
Almost everyone is implementing IVisualElementRenderer or VisualElementRenderer differently, there is a lot of code duplication and I think the best way would be to consolidate this a bit more.

Here are some examples:

There is implementation of EffectUtilities in Xamarin.Froms.Internals that is used by most Heads (Android, UWP, WPF etc.) to register the Effect on the Element but GTK is using a GTKContainer and registering the Effect on the Container.

EffectUtilities.RegisterEffectControlProvider(this, element);

GTK:

protected virtual void OnRegisterEffect(PlatformEffect effect)
{
    effect.SetContainer(this);
    effect.SetControl(Container);
}

iOS and MacOS on the other hand have their own EffectUtilities which is in implementation a duplication of the EffectUtilities in the Internals namespace.
But this is easy to fix and this is not where I see the most problems.
I implemented IDisposable on Element to clean up effects and detach them, so a call to Element.Dispose() is good enough for the Effects-issue.

But this is where it all starts. While going through all VisualElementRenderers to implement Element.Dispose() and call OnElementChanged() on SetElement I found that there are many Handler registrations, which are not cleaned up on the old Element. I started consolidating this and moved detaching of EventHandlers to SetElement() for the oldElement, and then making sure EventHandler registration on the new Element only is applied if the new Element is not null, so a call to SetElement(null) from the Dispose method is sufficient.
And here I discovered, that almos every VisualElementRednerer and class that is implementing IVisualElementRenderer attaches EventHandlers at a different point. Some in the Contructor, some in OnElementChanged others do it in Init-methods or OnViewDidLoad.
Now you could say that overtime with the product growing, these inconsistencies can happen, and I agree, but you can see this in even newer implementations like MediaElement.

Overall I think this is due to lack of clear guidance and therefor really hard to refactor, I often also came across of potential NREs, where I really started missing C# 8.0 nullability 😁

I would suggest to create an abstract VisualElementRenderer which then can be used by all Elements and it will provide a method for Setup (attaching EventHandlers, doing some other setup needed for that Element) and Teardown (dettaching and cleanup) that is then called internally by a virtual SetElement() method. This way we can make sure that it is always safe to call SetElement(null) from the virtual Dispose method on the abstract class.

I would provide an API for review, before I go down that path.

Meanwhile, to fix this issue for Android, I would just make a simple fix to the VisualElementRenderer that will fix the call to OnElementChanged() but it won't take care of dettaching Effects for now.

I hope this isn't too much and we can move forward, I really like to do the refactoring because i think this will also improve overall usage on resources (possible memory leaks and NREs). 😊

[samhouts]

This issue doesn't seem to have had any activity in a long time. We're working on prioritizing issues and resolving them as quickly as we can. To help us get through the list, we would appreciate an update from you to let us know if this is still affecting you on the latest version of Xamarin.Forms, since it's possible that we may have resolved this as part of another related or duplicate issue. If we don't see any new activity on this issue in the next 30 days, we'll evaluate whether this issue should be closed. Thank you!

[MaximMikhisor]

Do not sure is my issue related to this bug or not, but on Android PlatformEffect.OnDetached() method is not called for my custom effect.