MVVM-ize things #1346

grokys · 2017-11-28T12:41:32Z

This pull request migrates our code to use the MVVM pattern throughout. MVVM is generally considered to be the "right" way to do WPF and greatly improves the unit-testability of code by separating application layer and view layer code.

This is a big PR, sorry! To ease reviewing, the following attempts to explain the major changes and concepts.

UIController Removed

UIController was previously responsible for creating views and view models based on the UIViewType enumeration.

There is no concept equivalent to UIController after the refactor; it's view models all the way down!
Instead of UIViewType, view models and views are keyed on the type of the view model interface

NavigationController Removed

NavigationController was built on UIController to display the browser-style user interface in the GitHub pane. The equivalent now is NavigationViewModel, however this simply maintains a list of pages and a position in that list, it knows nothing about the pages it will display. That responsibility is handled by:

GitHubPaneViewModel

GitHubPaneViewModel exposes a Content property which:

When the user is logged-in and a GitHub repository is open, will be a NavigationViewModel
When the user is not logged in or there's not a GitHub repository open then the Content will be a view model representing this state such as an ILoggedOutViewModel

GitHubPaneViewModel supports navigation via URLs, such as github://pane/owner/repo/pull/5 (via NavigateTo(Uri)) or by calling a method such as ShowPullRequest("owner", "repo", 5).

Asynchronous Initialization

Previously view model initialization methods were all of the signature void Initialize(ViewWithData) (this was defined in the IViewModel base interface). This had three downsides:

Type-safely was lost as everything had to be cast to an object and placed in ViewWithData.Data
Asynchronous initialization was difficult
It mixed view and view model concerns

The base IViewModel interface no longer defines a one-fits-all initialization method and view models are now expected to define a (probably async) initialization method in their interface which must be called following creation.

Many of the changes in this PR consist of moving things from the ViewModel constructor to an async initialization method.

ViewLocator

At the root of each GitHub top-level control (i.e. dialog and the GitHub pane) there is a DataTemplate typed on ViewModelBase. This DataTemplate has a ViewLocator as its converter and is used to convert view models to their equivalent views. Using this technique the application layer code need have no concept of the view layer: the application state is built purely with view models which are converted to equivalent views by the view layer.

See the documentation here.

Views and View Models Moved

Previously all views and view models were all in the same folder/namespace, which was starting to get unwieldy. I've moved them under folders based on which part of the UI they appear in: Dialog, GitHubPane and TeamExplorer.

Developer Documentation Added

I've added a few pieces of developer documentation explaining some of the new features:

https://github.com/github/VisualStudio/blob/refactor/mvvm/docs/developer/readme.md

Fixes #1105
Fixes #1341
Fixes #1357

The MVVM refactor (#1346) was originally based on the SSO feature branch and had a _lot_ of WIP etc commits. Rebased it off of #1353 and squashed into a single commit.

grokys · 2017-11-28T18:52:45Z

src/GitHub.App/Authentication/TwoFactorRequiredUserError.cs

            : base(exception.Message, innerException: exception)
        {
            Guard.ArgumentNotNull(exception, nameof(exception));

-            TwoFactorType = exception.TwoFactorType;
+            TwoFactorType = twoFactorType;


Added this extra constructor because the detection of the two-factor type is deep within Octokit's header parsing code. This allows us to easily override that in unit tests.

grokys · 2017-11-28T19:01:45Z

src/GitHub.UI.Reactive/Assets/Controls/Validation/UserErrorMessages.xaml

-                                </DataTemplate>
-                            </ContentControl.Resources>
-                        </ContentControl>
+                        <TextBlock Grid.Column="1"


I had to make this change because for some reason the old way of doing it was causing a StackOverflowException in WPF's layouting code. This change should produce equivalent results and is in fact simpler.

...instead of `IGitHubServiceProvider` to create view models in `GitHubDialogWindowViewModel`. Also added some doc comments.

Fixes #1355.

And use it instead of defining `LoginView` and `NavigationView` separately.

MEF requires an array here.

When the page to be navigated to in the GitHub pane is the current page, do nothing.

jcansdale · 2017-11-29T18:30:15Z

src/GitHub.App/Factories/ViewViewModelFactory.cs

+            ICompositionService cc)
+        {
+            this.serviceProvider = serviceProvider;
+            cc.SatisfyImportsOnce(this);


I don't think importing ICompositionService or calling SatisfyImportsOnce is necessary. This should happen anyway after ViewViewModelFactory has been created.

I'm not sure AllowRecomposition = true is supported in Visual Studio's MEF implementation. I'm guessing this class some from somewhere else?

Not sure - this class used to be ExportFactoryProvider and the code comes from there.

Is this query worth putting in another issue under "needs investigation" so that we can look into it after this PR gets merged?

I put a breakpoint on the set method of Views and this property is indeed being set twice (with the same number of elements).

@sguthals Done, see #1368.

jcansdale · 2017-11-29T19:13:11Z

When I click on the margin to add inline comment, visual Studio crashes with the following:

meaghanlewis · 2017-11-29T21:04:26Z

I noticed that when not signed into GitHub and trying to view a repository's pull requests that the view just spins forever:

expected

actual

meaghanlewis · 2017-11-29T21:23:42Z

When checking out a PR branch, I can do so successfully but the view doesn't update from saying checkout test-pr-creation to local branch updated

expected

actual

And then im unable to view changes in solution or open file in solution

jcansdale · 2017-11-30T16:12:42Z

docs/developer/how-viewmodels-are-turned-into-views.md

+
+    </DataTemplate>
+  </Window.Resources>
+</pfui:DialogWindow>


Should this be </Window>?

jcansdale · 2017-11-30T16:15:15Z

docs/developer/how-viewmodels-are-turned-into-views.md

+
+      <!-- Display ViewModel.Greeting in a red border with rounded corners -->
+	  <Border Background="Red" CornerRadius="8">
+        <TextBlock Binding="{Binding Greeting}"


Missing a />.

This commit will automatically dispose of any preview PullRequestDetailViewModel object.

Keep CA happy by making AutoDispose static.

PullRequestDetailViewModel will be disposed in future commit.

Lightweight fix for PR detail view not refreshing

Conflicts: src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs

jcansdale · 2017-12-04T18:13:53Z

Should we #pragma warning disable CA1004 to keep the build clean?

jcansdale

Looking good!

See comment about the CA1004 warning.

`IVSGitExt.ActiveRepositoriesChanged` isn't notified on the UI thread which breaks things. Switch to the main thread when refreshing due to a notification from this.

jcansdale · 2017-12-05T11:14:00Z

docs/developer/how-viewmodels-are-turned-into-views.md

+
+And thus a view model becomes a view.
+
+[1]: it would be nice to make it apply to all classes that inherit `IViewModel` but unfortunately WPF's `DataTemplate`s don't work with interfaces.


I'm sure I'm missing something, but is this not a DataTemplate that uses an interface?
https://stackoverflow.com/a/25642988/121348

Hmm, interesting, I didn't know this was possible! Worth a try!

jcansdale · 2017-12-05T11:25:11Z

src/GitHub.App/Factories/ViewViewModelFactory.cs

+            cc.SatisfyImportsOnce(this);
+        }
+
+        [ImportMany(AllowRecomposition = true)]


Visual Studio's MEF implementation is static (the graph is created once and cached). New exports/assemblies can't be added to the catalog on the fly. Having AllowRecomposition = true is confusing because recomposition is impossible.

Ok, well I'm fine with removing it! We should probably do it in a new PR though in case it causes problems.

jcansdale · 2017-12-05T11:30:08Z

src/GitHub.App/Factories/ViewViewModelFactory.cs

+        /// <inheritdoc/>
+        public TViewModel CreateViewModel<TViewModel>() where TViewModel : IViewModel
+        {
+            return serviceProvider.ExportProvider.GetExport<TViewModel>().Value;


We could do the same as with CreateView and avoid any dependency on IGitHubServiceProvider.

Yep! Lets try that in a separate PR too.

drguthals · 2017-12-02T00:44:36Z

src/GitHub.App/Factories/ViewViewModelFactory.cs

+            ICompositionService cc)
+        {
+            this.serviceProvider = serviceProvider;
+            cc.SatisfyImportsOnce(this);


Is this query worth putting in another issue under "needs investigation" so that we can look into it after this PR gets merged?

drguthals · 2017-12-02T00:52:55Z

src/GitHub.App/GitHub.App.csproj

@@ -131,19 +132,40 @@
    <Reference Include="WindowsBase" />
  </ItemGroup>
  <ItemGroup>
+    <Compile Include="Factories\ViewViewModelFactory.cs" />


I'm assuming "ViewViewModelFactory.cs" is meant to have two "view"'s and isn't a typo. But thought it was worth asking ;)

Yeah, it's used for creating both views and view models. I did think it looked strange until I thought that MVVM stands for "model, view, view model" and so I decided to keep it.

drguthals · 2017-12-05T15:03:05Z

src/GitHub.App/ViewModels/GitHubPane/NavigationViewModel.cs

+        {
+            history = new ReactiveList<IPanePageViewModel>();
+            history.BeforeItemsAdded.Subscribe(BeforeItemAdded);
+            history.CollectionChanged += CollectionChanged;


Where are BeforeItemAdded and CollectionChanged coming from?

NVM - I see them below.

grokys mentioned this pull request Nov 28, 2017

Remove DesignTimeStyleHelper #1352

Closed

Rebase of MVVM refactor.

5106ff5

The MVVM refactor (#1346) was originally based on the SSO feature branch and had a _lot_ of WIP etc commits. Rebased it off of #1353 and squashed into a single commit.

grokys force-pushed the refactor/mvvm branch from 53bb5e6 to 5106ff5 Compare November 28, 2017 18:44

grokys changed the base branch from feature/sso to refactor/1352-remove-designtimestylehelper November 28, 2017 18:44

grokys commented Nov 28, 2017

View reviewed changes

grokys added 14 commits November 28, 2017 20:05

Added some doc comments.

c71b2ab

Use IViewViewModelFactory...

1a886bf

...instead of `IGitHubServiceProvider` to create view models in `GitHubDialogWindowViewModel`. Also added some doc comments.

Add DialogService to MenuBase.

8c2f286

Fix compile error.

c50b29a

Removed unused types.

91703a4

Added move developer docs.

d928d72

Include VSGitExt in GitHub.TeamFoundation.15.

a3fe54b

Fixes #1355.

Merge branch 'fixes/1355-vsgitext-vs2017' into refactor/mvvm

c578ecd

Added progress bar to Clone dialog page.

42c1d34

Added general purpose ContentView.

16df7cc

And use it instead of defining `LoginView` and `NavigationView` separately.

Supress CA error.`

7273371

MEF requires an array here.

Doc comments.

e7c4ef0

Don't add current page to current page.

22c8345

When the page to be navigated to in the GitHub pane is the current page, do nothing.

Display error messages in GitHubPane.

9ee5f0e

grokys changed the title ~~WIP: MVVM-ize things~~ MVVM-ize things Nov 29, 2017

grokys requested review from drguthals, meaghanlewis and jcansdale November 29, 2017 14:52

Fix unit tests.

f8e48fb

jcansdale reviewed Nov 29, 2017

View reviewed changes

Fixed spinner showing when logged out.

304485e

jcansdale reviewed Nov 30, 2017

View reviewed changes

meaghanlewis added the verified label Nov 30, 2017

grokys and others added 12 commits December 1, 2017 10:12

Fixed XAML errors in docs.

6f4fe80

Refresh PR detail view when active repo changes

6c4ff1b

This commit will automatically dispose of any preview PullRequestDetailViewModel object.

Make PullRequestDetailViewModel sealed

c3d5335

Keep CA happy by making AutoDispose static.

Don't worry about auto-disposing

81e29d3

PullRequestDetailViewModel will be disposed in future commit.

Show title in dialog window.

1c24639

Doc fixes.

67a8a8c

Added ShowDefaultPage to GitHubPaneViewModel.

c1c58c5

Added docs for implementing a pane page.

5372cad

Expose IGitHubPaneViewModel.NavigateTo(Uri).

b93c6f9

Merge pull request #1359 from github/fixes/1345-refresh-PR-detail-view

29b0536

Lightweight fix for PR detail view not refreshing

Merge branch 'master' into refactor/mvvm

68fd076

Conflicts: src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs

Fix broken tests.

2a85875

jcansdale suggested changes Dec 4, 2017

View reviewed changes

Switch to UI thread in ActiveRepositoriesChanged.

2eb687c

`IVSGitExt.ActiveRepositoriesChanged` isn't notified on the UI thread which breaks things. Switch to the main thread when refreshing due to a notification from this.

jcansdale reviewed Dec 5, 2017

View reviewed changes

Suppress CA error.

f771bb8

jcansdale approved these changes Dec 5, 2017

View reviewed changes

grokys mentioned this pull request Dec 5, 2017

Add lifecycle management for GitHub Pane pages #1366

Merged

drguthals approved these changes Dec 5, 2017

View reviewed changes

grokys merged commit f16656e into refactor/1352-remove-designtimestylehelper Dec 5, 2017

This was referenced Dec 5, 2017

MVVM-ize things (attempt 2) #1367

Merged

Navigating to the PR list causes a reload of all PRs #1105

Closed

GitHub Pane shows "No repository" while initializing #1357

Closed

Replace UIController #1341

Closed


		And thus a view model becomes a view.

		[1]: it would be nice to make it apply to all classes that inherit `IViewModel` but unfortunately WPF's `DataTemplate`s don't work with interfaces.

MVVM-ize things #1346

MVVM-ize things #1346

Uh oh!

Conversation

grokys commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UIController Removed

NavigationController Removed

GitHubPaneViewModel

Asynchronous Initialization

ViewLocator

Views and View Models Moved

Developer Documentation Added

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokys Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcansdale commented Nov 29, 2017

Uh oh!

meaghanlewis commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meaghanlewis commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcansdale commented Dec 4, 2017

Uh oh!

jcansdale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grokys commented Nov 28, 2017 •

edited

Loading

grokys Nov 30, 2017 •

edited

Loading

meaghanlewis commented Nov 29, 2017 •

edited

Loading

meaghanlewis commented Nov 29, 2017 •

edited

Loading