TSE: Replace TSM.Profile with a ProfileViewModel #8552

DHowett · 2020-12-11T02:01:23Z

It's generally accepted that in a graphical application you'll follow a
design pattern. The one this moves us towards is "MVVM" -- Model, View,
View Model.

In an ideal world, all of our display smarts would be in the
ViewModel and the View would literally just be XAML. This moves us
closer to that reality.

The only thing the Profile page ViewModel does right now is proxy
requests through to the Profile behind it while adding some basic
invalidation logic and detection of a setting being set to its current
value.

We have to do this because (1) MVVM is better and (2) separation of
concerns dictates that TSM::Profile should not have a
Windows.UI.Xaml dependency (in IPropertyChangedNotifier). It therefore
cannot conform to Xaml's notion of what a ViewModel should be.

I've done this by introducing a heap of code-generating macros and a
template that work together to make the process of adding more settings
easier (if not "easy").

We are eventually going to need to do this for all of the existing TSM
model objects. In an ideal world, even our navigation would be driven
through a viewmodel that supports a notion of an "active page".

All I've done so far is break ProfilesPage's dependency on Profile entirely.
Weird how that works.

Fixes "browse buttons do not actually update the profile"
Fixes "we need to have convoluted local overrides that update the
visibility of things like AcrylicOpacity"
Fixes "we'll eventually need a way to update a preview TermControl"
Fixes "we have no idea how we're going to update the Discard/Apply
buttons or the unsaved changes label"

Having a PVM that notifies on icon/name change will also make it easier
for the MainPage to update its NavViewItems.

It's generally accepted that in a graphical application you'll follow a design pattern. The one this moves us towards is "MVVM" -- Model, View, View Model. In an ideal world, _all of our display smarts_ would be in the ViewModel and the View would literally just be XAML. This moves us closer to that reality. The only thing the Profile page ViewModel does right now is proxy requests through to the Profile behind it while adding some basic invalidation logic and detection of a setting being set to its current value. We have to do this because (1) MVVM is better and (2) separation of concerns dictates that TSM::Profile should **not** have a Windows.UI.Xaml dependency (in IPropertyChangedNotifier). It therefore **cannot** conform to Xaml's notion of what a ViewModel should be. I've done this by introducing a heap of code-generating macros and a template that work together to make the process of adding more settings easier (if not "easy"). We are eventually going to need to do this for all of the existing TSM model objects. In an ideal world, even our navigation would be driven through a viewmodel that supports a notion of an "active page". All I've done so far is break ProfilesPage's dependency on Profile entirely. Weird how that works. Fixes "browse buttons do not actually update the profile" Fixes "we need to have convoluted local overrides that update the visiblity of things like AcrylicOpacity" Fixes "we'll eventually need a way to update a preview TermControl" Fixes "we have no idea how we're going to update the Discard/Apply buttons or the unsaved changes label" Having a PVM that notifies on icon/name change will also make it easier for the MainPage to update its NavViewItems.

DHowett · 2020-12-11T03:01:03Z

src/cascadia/TerminalSettingsEditor/ViewModelHelpers.h

+        if (name() != value)                              \
+        {                                                 \
+            target.name(value);                           \
+            _NotifyChanges(L"Has" #name, L#name);         \


this notifies for "HasX" and "X" simultaneously

omg, a macro for a template. You've gone wild with power hahaha

zadjii-msft

I guess I understand why we need this, I just, hate it. Whatever, we java now boys.

zadjii-msft · 2020-12-11T12:07:24Z

src/cascadia/TerminalSettingsEditor/ViewModelHelpers.idl.h

@@ -0,0 +1,13 @@
+// Copyright (c) Microsoft Corporation.


.idl.h? what the heck

YOU CAN DEFINE MACROS FOR THE IDL

YEP! I'm using .idl.h as a signal/suggestion, not a contract.

zadjii-msft · 2020-12-11T12:15:58Z

src/cascadia/TerminalSettingsEditor/Profiles.h

+
+        PERMANENT_OBSERVABLE_PROJECTED_SETTING(_profile, Guid);
+        PERMANENT_OBSERVABLE_PROJECTED_SETTING(_profile, ConnectionType);
+        OBSERVABLE_PROJECTED_SETTING(_profile, Name);


as happy as these new macros make me, I friggin hate that there's another place we'll need to make sure to add the setting to. Chalk this up as another reason to have AddATerminalSetting.md

I know, I know.

I'll actually go ahead and add AddATerminalSetting.md to my follow-up tasks.

zadjii-msft · 2020-12-11T12:17:44Z

src/cascadia/TerminalSettingsEditor/Profiles.idl

 namespace Microsoft.Terminal.Settings.Editor
 {
+    runtimeclass ProfileViewModel : Windows.UI.Xaml.Data.INotifyPropertyChanged


Am I dumb for thinking there could be an IProfile that defines all these, then Profile and ProfileViewModel implement that? So these could be defined in a single source of truth?

My gut tells me the interface would be good now, but bad in the future. As we implement UI-driven features that diverge from the representation in the settings model (which we will, make no mistake), we'll have to add things to this idl manually anyway.

Already, this is incorrect. We should move the bindable enum stuff up to the ViewModel because it's a viewmodel of an enum value, and make the active values in the ProfileViewModel use EnumEntries for enum fields. Much of the logic that is in the Profiles page CodeBehind should be here.

This is just a start, and I think doing the interface thing would take us on the wrong path. What do you think?

I know the redundancy sucks. FromJson, ToJson, Clone, Copy, TSM idl, TSE idl, TSE xaml, IControlSettings, ICoreSettings, IAppearanceConfig. It's awful.

That makes sense to me

DHowett · 2020-12-11T17:34:49Z

The SA failure is because feature/sui has not merged main, and so this has not merged main.

carlos-zamora · 2020-12-11T17:41:32Z

src/cascadia/TerminalSettingsEditor/ViewModelHelpers.h

+        if (name() != value)                              \
+        {                                                 \
+            target.name(value);                           \
+            _NotifyChanges(L"Has" #name, L#name);         \


omg, a macro for a template. You've gone wild with power hahaha

carlos-zamora · 2020-12-11T17:45:28Z

src/cascadia/TerminalSettingsEditor/Profiles.h

+
+        PERMANENT_OBSERVABLE_PROJECTED_SETTING(_profile, Guid);
+        PERMANENT_OBSERVABLE_PROJECTED_SETTING(_profile, ConnectionType);
+        OBSERVABLE_PROJECTED_SETTING(_profile, Name);


I'll actually go ahead and add AddATerminalSetting.md to my follow-up tasks.

carlos-zamora · 2020-12-11T17:49:01Z

src/cascadia/TerminalSettingsEditor/MainPage.h

@@ -30,13 +30,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
    private:
        Model::CascadiaSettings _settingsSource;
        Model::CascadiaSettings _settingsClone;
-        winrt::Windows::Foundation::Collections::IMap<Model::Profile, winrt::Microsoft::UI::Xaml::Controls::NavigationViewItem> _profileToNavItemMap;


oh geez, I must've added that in on accident. My bad!

DHowett added 2 commits December 10, 2020 17:52

This is a header file so it follows clang rules even though it's for idl

35fdece

DHowett commented Dec 11, 2020

View reviewed changes

zadjii-msft approved these changes Dec 11, 2020

View reviewed changes

carlos-zamora mentioned this pull request Dec 11, 2020

Implement the Settings UI #8048

Merged

carlos-zamora approved these changes Dec 11, 2020

View reviewed changes

DHowett merged commit 74a85db into feature/settings-ui Dec 11, 2020

DHowett deleted the dev/duhowett/tsvm_pvm branch December 11, 2020 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TSE: Replace TSM.Profile with a ProfileViewModel #8552

TSE: Replace TSM.Profile with a ProfileViewModel #8552

DHowett commented Dec 11, 2020 •

edited

Loading

DHowett Dec 11, 2020

carlos-zamora Dec 11, 2020

zadjii-msft left a comment

zadjii-msft Dec 11, 2020

zadjii-msft Dec 11, 2020

zadjii-msft Dec 11, 2020

DHowett Dec 11, 2020

zadjii-msft Dec 11, 2020

DHowett Dec 11, 2020

carlos-zamora Dec 11, 2020

zadjii-msft Dec 11, 2020

DHowett Dec 11, 2020

DHowett Dec 11, 2020

zadjii-msft Dec 11, 2020

DHowett commented Dec 11, 2020

carlos-zamora Dec 11, 2020

carlos-zamora Dec 11, 2020

carlos-zamora Dec 11, 2020

TSE: Replace TSM.Profile with a ProfileViewModel #8552

TSE: Replace TSM.Profile with a ProfileViewModel #8552

Conversation

DHowett commented Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a 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

DHowett commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Dec 11, 2020 •

edited

Loading