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

Shell, Navigation and Page instantiation #9300

Open
pfafft33 opened this issue Aug 10, 2022 Discussed in #5810 · 39 comments
Open

Shell, Navigation and Page instantiation #9300

pfafft33 opened this issue Aug 10, 2022 Discussed in #5810 · 39 comments
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout p/1 Work that is critical for the release, but we could probably ship without s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working

Comments

@pfafft33
Copy link

pfafft33 commented Aug 10, 2022

Discussed in #5810

Originally posted by ioiooi April 4, 2022
Hello,

are Pages that are defined in the visual hierarchy only ever instantiated once? And if that is the case is there a way to clear the cache and force the creation of a new Page instance the next time I navigate to it?

For example the following hierachy is described in a AppShell.xaml:

<ShellContent Route="LoginPage" Shell.FlyoutBehavior="Disabled" ContentTemplate="{DataTemplate local:LoginPage}" />

<FlyoutItem FlyoutDisplayOptions="AsMultipleItems">
    <ShellContent Title="About" Icon="tab_about.png" Route="AboutPage" ContentTemplate="{DataTemplate local:AboutPage}" />
    <ShellContent Title="Browse" Icon="tab_feed.png" ContentTemplate="{DataTemplate local:ItemsPage}" />
</FlyoutItem>

<ShellContent Route="SecretPage" Shell.FlyoutBehavior="Disabled" ContentTemplate="{DataTemplate local:SecretPage}" />

The LoginPage constructor gets called when the App is started. Navigate to the AboutPage with Shell.Current.GoToAsync($"//{nameof(AboutPage)}") and its constructor gets called.

Now lets say the AboutPage has a Logout-Button which does some things and at the end it calls Shell.Current.GoToAsync($"//{nameof(LoginPage)}"). The LoginPage constructor does not get called. Navigate back from the LoginPage to the AboutPage and its constructor also does not get called.

The same deal if I would navigate from the LoginPage to the SecretPage. The constructor of SecretPage would only get called the first time.

Lets say I use ViewModels for all those Pages. I can't really create a state in the ViewModel constructor because the Page only gets instantiated once and so in my case the ViewModel also only gets instantiated once. I would need to do the work in a OnAppearing.

I would want for the Pages after the LoginPage to always be newly constructed in order to make sure that theres a clean state.

PS:
A lot of examples also contain DetailsPages... something like ItemsDetails, MonkeyDetails, CatDetails etc... Those Pages are never declared in the AppShell.xaml but are registered like so

Routing.RegisterRoute("monkeys/details", typeof(MonkeyDetailPage));

and from what Ive seen seem to be always instantiated every single time theyre navigated to.

Workaround

#9300 (comment)

@pfafft33
Copy link
Author

I have run into this issue as well, and it's a BIG one!
Also, I just upgraded to VS Community 2022 Preview v 17.4.0 Preview 1.0 hoping that it would be fixed.
Unfortunately, it was NOT fixed in this latest update.

I have had to resort to creating a "REFRESH PAGE" button at the top of each non-detail page, that executes the same code as the Constructor. This is the ONLY way I have found to get my data updated on the non-detail pages.

PLEASE, PLEASE, PLEASE fix this ASAP. There is NO WAY that I'm going to release a Production app that requires my users to click a REFRESH PAGE button every time that they navigate to a non-detail page.

@angelru
Copy link

angelru commented Aug 10, 2022

I faced the same "issue", I ended up overriding the OnNavigated and OnNavigating in AppShell. There I get the navigation path and then execute the method of my ViewModel.

@pfafft33
Copy link
Author

@angelru Thanks for the tip!

@PureWeen
Copy link
Member

@pfafft33
Copy link
Author

@PureWeen - I tried OnNavigatedTo like you suggested. I put the following code into my StoreListPage.xaml.cs file:

protected virtual void OnNavigatedTo(NavigationEventArgs e)
{
    _viewModel.StoreListViewModelConstructorCode();
}

And, I set a breakpoint and ran my App.
As it turns out, this method does NOT get called when I navigate to my Page.

@pfafft33
Copy link
Author

@angelru - I tried your suggestion. I went into my AppShell.xaml.cs file and typed in the following code:

public override void OnNavigated()
{

}

public override void OnNavigating()
{

}

I got Compiler Error CS0115: no suitable method found to override

By your message, it sounds like you got this to work. What am I missing?

@angelru
Copy link

angelru commented Aug 11, 2022

This code is from an application in Xamarin Forms, in .NET MAUI I have not tried.

        protected override void OnNavigated(ShellNavigatedEventArgs args)
        {
            string location = args.Current?.Location?.ToString();
            string previous = args.Previous?.Location?.ToString();

            if (location is "//Main/Workouts")
            {
                switch (previous)
                {
                    case "//MainPage":
                    case "//MainPage/Login":
                    case "//Main/Workouts/WorkoutQuestionnaire":
                        _ = WorkoutsPageViewModel.Current.RunSafeAsync(WorkoutsPageViewModel.Current.InitWorkoutsAsync);
                        break;
                }
            }

            base.OnNavigated(args);
        }

        protected override void OnNavigating(ShellNavigatingEventArgs args)
        {
            string location = args.Target.Location.ToString();

            if (location == "//Main")
            {
                vm.InitHankuk();
                LocalNotification();
            }

            base.OnNavigating(args);
        }

@pfafft33
Copy link
Author

@angelru - Using your example code, I was able to get it to work with just the following code added to AppShell.xaml.cs

protected override void OnNavigated(ShellNavigatedEventArgs args)
{
    string location = args.Current?.Location?.ToString();

    if (location is $"//{nameof(StoreListPage)}")
    {
        ((StoreListPage)Shell.Current.CurrentPage)._viewModel.StoreListViewModelConstructorCode();
    }

    base.OnNavigated(args);
}

Thanks SO much for your help!

I STILL think that this is a bug, and that it should get fixed ASAP.
We shouldn't have to resort to this kind of work-around. A page should automatically get refreshed / re-instantiated each time it is navigated to.

@PureWeen
Copy link
Member

Related #7354

@PureWeen PureWeen added this to the Backlog milestone Aug 11, 2022
@PureWeen PureWeen added the area-controls-shell Shell Navigation, Routes, Tabs, Flyout label Aug 11, 2022
@PureWeen PureWeen modified the milestones: Backlog, .NET 8 Planning Aug 11, 2022
@dotnet dotnet deleted a comment Aug 11, 2022
@PureWeen
Copy link
Member

@pfafft33 can you log a bug?

From my tests it gets called.

image

@MatthewB05
Copy link

OnNavigatedTo is broken as soon as you have tabs within the flyout item, as per the example at the top of this issue. It works for the first tab, but not on any other tab, until you go to a different flyout item, and then back again, at which point the OnNavigatedTo will fire for whichever tab was previously shown. Certainly the case with both Windows and Android on latest preview downloaded on the date of this comment.

@AlexanderSCK
Copy link

You need to register your pages and the viewmodels associated with the pages as either Singletons or Transient in MauiProgram.cs. If regestered as Singleton the page is only instantianted once and the state of the page and the viewmodel is kept, where as for details pages for example you would use transient so its creted and destroyed each time you navigate to/back from it. Also use the EventToCommand behavior from the CommunityToolkit to call the pages "Appearing" and bind your code to a command in the viewmodel. Register the viewmodels the same as the pages in the MauiProgram.cs file.

@papafe
Copy link

papafe commented Dec 19, 2022

@AlexanderSCK It seems that when using Shell, the pages are reused even if using Transient, so unfortunately this does not solve the issue.

@EntityBox
Copy link

It seems like only the AppShell pages that has the routing attribute in xaml has this issue, if you do a second page navigation, the DI works as expected with the Singleton/Transient. Creating methods to perform the functions of the constructor is a workaround but not the best solution.

@shadowfoxish
Copy link

Perhaps what is needed is a way OnDisappearing or a navigation event handler detecting "Pop" or "Go Back" to set a flag to indicate whether the view should be destructed or if it can be reused?

On the positive side, navigation is much much quicker than with Stack based navigation from XF. Using good practices and not storing a lot of state in the views themselves helps. Using OnAppearing in conjunction with a call to your view model and ApplyQueryAttributes (from IQueryAttributable on the ViewModel classes) in should let reused instances reset themselves.

@fgiacomelli
Copy link

I have a similar requirement in a new app, login page with registration flow, and then a list of pages under a tab menu.

I have registered some pages and viewmodels under the tabs as Transient, and I expected that navigating to that pages they should reconstructed as new instances. This is because I can logout with a user, returning to the login page (setting it as root and clearing in this way the navigation stack), and relogging with another user, and I need that pages and viewmodels be reinitialized.

But what I see at this time is that the pages and viewmodels are never recreated. I did not registered the route to this pages using the appshell.xaml, but forcing the route registration in appshell.xaml.cs (code behind).

I found a workaround declaring a property in a baseViewmodel of each viewModel that I use to understand if call the Initialize method of each and resetting it from codebehind, forcing a sort of reinitialization when the page appears, but it's very tricky, and for use it I need to declare all the pages and viewmodels as singleton.

@fgiacomelli
Copy link

It seems like only the AppShell pages that has the routing attribute in xaml has this issue, if you do a second page navigation, the DI works as expected with the Singleton/Transient. Creating methods to perform the functions of the constructor is a workaround but not the best solution.

in my situation I did not register the route from xaml but from code behind, but the pages are never recreated

@EntityBox
Copy link

EntityBox commented Feb 1, 2023

in my situation I did not register the route from xaml but from code behind, but the pages are never recreated

Then it might be a routing issue in general. seems like GoToAsync($"//{nameof(SomePage)}") the // is suppose to remove everything from the NavigationStack and in theory dispose of any Transient pages, but this does not happen.

Having a look at MAUI Docs the wording use is "replace the navigation stack", maybe this should be the hint that it does not dispose on "replace"?

@PureWeen - This is an issue if DI Transient Pages and ViewModels are not disposed. Do we perhaps have estimated fix, PR or Merge on this problem.

@fgiacomelli
Copy link

in my situation I did not register the route from xaml but from code behind, but the pages are never recreated

Then it might be a routing issue in general. seems like GoToAsync($"//{nameof(SomePage)}") the // is suppose to remove everything from the NavigationStack and in theory dispose of any Transient pages, but this does not happen.

It could be, but if you look in the Navigation stack after Navigated to the // page you can see that it just contains the root page.

In the meantime I just created a sample to reproduce the problem, so if we are misunderstanding some concepts of DI or AppShell navigation somebody could help us, because I really can't understand if I'm doing something wrong, if I have a wrong expectation of if this is another bug of MAUI.

In this app (that it's really raw because I just modified another repo created to reproduce a different bug of MAUI) the MainPage represent the LoginPage, pressing a button we navigate to a tabbed page, where the "OtherPage" Page is registered as transient as its viewmodel. I can see that the viewmodel and page constructors are called only once, then returning to the mainPage and after to the "OtherPage" I would expect to see new instances but it doesn't happen.

Here is the repository: https://github.com/fgiacomelli/DependencyInjectionOnTransient

@angelru
Copy link

angelru commented Feb 2, 2023

Doesn't it make sense for views to be reused? In my opinion it would be faster for navigation. But ViewModels shouldn't be like this, since in them we get data and have changes to the views. But I understand that we can't have the view as a singleton and a ViewModel associated with that view as a transient, because this would also be a singleton.

What would be the correct approach then?

@fgiacomelli
Copy link

Sure that it makes sense, but like in any container we must be able to configure the classes registered as singleton or new on each request, and the architecture need to implement the correct behavior

@angelru
Copy link

angelru commented Feb 3, 2023

Sure enough, I have a page added in AppShell and I registered it as a Transient next to its ViewModel, and the constructor is only called 1 time when I navigate to it.

@PureWeen
Copy link
Member

PureWeen commented Feb 3, 2023

in my situation I did not register the route from xaml but from code behind, but the pages are never recreated

Then it might be a routing issue in general. seems like GoToAsync($"//{nameof(SomePage)}") the // is suppose to remove everything from the NavigationStack and in theory dispose of any Transient pages, but this does not happen.

Having a look at MAUI Docs the wording use is "replace the navigation stack", maybe this should be the hint that it does not dispose on "replace"?

@PureWeen - This is an issue if DI Transient Pages and ViewModels are not disposed. Do we perhaps have estimated fix, PR or Merge on this problem.

Yea :-/ this was definitely a very unfortunate oversight. I think some users will want GoToAsync($"//{nameof(SomePage)}" to clear the navigation stack and others won't. According to the docs and the rules of the system it should clear the stack and the fact that it doesn't is confusing. I'm still thinking through various paths here to enable better intention here.

Perhaps just tying into Transient/scoped/singleton here is enough. If you register as Transient, it'll start you over if you don't it'll maintain the stack.

No specific ETA on this, but I'm going to see if I can work out some helpers to enable the features here. Get some active feedback going!

@lemcoder
Copy link

lemcoder commented Feb 4, 2023

I've created a workaround using ServiceProvider class which is avaliable via DI in MAUI project. I've defined a generic TransientContentPage which resolves viewModel when OnAppearing event occurs:

public class TransientContentPage<TViewModel> : ContentPage
  {
      private readonly IServiceProvider serviceProvider;
      public TransientContentPage(IServiceProvider serviceProvider)
      {
          this.serviceProvider = serviceProvider;
      }

      protected override void OnAppearing()
      {
          BindingContext = serviceProvider.GetService<TViewModel>();
          base.OnAppearing();
      }
  }

This approach seems to work fine for me, still it is far from perfect.

@PureWeen
Copy link
Member

PureWeen commented Feb 5, 2023

Let me know if this works.

public partial class AppShell : Shell
{
	public AppShell()
	{
		InitializeComponent();
	}

	ShellContent _previousShellContent;

	protected override void OnNavigated(ShellNavigatedEventArgs args)
	{
		base.OnNavigated(args);
		if (CurrentItem?.CurrentItem?.CurrentItem is not null &&
			_previousShellContent is not null)
		{
			var property = typeof(ShellContent)
				.GetProperty("ContentCache", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.FlattenHierarchy);

			property.SetValue(_previousShellContent, null);
		}

		_previousShellContent = CurrentItem?.CurrentItem?.CurrentItem;
	}
}

Or you can install this nuget
https://github.com/PureWeen/ShanedlerSamples

and use ShellContentDI instead of ShellContent

@fgiacomelli
Copy link

I made some other tests and I can confirm that the problem only happens when using appshell, in my situation with tabs, and when the routes are registered in appshell.xaml where the tab hierarchy is declared.

If I don't declare the route in the tab hierarchy the transient pages are recreated as expected on every navigation, but they don't show the tabs.

Set the route property of the pages in code behind has the same problem that the pages are not recreated.

Any suggestion to find a workaround?

@rbwsoftware
Copy link

I changed both the viewmodel and page to transient and now the page constructor works each time the page is navigated to using shell. At first I just changed the viewmodel and it still only created once, but changing both worked for me.

@fgiacomelli
Copy link

fgiacomelli commented Feb 23, 2023

I changed both the viewmodel and page to transient and now the page constructor works each time the page is navigated to using shell. At first I just changed the viewmodel and it still only created once, but changing both worked for me.

How did you register routes?

In my test I have this shell hierarchy, I just need to navigate from MainPage (that represent the login page) to the tabbed section clicking on a button :

<ShellContent
        Title="Login"
        ContentTemplate="{DataTemplate local:MainPage}"
        Route="MainPage" />

    <TabBar>
        <Tab Title="Dashboard">
            <ShellContent
                Route="DashboardPageA"
                Title="DashboardPageA"
                ContentTemplate="{DataTemplate local:DashboardPageA}"
                Shell.NavBarIsVisible="False" />
        </Tab>
        <Tab Title="OtherPage">
            <ShellContent
                Route="OtherPage"
                ContentTemplate="{DataTemplate local:OtherPage}"
                Shell.NavBarIsVisible="False" />
        </Tab>
    </TabBar>

when the app starts, I navigate to the MainPage, and then using Shell navigation:

await AppShell.Current.GoToAsync($"//{nameof(DashboardPageA)}");

I navigate to the tab section. In that section I navigate another time to the MainPage, using the same method (I don't want to pop the page but I want to navigate to a new instance):

await AppShell.Current.GoToAsync($"//{nameof(MainPage)}");

and then if I navigate another time to the DashboardPageA I never see their constructors to be called, both the pages are always recycled.

Both pages have a specific viewmodel injected in the constructor, and all (view and viewmodels) are registered as transient.

@ricardodamaceno
Copy link

In my case it is like this. See that I'm using more than one ShellContent inside the Tab. The behavior is the following: When I navigate to the "Menu" Tab, it loads the "Cafe" page, which in turn ends up being the only page associated with the "Menu" Tab. When navigating to other ShellContents, as they are being loaded, they are saved in memory and when I navigate to them again, no method is triggered, they have the body of static pages. But when I navigate to another Tab, they trigger OnAparing and I manage to update them.

`

        <ShellContent
            Title="Mesas"
            ContentTemplate="{DataTemplate main:MainPage}"
            Icon="mesa.png"/>
        
        <Tab Title="Cardápio"
             Icon="cardapio.png" 
             Route="Cardapio">
        
            <ShellContent Title="Café"  ContentTemplate="{DataTemplate localTab:Cafe}" />
            
            <ShellContent Title="Capuccino"  ContentTemplate="{DataTemplate localTab:Capuccino}"/>

            <ShellContent Title="Sucos" ContentTemplate="{DataTemplate localTab:Suco}"/>

            <ShellContent Title="Pratos Quentes" ContentTemplate="{DataTemplate localTab:PratosQuentes}"/>

            <ShellContent Title="Pães" ContentTemplate="{DataTemplate localTab:Pao}"/>
            
        </Tab>
        
        <ShellContent
            Title="Fechamento"
            ContentTemplate="{DataTemplate local:Fechamento}"
            Icon="fechamento.png" />
        
        <ShellContent
                Title="Opções"
                ContentTemplate="{DataTemplate local:Menu}"
                Icon="menu.png">
        </ShellContent>
    </TabBar>`

@ederbond
Copy link
Contributor

ederbond commented Mar 2, 2023

@PureWeen @davidortinau @maddymontaquila Any new about fixing this issue?
I'm still facing this issue on MAUI 7.0.59 Service Release 3 these kind of bugs it's what sucks the most on MAUI.

BTW I've found a workaround here #5810

I'm just not sure about other issues this workaround could cause, like memory leak and etc @jonathanpeppers and @PureWeen

@jonathanpeppers
Copy link
Member

@ederbond completely replacing App.MainPage = new AppShell() seems like it should be fine to do.

If you find there is actually a memory leak when doing that, file an issue. Thanks!

@PureWeen
Copy link
Member

PureWeen commented Mar 3, 2023

I posted a workaround here. Let me know how well that works/doesn't work
#9300 (comment)

@fgiacomelli
Copy link

I posted a workaround here. Let me know how well that works/doesn't work #9300 (comment)

@PureWeen your solution worked for me, thank you for sharing!

@pikausp
Copy link

pikausp commented May 1, 2023

Let me know if this works.

public partial class AppShell : Shell
{
	public AppShell()
	{
		InitializeComponent();
	}

	ShellContent _previousShellContent;

	protected override void OnNavigated(ShellNavigatedEventArgs args)
	{
		base.OnNavigated(args);
		if (CurrentItem?.CurrentItem?.CurrentItem is not null &&
			_previousShellContent is not null)
		{
			var property = typeof(ShellContent)
				.GetProperty("ContentCache", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.FlattenHierarchy);

			property.SetValue(_previousShellContent, null);
		}

		_previousShellContent = CurrentItem?.CurrentItem?.CurrentItem;
	}
}

Or you can install this nuget https://github.com/PureWeen/ShanedlerSamples

and use ShellContentDI instead of ShellContent

@PureWeen I applied this workaround a while back and it seemed to have been working fine. When I needed to tap into the navigation system I found out it breaks something. When I go back from a detail page to list page, I do not receive navigation notifications in the form of OnNavigated, OnNavigatedTo, OnAppearing and not sure how many others. Same thing with navigating to products from products/create after a product is created. When I remove your workaround the methods are called as expected.

@pikausp
Copy link

pikausp commented May 12, 2023

in my situation I did not register the route from xaml but from code behind, but the pages are never recreated

Then it might be a routing issue in general. seems like GoToAsync($"//{nameof(SomePage)}") the // is suppose to remove everything from the NavigationStack and in theory dispose of any Transient pages, but this does not happen.
Having a look at MAUI Docs the wording use is "replace the navigation stack", maybe this should be the hint that it does not dispose on "replace"?
@PureWeen - This is an issue if DI Transient Pages and ViewModels are not disposed. Do we perhaps have estimated fix, PR or Merge on this problem.

Yea :-/ this was definitely a very unfortunate oversight. I think some users will want GoToAsync($"//{nameof(SomePage)}" to clear the navigation stack and others won't. According to the docs and the rules of the system it should clear the stack and the fact that it doesn't is confusing. I'm still thinking through various paths here to enable better intention here.

Perhaps just tying into Transient/scoped/singleton here is enough. If you register as Transient, it'll start you over if you don't it'll maintain the stack.

No specific ETA on this, but I'm going to see if I can work out some helpers to enable the features here. Get some active feedback going!

Is there any workaround for clearing the stack? I have a ShellItem which uses DataTemplateSelector to render different page based on the state of the application and I need to navigate to //dynamic-page and have the page created even if the route was //dynamic-page (or a child path).

@samhouts samhouts added t/bug Something isn't working p/1 Work that is critical for the release, but we could probably ship without labels Jul 31, 2023
@jingliancui
Copy link

@ederbond completely replacing App.MainPage = new AppShell() seems like it should be fine to do.

If you find there is actually a memory leak when doing that, file an issue. Thanks!

awesome

@homeyf
Copy link
Collaborator

homeyf commented Nov 7, 2023

Verified this issue with Visual Studio Enterprise 17.8.0 Preview 5.0. Can repro on windows platform.
https://github.com/PureWeen/ShanedlerSamples
image

@homeyf homeyf added s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage labels Nov 7, 2023
@Redth Redth modified the milestones: .NET 8 SR3, .NET 8 + Servicing Jan 10, 2024
@robertok77
Copy link

I would appreciate if ContentCache would be an option ...

@fekberg
Copy link

fekberg commented May 23, 2024

Any updates on when this is planned to be fixed @samhouts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout p/1 Work that is critical for the release, but we could probably ship without s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
None yet
Development

No branches or pull requests