[BUG] PopupService ignores the specified ViewModel instance

[elparasite]

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

When using the method ShowPopupAsync(viewmodel) from PopupService, PopupService ignores the specified ViewModel and raises an InvalidOperationException during ValidateBindingContext() if the constructor of the PopUp doesn't accept injection of the viewmodel the ViewModel.

Even if the constructor sets the view model, the specified view model won't be used.

Expected Behavior

The method ShowPopupAsync(viewmodel) should set the specified ViewModel before Validating the binding context.

Steps To Reproduce

1. Open and run sample project (android)
2. Click on "Open Popup"

Link to public reproduction project repository

https://github.com/elparasite/mauipopupserviceissue

Environment

- .NET MAUI CommunityToolkit: 7.0.1
- OS: Android API 26
- .NET MAUI: 8.0.7

Anything else?

No response

[bijington]

We've decided that this method was a bit of a mistake and will likely be removed from future versions. What you can do is follow the pattern from this example:

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

So it is up to you as a developer to set the BindingContext.

[dartasen]

@bijington Not sure to understand why it has to be removed.
What if you want to create a dynamic popup system, I don't want to create 20 ad-hoc viewmodels for each message I wanna display nor over-OOPING my viewmodels to implement a simple error message

[bijington]

@bijington Not sure to understand why it has to be removed.

What if you want to create a dynamic popup system, I don't want to create 20 ad-hoc viewmodels for each message I wanna display nor over-OOPING my viewmodels to implement a simple error message

You can achieve this through the onPresenting parameter. Something like

popuoService.Show<MessageViewModel>(viewModel => viewModel.Message = "This is how you do it");

[urielginsburg]

@dartasen I also ran into this and indeed the community toolkit code does not assign a viewmodel instance to the bindingcontext of the popup. I also agree this pattern seems convenient and would be nice to have it working. It can be made to work in this way:

MauiProgram.cs

builder.UseMauiCommunityToolkit()
builder.Services.AddTransientPopup<MyPopup, MyPopupViewmodel>();

MyPopup.xaml

<?xml version="1.0" encoding="utf-8" ?>
<toolkit:Popup xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
               xmlns:toolkit="http://schemas.microsoft.com/dotnet/2022/maui/toolkit"
               xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
               xmlns:local="clr-namespace:MauiApp2"
               x:Class="MauiApp.MyPopup">
    <!-- Instantiate the viewmodel -->
    <toolkit:Popup.BindingContext>
        <local:MyPopupViewmodel/>
    </toolkit:Popup.BindingContext>
    <VerticalStackLayout >
        <Label Text="{Binding Message}"/>
    </VerticalStackLayout>
</toolkit:Popup>

MyPopupViewmodel.cs

public class MyPopupViewmodel : INotifyPropertyChanged
{
    private string _message = string.Empty;

    public event PropertyChangedEventHandler? PropertyChanged;

    public string Message
    {
        get => _message;
        set
        {
            _message = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Message)));   
        }
    }

    public static async Task<object?> ShowAsync(string message, CancellationToken cancellationToken)
    {
        // This is for Windows. For other platforms, you're going to need different code e.g., for MacCatalyst
        // it's going to be IPlatformApplication.Current.Services , etc.
        var popupService = MauiWinUIApplication.Current.Services.GetRequiredService<IPopupService>();
        return await popupService.ShowPopupAsync<MyPopupViewmodel>(viewModel => viewModel.Message = message, cancellationToken);
    }
}

MyPage.xamls.cs

public partial class MainPage : ContentPage
{
    public MainPage()
    {
        InitializeComponent();
    }

    private async void OnButtonClicked(object sender, EventArgs e)
    {
        var res = await MyPopupViewmodel.ShowAsync("message", CancellationToken.None);
    }
}

@bijington
If this is going to be taken out of the framework, it may be a good idea to take it out of the documentation, or at least add a big warning about it there. That could probably save a lot of people a lot of time :)

[bijington]

@urielginsburg the documentation that you linked to doesn't refer to the method that will be removed. What it does do is refer to our example application that shows how to achieve your scenario. I understand that your solution works but it isn't very DI friendly, an alternative approach (based on our sample) would be to do the following):

MauiProgram.cs

builder.UseMauiCommunityToolkit()
builder.Services.AddTransientPopup<MyPopup, MyPopupViewmodel>();
builder.Services.AddTransient<MainPage>();

MyPopup.xaml

<?xml version="1.0" encoding="utf-8" ?>
<toolkit:Popup xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
               xmlns:toolkit="http://schemas.microsoft.com/dotnet/2022/maui/toolkit"
               xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
               xmlns:local="clr-namespace:MauiApp2"
               x:Class="MauiApp.MyPopup"
               x:DataType="local:MyPopupViewModel"> <!-- use this for better validation of your bindings and a slight performance boost -->
    <VerticalStackLayout >
        <Label Text="{Binding Message}"/>
    </VerticalStackLayout>
</toolkit:Popup>

MyPopup.xaml.cs

public partial class MyPopup : Popup
{
    public MyPopup(MyPopupViewModel viewModel) // Let DI create the view model instance for you
    {
        InitializeComponent();

        BindingContext = viewModel;
    }

    private async void OnButtonClicked(object sender, EventArgs e)
    {
        var res = await MyPopupViewmodel.ShowAsync("message", CancellationToken.None);
    }
}

MyPopupViewModel

public class MyPopupViewmodel : INotifyPropertyChanged
{
    private string _message = string.Empty;

    public event PropertyChangedEventHandler? PropertyChanged;

    public string Message
    {
        get => _message;
        set
        {
            _message = value;
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Message)));   
        }
    }
}

MainPage.xaml.cs

public partial class MainPage : ContentPage
{
    readonly IPopupService _popupService;

    public MainPage(IPopupService popupService)
    {
        InitializeComponent();
        _popupService = popupService;
    }

    private async void OnButtonClicked(object sender, EventArgs e)
    {
        var res = await _popupService.ShowAsync<MyPopupViewModel>(viewModel => viewModel.Message = "message");
    }
}

[urielginsburg]

@bijington I apologize for misunderstanding...
This section: https://learn.microsoft.com/en-us/dotnet/communitytoolkit/maui/views/popup#displaying-popups (which is under https://learn.microsoft.com/en-us/dotnet/communitytoolkit/maui/views/popup#ipopupservice) demonstrates using IPopupService.ShowPopup<TViewmodel>, which is virtually the same as ShowPopupAsync<TViewmodel> which is the OP's concern.

what method is going to be removed?

[bijington]

It's quite subtle but the OPs issue was with ShowPopup<TViewModel>(TViewModel viewModel); this allowed you to pass an instance of a view model into the popup service but because we aren't assigning it to the BindingContext it isn't helpful. This is the method that will be removed.

[Orgbrat]

@bijington There should be a way to pass in a preconfigured instance of the Popup viewmodel from the pages viewmodel so all the popup data can be set at display time instead of the popup having an empty Popup viewmodel.

[bijington]

@bijington There should be a way to pass in a preconfigured instance of the Popup viewmodel from the pages viewmodel so all the popup data can be set at display time instead of the popup having an empty Popup viewmodel.

That was my original intention but that functionality was removed before it merged. Does the onPresenting parameter not help here?

[Orgbrat]

@bijington It does not help if you need properties set inside the Popup constructor before it appears since the viewmodel injected is empty. I guess this is only a problem when opening from page viewmodel. But that is really where I open all my Popups not from inside the page code behind. The onPresenting parameter does work to get the data into the Popup veiwmodel but then you have to use the Popup Opened event to then set stuff from it's viewmodel.

[bijington]

@Orgbrat Sorry I don't follow why you need to use the Opened event, can't you use bindings to the view model? I'm keen to discuss so we make sure the toolkit offers enough

[Orgbrat]

@bijington Well I guess I was making it harder than I should have. Removed the Opened event and added a binding to the Popup Xaml top definition Size="{Binding PopupSize}" and it solved my issue and it also solved the discussion I opened "Windows Popup Location When Opened From ViewModel #1919". Although I am not sure why :)

Thanks for the conversation and wake-up of why you doing that....

[bijington]

Thank you for the discussion also. I'm glad you resolved the issue. As for the why I wonder if it is caused by the same issue as #1887 which appears to be a timing issue on Windows

[Orgbrat]

@bijington I know we talked about adding this popup service when it was just a thought from you. But I am in the process of migrating my custom popup DialogService to the CommunityToolkit.Maui PopupService so I will let you know if all the issues are handled and I hope this windows timing issue does not rear it's ugly head. Charge...

[bijington]

Best of luck!