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

[WPF] Interactivity improvements #864

Closed
dvorn opened this issue Dec 1, 2016 · 41 comments
Closed

[WPF] Interactivity improvements #864

dvorn opened this issue Dec 1, 2016 · 41 comments
Labels

Comments

@dvorn
Copy link
Contributor

dvorn commented Dec 1, 2016

InteractionRequest feature in the current version of Prism is flawed to the extent that even the authors (@brianlagunas , @briannoyes) do not recommend to use it: PrismLibrary/Prism-Samples-Wpf#24

However, I think that interaction request feature is a great approach and the implementation has accumulated a lot of great ideas and it is possible to make it shine by some tweaking of the source code. Albeit this will be a breaking change. (How to handle this is a different matter - maybe supply a second alternative implementation under different namespace, or whatever.)

What I do not like:

(1) Built-in notification and confirmation windows are not really usable in real-world applications. The layout and styling of the windows do not fit the look and feel of the production application. In real-world production applications, only custom dialogs will be used. Thus, built-in windows are good only to create simple demos. I would not be disappointed missing the built-in windows entirely.

(2) In each invocation of InteractionRequest.Raise you need to create an instance of an intermediate class, Notification or Confirmation. This is annoying.

Note that actually Raise is passing the data in the event as yet another instance of on intermediate InteractionRequestedEventArgs.

One intermediate, transparent for the user, would be enough.

(3) The Confirmed member of IConfirmation/Confirmation is essentially not used. It is not used in the PopupWindowAction. Thus, it returns to the user unmodified, and the user is left to manage it by himself in user code.

It is used by built-in dialogs, but the user cannot reuse that functionality.

(4) The Notification/Confirmation classes are in fact view models/datacontext for the notification window. However, they have no INotifyPropertyChanged support which leads to classes such as https://github.com/PrismLibrary/Prism-Samples-Wpf/blob/master/State-Based%20Navigation/State-Based%20Navigation.Desktop/ViewModels/SendMessageViewModel.cs

The view model class derives from BindableBase and implement IConfirmation. It cannot derive from Confirmation.

(5) The architecture of a custom popup window is insane. See ItemSelection sample in Interactivity Quickstart.

Actually, two (!) view models/datacontexts are provided for the same window. One is https://github.com/PrismLibrary/Prism-Samples-Wpf/blob/master/InteractivityQuickstart/InteractivityQuickstart/Notifications/ItemSelectionNotification.cs and it is set as a DataContext for the entire popup window, and the other is https://github.com/PrismLibrary/Prism-Samples-Wpf/blob/master/InteractivityQuickstart/InteractivityQuickstart/ViewModels/ItemSelectionViewModel.cs which is set as a DataContext to the windows's Content. Obviously, the latter masks the former making it inaccessible for binding. And now the IInteractionRequestAware implemented by the latter view model provides Notification member which contains that masked data context...

(6) In the IInteractionRequestAware inteface the presence of Notification member looks very strange. The user has supplied the Notification in the Raise request. Why would he needed that notification in some backdoor when the notification is already at hand? Ah, the reason is (5)...

(7) The InteractionRequest stuff is all about MVVM. However, the implementation of default built-in windows and some samples (https://github.com/PrismLibrary/Prism-Samples-Wpf/blob/master/InteractivityQuickstart/InteractivityQuickstart/Views/CustomPopupView.xaml) use code-behind. Not a crime, but anyway...

@dvorn
Copy link
Contributor Author

dvorn commented Dec 1, 2016

What I suggest:

    public interface IInteractionRequest
    {
        event EventHandler<InteractionRequestedEventArgs> Raised;
    }

but InteractionRequestedEventArgs is different:

    public class InteractionRequestedEventArgs : EventArgs
    {
        public string Title { get; private set; }
        public object ViewModel { get; private set; }
        public Action Callback { get; private set; }
    }

and a change in Raise method in InteractionRequest

    public class InteractionRequest<T> : IInteractionRequest
    {
        public void Raise(string title, T viewModel, Action<T> callback) {...}
    }

The viewModel parameter is allowed to be null if there is no need to pass data from view model to popup window and ViewModelLocator is used for the view.

Other related interfaces:

    public interface INotification
    {
        object Content { get; set; }
    }
    public interface IConfirmation : INotification
    {
        bool Confirmed { get; set; }
    }
    public interface IInteractionRequestAware
    {
        Action FinishInteraction { get; set; }
    }

Note there is no Title in INotification and no Notification in IInteractionRequestAware.
Helper classes to create view models:

    public class NotificationBase : BindableBase, INotification, IInteractionRequestAware
    {
        public object Content { get; set; }
        public ICommand AcceptCommand { get; private set; }
        ...
    }

    public class ConfirmationBase : NotificationBase, IConfirmation
    {
        public object Content { get; set; }
        bool Confirmed { get; set; }
        public ICommand AcceptCommand { get; private set; }
        public ICommand CancelCommand { get; private set; }
        ...
    }

and of course the commands close the window by calling FinishInteraction.

Finally, PopupWindowAction. It remains mostly the same, but all stuff related to built-in notification and confirmation windows is removed. If non-null viewModel is passed in the Raised event, it is set as DataContext to the window's Content. When the window is closed, its DataContext is passed back via callback.

Basically, this is all what is needed for custom popup windows.

@dvorn
Copy link
Contributor Author

dvorn commented Dec 1, 2016

If we need to support built-in notification and confirmation popups (e.g., in order to keep Interactivity Quickstart alive), this can be added as follows:

Add DefaultConfirmation and DefaultNotification UserControls (markup only, no code behind). Those should be specified as PopupWindowAction.WindowContent in the parent view.

Add extension methods to be used in view models

public static void RaiseNotification(this InteractionRequest<INotification> request, string title, string message, Action callback) {...}
public static void RaiseConfirmation(this InteractionRequest<IConfirmation> request, string title, string message, Action<bool> callback) {...}

The extension methods will create an instance of NotificationBase/ConfirmationBase and pass it to ordinary Raise.

@dvorn
Copy link
Contributor Author

dvorn commented Dec 1, 2016

If there is interest in this approach I can create a PR (as time permits).

@briannoyes
Copy link
Contributor

Hi @dvorn, you are clearly passionate about this! :)
I agree the original implementation left a lot to be desired, which is part of the reason I have rarely used it. I think the original intention was that the Confirmation and Notification classes would be used as a base class for the ViewModel class of a custom dialog that could add on to that for whatever other content the dialog was going to have, but again have not used enough or even focused on it in a long time to defend it as is or how it is supposed to work. I'll discuss with @brianlagunas and decide what we want to do here and get back to you.

@brianlagunas
Copy link
Member

I would like to greatly simplify the entire process of showing dialogs. As it is now, using the Interactionrequest is way too complicated. It is so much easier to use a simple IDialogService. I think we can add a new and improved implementation and just use a different name. Maybe DialogRequest and ShowDialogAction, or something like that. Then deprecate the old way in favor of the new way. The new way needs to be stupid simple and not require a lot of plumbing to get it to work.

@brianlagunas
Copy link
Member

I am linking #430 into this thread to keep track of the requested scenario.

@dvorn
Copy link
Contributor Author

dvorn commented Feb 28, 2017

I created a 0.5 version along the lines described above. It can be found in the following branch:

https://github.com/dvorn/Prism/tree/ShowDialogRequest

The library code is in

https://github.com/dvorn/Prism/tree/ShowDialogRequest/Source/Wpf/Prism.Wpf/ShowDialog

Sample application (adopted from existing demo for InteractionRequest) here:

https://github.com/dvorn/Prism/tree/ShowDialogRequest/Sandbox/28-CustomRequest

Some design work (e.g. extensibility) is pending. Also, comments, unit tests, etc.

I welcome reviews/discussion/suggestions/criticism. In particular, names, namespaces, folders, etc.

@noufionline
Copy link
Contributor

Hi, @dvorn Will this feature be part of Prism 7.0?

@dvorn
Copy link
Contributor Author

dvorn commented Nov 15, 2017

@noufionline This is a question for @brianlagunas, but seemingly not.

However, the ShowDialog code is independent of Prism and you can grab the source code and include it in your application.

Decision to include this feature into Prism depends, among other things, on the users activity and their requests for the feature. Thus, give it a try and tell us what you think.

@brianlagunas
Copy link
Member

This will not make it into v7.0. I would actually like to remove the requirement of the Interactivity dependency altogether and move to more of an IDialogService that is called from within a ViewModel. I really hate how much ceremony is involved in creating a simple dialog with the current implementation.

@brianlagunas brianlagunas added this to the Prism 7.0 for WPF milestone Jan 2, 2018
@pdinnissen
Copy link
Contributor

pdinnissen commented Jan 16, 2018

The following is one of the simplest solutions to a DialogService that I have found:
https://github.com/FantasticFiasco/mvvm-dialogs
It is WPF & UWP and all you have to do is supply a ViewModel and it uses a locator to find the View.

@brianlagunas
Copy link
Member

brianlagunas commented Jan 16, 2018

This is something that I will have to think about, but the improved API will not require the creation of a ViewModel in order to show a dialog. This is a coupling that is not recommended by Prism guidance. Instead it will follow current established patterns within Prism. Thanks for the link though. We may be able to use some ideas from there..

@dvorn
Copy link
Contributor Author

dvorn commented Jan 17, 2018

@brianlagunas Could you elaborate on this one please:

the improved API will not require the creation of a ViewModel in order to show a dialog. This is a coupling that is not recommended by Prism guidance.

View model is required for everything more complicated than a simple MessageBox. The sample 28-CustomRequest uses ItemSelectionViewModel. When the dialog is raised, indeed, the view model is not supplied. Instead, the MainWindow contains ItemSelectionView using Interactivity (which should be avoided) and ItemSelectionViewModel is auto-wired to it.

If there is no Interactivity and no view model, what is the way to go?

@brianlagunas
Copy link
Member

What I mean is that the API will not require a ViewModel type different than the calling VM. For example

private void ShowDialog()
    {
        var dialogViewModel = new AddTextDialogViewModel(); // NOT ALLOWED in Prism

        bool? success = dialogService.ShowDialog(this, dialogViewModel);
        if (success == true)
        {
            Texts.Add(dialogViewModel.Text);
        }
    }

Or even

_dialogService.Show<SomeViewModelForTheDialog>();

Instead, it should follow the current navigation patterns such as

_dialogService.Show("NameOfDialog", parameters)

Not saying that is the API, just trying to clarify my original statement.

@dvorn
Copy link
Contributor Author

dvorn commented Jan 17, 2018

Assuming that it was preceded by something like

_dialogService.RegisterDialog("NameOfDialog", DialogViewType, DialogViewModelType) ?

@brianlagunas
Copy link
Member

brianlagunas commented Jan 17, 2018

Yeah, I can see the need to register custom dialogs with the service somewhere in the module or boostrapper/Prism application class.

I haven't really put much thought into this yet. I have been slammed with other stuff, but this is definitely on my radar for the v7 release for WPF.

@pdinnissen
Copy link
Contributor

The prospect of having such a DialogService built into Prism is very enticing.

I think they'll still need to be some type of IDialogAware interface for the ViewModel in order to accept the parameters/payload (and use that same class to have data returned. ShowDialog would then need to return a "DialogResult" composed of this type and the actual Nullable dialog result. It would also be nice to move away from using "object" for the payload and use a template to be strongly typed. However about something like this:

    public interface IDialogAware<T>
    {
        void OnDialogShow(T parameters);
        T OnDialogFinish();
    }

    public class DialogResult<T>
    {
        public bool? Result { get; }
        public T Parameters { get; }
    }

And then to be consistent, the registering of the dialog should also contain the type of the parameters and shouldn't the registering also be made on the IoC Container?:

_container.RegisterTypeForDialog<DialogViewType, DialogViewModelType, DialogParameterType>("NameOfDialog");

A check could then be done at registration to ensure that the ViewModelType implements IDialogAware.

@dvorn
Copy link
Contributor Author

dvorn commented Jan 22, 2018

The DialogService may reuse some infrastructure of navigation system.

When _dialogService.Show("NameOfDialog", parameters) is called, the service may create a view by instantiating a type registered for NameOfDialog, as it does for navigation. Then create a window and set its Content to the created view. The view may use AutoWire to create and set the view model. Then, if the view (or view model) implements INavigatipnAware, pass the parameters to the view via OnNavigatedTo.

Properties like IsModal, WindowStartupLocation, etc. could be predefined parameters.

Dialog windows are a kind of special case of a predefined Region.

@brianlagunas
Copy link
Member

@dvorn That is a great idea. I like the way you're thinking. I'm wondering if we could just reuse the NavigationParameters instead of using a DialogResult<T>, where T is just some random parameters type. This would also provide a better mechanism for providing multiple parameter to control window properties.

How ever, I wonder how we could control the dialog parent (maybe we always use the active window). Also, I would hate to provide a mapping for each parameter to control specific properties (WindowStartupLocation, etc). I wonder what we can do here. Some type of implicit style?

@dvorn
Copy link
Contributor Author

dvorn commented Jan 22, 2018

Yes, DialogResult is not needed. When the dialog is closed, OnNavigatedFrom method is called and the dialog may provide whatever result it wants in NavigationParameters. _dialogService.Show, as well as RequestNavigate, may return NavigationResult which contains updated parameters as well as Result property which may be reused.

@noufionline
Copy link
Contributor

Is this feature really going to be part of 7.0 release?

@brianlagunas
Copy link
Member

That's what I'm hoping.

@bzuillsmith
Copy link

bzuillsmith commented Apr 6, 2018

I implemented something like this for my application. I created a WindowNavigationService that would open up a new window, set a localized RegionManager to the window, and it also set a localized WindowManager to the window. This WindowManager would could be given to anything that implemented IWindowNavigationAware and allowed a view model in the window to call WindowManager.ShowDialog(parameters) and WindowManager.GoBack(optional object) ( GoBack is for closing the current window the that view model was open in).

My WindowNavigationService keeps track of the windows opened and their associated WindowManager so that it knows which window to close based on the WindowManager instance that called GoBack().

For things like the window size, I used attached properties on the injected view that I can grab during the Window.SourceIntialized event and apply sizing to the window. That helps keep view things in the XAML. I also try to set the WindowStartupLocation in SourceInitialized but I can't recall if that works properly.

Let me know if you'd be interested in looking at some of the code, I could probably throw it in simple project

@brianlagunas
Copy link
Member

@bzuillsmith Always interested in seeing code. How do you deal with substituting your own custom window type, or do you handle that yet?

@bzuillsmith
Copy link

I don't handle that, no. All we needed was a single generic Window to throw content into so I haven't made it flexible in that way. I imagine if you need different window types, you'd have to start registering them ahead of time which my application currently doesn't do.

@bzuillsmith
Copy link

bzuillsmith commented Apr 6, 2018

I'll see if I can get a cleaned up sample this evening or weekend into a repository.

@brianlagunas
Copy link
Member

That would be great. No hurry, I am still swamped with my day job and working on aligning modularity across all 3 platforms.

@bzuillsmith
Copy link

Here is rough sample of what we are doing right now in our application. https://github.com/bzuillsmith/PrismWindowNavExample

@noufionline
Copy link
Contributor

@brianlagunas Is it possible to skip this feature for the time being and release a stable version? It will be a great help for us to move on.

@korchak-aleksandr
Copy link

@brianlagunas Is it possible to release a stable version? Or any planned date of release?

@brianlagunas
Copy link
Member

@sanekpr If you noticed, we just released our final preview or Prism 7.1. More testing is needed for WPF before we can release. Since the community is not providing much feedback on our previews, it places that testing on the team which takes much longer. It would be great if you started testing the latest preview in your applications and let us know if everything still works :)

@noufionline
Copy link
Contributor

@brianlagunas ever since you release the preview of PrismApplication class I am using prism in production and till date I didnt face any issues. It looks great 👍

@brianlagunas brianlagunas removed this from the Prism 7.0 for WPF milestone Sep 24, 2018
@steji113
Copy link

@brianlagunas did new interactivity enhancements make it into 7.1?

@brianlagunas
Copy link
Member

@steji113 no.

@DanoPTT
Copy link

DanoPTT commented Oct 18, 2018

Is plan to implement something like lazy loading. Actual implementation loads View and its' ViewModel (Custom interaction used) when parent view is created even when no interaction is ever activated (no editing for example). It slows down parent View loading.

@brianlagunas
Copy link
Member

@DanoPTT I'm not sure I understand the question. Any new dialog service will not interfere with the parent view loading

@wldevries
Copy link
Contributor

It would be great if this new dialog solution would also support non-Windowbased solutions. We currently have a custom implementation that adds dialog views in a separate adorner layer in our Shell to allow easier overlay effects (darkening) and clicking besides the dialog to dismiss it. I could imagine this working with a modified IRegion and IActivatable views.

    <Grid>
        <AdornerDecorator>
            <ContentControl prism:RegionManager.RegionName="{x:Static interface:RegionNames.Page}" />
        </AdornerDecorator>
        <AdornerDecorator>
            <Grid x:Name="dialogRoot" />
        </AdornerDecorator>
        <c:ToastView />
    </Grid>

@brianlagunas
Copy link
Member

brianlagunas commented Jan 17, 2019

I've started working on an initial attempt at a new DialogService that does not rely on any interactivity references.

You can check it out here: https://github.com/PrismLibrary/Prism/tree/Interactivity-Improvements/Source/Wpf/Prism.Wpf/Services/Dialogs

@dvorn what do you think of this direction?

@steji113
Copy link

@brianlagunas FWIW, I really like where this is headed. I am currently embarking on refactoring a large WPF project that has tons of close coupling of opening dialogs. In the past we have had to roll our own dialog services due to the interactivity interfaces not being scalable. This would be a great addition and help.

@brianlagunas
Copy link
Member

I'm closing this as I have created a new issues specifically for the new IDialogService. See #1666

@lock
Copy link

lock bot commented Jan 28, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests