Implement DialogView for Uap #3074

andres-gimenez · 2018-08-20T15:14:58Z

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

New feature. Implement ModalView for Uap

I haven't been able to prove it well, because the Playground.Uwp project doesn't start.

⤵️ What is the current behavior?

🆕 What is the new behavior (if this is a feature change)?

💥 Does this PR introduce a breaking change?

🐛 Recommendations for testing

📝 Links to relevant issues/docs

🤔 Checklist before submitting

All projects build
Follows style guide lines (code style guide)
Relevant documentation was updated (docs style guide)
Rebased onto current develop

martijn00 · 2018-08-20T15:49:47Z

@nickrandolph can you have a look at why the playground doesn't start?

nmilcoff · 2018-08-20T15:52:53Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

@@ -23,6 +23,9 @@ public class MvxWindowsViewPresenter
    {
        protected readonly IMvxWindowsFrame _rootFrame;

+        private MvxWindowsContentDialog mDialogView;


Can we avoid by any chance holding a strong reference to the dialog view here?

I don't know how to do it. You have access to the ViewModel, but not to the View.

Doesn't follow the naming conventions we use.

andres-gimenez · 2018-08-20T16:07:23Z

To run the Playground, throw this exception. I don't now if this error is reported.

MvvmCross.Exceptions.MvxException
HResult=0x80131500
Message=Problem navigating to ViewModel RootViewModel
Source=MvvmCross
StackTrace:
at MvvmCross.ViewModels.MvxAppStart`1.d__1.MoveNext() in D:\Apl\Xamarin\MvvmCross\MvvmCross\MvvmCross\ViewModels\MvxAppStart.cs:line 83
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
at MvvmCross.ViewModels.MvxAppStart.d__5.MoveNext() in D:\Apl\Xamarin\MvvmCross\MvvmCross\MvvmCross\ViewModels\MvxAppStart.cs:line 43
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
at MvvmCross.ViewModels.MvxAppStart.Start(Object hint) in D:\Apl\Xamarin\MvvmCross\MvvmCross\MvvmCross\ViewModels\MvxAppStart.cs:line 28
at MvvmCross.Platforms.Uap.Views.MvxApplication.RunAppStart(IActivatedEventArgs activationArgs) in D:\Apl\Xamarin\MvvmCross\MvvmCross\MvvmCross\Platforms\Uap\Views\MvxApplication.cs:line 72
at MvvmCross.Platforms.Uap.Views.MvxApplication.OnLaunched(LaunchActivatedEventArgs activationArgs) in D:\Apl\Xamarin\MvvmCross\MvvmCross\MvvmCross\Platforms\Uap\Views\MvxApplication.cs:line 46

Inner Exception 1:
MvxException: Failed to construct and initialize ViewModel for type Playground.Core.ViewModels.RootViewModel from locator MvxDefaultViewModelLocator - check InnerException for more information

Inner Exception 2:
MvxException: Problem running viewModel lifecycle of type RootViewModel

Inner Exception 3:
NullReferenceException: Object reference not set to an instance of an object.

Cheesebaron · 2018-08-21T06:03:23Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

@@ -23,6 +23,9 @@ public class MvxWindowsViewPresenter
    {
        protected readonly IMvxWindowsFrame _rootFrame;

+        private MvxWindowsContentDialog mDialogView;


Doesn't follow the naming conventions we use.

Cheesebaron · 2018-08-21T06:04:37Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

+            try
+            {
+                var modalAttribute = (MvxModalViewPresentationAttribute)attribute;
+                var instanceReques = (MvxViewModelInstanceRequest)request;


Move this cast closer to usage. Also typo.

Cheesebaron · 2018-08-21T06:06:03Z

MvvmCross/Platforms/Uap/Views/MvxWindowsContentDialog.cs

+        {
+            get
+            {
+                return _viewModel;


Can you please use Expression body

Cheesebaron · 2018-08-21T06:06:20Z

MvvmCross/Platforms/Uap/Views/MvxWindowsContentDialog.cs

+    {
+        public new TViewModel ViewModel
+        {
+            get { return (TViewModel)base.ViewModel; }


Expression body

Changes are implemented

martijn00 · 2018-08-22T07:47:26Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

@@ -240,5 +250,39 @@ protected virtual Task<bool> ShowPage(Type viewType, MvxBasePresentationAttribut
                return Task.FromResult(false);
            }
        }
+
+        protected virtual async Task<bool> ShowModal(Type viewType, MvxBasePresentationAttribute attribute, MvxViewModelRequest request)


Should you pass in MvxModalViewPresentationAttribute as type?

martijn00 · 2018-08-22T07:47:38Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

+            {
+                if (Activator.CreateInstance(viewType) is MvxWindowsContentDialog modalView)
+                {
+                    var instanceReques = (MvxViewModelInstanceRequest)request;


martijn00 · 2018-08-22T07:48:29Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

@@ -23,6 +23,8 @@ public class MvxWindowsViewPresenter
    {
        protected readonly IMvxWindowsFrame _rootFrame;

+        private MvxWindowsContentDialog _windowsContentDialog;


I don't think it is good to keep an instance reference like this. We probably need something to find the view back in the stack.

martijn00 · 2018-08-22T07:49:07Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

+
+        protected virtual Task<bool> CloseModal(IMvxViewModel viewModel, MvxBasePresentationAttribute attribute)
+        {
+            _windowsContentDialog?.Hide();


Is Hide correct, or do we really need to remove it as well?

I think it's the right one. PrimaryButtonCommand can be invoked, but it's rare.

martijn00 · 2018-08-22T07:50:07Z

MvvmCross/Platforms/Uap/Presenters/Attributes/MvxModalViewPresentationAttribute.cs

+
+namespace MvvmCross.Platforms.Uap.Presenters.Attributes
+{
+    public sealed class MvxModalViewPresentationAttribute : MvxBasePresentationAttribute


Don't make it sealed

martijn00 · 2018-08-23T08:51:55Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

+               typeof(MvxModalViewPresentationAttribute),
+               new MvxPresentationAttributeAction
+               {
+                   ShowAction = (view, attribute, request) => ShowModal(view, (MvxModalViewPresentationAttribute)attribute, (MvxViewModelInstanceRequest)request),


Should not cast to MvxViewModelInstanceRequest here, but in the method

I do not understand why attribute yes and request no. What criterion do you have?

Consistency is the criteria: https://github.com/MvvmCross/MvvmCross/blob/develop/MvvmCross/Platforms/Android/Presenters/MvxAndroidViewPresenter.cs#L104

martijn00 · 2018-08-23T08:52:33Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

+        {
+            try
+            {
+                if (Activator.CreateInstance(viewType) is MvxWindowsContentDialog modalView)


Isn't there a view creator for Windows? @nickrandolph

ContentDialog is created with the new command. But if you know another way to do it, propose it.

This way viewmodel events like prepare and Initialize are not triggered. If there is nothing for this on Windows yet we need to make it.

The ContentDialog not is a page. I can create a method to create the ContentDialog. I can't think of anything else.

martijn00 · 2018-08-23T08:54:02Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

+
+        protected virtual Task<bool> CloseModal(IMvxViewModel viewModel, MvxBasePresentationAttribute attribute)
+        {
+            var popups = VisualTreeHelper.GetOpenPopups(Window.Current).FirstOrDefault(p => p.Child is ContentDialog);


Check for MvxWindowsContentDialog to get a closer match. If possible also check the viewmodel type or something to make sure we don't close the wrong one when multiple modals are open.

You cannot open two ContentDialog at a time.

martijn00 · 2018-08-24T09:52:08Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

+               typeof(MvxModalViewPresentationAttribute),
+               new MvxPresentationAttributeAction
+               {
+                   ShowAction = (view, attribute, request) => ShowModal(view, (MvxModalViewPresentationAttribute)attribute, (MvxViewModelInstanceRequest)request),


Consistency is the criteria: https://github.com/MvvmCross/MvvmCross/blob/develop/MvvmCross/Platforms/Android/Presenters/MvxAndroidViewPresenter.cs#L104

martijn00 · 2018-08-24T10:17:28Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

+        {
+            try
+            {
+                if (Activator.CreateInstance(viewType) is MvxWindowsContentDialog modalView)


This way viewmodel events like prepare and Initialize are not triggered. If there is nothing for this on Windows yet we need to make it.

martijn00 · 2018-08-24T10:18:00Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

+            {
+                if (Activator.CreateInstance(viewType) is MvxWindowsContentDialog modalView)
+                {
+                    modalView.ViewModel = request.ViewModelInstance;


Same here, you should not set the instance manually. It should be done from a helper or something.

Cheesebaron · 2018-08-29T05:53:58Z

MvvmCross/Platforms/Uap/Presenters/MvxWindowsViewPresenter.cs

@@ -256,14 +256,15 @@ protected virtual async Task<bool> ShowModal(Type viewType, MvxModalViewPresenta
        {
            try
            {
-                if (Activator.CreateInstance(viewType) is MvxWindowsContentDialog modalView)
+                IMvxWindowsContentDialog contentDialog;
+                if ((contentDialog = CreateContentDialog(attribute, viewType)) != null)


Please split assignment and null check

andres-gimenez · 2018-08-29T20:30:20Z

I changed the modal name to dialog. A ContentDialog is not a modal window. And it can confuse.

andres-gimenez · 2018-09-01T11:04:37Z

This is a sample for can use x:Bind in XAML.

andres-gimenez · 2018-09-06T10:20:07Z

Is there anything left to do? Codefactor's warnings are for putting two classes in the same file.

nickrandolph · 2018-09-06T10:28:54Z

Apologies been unavailable to contribute. A quick glance - I think in the close method we should be confirming that the open dialogue is for the viewmodel being closed

andres-gimenez · 2018-09-10T11:29:17Z

If it leaves you calmer. Here's the modification. But you can only open one ContentDialog per window.

I'm trying to be able to open Standalone windows. And if you can open two ContentDialog. But they are placed in different visual trees. When you closed this change I send the window modal. I need help because you have to modify MvxWindowsMainThreadDispatcher.

Implemented changes

martijn00 · 2018-10-02T13:04:05Z

@andrechi1 @nickrandolph @Cheesebaron i've done a rebase and implemented the changes to get this merged in.

martijn00 requested a review from nickrandolph August 20, 2018 15:49

nmilcoff reviewed Aug 20, 2018

View reviewed changes

Cheesebaron previously requested changes Aug 21, 2018

View reviewed changes

martijn00 suggested changes Aug 22, 2018

View reviewed changes

andres-gimenez requested a review from a team August 22, 2018 15:56

martijn00 suggested changes Aug 23, 2018

View reviewed changes

martijn00 previously requested changes Aug 24, 2018

View reviewed changes

Cheesebaron requested changes Aug 29, 2018

View reviewed changes

andres-gimenez changed the title ~~Implement ModalView for Uap~~ Implement DialogView for Uap Aug 29, 2018

Cheesebaron previously approved these changes Sep 11, 2018

View reviewed changes

andres-gimenez added 12 commits October 2, 2018 13:36

Implement ModalView for Uap

ac4ff2f

Recomposition ModalView for Uap

7f0a0f7

Repair CodeFactor rule.

7742134

Change past type.

5708123

Remove _windowsContentDialog property.

d7ac567

MvxModalViewPresentationAttribute not sealed.

a848970

Lifecycle of MvxWindowsContentDialog

452e5a5

Order lyfecicle.

6d81fe6

Change past type.

ffa1dcc

Recomposition

35aac2b

Create CreateContentDialog method.

ea10759

Split assignment and null check

5ee6d15

andres-gimenez added 5 commits October 2, 2018 13:36

Change name. Modal to Dialog.

ccf0453

Sample for can use x:Bind

1db1069

Recomposition

33111a4

Correct name MvxDialogViewPresentationAttribute

bbce3af

Checks for that the view to close corresponds to the ViewModel

3049d1f

martijn00 dismissed Cheesebaron’s stale review via 3049d1f October 2, 2018 11:37

martijn00 added 2 commits October 2, 2018 14:21

Fix disposing of modal

2d351c7

Make sure viewmodel is loaded

6699af0

MvvmCross deleted a comment from Cheesebaron Oct 2, 2018

martijn00 added this to the 6.2.1 milestone Oct 2, 2018

Make interface simpler

42659c2

martijn00 merged commit 58fa58c into MvvmCross:develop Oct 3, 2018

Implement DialogView for Uap #3074

Implement DialogView for Uap #3074

Conversation

andres-gimenez commented Aug 20, 2018

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

⤵️ What is the current behavior?

🆕 What is the new behavior (if this is a feature change)?

💥 Does this PR introduce a breaking change?

🐛 Recommendations for testing

📝 Links to relevant issues/docs

🤔 Checklist before submitting

martijn00 commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andres-gimenez commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andres-gimenez commented Aug 29, 2018

andres-gimenez commented Sep 1, 2018

andres-gimenez commented Sep 6, 2018

nickrandolph commented Sep 6, 2018

andres-gimenez commented Sep 10, 2018

martijn00 commented Oct 2, 2018