ViewDestroy lifecycle method is broken

[arkadiuszokon]

🔙 Regression

commit which breaks this: 32b6868

How we should use ViewDestroy now? It's pointless - it's called every time when application goes to background with "viewFinishing" flag set by default to true.

Old (and correct) behavior

ViewDestroy was called with corresponding native methods of view lifecycle

Current behavior

ViewDestroy is called with OnDisappearing method in mvx view classes.

Reproduction steps

Configuration

Version: 6.2

Platform:

• 📱 iOS
• 🤖 Android
• 🏁 WPF
• 🌎 UWP
• 🍎 MacOS
• 📺 tvOS
• 🐒 Xamarin.Forms

[piotrekbigu]

Hi,
When we can expect a version where this issue will be fixed?

ViewDestroy method is very important in view lifecycle (releasing resources, unsubscribing, etc) and fact that is called whenever view disappears is really critical bug.

[nmilcoff]

I think ViewDestroy was not being called before, since obviously classes like ContentPage do not extend any MvvmCross class.

I'm not sure removing the call to ViewDestroy for all Xamarin Forms views is the best solution here.

[piotrekbigu]

@nmilcoff
I am not talking about removing the call to ViewDestroy. I am rather pointing fact that proper resolution of this bug is very important.

As I good understood changes made by author of commit 32b6868 was aimed at fix of lack of call to ViewDestroy by adding it to ViewDisappearing. From my perspective this is not a fix beacuse we cannot differentiate between view destruction and hiding. As @arkadiuszokon said the ideal fix would be when:

ViewDestroy was called with corresponding native methods of view lifecycle

[Cheesebaron]

@piotrekbigu please don't ask when it is going to be fixed. Instead, if it is such a pressing matter for you, open a PR fixing this and it will most likely get out next time we release.

[nmilcoff]

Since ContentPage and all the siblings are exposed by Xamarin.Forms, there's no easy way for us to call ViewDestroy from a "native" view object (as @arkadiuszokon proposes). We can only use what Xamarin.Forms exposes.

In the past ViewDestroy wasn't being called at all - so I wouldn't say this is a regression 🤔

[MysterDru]

This could be fixed by creating custom renderers for all of the MvvmCross pages in each platform and making to call to ViewDestory from the Dispose method of the renderer. I've verified that this works on Android & iOS. I'm would assume the same would be the case for other platforms.

protected override void Dispose(bool disposing)
{
    if (disposing)
    {
        var viewModel = this.Element?.BindingContext as IMvxViewModel;
        viewModel?.ViewDestroy();
    }

    base.Dispose(disposing);
}

I'm not setup to build/develop on the mvvmcross solution on my machine (i don't have a windows vm). Otherwise I'd look at submitting a PR for this.

[nmilcoff]

@keannan5390 although that approach works, it's not ideal from a design perspective. If a developer wants to write a custom renderer for a page (and of course is using MvvmCross), our custom renderer will needs to be the base class for it - otherwise ViewDestroy won't be called.

[MysterDru]

@nmilcoff Understandable from a design perspective that it doesn't make sense. Forcing the mvx renderers wouldn't work in all situations. However, as the OP calls out, I believe this is a broken implementation currently -- especially if you are using the IMvxViewModelResult<TResult> implementation. That logic relies on the return navigation task being called from ViewDestory in the viewmodel to cancel the task if it hasn't currently been completed.

Right now, ViewDestroy is called from ViewDisappearing in the forms page, this can be problematic as it will result in a navigation task being cancelled.

Say we have the following navigation:
ViewModelA -> ViewModelB -> ViewModelC

ViewModelB returns a value to ViewModel A, and ViewModelC returns a result to ViewModelB. When you navigate to C from B, ViewDestroy will be called from PageB->ViewModelB and will cancel the task navigation return task from that ViewModelA created.

Alternatively, if you were to background the app (as the OP mentioned) while on ViewModelB that same navigation task between A & B will be cancelled/completed without returning a result to A.

I have not upgraded my applications to 6.2 as a result of this. I have one app especially that uses the IMvxViewModelResult heavily.

[nmilcoff]

I understand your concern. An idea would be to avoid calling ViewDestroy when navigating forwards, but I don't think Xamarin.Forms exposes that

[abdomh]

A Temporary workaround for this issue for me was to implement a base view-model from IMvxViewModel<TParameter,TResult> and added a new method called ViewPopped with this code

if (CloseCompletionSource != null && !CloseCompletionSource.Task.IsCompleted &&
             !CloseCompletionSource.Task.IsFaulted)
         {
             CloseCompletionSource?.TrySetCanceled();
         }

now in xamarin forms i listened to PageAppearing now whenever a page appears i check if it's a navigation page if it's i listen to Popped event like this

if (page is NavigationPage nvPage) nvPage.Popped += OnPagePopped;

now when a page popped i send a call to ViewPopped method and if i popped a navigation page i remove the event handler

 if (e.Page is NavigationPage nvPage) 
                nvPage.Popped -= OnPagePopped;

if (e.Page is IMvxPage mvxPage && mvxPage?.ViewModel is ViewModelBase viewModel)
                viewModel.ViewPopped();

also dont forget to call ViewDestroy from your ViewPopped
also you may want to listen to ModalPopped as well in the application

[MysterDru]

I added a comment for this on the attached PR, but i'll post here for clarity as well
#3292 (comment)

I think i found a more reliable way to properly call the OnDestroy lifecycle of the view models that doesn't rely on developers extending from the MvxFormsApplication or an MvxRenderer. Some variation of the following code should work.

Whenever a renderer is disposed at the platform layer, the attached Renderer property for the element is set to null. This is a non public value, so there is certainly some risk with doing it this way since it relies on reflection, but it seems like it might be the most reliable.

The following could be added to each of the Mvx page implementations:

protected override void OnPropertyChanged([CallerMemberName] string propertyName = null)
{
    base.OnPropertyChanged(propertyName);

    // when Dispose is called on the renderers, the body of this if statement will be set executed
    // the Dispose code of the renderers will set the attached bindable property for "Renderer" on the 
   // forms platform class. So if Renderer property changed, and its value is null, we can assume that the view was destroyed
    if(propertyName == "Renderer" && this.GetValue(this.GetRendererProperty()) == null)
    {
        this.ViewModel.ViewDestroy(true);
    }
}

And the implementation for GetRendererProperty is this:

public static BindableProperty GetRendererProperty(this Page formsPage)
{
    // borrowed from James Monetemagno's PullToRefresh layout control
    // https://github.com/jamesmontemagno/Xamarin.Forms-PullToRefreshLayout/blob/ad63e21a5010dd04b570a89eca5c5287ac36e0ed/PullToRefresh/PullToRefresh.Standard/PullToRefreshLayoutRenderer.android.cs#L132

  // this might need to change a bit depending on platform, but its just the general idea of resolving the platform specific assembly name & type
  string runtime = Xamarin.Forms.Device.RuntimePlatform;
   var type = Type.GetType($"Xamarin.Forms.Platform.{runtime}.Platform, Xamarin.Forms.Platform.{runtime}");
    var prop = type.GetField("RendererProperty", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static);
    var val = prop.GetValue(null);
    var rendererProperty = val as BindableProperty;

    return rendererProperty;
}

EDIT: edited to be not android specific

[martijn00]

@MysterDru can you make a PR with your proposal?

[Weldryn]

Any news about this issue?

[henda79]

How can I fix this issue ?

I've hit this issue when "waiting" for a result from a modal page. I've tried cloning this repo and making the changes the suggested myself as it looks like the PR is still waiting on approval and it was created back in Feb, however I can't get it to build on my machine.