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

[Enhancement] Support IInitialize with Dialog ViewModel #3223

Open
YZahringer opened this issue Aug 14, 2024 · 9 comments
Open

[Enhancement] Support IInitialize with Dialog ViewModel #3223

YZahringer opened this issue Aug 14, 2024 · 9 comments
Labels
Commercial Plus 🚀 Issue or Pull Request opened by a Commercial Plus license holder enhancement

Comments

@YZahringer
Copy link

YZahringer commented Aug 14, 2024

Summary

In addition of the IDialogAware, provide a Dialog ViewModel initialization, similar of IInitialize and IInitializeAsync of page navigation.

API Changes

Add the equivalent of IInitialize & IInitializeAsync interfaces for the Dialogs.

public interface IDialogInitialize
{
    void Initialize(IDialogParameters parameters);
}

public interface IDialogInitializeAsync
{
    Task InitializeAsync(IDialogParameters parameters);
}

Intended Use Case

Support the synchronous and asynchronous initialization of a Dialog ViewModel.

@dansiegel
Copy link
Member

Please explain what the benefit / scenario is that you hope to solve here? I'm assuming you are used to Forms/MAUI Page Navigation hence the request. Dialogs and Page Navigation work differently. There is in fact no actual need for an "Initialize" interface.

  1. Dialogs are Navigated to once and then closed, while Pages can be Navigated to and then Navigated Back to...
  2. The existing API already initializes the Dialog before it is presented.

@YZahringer
Copy link
Author

Thanks for the feedback, @dansiegel. I understand the distinction between Dialogs and Page Navigation, but my suggestion is focused on supporting asynchronous initialization. The current IDialogAware only supports synchronous operations, which can be limiting for scenarios that require async tasks before displaying the Dialog.

Introducing IDialogInitializeAsync would address this need and create consistency with the IInitializeAsync interface used in Page Navigation.

Alternatively, reusing the IInitializeAsync interface for Dialogs could be considered, but it would introduce a breaking change, as the parameter type would need to change from INavigationParameters to IParameters.

@brianlagunas
Copy link
Member

There is a major drawback to an async interface when showing a dialog. If you make a call to a service or perform some other async operation, the Dialog will not show until that operation is completed, which can cause some very bad user experiences. Imagine clicking a button to show a dialog, but first a network call is made before the dialog is shown, there will be a noticeable delay from when the button was pushed to when the dialog was shown. Of course, the first instinct will be to blame Prism, when in actuality it's the async nature of the code.

We had this exact same problem when we provided support for the IInitializeAsync for navigation. Suddenly everyone's navigation started to lag and feeling "slow". Of course, Prism got the blame but turns out it was their code in the Async interface that was causing the issues because the navigation couldn't complete until their async code finished.

Why do you want to have the dialog wait on a task before opening? Why not show the dialog immediately and show a loading indicator when making any async calls you need. This way your UI stays very responsive.

@YZahringer
Copy link
Author

Thanks for the explanation. I understand the concern about delays with async dialogs, but since IInitializeAsync is already used in navigation, it’s about consistency. If we allow async initialization for navigation, it makes sense to offer the same for dialogs.

In my case, the async task is brief, and I currently have to use async void OnDialogOpened, which I want to avoid. I agree that long tasks should be handled with loading indicators, but having InitializeAsync for dialogs would provide a consistent pattern for developers to use when appropriate.

@brianlagunas
Copy link
Member

Trying to have consistency with patterns that are not recommended, and can actually be a trap, isn't a good reason to add a new API.

What is your main concern with async void? Is there technical concern or does it just make you feel weird because you always hear the "don't use async void" mantra?

@YZahringer
Copy link
Author

Trying to have consistency with patterns that are not recommended, and can actually be a trap, isn't a good reason to add a new API.

I totally agree, but then if the IInitializeAsync is not recommended, what is the alternative/recommendation? Fire and forget a Task in the constructor? Fire and forget a Task in the void Initialize?

What is your main concern with async void? Is there technical concern or does it just make you feel weird because you always hear the "don't use async void" mantra?

As mentioned, the Task I execute is very short, some milliseconds. So there is no noticeable delay on the user experience. In this case, it's better to load it before the view is displayed, that avoid a re-render in a very short time that can produce blinks in the view. And yes, if we can avoid the async void, that's a cosmetic extra point.

@brianlagunas
Copy link
Member

I totally agree, but then if the IInitializeAsync is not recommended, what is the alternative/recommendation? Fire and forget a Task in the constructor? Fire and forget a Task in the void Initialize?

Yes, async void is a perfectly valid paradigm. It's used in events and commands. I didn't even want to add the IInitializeAsync because what I feared would happen did indeed happen. And the same thing will happen with dialogs.

To be clear, I'm not against adding it, I'm just having a hard time being convinced it should be added.

Are you by chance a Commercial Plus license holder?

@YZahringer
Copy link
Author

YZahringer commented Aug 14, 2024

Are you by chance a Commercial Plus license holder?

Yes

@dansiegel dansiegel added the Commercial Plus 🚀 Issue or Pull Request opened by a Commercial Plus license holder label Aug 14, 2024
@brianlagunas brianlagunas removed the Commercial Plus 🚀 Issue or Pull Request opened by a Commercial Plus license holder label Aug 14, 2024
@brianlagunas
Copy link
Member

I can't find your account. Connect with us on our Discord to discuss.

@brianlagunas brianlagunas added the Commercial Plus 🚀 Issue or Pull Request opened by a Commercial Plus license holder label Aug 14, 2024
@dansiegel dansiegel changed the title [Enhancement] Support Dialog ViewModel initialization [Enhancement] Support IInitialize with Dialog ViewModel Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Commercial Plus 🚀 Issue or Pull Request opened by a Commercial Plus license holder enhancement
Projects
None yet
Development

No branches or pull requests

3 participants