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

Navigation data is retained and also presented to IQueryAttributable on Back navigation. #10294

Closed
bakerhillpins opened this issue Sep 23, 2022 · 52 comments · Fixed by #15585
Closed
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout area-navigation NavigationPage fixed-in-8.0.0-preview.7.8842 Look for this fix in 8.0.0-preview.7.8842! p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 s/needs-attention Issue has more information and needs another look t/bug Something isn't working
Milestone

Comments

@bakerhillpins
Copy link

bakerhillpins commented Sep 23, 2022

Description

I'm using Shell with my MAUI app and am passing parameters during navigation with Shell.GoToAsync() using the Dictionary<string, object> and the IQueryAttributable interface as discussed in the MS docs.

var navigationParameter = new Dictionary<string, object>
    {
        { "Bear", animal }
    };
await Shell.Current.GoToAsync($"beardetails", navigationParameter);
public class BearDetailViewModel : IQueryAttributable, INotifyPropertyChanged
{
    public Animal Bear{ get; private set; }

    public void ApplyQueryAttributes(IDictionary<string, object> query)
    {
        Bear= query["Bear"] as Animal;
        OnPropertyChanged("Bear");
    }
    ...
}

I noted that my bindings and initialization code for the ViewModel kept being re-executed when I would Navigate Back to the parent page. It appears that any data that's passed as navigation parameters is retained in memory for the life of the page and is subsequently returned to that page during a "Navigate Back" operation. Overriding the back button operation with code that doesn't pass any parameters didn't change the result. Such as this:

    <Shell.BackButtonBehavior>
        <BackButtonBehavior
            Command="{Binding BackCommand, Mode=OneTime, Source={x:Reference self}}"/>
    </Shell.BackButtonBehavior>
        }
            this.BackCommand = new Command( this.OnBackPressed );
        }

        private async void OnBackPressed()
        {
            await Shell.Current.GoToAsync( ".." );
        }

Any attempt to clear or supply a different set of data via the method call simply serves to add to the already stored query data. Thus:

Shell.Current.GoToAsync( "..", new Dictionary<string, object> { {"w", new object()}  });

results in the "w" parameter being added to the current store of parameters. And now "w" and "Bear" are retained until the page is removed from the stack.

Maybe I'm used to external navigation paradigms but my presumption was that the data was transient for the life of the navigation operation only. Thus the retention of data was not expected. The docs fail to mention anything about the data being retained for the life for the page as well. Off the cuff this seems like a poor model as it increases memory footprint and possibly has unintended consequences if any system resources were shuttled as parameters.

Steps to Reproduce

Pass parameters to any page as you navigate down a hierarchy. When the back button is pressed the query data that was passed to any particular page on the initial navigation TO that page is returned via the IQueryAttributable interface.

Link to public reproduction project repository

https://github.com/bakerhillpins/Issues/tree/NetMauiIssue10294

Version with bug

6.0.486 (current)

Last version that worked well

Unknown/Other

Affected platforms

iOS, Android, Windows, macOS

Affected platform versions

Android 9.0, API 28

Did you find any workaround?

As the Query parameters are passed via IDictionary<string, object> query the author would need to manually cleanup individual resources with IDictionary<string, object>.Remove() or IDictionary<string, object>.Clear() in an implementation of the IQueryAttributable interface:

public class BearDetailViewModel : IQueryAttributable, INotifyPropertyChanged
{
    public Animal Bear{ get; private set; }

    public void ApplyQueryAttributes(IDictionary<string, object> query)
    {
        Bear= query["Bear"] as Animal;
        OnPropertyChanged("Bear");

        query.Clear();
    }
    ...
}

If one was using the Attribute:

[QueryProperty(nameof(Name), "name")]
[QueryProperty(nameof(Location), "location")]

they'd be forced to implement the interface to remove the data. Or, I suppose their property setters must qualify any update with a test for != or their bindings would re-execute when they returned to the page.

Relevant log output

No response

@bakerhillpins bakerhillpins added the t/bug Something isn't working label Sep 23, 2022
bakerhillpins added a commit to bakerhillpins/Issues that referenced this issue Sep 26, 2022
Code to reproduce 10294
dotnet/maui#10294
@bakerhillpins
Copy link
Author

I've added a link to a GitHub project/branch with code to demonstrate the issue.

Note that I'm not sure how this would end up behaving for Deep navigation (pushing multiple pages). It would seem that the workaround of clearing the query data would end up removing data required by lower/child pages. Thus you'd need to selectively remove data on each page. Which is less convenient.

@PureWeen
Copy link
Member

@bakerhillpins if I'm following you correctly this is by design

if you have 3 pages

page1 => page2 (with a query parameter) => page3

If the user hits the back button then they want to return to page2 with the state that page2 was at when they navigated away.

If you want page2 to change after the user has gone to page3 then I'd recommend wiring into the "NavigatingTo/From" events to modify page2 or you can clear out the query parameter

"..?Bear="

@PureWeen PureWeen added the s/needs-info Issue needs more info from the author label Sep 26, 2022
@ghost
Copy link

ghost commented Sep 26, 2022

Hi @bakerhillpins. We have added the "s/needs-info" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@bakerhillpins
Copy link
Author

bakerhillpins commented Sep 26, 2022

@PureWeen - I can't make sense of what you've written, sorry. I'll try to expand on what I'm reporting.

First:
Any objects added to the IDictionary parameters parameter of the GoToAsync calls, such as:

public Task GoToAsync(ShellNavigationState state, bool animate, IDictionary<string, object> parameters);`

are retained in memory with the page after the navigation operation completes. They are first wrapped in ShellRouteParameters and then eventually saved to the attached ShellContent.QueryAttributesProperty where they are retained until that page is removed. Therefore, the IDictionary object, and it's contents, share the lifetime of the page that you've navigated to. These parameters are not released until you pop that page from the nav stack. I don't think MAUI should assume that an author wants their business objects retained, what's the goal behind that?

Second:
As a result of the previous, performing back navigation results in the page receiving the same query parameters again. So:

  1. Page1 calls GoToAsync(Page2) with query parameter "MyBufferOfData".
  2. Page2 will receive "MyBufferOfData" in query parameter of IQueryAttributable implementation.
  3. Page2 calls GoToAsync(Page3) with NO query parameters
  4. Page3 will receive no items in the query parameter of IQueryAttributable implementation.
  5. Page3 pops of stack by call to GoToAsync("..") with NO query parameters
  6. Page2 will receive "MyBufferOfData" in query parameter of IQueryAttributable implementation. (the parameters set in step 1)
  7. Page2 pops of stack by call to GoToAsync("..") with NO query parameters
  8. Only now can the data associated with "MyBufferOfData" be garbage collected because the original Dictionary is released.
  9. Page1 will receive no items in the query parameter of IQueryAttributable implementation.

@ghost ghost added s/needs-attention Issue has more information and needs another look and removed s/needs-info Issue needs more info from the author labels Sep 26, 2022
@bakerhillpins
Copy link
Author

@PureWeen I've added code in the repro example to show the data being retained in the navigation hierarchy. As you navigate down it creates objects. You can force the GC to run but the objects are not released until you pop the page (navigate back).

@bakerhillpins
Copy link
Author

bakerhillpins commented Sep 26, 2022

If the user hits the back button then they want to return to page2 with the state that page2 was at when they navigated away.

So after sitting on this statement for a while this doesn't happen, because when I return to page 2 it is presented with the query data that was active when we navigated TO page2, not when we navigated away.

When we navigate to page 2 we present INITIAL data via the query dictionary. That state may be altered or deleted, or whatnot before we navigate to page3. So returning to page2 and being presented with the INITIAL query data again, doesn't retain page2's state when we navigated away, it changes it back to INITIAL conditions.

@mavispuford
Copy link

I've also been experiencing this issue. When I navigate to a page with a query dictionary, I expect it to pass those params a single time so I can consume them and throw them away. However, when I push a new page on the stack and then navigate back, those same params that I've already consumed are passed into ApplyQueryAttributes() as if I had just created them again. It's very confusing.

@PureWeen
Copy link
Member

I'm probably still missing something obvious here

Page1 -> Page2?ContactId=3 => Page3

If I'm on Page3 and hit the physical back button what query parameters should Page2 get?

@bakerhillpins
Copy link
Author

bakerhillpins commented Sep 29, 2022

If I'm on Page3 and hit the physical back button what query parameters should Page2 get?

Nothing...

But it get's ContactId=3

@PureWeen Have you tried the example project?

@rachelkang rachelkang added this to the Backlog milestone Sep 29, 2022
@ghost
Copy link

ghost commented Sep 29, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@rachelkang rachelkang added area-navigation NavigationPage area-controls-shell Shell Navigation, Routes, Tabs, Flyout labels Sep 29, 2022
@Kaiffa
Copy link

Kaiffa commented Oct 6, 2022

Same issue.
I agree with @bakerhillpins. The query parameters should be used only for the navigation where they are set explicitly and not for later back navigations.

@bakerhillpins
Copy link
Author

@Kaiffa Don't forget to remove/clear any passed objects at the end of the navigation ApplyQueryAttributes method (The workaround) if you are using it. MAUI is retaining them in the query dictionary and therefor your business object will NOT be garbage collected until after the page is popped from the stack.

@attributeyielding
Copy link

Any work around this issue ?, I have a slave page that return a bar code value to main page, if I scan code bar once and return that value to my main, that value still in the query, when I open my slave and close without scanning a bar code, the old value still in the memory, this behaviour is weird, main and slave are transient pages.

@attributeyielding
Copy link

attributeyielding commented Oct 21, 2022

A workaround for me : clear query dictionary after receiving it, I am able to do it in view model class that implement IQueryAttributable :

`public void ApplyQueryAttributes(IDictionary<string, object> query)
{
if (query.ContainsKey("querydataKey"))
{
//process here query["querydataKey"]

}

//clear the query to release the data
query. Clear();
}`

IQueryAttributable work nicely with ObservableObject ofCommunity toolkit MAUI mvvm
hope this helps some that tries to send data througth appShell page navigation !

@ToolmakerSteve
Copy link

@PureWeen

If the user hits the back button then they want to return to page2 with the state that page2 was at when they navigated away.

True, but what this issue shows is that "re-applying query parameters" does not restore that state (when navigated away), it restores the page's initial state.

Can a page on nav stack retain its "current" state, so it can show it again?

@Ghostbird
Copy link
Contributor

Ghostbird commented Jan 13, 2023

Personally I wasn't very surprised by this. It makes perfect sense if you're used to web development, and the MAUI AppShell navigation makes it abundantly clear.

If on the web I:

  1. Navigate to https://example.org/page?foo=bar
  2. Navigate to https://example.org/otherpage
  3. Navigate to the previous page, which is https://example.org/page?foo=bar

It makes perfect sense to me that if I navigate back, I navigate to the exact page I requested, including the query.
MAUI AppShell navigation clearly lets you register and navigate to pages as routes, which matches the style of routing used on the web.

What actually baffled me, is that the same also happens when you use PopModalAsync, which breaks the metaphor, and was unexpected to me. In my opinion the whole point of pushing a modal rather than navigating to a page is that you never navigate away from the underlying page, hence I did not expect any navigation events to fire when I popped the modal. But that's going too far off on a tangent.

@Ghostbird
Copy link
Contributor

Any work around this issue ?, I have a slave page that return a bar code value to main page, if I scan code bar once and return that value to my main, that value still in the query, when I open my slave and close without scanning a bar code, the old value still in the memory, this behaviour is weird, main and slave are transient pages.

I have this exact issue, but put the barcode scanner in a modal, for reasons explained above. However I'm not willing to clear my query attributes, because having them there makes perfect sense to me. I think ApplyQueryAttributes should not fire when I pop a modal.

@Ghostbird
Copy link
Contributor

I've made #12630 to talk about the modal specific use case.

@bakerhillpins
Copy link
Author

bakerhillpins commented Jan 13, 2023

Personally I wasn't very surprised by this. It makes perfect sense if you're used to web development, and the MAUI AppShell navigation makes it abundantly clear.

If on the web I:

1. Navigate to https://example.org/page?foo=bar

2. Navigate to https://example.org/otherpage

3. Navigate to the previous page, which is https://example.org/page?foo=bar

It makes perfect sense to me that if I navigate back, I navigate to the exact page I requested, including the query. MAUI AppShell navigation clearly lets you register and navigate to pages as routes, which matches the style of routing used on the web.

I hear you but I don't agree because I'm not actually "navigating" to a new page as you describe above. The Pop/Back operation doesn't take in a Uri, It simply removes a View from the z-order and returns us to the same page (same instance) we were viewing previously. Nothing new was created. I'm popping a view off the stack and I didn't apply any navigation parameters to that action. In your example you show navigation to page twice. There's 2 specific actions you've requested in there.

  • You're navigating to a new Page and thus new instance of the Page object. In MAUI this would result in a stack of /Page/OtherPage/Page. This issue discusses going from /Page/OtherPage back to /Page, so we're not increasing the z-order.
  • In both navigation steps you've supplied in the URL data to be sent into the page (?foo=bar). In this situation you've actually requested a different instance of the Bar object to be supplied to each navigation operation and that's not what's happening here. The pop action results in the same instances of data being delivered to the return operation that was applied when the view was initially navigated to.

@bakerhillpins
Copy link
Author

bakerhillpins commented Jan 13, 2023

Personally I wasn't very surprised by this. It makes perfect sense if you're used to web development, and the MAUI AppShell navigation makes it abundantly clear.

So I'm following up here because technically the my 1st bullet isn't drawing a proper analogy between the web example and MAUI. Though I used a navigation stack with multiple pages because that's what this issue is rooted in. Specifically, the web navigation pointed out by @Ghostbird would result in the following navigation stack changes on MAUI.

/Page to /OtherPage and then /Page.

At no point in that web example is a Navigation Stack built up. It's simply one page at a time at the root. And in that regard MAUI does work as he mentions. But the issue isn't about new pages at the root, it's when we move backwards through the navigation stack and so the web example isn't demonstrating the issue situation.

And this brings up an important point. MAUI and Web applications are different tech stacks and they behave differently because of that. Outwardly they may use some of the same concepts like Uri based navigation but effectively the similarities end there. To my knowledge (which is limited in web) web applications don't remember/retain previous views and in that regard are stateless. Each request is completely new and thus each new page served up as the only one in existence. In fact the web app isn't rendering the view, the browser is. Thus, we shouldn't confuse navigation between webpages and browser back button behavior. The browser after all isn't the web application, it's what we use to view state. The Back button is a feature of the browser and is simply saving the previous view (the actual instance) on a stack and pushing/popping those saved LOCAL states back to the user.

I've made #12630 to talk about the modal specific use case.

It's unfortunate that you've splintered the conversation to #12630 because the situation you describe there IS in fact identical to this issue in that the user is navigating backwards through the navigation stack.

The modal web example you provide is actually better aligned to what we have occurring in device based apps since the navigation is happening within the root view (it's instance isn't lost).

@ToolmakerSteve
Copy link

ToolmakerSteve commented Jan 14, 2023

MAUI and Web applications are different tech stacks and they behave differently because of that.

Lets be clear here. It isn't "MAUI" (in general) that is behaving this way. It is specifically "AppShell".

The behavior we see is specific to AppShell's design, which seems to me to be different than the mobile platform's native navigation paradigms. Its also different than Xamarin.Forms older navigation mechanism, NavigationPage.

begin rant

My view of AppShell is that its navigation paradigm is out in left field, for no good reason.

I'm not a fan. This unexpected behavior is one of many that can be seen by searching StackOverflow for questions tagged Xamarin.Forms or Xamarin, and mention Shell or AppShell. Quite a few app devs have struggled to get this to behave the way they need. Read those questions; it clearly does not match most people's intuitions about navigation.

By contrast, NavigationPage is a straightforward design. Straightforward, yet fully flexible, because it is easy to undersand what Nav stack is doing, and to programmatically manipulate it. AFAIK, never caused anybody serious problems.

Still works great today in Maui. Just change one line of code: MainPage = new NavigationPage(); in App.xaml.cs.

Of course you then don't have any of AppShell's features. Its a tradeoff.

end rant

@williambuchanan2
Copy link

@williambuchanan2 Have you looked at the workaround? You can call clear() on the dictionary passed in via IQueryAttributable

Hi, yes I looked at that, but then I had another strange problem. It looks like the ApplyQueryAttributes method was only ever being called once. This is a problem for me because the issue is on the way back. I.e. :
Page 1 - calls page 2
Page 2 - ApplyQueryAttributes
Page 2 - calls page 3
Page 3 - go back to page 2 - ApplyQueryAttributes not called.

So when page 3 goes back to page 2, I am unable to get the query attributes.

@bakerhillpins
Copy link
Author

It's difficult to say what's going on without seeing how your navigation code is setup. Do you have a repo/code you can point to?

@williambuchanan2
Copy link

It's difficult to say what's going on without seeing how your navigation code is setup. Do you have a repo/code you can point to?

I don't have one for this. To be honest i've reached the stage where i'm just looking for workarounds for all the problems now rather than reporting them.

@PureWeen
Copy link
Member

PureWeen commented May 7, 2023

Here's my first go at addressing some of the complaints here.

#14965

Let me know if you have any thoughts/suggestions/questions.
I think this will give the option to pivot between query parameters vs navigation only parameters depending on your scenario.

Ultimately, I'd like to add a richer set of interfaces, but I think this is a useful modification for NET8

@hartez hartez moved this from Todo to In Progress in MAUI SDK Ongoing May 9, 2023
@bakerhillpins
Copy link
Author

@PureWeen
As I can't comment in the PR-
What you have seems clear enough to me as far as the proposal goes.

For my part in all of this it still seems odd to retain any parameters since the uri that's being applied to navigate back via GotoAsync in your examples, specifically "..", doesn't actually contain any parameters. I guess I never felt comfortable with my understanding of what is/was the intended feature design to begin with. Was it to represent the application page Z-order within a URI or was it to behave like a browser back button combined with URIs.

@PureWeen
Copy link
Member

PureWeen commented May 11, 2023

@bakerhillpins

behave like a browser back button combined with URIs.

Basically this.

We're still discussing some other approaches for that PR. Possibly just adding a configuration property so you can disable the current behavior. We have to be careful about not breaking users that depend on aspects of this behavior (as confusing as it might be). The current PR was primarily proposing an approach with minimal breaking.

One additional piece of information here is that the BP being used for the parameters was for hot reload. There are scenarios where a page might get recreated by hotreload, so the parameters need to get re-applied. It's unfortunate that bleed into the actual behavior so much but that's the background here.

@TWTIC
Copy link

TWTIC commented Jun 25, 2023

It drove me crazy. Here is my solution:

`
// ViewModel:
[QueryProperty(nameof(Prop1), "Prop1")]
[QueryProperty(nameof(Prop2), "Prop2")]

[ObservableProperty]
item prop1;
item _LastProp1;

[ObservableProperty]
item prop2;
item _LastProp2;
...

protected override void OnPropertyChanged(PropertyChangedEventArgs e)
{
base.OnPropertyChanged(e);
if (e.PropertyName == "Prop1" && Prop1 != null && Prop1 != _LastProp1)
{
...
_LastProp1 = Prop1;
}
else if (e.PropertyName == "Prop2" && Prop2 != null && Prop2 != _LastProp2)
{
...
_LastProp2 = Prop2;
}
...
}
`

@Ghostbird
Copy link
Contributor

Ghostbird commented Jun 25, 2023

I think you mean that the ellipses inside the ifs stand for the code you want executed only on change? Then that implementation would make sense.
Would the approach I took (clear the dictionary on every navigation) have worked for you too?
An advantage of that approach is that you can omit the change detection code. A disadvantage is that you have to add more property set code. In this case when a new navigation to the same value happens explicitly, the property change is fired, even if the property is changed to the same value. This can be an advantage, or a disadvantage depending on your use case.

You'd get something like this:

public void ApplyQueryAttributes(IDictionary<string, object> query)
{
  if (query.TryGetValue(nameof(Prop1), out var newProp1)
  {
    Prop1 = newProp1;
  }
  // To clear all route data
  query.Clear();
}

@TWTIC
Copy link

TWTIC commented Jun 26, 2023

Yes, you are right, the effect would be the same.
The problem occurs with views where I call other views to select something and return it as a query property.
Since the query property survives, I have to mark it as used somehow, otherwise it will be processed again on each back navigation.
Since I always use the PropertyChanged method anyway, my solution is the best for me. I also hope that maybe in the future there will be a solution to mark Query Properties for a single use ...


// Some pseudo code
[QueryProperty(nameof(SelectedItem), "SelectedItem")]
public partial class OrderViewViewModel : BaseViewModel
{
        [ObservableProperty]
        Item selectedItem;
        Item _LastSelectedItem;
...
        [RelayCommand]
        async Task SelectItem()
        {
            await Shell.Current.GoToAsync(nameof(SelectItemView));
        }
...
        protected override void OnPropertyChanged(PropertyChangedEventArgs e)
        {
        base.OnPropertyChanged(e);
        if (e.PropertyName == "SelectedItem" && SelectedItem != null && SelectedItem != _LastSelectedItem)
        {
           doSomethingWithSelectedItem (SelectedItem);
          _LastSelectedItem = SelectedItem;
        }
...
}
...
public partial class SelectItemViewModel : BaseViewModel
{
...
        [RelayCommand]
        async Task OK()
        {
            await Shell.Current.GoToAsync("../", true,
                new Dictionary<string, object>
                {
                   {"SelectedItem",CollectionViewSelectedItem}
                }
                ); 
        }

        [RelayCommand]
        async Task Cancel()
        {
            await Shell.Current.GoToAsync("../");
        }


}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout area-navigation NavigationPage fixed-in-8.0.0-preview.7.8842 Look for this fix in 8.0.0-preview.7.8842! p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 s/needs-attention Issue has more information and needs another look t/bug Something isn't working
Projects
None yet