Fixing Master-Detail Forms implementation #2387

nickrandolph · 2017-11-14T10:44:39Z

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

Bug fix

⤵️ What is the current behavior?

Master Details didn't really work using attributes

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

Master Details works - 2 scenarios
Define only Master and Detail pages/view models - automatically generates the MasterDetailsPage
Define Root page/view model where the page inherits from MasterDetailsPage

💥 Does this PR introduce a breaking change?

No

🐛 Recommendations for testing

Playground sample now works for Master-Detail

📝 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)
Nuspec files were updated (when applicable)
[ X ] Rebased onto current develop

martijn00 · 2017-11-14T15:44:47Z

MvvmCross-Forms/MvvmCross.Forms/Views/MvxFormsPagePresenter.cs

@@ -199,6 +200,42 @@ public virtual bool CloseCarouselPage(IMvxViewModel viewModel, MvxCarouselPagePr
            }
        }

+        public virtual MvxBasePresentationAttribute CreatePresentationAttribute(Type viewModelType, Type viewType)
+        {
+            if (viewType.GetTypeInfo().IsSubclassOf(typeof(ContentPage)))


We might be able to use this directly again on .NET Standard.

martijn00 · 2017-11-14T15:50:37Z

MvvmCross-Forms/MvvmCross.Forms/Views/Attributes/MvxMasterDetailPagePresentationAttribute.cs

+            // If this page is to be the master, the default behaviour should be that the page is not wrapped
+            // in a navigation page. This is not the case for Root or Detail pages where default behaviour
+            // would be to support navigation
+            if (position == MasterDetailPosition.Master)


martijn00 · 2017-11-14T15:51:43Z

MvvmCross-Forms/MvvmCross.Forms/Views/MvxFormsPagePresenter.cs

                        masterDetailHost.Detail = page;
                    else
                        masterDetailHost.Detail = new MvxNavigationPage(new MvxContentPage() { Title = !string.IsNullOrEmpty(attribute.Title) ? attribute.Title : nameof(MvxMasterDetailPage) });

-                    PushOrReplacePage(FormsApplication.MainPage, masterDetailHost, attribute);
+                    var masterDetailRootAttribute = new MvxMasterDetailPagePresentationAttribute {Position =  MasterDetailPosition.Root, WrapInNavigationPage = attribute.WrapInNavigationPage, NoHistory = attribute.NoHistory};


This means that we enforce this? Is it really necessary?

martijn00 · 2017-11-14T16:39:04Z

MvvmCross-Forms/MvvmCross.Forms/Views/MvxFormsPagePresenter.cs

@@ -552,6 +607,31 @@ public virtual bool ClosePage(Page rootPage, Page page, MvxPagePresentationAttri
                return rootPage as TPage;
        }

+        public virtual void ReplaceMasterDetailRoot(Page existingMasterDetailPage, Page newRootPage, MvxMasterDetailPagePresentationAttribute masterDetailAttribute)


I don't understand what this tries to solve. Can you explain the thought behind it?

nickrandolph · 2017-11-21T04:15:26Z

@martijn00, @Cheesebaron hey let me know thoughts on this PR and what needs to be changed/done to get this approved.

martijn00 · 2017-11-21T10:26:55Z

MvvmCross-Forms/MvvmCross.Forms.Droid/Platform/MvxFormsAndroidSetup.cs


 namespace MvvmCross.Forms.Droid.Platform
 {
-    public abstract class MvxFormsAndroidSetup : MvxAndroidSetup
+    public abstract class MvxFormsAndroidSetup<TForms> : MvxAndroidSetup


Why does this need to add a generic? I don't see any value?

I'd suggest I revert to non-generic and that we have a separate generic version of the setup class.

martijn00 · 2017-11-21T10:27:22Z

MvvmCross-Forms/MvvmCross.Forms.Droid/MvvmCross.Forms.Droid.csproj

    </Reference>
    <Reference Include="Xamarin.Forms.Platform.Android, Version=2.0.0.0, Culture=neutral, processorArchitecture=MSIL">
-      <HintPath>..\..\packages\Xamarin.Forms.2.4.0.74863\lib\MonoAndroid10\Xamarin.Forms.Platform.Android.dll</HintPath>
+      <HintPath>..\..\packages\Xamarin.Forms.2.5.0.91635\lib\MonoAndroid10\Xamarin.Forms.Platform.Android.dll</HintPath>


We should update the nuspec as well if this changes to 2.5

martijn00 · 2017-11-21T10:28:20Z

MvvmCross-Forms/MvvmCross.Forms.Droid/Platform/MvxFormsAndroidSetup.cs

                return _formsApplication;
            }
        }

-        protected virtual MvxFormsApplication CreateFormsApplication() => new MvxFormsApplication();
+        protected virtual TForms CreateFormsApplication() => new TForms();


Maybe we should make this abstract if it gives bugs?

As per my other comment, I think we should revert this change. In the generic setup class the CreateFormsApplication can return a new instance of the generic TForms class

martijn00 · 2017-11-21T10:32:13Z

MvvmCross-Forms/MvvmCross.Forms/Views/MvxFormsPagePresenter.cs

-                        masterDetailHost.Detail = page;
-                    else
-                        masterDetailHost.Detail = new MvxNavigationPage(new MvxContentPage() { Title = !string.IsNullOrEmpty(attribute.Title) ? attribute.Title : nameof(MvxMasterDetailPage) });
+                    //if (attribute.Position == MasterDetailPosition.Master)


Can you remove any out commented code?

martijn00 · 2017-11-21T10:32:43Z

MvvmCross-Forms/MvvmCross.Forms/Views/MvxFormsPagePresenter.cs

            {
                if (page is MasterDetailPage masterDetailRoot)
                {
                    if (masterDetailRoot.Master == null)
-                        masterDetailRoot.Master = new MvxContentPage() { Title = !string.IsNullOrEmpty(attribute.Title) ? attribute.Title : nameof(MvxMasterDetailPage)  };
+                        masterDetailRoot.Master = new ContentPage() { Title = !string.IsNullOrEmpty(attribute.Title) ? attribute.Title : nameof(MvxMasterDetailPage) };


Why change from using MvxContentPage to ContentPage?

We were seeing issues where the View Model was being set on the MvxContentPage (passed down from parent to the Master page) which resulted in the OnAppearing method being called multiple times on the master detail root view model

martijn00 · 2017-11-21T10:33:31Z

MvvmCross/Core/Core/Platform/LogProviders/NullLogProvider.cs

@@ -0,0 +1,7 @@
+namespace MvvmCross.Core.Platform.LogProviders
+{
+    internal sealed class NullLogProvider : MvxBaseLogProvider


This should be removed, and instead set the logger to None.

Without this class, the None option for logging will break any code which requests ILogProvider as an injected value. Do you have another alternative to this?

martijn00 · 2017-11-21T10:40:23Z

MvvmCross-Forms/MvvmCross.Forms/Views/MvxFormsPagePresenter.cs

        public virtual void PushOrReplacePage(Page rootPage, Page page, MvxPagePresentationAttribute attribute)
        {
-            if (attribute.NoHistory)
+            // Make sure we always have a rootPage


I'm a bit unsure if this behaviour works in all situations. Can you confirm?

martijn00 · 2017-11-21T10:48:04Z

There is definitely some breaking changes in here in terms of behaviour. When testing the playground the modal navigation is different, and the master detail views don't show up with a toolbar with hamburger icon anymore. Also when navigating and coming back the buttons stay disabled. I didn't notice these things before so i think there might be some bugs in here.

PS, can you do a rebase?

nickrandolph · 2017-11-21T15:06:16Z

A couple of questions/comments:

Can you be specific on the changes to modal navigation - I tried to make it more consistent as there were cases where it wouldn't show a modal, or when the modal was closed it didn't go back to the master detail
I'll fix the issue with the icon
The disabled buttons are actually due to a Delay that's in the Initialize method in one of the view models. Should this prevent navigation until this has completed?

…ctoring the presenters

Fixing UWP sample to resolve forms presenter

Switching to using injected NavigationPage and ContentPage instead of MVX equivalents

Code cleanup

Removing default content in navigationpage for details

… HasNavigationBar is set to false before first navigation

…ive setup class which accepts generics to make it easier for developers to get started with Forms

Removing NullLogProvider Fixed packages.config (again)

martijn00 reviewed Nov 14, 2017

View reviewed changes

martijn00 suggested changes Nov 14, 2017

View reviewed changes

nickrandolph force-pushed the btr-masterdetail-forms branch from bedde7d to 6b8df42 Compare November 17, 2017 13:49

martijn00 added this to the 5.5.0 milestone Nov 21, 2017

martijn00 suggested changes Nov 21, 2017

View reviewed changes

Michael Bui and others added 22 commits November 22, 2017 17:43

get UWP forms presenter working and update playground sample and refa…

90b0915

…ctoring the presenters

attempt to get master detail root working

1d7c9c2

Tweaking presentation to get master detail to work

f868fa6

Fixing default master behaviour

75db409

Adding "Root" attribute when creating masterdetailpage

6c76f6b

Fixing UWP sample to resolve forms presenter

Handling of nested navigation for master details

563c4f3

Reducing code duplication across attribute presenters

4dd92e1

Fixing up nested navigation

891ee68

Upgrading Xamarin Forms 2.5 SR1

5f9189a

Switching to using injected NavigationPage and ContentPage instead of MVX equivalents

Adding Null Provider for None log provider scenario

6c128d1

Refactoring setup logic

29dab26

Removing NavigationBar experiement

4569988

Removing page tagging attempt

b6ad407

Reverting changes to iOS projects due to VS 15.5 preview

1508833

Removing RegionId

ba9c48a

Fixing content and modal page navigation

a4b2d4a

Code cleanup

Fixing missing CreatePage

a8ed7f3

Removing default content in navigationpage for details

Fixing Android build

b80a808

Fixing bug where navigation bar is showing in master detail even when…

877e9c2

… HasNavigationBar is set to false before first navigation

Fixing code factor spacing issues

250c2f6

Simplifying TopNavigation

d5198cd

Fixing up WPF presenters to use attributes

1566f42

nickrandolph force-pushed the btr-masterdetail-forms branch from a0592ea to 1566f42 Compare November 22, 2017 07:01

nickrandolph added 5 commits November 23, 2017 08:10

Reverting existing Forms setup to remove generic type. Added alternat…

b45fddd

…ive setup class which accepts generics to make it easier for developers to get started with Forms

Fixing issues where icon not appearing for master detilas

fa53fa2

Fixing loading of droid project

a9ae646

Removing typed setup

0db391f

Removing NullLogProvider Fixed packages.config (again)

Reverting modal behaviour

d9d6ed8

martijn00 merged commit d9d6ed8 into MvvmCross:develop Nov 23, 2017

nickrandolph deleted the btr-masterdetail-forms branch January 13, 2018 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing Master-Detail Forms implementation #2387

Fixing Master-Detail Forms implementation #2387

nickrandolph commented Nov 14, 2017

martijn00 Nov 14, 2017

martijn00 Nov 14, 2017

martijn00 Nov 14, 2017

martijn00 Nov 14, 2017

nickrandolph commented Nov 21, 2017

martijn00 Nov 21, 2017

nickrandolph Nov 21, 2017

martijn00 Nov 21, 2017

martijn00 Nov 21, 2017

nickrandolph Nov 21, 2017

martijn00 Nov 21, 2017

nickrandolph Nov 21, 2017

martijn00 Nov 21, 2017

nickrandolph Nov 21, 2017

martijn00 Nov 21, 2017

nickrandolph Nov 21, 2017

martijn00 Nov 21, 2017

martijn00 commented Nov 21, 2017 •

edited

nickrandolph commented Nov 21, 2017

Fixing Master-Detail Forms implementation #2387

Fixing Master-Detail Forms implementation #2387

Conversation

nickrandolph commented Nov 14, 2017

✨ 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

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

nickrandolph commented Nov 21, 2017

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

martijn00 commented Nov 21, 2017 • edited

nickrandolph commented Nov 21, 2017

martijn00 commented Nov 21, 2017 •

edited