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

ApplyQueryAttributes is called with old parameters #17976

Open
Telison opened this issue Oct 12, 2023 · 6 comments
Open

ApplyQueryAttributes is called with old parameters #17976

Telison opened this issue Oct 12, 2023 · 6 comments
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout platform/android 🤖 t/bug Something isn't working
Milestone

Comments

@Telison
Copy link

Telison commented Oct 12, 2023

Description

If you navigate to a page with Shell.Current.GoToAsync() and include a navigation parameter, like //page1?param1=value1, this parameter will be replayed in future navigations to the page, when navigating from the shell, with no parameter.

Other reports have discussed similar issues, related to back navigation and modal pages, but this one is even simpler and seems like an obvious bug.

I have verified that this issue exists in the .Net 8 RC2 version, but also in .Net 7. I have only tested with the Android emulator from Visual Studio in Windows.

Steps to Reproduce

Create a MAUI app with several pages defined in AppShell, enable flyout navigation:

<ShellContent Title="Page1" ContentTemplate="{DataTemplate local:Page1}" Route="Page1" />
<ShellContent Title="Page2" ContentTemplate="{DataTemplate local:Page2}" Route="Page2" />
<ShellContent Title="Page3" ContentTemplate="{DataTemplate local:Page3}" Route="Page3" />

In one of the pages, add code to navigate to another page with a parameter:

private async void Button_OnClicked(object sender, EventArgs e)
{
    await Shell.Current.GoToAsync("//Page1?from=page2");
}

Implement IQueryAttributable.ApplyQueryAttributes on the destination page

When performing the GoToAsync navigation, ApplyQueryAttributes is called with the parameter as expected. Now after this step, you can do any number of navigations from the Shell, by clicking the flyout items. But whenever you click this one:

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

...the parameter that was used in GoToAsync previously will be replayed in ApplyQueryAttributes! Obviously it is expected that ApplyQueryAttributes should be called without any parameters (or possibly not be called at all, but that would also be a bit unexpected)

One interesting note. Before GoToAsync has been called, any navigation to Page1 from the shell will not call IQueryAttributable.ApplyQueryAttributes, but as soon as GoToAsync has been called once, future navigations to Page1 from the shell will call IQueryAttributable.ApplyQueryAttributes with the "old" parameter.

Link to public reproduction project repository

https://github.com/Telison/ApplyQueryAttributesIssue

Version with bug

8.0.0-rc.1.9171

Is this a regression from previous behavior?

No, this is something new

Last version that worked well

Unknown/Other

Affected platforms

Android, I was not able test on other platforms

Affected platform versions

Tested on Android 12.1, API 32

Did you find any workaround?

I have not found a workaround. Using QueryProperty suffers from the same problem, old navigation parameters are set.

Relevant log output

No response

@Telison Telison added the t/bug Something isn't working label Oct 12, 2023
@CheetoVendor
Copy link

CheetoVendor commented Oct 13, 2023

I'm tired and still newer to .net maui, so if I'm misunderstanding, I apologize.
This is what I did to stop it from querying that parameter again.

if query has key
{
do your data stuff
remove key from query.
}

It's a temporary workaround and works for what I'm doing with it. It'll still run "applyqueryattributes", iirc, but at least it wont mess with your stuff

@mattleibow mattleibow added the area-controls-shell Shell Navigation, Routes, Tabs, Flyout label Oct 13, 2023
@PureWeen
Copy link
Member

@Telison if you use pass the parameters via ShellNavigationQueryParameters does that fix your issue?

#15585

@PureWeen PureWeen added the s/needs-info Issue needs more info from the author label Oct 14, 2023
@ghost
Copy link

ghost commented Oct 14, 2023

Hi @Telison. 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.

@Telison
Copy link
Author

Telison commented Oct 16, 2023

Using ShellNavigationQueryParameters does work as expected, so it is an acceptable workaround.

In the unlikely event that you take this as a reason to close this item, let me preemptively argue against that :) We now have these three flavors of GoToAsync

1 Shell.Current.GoToAsync("//Page1?from=page2");

2 Shell.Current.GoToAsync("//Page1",
  new Dictionary<string, object>{ ["from"] = "page2" });

3 Shell.Current.GoToAsync("//Page1",
  new ShellNavigationQueryParameters { ["from"] = "page2" });

Number three works as expected, number one and two suffer from the bug, where the parameter will be present later even when navigating without it. This cannot be by design, or if it is the designers may be crazy ;)

Also, check my last paragraph in the original report, that may be part of the bug.

@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 Oct 16, 2023
@mattleibow mattleibow removed the s/needs-attention Issue has more information and needs another look label Oct 24, 2023
@mattleibow mattleibow added this to the Backlog milestone Oct 24, 2023
@ghost
Copy link

ghost commented Oct 24, 2023

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.

@PureWeen
Copy link
Member

Completely ignoring the extra parameters APIs for a second

The idea of Shells "routes" is that those routes represent a way to rebuild that entire page purely from the string. For example, if the app completely goes away you could replay your entire navigation stack (plus query parameters) just from the URI string.

Shell was aggressively built around the idea of deep linking and that the routes could be replayed to recreate the entire navigation stack with parameters. This also had implications around hot reload.

For example, you can route like this to page parameters to middle routes

//root/page1/page2?page1.param="cat"

I would call most of that strategy fairly half baked, and not something that was fully realized correctly.

When we added the Dictionary overload in NET6 we should have made it so those acted like temporary parameters because that overload fundamentally goes against everything I said above.

We can't really just change this behavior outright because this is how Shell has worked since it was created so it would break too many users.

It'd be good to add better API docs and then at least obsolete the Dictionary API version here so we can at least reduce that confusion.

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 platform/android 🤖 t/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants