Skip to content

Add Support for Shell + IQueryAttributable for PopupV2 #2661

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

Conversation

TheCodeTraveler
Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler commented May 9, 2025

Description of Change

This PR augments #1581, adding support for Shell and IQueryAttributable. I.e., this PR will be merged into the open PopupV2 PR, not the main branch.

Specifically, this PR was created in response to @bijington's review noting that users will no longer be able to pass data from ViewModel to ViewModel after removing the onPresenting parameter: #1581 (comment)

Additional information

This PR is currently in Draft because it still requires Unit Tests.

I updated SimplePopup and CsharpBindingPopup to demonstrate using IQueryAttributable in the Sample App.

@TheCodeTraveler TheCodeTraveler requested a review from bijington May 9, 2025 22:16
@bijington
Copy link
Contributor

bijington commented May 10, 2025

@TheCodeTraveler I love this! I hope you don't mind I pushed some XML docs improvements and will follow up with more on the other public areas if that's ok? Also I added this as documentation for the CancellationToken parameter on the ShowAsync methods:

/// <param name="token">A <see cref="CancellationToken"/> providing support for canceling the wait for a result to be returned. This will <b>not</b> close the popup.</param>

Is this correct (I haven't tested it)? Does the cancel only bail out of awaiting the ShowAsync call and does not close the popup?

The only question I will ask (and I am happy to be shot down) is whether we could/should include something similar for non-Shell based navigation? Basically including something like the following into the INavigation based extension methods:

if (view is IQueryAttributable queryAttributable)
{
    queryAttributable.ApplyQueryAttributes(shellParameters);
}
else if (view.BindingContext is IQueryAttributable queryAttributableBindingContext)
{
    queryAttributableBindingContext.ApplyQueryAttributes(shellParameters);
}

I understand that IQueryAttributable is related to Shell and is promoted by MS but if there are people out there using INavigation they could also benefit. I am also happy to defer this as a possible improvement if people ask. What do you think?

@TheCodeTraveler TheCodeTraveler marked this pull request as ready for review May 10, 2025 20:16
@TheCodeTraveler
Copy link
Collaborator Author

TheCodeTraveler commented May 10, 2025

Ok, @bijington - I've added Unit Tests, squishing some bugs along the way, and this is now ready for review!!

The only question I will ask (and I am happy to be shot down) is whether we could/should include something similar for non-Shell based navigation? Basically including something like the following into the INavigation based extension methods

We totally could and it'd be fairly easy too implement based on how I've designed PopupPage. In short, the .ShowPopup() methods that extend INavigation would just add a IDictionary<string, object>? parameter and pass it into their initialization of new PopupPage(View, IPopupOptions, IDictionary<string, object>? ).

However, I see a few faults with adding support for IQueryAttributable for non-Shell Popup extension methods:

  1. IQueryAttributable is only used by MAUI when Shell.Current is not null
    • i.e. You can only use IQueryAttributable if you're using Shell
  2. Behind the scenes, Shell.GotoAsync() invokes IQueryAttributable on the BindingContext automatically for us
    • We would need to add logic to implement this manually to support users who are not using Shell
    • It's doable, but it's a bit messy and requires additional unit tests
  3. The whole philosophy behind Popup v2 is to move away from our custom implementation and instead use the underlying MAUI architecture for Popup
    • If we add IQueryAttributable for non-Shell apps, this breaks the paradigm we aim to achieve in Popup v2

All that said, let's not add IQueryAttributable support for non-Shell apps now. And if/when users start asking for the feature, we can circle back to this conversation!

Copy link
Contributor

@bijington bijington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @TheCodeTraveler! I just have one question which more to help me understand the change rather than question whether we need it.

@TheCodeTraveler TheCodeTraveler merged commit 26a56bb into feature/sl/remove-popup-constraint-from-popup-service May 11, 2025
2 checks passed
@TheCodeTraveler TheCodeTraveler deleted the Add-Shell-Routing-to-Support-`IQueryAttributable` branch May 11, 2025 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants