[BUG] IPopupService.ShowPopup(new ViewModel) doesn't assign viewmodel to popup BindingContext

[GuidoNeele]

Is there an existing issue for this?

• I have searched the existing issues

Did you read the "Reporting a bug" section on Contributing file?

• I have read the "Reporting a bug" section on Contributing file: https://github.com/CommunityToolkit/Maui/blob/main/CONTRIBUTING.md#reporting-a-bug

Current Behavior

The viewmodel in this snippet of code is not assigned to the popup which you would expect if you create the viewmodel yourself.

Maui/src/CommunityToolkit.Maui/PopupService.cs

Lines 59 to 68 in d00c4a0

	public void ShowPopup<TViewModel>(TViewModel viewModel) where TViewModel : INotifyPropertyChanged
	{
	ArgumentNullException.ThrowIfNull(viewModel);
	var popup = GetPopup(typeof(TViewModel));
	ValidateBindingContext<TViewModel>(popup, out _);
	CurrentPage.ShowPopup(popup);
	}

Expected Behavior

When using ShowPopup(new ViewModel()) I expect the passed instance of ViewModel to be assigned to the BindingContext of the newly created popup.

Steps To Reproduce

1. Start new MAUI app and add reference to CommunityToolkit.Maui
2. Create a popup and corresponding viewmodel
3. Register popup and viewmodel
   PopupService.AddTransientPopup<MyPopup, MyViewModel>(services);
4. Show popup by calling IPopupService.ShowPopupAsync(new MyViewModel());
5. Verify that instance isn't used as the BindingContext of the popup.

Link to public reproduction project repository

https://github.com/GuidoNeele/mct_popupservice_issue

Environment

- .NET MAUI CommunityToolkit: 7.0.0
- OS: Windows 10 Build 10.0.19041.0
- .NET MAUI: 8.0

Anything else?

No response

[bijington]

@brminnick it only took 14 days for someone to request this 😂

#1165 (review)

[brminnick]

Hi @GuidoNeele! The good news is that you're experiencing the expected behavior; PopupService does not assign a Popup's BindingContext.

Looking at the PR you've submitted, you are assigning the Popup.BindingContext in ShowPopup(). I strongly disagree that ShowPopup() should do anything other than display the Popup on the screen; it shouldn't be modifying any of the properties inside Popup. For this reason, I've added the do not merge Do not merge this PR label to the PR for now.

When using ShowPopup(new ViewModel()) I expect the passed instance of ViewModel to be assigned to the BindingContext of the newly created popup.

Can you help me understand your expectation? Is it written in our documentation that PopupService assigns / overwrites Popup.BindingContext? Is there an example from a Dependency Injection library (eg Microsoft.Extensions.DependencyInjection) where it assigns property values when adding files to IServiceProvider? We could use leverage an existing implementation if one exists.

I strongly feel that it is out-of-scope for PopupService to overwrite the value of any properties in your Popup class.

That being said, I'm happy to be proven wrong. If there are existing examples where a Dependency Injection library has similar behavior, please do share them.

[GuidoNeele]

I'm sorry Brandon, but I feel your comment is a bit strange and trying to send me into the woods. It's clear by Shawn's previous comment that you already internally have discussed this or something like it. I somewhat understand what your stance is but I don't see any problem with the issue I opened or the pull request I proposed.

The main problem here is that the PopupService is exposing 2 methods which require newing up an instance of a view model. They are not asking for a Type, they are asking for an object. The question is how can I not expect that object to be attached to the popups BindingContext? Why else would I create that object?

[brminnick]

I'm sorry Brandon, but I feel your comment is a bit strange and trying to send me into the woods. It's clear by Shawn's previous comment that you already internally have discussed this or something like it. I somewhat understand what your stance is but I don't see any problem with the issue I opened or the pull request I proposed.

Yes, we did have a previous conversation, and I'm looking for more examples and evidence. The implementation in your PR breaks the Single Responsibility principle of ShowPopup(). We cannot move forward with that implementation. Additional examples + evidence will help us shape this conversation and a possible implementation.

The main problem here is that the PopupService is exposing 2 methods which require newing up an instance of a view model. They are not asking for a Type, they are asking for an object. The question is how can I not expect that object to be attached to the popups BindingContext? Why else would I create that object?

Quick correction - they don't require newing up an instance of a view model. You should only be using IPopupService.ShowPopup<TViewModel>(TViewModel viewModel); when you have access to the ViewModel instantiated by the IServiceCollection (ie only use is when the ViewModel is injected into the class via dependency injection).

Looking at your sample code, you are using the incorrect PopupService API. I've submitted a PR that demonstrates that you should be using IPopupService.ShowPopup<MyViewModel>() in your ContentPage: GuidoNeele/mct_popupservice_issue#1

I've annotated the code in my PR to help explain the differences between the two PopupService APIs.

[smalgin]

Is adding an IoC-style factory function for a popup configuration - also no-go?

Like this:

Task<object?> ShowPopupAsync<TViewModel>(Action<Popup, TViewModel> configure, 
CancellationToken token = default(CancellationToken)) where TViewModel : INotifyPropertyChanged;

[smalgin]

I just thought the reason I am asking is not clear...

When you run ShowPopupAsync() via IPopupService, you don't have access to the instance of the Popup. As such, you are crippled because you can't show popups that have to be closed by the caller. Use case: some background task wants to kill 'loading' popup spinner when its, in fact, done loading. No, adding 'All done please press one more button' is not a good UI.

[smalgin]

Example of the code that doesn't work

        static public async Task<LoadingPopupViewModel> CreateAndShow(string htmlText)
        {
            LoadingPopupViewModel model = null;
            await MainThread.InvokeOnMainThreadAsync(() =>
            {
                var popupService = IoC.Services.GetRequiredService<IPopupService>();

                //cannot block! We need a background task to complete before we close it!
                _ = popupService.ShowPopupAsync<LoadingPopupViewModel>(
                    onPresenting: (m) =>
                    {
                        m.HtmlText = "Custom loading cue";
                        //when is this going to happen? No idea.
                        model = m;
                    });
            });

            return model; //will return NULL because the lambda above wasn't executed yet!
        }

[bijington]

I just thought the reason I am asking is not clear...

When you run ShowPopupAsync() via IPopupService, you don't have access to the instance of the Popup. As such, you are crippled because you can't show popups that have to be closed by the caller. Use case: some background task wants to kill 'loading' popup spinner when its, in fact, done loading. No, adding 'All done please press one more button' is not a good UI.

You aren't crippled, we just haven't made that part any easier for you yet. You can follow this approach which fires an event:

https://github.com/CommunityToolkit/Maui/blob/main/samples/CommunityToolkit.Maui.Sample/Views/Popups/UpdatingPopup.xaml.cs

Note the above example also shows how to assign the BindingContext which I have seen you ask somewhere.

For the ability to close a Popup from the caller, we do have a PR open for that and are just working through testing it. This it the PR #1688