dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.12k stars 1.73k forks source link

[Android] Unloaded event is incorrectly raised during navigation for a view in a custom layout #16697

Closed marchev-prgs closed 9 months ago

marchev-prgs commented 1 year ago

Description

I have a custom Layout. During the MeasureOverride() of the custom layout I create some inner element and add it to Children. Later, when navigation to another page happens - the Unloaded event of the inner element is raised (the Unloaded of the custom layout is not). When I navigate back - the Loaded event is not raised. This leaves custom logic in Loaded/Unloaded event handlers in a inconsistent state.

Steps to Reproduce

  1. Download the repo from the link (https://github.com/telerik/ms-samples/tree/main)
  2. Go to folder Maui\UnloadedRaisedInScenarioWithCustomLayout
  3. Open the solution
  4. Run the App in Android
  5. See in the collection view log that Loaded was raised for both the custom layout and the innerGrid
  6. Click the "navigate" button (it navigates to an empty page)
  7. Navigate back to the previous page
  8. See that Unloaded event was raised for innerGrid

Actual: See the log history in the collection view - the innerGrid received Unloaded when we navigated to the empty page, but Loaded was never raised after that.

Expected: The Unloaded for innerGrid should have not been raised, as the Unloaded for the custom layout was not raised.

unloaded

Link to public reproduction project repository

https://github.com/telerik/ms-samples/tree/main

Version with bug

8.0.0-preview.6.8686

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

Android

Affected platform versions

No response

Did you find any workaround?

N/A

Relevant log output

No response

PureWeen commented 1 year ago

Related https://github.com/dotnet/maui/issues/15833

ghost commented 1 year ago

We've added this issue to our backlog, and we will work to address it as time and resources allow. If you have any additional information or questions about this issue, please leave a comment. For additional info about issue management, please read our Triage Process.

AnnYang01 commented 1 year ago

Verified this on Visual Studio Enterprise 17.8.0 Preview 1.0 using below Project (.NET 8.0), This issue repro on Android 13.0-API33. MauiApp11.zip CustomLayout

MichaelRumpler commented 1 year ago

I saw the same problem on iOS and Windows. The Unloaded event is raised when a new page is pushed, but the VisualElement is still on the navigation stack. I saw that in .NET8 RC1. My customers tell me that this worked before in the preview versions.

mjo151 commented 1 year ago

I am seeing this problem as well with .NET 8 RC1. It occurs on iOS and Android and I haven't tried Windows yet. When a page is pushed onto the navigation stack, the unloaded event is raised on that page. This is a change in behavior from .NET 7 and has major consequences. Is this expected behavior or a bug? It seems like a bug since the page has not been unloaded since it's still on the navigation stack. I would only expect OnDisappearing to be called in that case.

Uridel commented 11 months ago

I'm seeing the same issue in my project as well. Are there any plans to fix this, or what the correct handling should be?

ibrahimmd90 commented 11 months ago

This issue has messed up resources management. Unload event is a vital event for releasing resources. I wish it gets higher attention to fix it. This issue started when I switched to .NET 8 production release from .NET 7.

ibrahimmd90 commented 11 months ago

There could be different workarounds depending on your application. In my application, I use shell tabs and navigation. It makes it little tricky to fix it. Here are what I have done which seems acceptable until now: 1- Create an abstract BasePage that overrides OnNavigatedFrom and OnNavigatedTo. Here it is. C# only:

public abstract class BasePage : ContentPage
{
    public BasePage() : base()
    {

    }
    protected override void OnNavigatedFrom(NavigatedFromEventArgs args)
    {
        var shellSection = Shell.Current.CurrentItem.CurrentItem;
        if(shellSection != this.shellSection)
        {
            return;
        }

        var navigation = Application.Current.MainPage?.Navigation;
        var stack = navigation?.NavigationStack;
        var cnt = stack?.Count;
        if(cnt == pageCnt - 1)
        {
            if(BindingContext is BaseViewModel vm)
            {
                vm.NavigatedAway();
            }
        }
        base.OnNavigatedFrom(args);
    }

    private int? pageCnt;

    private ShellSection shellSection;

    protected override void OnNavigatedTo(NavigatedToEventArgs args)
    {
        if(pageCnt.HasValue)
        {
            return;
        }
        this.shellSection = Shell.Current.CurrentItem.CurrentItem;
        var navigation = Application.Current.MainPage?.Navigation;
        var stack = navigation?.NavigationStack;
        pageCnt = stack?.Count;
        base.OnNavigatedTo(args);
    }
}

2- As you could see, I am using an abstract BaseViewModel that has NavigatedAway method. Here is the implementation of this particular method:

/// <summary>
/// This flag is set to true when the ViewModel is created and cleared its view calls <see cref="NavigatedAway"/> 
/// to let whatever resource managment code dispose it.
/// </summary>
public bool ShouldRemainAlive { get; private set; } = true;

/// <summary>
/// Called by the view of this ViewModel when the view detects events that mean the resources associated with the view
/// should be disposed.
/// </summary>
public void NavigatedAway()
{
    if (disposedValue) return;
    ShouldRemainAlive = false;
}

3- In the place where you handle pages unload event, you check for this flag and see if it is time to release the ViewModel of this page. In my case, I use an extension method:

internal static class ServiceCollectionExtensions
{
    public static void AddPage<T>(this IServiceCollection serviceCollection) where T : VisualElement
    {
        serviceCollection.AddTransient(PageWithDisposableContext<T>);
    }
    private static T PageWithDisposableContext<T>(IServiceProvider serviceProvider) where T : VisualElement
    {
        var page = ActivatorUtilities.CreateInstance<T>(serviceProvider);
        page.Unloaded += (sender, e) =>
        {
            if(sender is VisualElement view)
            {
                if(view.BindingContext is BaseViewModel viewModel)
                {
                    if(!viewModel.ShouldRemainAlive)
                    {
                        viewModel.Dispose();
                    }
                }
            }
        };

        return page;
    }
}

I use this extension when I add pages during app building: builder.Services.AddPage<YouPageThatImplementsBasePage>();

shobanasuresh commented 10 months ago

We're also hitting this issue in our projects after upgrading to .Net 8. The loaded event is raised when returning to a page that's already on the navigation stack. This causes problems with the initialization logic. Is this an intended change introduced in .Net 8 or a bug? If it's an intentional change, what's the correct event to handle one time initialization?

MichaelRumpler commented 10 months ago

I used HandlerChanging instead (my issue). That event is even documented.

dotMorten commented 10 months ago

I'm confused about this bug report. When a UI component disappears from the view, I'd expect Unload to fire, and when it reappears I'd expect it to Load again. I don't get the argument that if it's in the navigation stack it shouldn't raise unload. If you're interested in navigation events, listen for navigation events.

The loaded event is raised when returning to a page that's already on the navigation stack. This causes problems with the initialization logic.

It sounds to me like you were relying on a bug in the load/unload lifecycle. They finally fixed this bug, and that is now causing problems for you. I think the more correct thing to do is move your initialization logic to the right place.

marchev-prgs commented 10 months ago

I used HandlerChanging instead (my issue). That event is even documented.

I modified the original test project (from the description) and the HandlerChanging event is not raised when navigating to another page, nor when navigating back from that page. My point is that the HandlerChanging event cannot always be used as a work-around.

PureWeen commented 9 months ago

I'm confused about this bug report. When a UI component disappears from the view, I'd expect Unload to fire, and when it reappears I'd expect it to Load again. I don't get the argument that if it's in the navigation stack it shouldn't raise unload. If you're interested in navigation events, listen for navigation events.

The loaded event is raised when returning to a page that's already on the navigation stack. This causes problems with the initialization logic.

It sounds to me like you were relying on a bug in the load/unload lifecycle. They finally fixed this bug, and that is now causing problems for you. I think the more correct thing to do is move your initialization logic to the right place.

Yea, unloaded/loaded are tied to platform events indicating that the view is part of the visual tree or has been removed from the visual tree. It's definitely unfortunate this was broken in NET7.

I used HandlerChanging instead (my issue). That event is even documented.

@MichaelRumpler

I am curious why unloaded/loaded still didn't work for you? It makes me think there's some nuance to this issue I'm missing. You can still connect gestures inside loaded and then disconnect them inside unloaded. Is there a reason you need gestures to still be present on a control that's no longer part of the visual tree?

AFAICT the original issue was somewhat a bug.

Actual: See the log history in the collection view - the innerGrid received Unloaded when we navigated to the empty page, but Loaded was never raised after that.

Now with the original sample, the unloaded and loaded events correctly fire for the innerGrid and the custom layout when returning back to the initial page.

I realize the fix doesn't match the "Expected" but it's at least now not completely unexpected :-)

PureWeen commented 9 months ago

I've checked with the original reporter of this issue, and it is now working in .NET 8 to their satisfaction.

If you have a variation to the original issue that you'd like to report, please create a new issue.