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

TSE: Replace TSM.Profile with a ProfileViewModel #8552

Merged
merged 2 commits into from
Dec 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions src/cascadia/TerminalSettingsEditor/MainPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,14 @@ static const std::wstring_view globalAppearanceTag{ L"GlobalAppearance_Nav" };

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
static Editor::ProfileViewModel _viewModelForProfile(const Model::Profile& profile)
{
return winrt::make<implementation::ProfileViewModel>(profile);
}

MainPage::MainPage(const CascadiaSettings& settings) :
_settingsSource{ settings },
_settingsClone{ settings.Copy() },
_profileToNavItemMap{ winrt::single_threaded_map<Model::Profile, MUX::Controls::NavigationViewItem>() }
_settingsClone{ settings.Copy() }
{
InitializeComponent();

Expand Down Expand Up @@ -105,7 +109,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
if (_settingsClone.FindProfile(profile.Guid()))
{
// Navigate to the page with the given profile
contentFrame().Navigate(xaml_typename<Editor::Profiles>(), winrt::make<ProfilePageNavigationState>(profile, _settingsClone.GlobalSettings().ColorSchemes(), *this));
contentFrame().Navigate(xaml_typename<Editor::Profiles>(), winrt::make<ProfilePageNavigationState>(_viewModelForProfile(profile), _settingsClone.GlobalSettings().ColorSchemes(), *this));
return;
}
}
Expand Down Expand Up @@ -190,7 +194,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_Navigate(*navString);
}
}
else if (const auto profile = clickedItemContainer.Tag().try_as<Model::Profile>())
else if (const auto profile = clickedItemContainer.Tag().try_as<Editor::ProfileViewModel>())
{
// Navigate to a page with the given profile
contentFrame().Navigate(xaml_typename<Editor::Profiles>(), winrt::make<ProfilePageNavigationState>(profile, _settingsClone.GlobalSettings().ColorSchemes(), *this));
Expand All @@ -214,7 +218,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}
else if (clickedItemTag == globalProfileTag)
{
contentFrame().Navigate(xaml_typename<Editor::Profiles>(), winrt::make<ProfilePageNavigationState>(_settingsClone.ProfileDefaults(), _settingsClone.GlobalSettings().ColorSchemes(), *this));
contentFrame().Navigate(xaml_typename<Editor::Profiles>(), winrt::make<ProfilePageNavigationState>(_viewModelForProfile(_settingsClone.ProfileDefaults()), _settingsClone.GlobalSettings().ColorSchemes(), *this));
}
else if (clickedItemTag == colorSchemesTag)
{
Expand Down Expand Up @@ -266,7 +270,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// profile changes.
for (const auto& profile : _settingsClone.AllProfiles())
{
auto navItem = _CreateProfileNavViewItem(profile);
auto navItem = _CreateProfileNavViewItem(_viewModelForProfile(profile));
SettingsNav().MenuItems().Append(navItem);
}

Expand All @@ -287,27 +291,26 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void MainPage::_CreateAndNavigateToNewProfile(const uint32_t index)
{
const auto newProfile{ _settingsClone.CreateNewProfile() };
const auto navItem{ _CreateProfileNavViewItem(newProfile) };
const auto profileViewModel{ _viewModelForProfile(newProfile) };
const auto navItem{ _CreateProfileNavViewItem(profileViewModel) };
SettingsNav().MenuItems().InsertAt(index, navItem);

// Select and navigate to the new profile
SettingsNav().SelectedItem(navItem);
contentFrame().Navigate(xaml_typename<Editor::Profiles>(), winrt::make<ProfilePageNavigationState>(newProfile, _settingsClone.GlobalSettings().ColorSchemes(), *this));
contentFrame().Navigate(xaml_typename<Editor::Profiles>(), winrt::make<ProfilePageNavigationState>(profileViewModel, _settingsClone.GlobalSettings().ColorSchemes(), *this));
}

MUX::Controls::NavigationViewItem MainPage::_CreateProfileNavViewItem(const Profile& profile)
MUX::Controls::NavigationViewItem MainPage::_CreateProfileNavViewItem(const Editor::ProfileViewModel& profile)
{
MUX::Controls::NavigationViewItem profileNavItem;
profileNavItem.Content(box_value(profile.Name()));
profileNavItem.Tag(box_value<Model::Profile>(profile));
profileNavItem.Tag(box_value<Editor::ProfileViewModel>(profile));

const auto iconSource{ IconPathConverter::IconSourceWUX(profile.Icon()) };
WUX::Controls::IconSourceElement icon;
icon.IconSource(iconSource);
profileNavItem.Icon(icon);

_profileToNavItemMap.Insert(profile, profileNavItem);

return profileNavItem;
}
}
3 changes: 1 addition & 2 deletions src/cascadia/TerminalSettingsEditor/MainPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


std::optional<HWND> _hostingHwnd;

void _InitializeProfilesList();
void _CreateAndNavigateToNewProfile(const uint32_t index);
winrt::Microsoft::UI::Xaml::Controls::NavigationViewItem _CreateProfileNavViewItem(const Model::Profile& profile);
winrt::Microsoft::UI::Xaml::Controls::NavigationViewItem _CreateProfileNavViewItem(const ProfileViewModel& profile);

void _Navigate(hstring clickedItemTag);
void _RefreshCurrentPage();
Expand Down
8 changes: 4 additions & 4 deletions src/cascadia/TerminalSettingsEditor/Profiles.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
StorageFile file = co_await picker.PickSingleFileAsync();
if (file != nullptr)
{
BackgroundImage().Text(file.Path());
_State.Profile().BackgroundImagePath(file.Path());
}
}

Expand All @@ -124,7 +124,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
StorageFile file = co_await picker.PickSingleFileAsync();
if (file != nullptr)
{
Icon().Text(file.Path());
_State.Profile().Icon(file.Path());
}
}

Expand All @@ -142,7 +142,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
StorageFile file = co_await picker.PickSingleFileAsync();
if (file != nullptr)
{
Commandline().Text(file.Path());
_State.Profile().Commandline(file.Path());
}
}

Expand All @@ -157,7 +157,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
if (folder != nullptr)
{
StorageApplicationPermissions::FutureAccessList().AddOrReplace(L"PickedFolderToken", folder);
StartingDirectory().Text(folder.Path());
_State.Profile().StartingDirectory(folder.Path());
}
}

Expand Down
57 changes: 54 additions & 3 deletions src/cascadia/TerminalSettingsEditor/Profiles.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,66 @@

#include "Profiles.g.h"
#include "ProfilePageNavigationState.g.h"
#include "ProfileViewModel.g.h"
#include "Utils.h"
#include "ViewModelHelpers.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
struct ProfileViewModel : ProfileViewModelT<ProfileViewModel>, ViewModelHelper<ProfileViewModel>
{
public:
ProfileViewModel(const Model::Profile& profile) :
_profile{ profile } {}

PERMANENT_OBSERVABLE_PROJECTED_SETTING(_profile, Guid);
PERMANENT_OBSERVABLE_PROJECTED_SETTING(_profile, ConnectionType);
OBSERVABLE_PROJECTED_SETTING(_profile, Name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, I know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OBSERVABLE_PROJECTED_SETTING(_profile, Source);
OBSERVABLE_PROJECTED_SETTING(_profile, Hidden);
OBSERVABLE_PROJECTED_SETTING(_profile, Icon);
OBSERVABLE_PROJECTED_SETTING(_profile, CloseOnExit);
OBSERVABLE_PROJECTED_SETTING(_profile, TabTitle);
OBSERVABLE_PROJECTED_SETTING(_profile, TabColor);
OBSERVABLE_PROJECTED_SETTING(_profile, SuppressApplicationTitle);
OBSERVABLE_PROJECTED_SETTING(_profile, UseAcrylic);
OBSERVABLE_PROJECTED_SETTING(_profile, AcrylicOpacity);
OBSERVABLE_PROJECTED_SETTING(_profile, ScrollState);
OBSERVABLE_PROJECTED_SETTING(_profile, FontFace);
OBSERVABLE_PROJECTED_SETTING(_profile, FontSize);
OBSERVABLE_PROJECTED_SETTING(_profile, FontWeight);
OBSERVABLE_PROJECTED_SETTING(_profile, Padding);
OBSERVABLE_PROJECTED_SETTING(_profile, Commandline);
OBSERVABLE_PROJECTED_SETTING(_profile, StartingDirectory);
OBSERVABLE_PROJECTED_SETTING(_profile, BackgroundImagePath);
OBSERVABLE_PROJECTED_SETTING(_profile, BackgroundImageOpacity);
OBSERVABLE_PROJECTED_SETTING(_profile, BackgroundImageStretchMode);
OBSERVABLE_PROJECTED_SETTING(_profile, BackgroundImageAlignment);
OBSERVABLE_PROJECTED_SETTING(_profile, AntialiasingMode);
OBSERVABLE_PROJECTED_SETTING(_profile, RetroTerminalEffect);
OBSERVABLE_PROJECTED_SETTING(_profile, ForceFullRepaintRendering);
OBSERVABLE_PROJECTED_SETTING(_profile, SoftwareRendering);
OBSERVABLE_PROJECTED_SETTING(_profile, ColorSchemeName);
OBSERVABLE_PROJECTED_SETTING(_profile, Foreground);
OBSERVABLE_PROJECTED_SETTING(_profile, Background);
OBSERVABLE_PROJECTED_SETTING(_profile, SelectionBackground);
OBSERVABLE_PROJECTED_SETTING(_profile, CursorColor);
OBSERVABLE_PROJECTED_SETTING(_profile, HistorySize);
OBSERVABLE_PROJECTED_SETTING(_profile, SnapOnInput);
OBSERVABLE_PROJECTED_SETTING(_profile, AltGrAliasing);
OBSERVABLE_PROJECTED_SETTING(_profile, CursorShape);
OBSERVABLE_PROJECTED_SETTING(_profile, CursorHeight);
OBSERVABLE_PROJECTED_SETTING(_profile, BellStyle);

private:
Model::Profile _profile;
};

struct ProfilePageNavigationState : ProfilePageNavigationStateT<ProfilePageNavigationState>
{
public:
ProfilePageNavigationState(const Model::Profile& profile, const Windows::Foundation::Collections::IMapView<hstring, Model::ColorScheme>& schemes, const IHostedInWindow& windowRoot) :
_Profile{ profile },
ProfilePageNavigationState(const Editor::ProfileViewModel& viewModel, const Windows::Foundation::Collections::IMapView<hstring, Model::ColorScheme>& schemes, const IHostedInWindow& windowRoot) :
_Profile{ viewModel },
_Schemes{ schemes },
_WindowRoot{ windowRoot }
{
Expand All @@ -22,8 +73,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
Windows::Foundation::Collections::IMapView<hstring, Model::ColorScheme> Schemes() { return _Schemes; }
void Schemes(const Windows::Foundation::Collections::IMapView<hstring, Model::ColorScheme>& val) { _Schemes = val; }

GETSET_PROPERTY(Model::Profile, Profile, nullptr);
GETSET_PROPERTY(IHostedInWindow, WindowRoot, nullptr);
GETSET_PROPERTY(Editor::ProfileViewModel, Profile, nullptr);

private:
Windows::Foundation::Collections::IMapView<hstring, Model::ColorScheme> _Schemes;
Expand Down
47 changes: 46 additions & 1 deletion src/cascadia/TerminalSettingsEditor/Profiles.idl
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,58 @@
import "EnumEntry.idl";
import "MainPage.idl";

#include "ViewModelHelpers.idl.h"

namespace Microsoft.Terminal.Settings.Editor
{
runtimeclass ProfileViewModel : Windows.UI.Xaml.Data.INotifyPropertyChanged
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me

{
OBSERVABLE_PROJECTED_SETTING(String, Name);
OBSERVABLE_PROJECTED_SETTING(Guid, Guid);
OBSERVABLE_PROJECTED_SETTING(String, Source);
OBSERVABLE_PROJECTED_SETTING(Guid, ConnectionType);
OBSERVABLE_PROJECTED_SETTING(Boolean, Hidden);
OBSERVABLE_PROJECTED_SETTING(String, Icon);
OBSERVABLE_PROJECTED_SETTING(Microsoft.Terminal.Settings.Model.CloseOnExitMode, CloseOnExit);
OBSERVABLE_PROJECTED_SETTING(String, TabTitle);
OBSERVABLE_PROJECTED_SETTING(Windows.Foundation.IReference<Windows.UI.Color>, TabColor);
OBSERVABLE_PROJECTED_SETTING(Boolean, SuppressApplicationTitle);
OBSERVABLE_PROJECTED_SETTING(Boolean, UseAcrylic);
OBSERVABLE_PROJECTED_SETTING(Double, AcrylicOpacity);
OBSERVABLE_PROJECTED_SETTING(Microsoft.Terminal.TerminalControl.ScrollbarState, ScrollState);
OBSERVABLE_PROJECTED_SETTING(String, FontFace);
OBSERVABLE_PROJECTED_SETTING(Int32, FontSize);
OBSERVABLE_PROJECTED_SETTING(Windows.UI.Text.FontWeight, FontWeight);
OBSERVABLE_PROJECTED_SETTING(String, Padding);
OBSERVABLE_PROJECTED_SETTING(String, Commandline);
OBSERVABLE_PROJECTED_SETTING(String, StartingDirectory);
OBSERVABLE_PROJECTED_SETTING(String, BackgroundImagePath);
OBSERVABLE_PROJECTED_SETTING(Double, BackgroundImageOpacity);
OBSERVABLE_PROJECTED_SETTING(Windows.UI.Xaml.Media.Stretch, BackgroundImageStretchMode);
OBSERVABLE_PROJECTED_SETTING(Microsoft.Terminal.Settings.Model.ConvergedAlignment, BackgroundImageAlignment);
OBSERVABLE_PROJECTED_SETTING(Microsoft.Terminal.TerminalControl.TextAntialiasingMode, AntialiasingMode);
OBSERVABLE_PROJECTED_SETTING(Boolean, RetroTerminalEffect);
OBSERVABLE_PROJECTED_SETTING(Boolean, ForceFullRepaintRendering);
OBSERVABLE_PROJECTED_SETTING(Boolean, SoftwareRendering);
OBSERVABLE_PROJECTED_SETTING(String, ColorSchemeName);
OBSERVABLE_PROJECTED_SETTING(Windows.Foundation.IReference<Windows.UI.Color>, Foreground);
OBSERVABLE_PROJECTED_SETTING(Windows.Foundation.IReference<Windows.UI.Color>, Background);
OBSERVABLE_PROJECTED_SETTING(Windows.Foundation.IReference<Windows.UI.Color>, SelectionBackground);
OBSERVABLE_PROJECTED_SETTING(Windows.Foundation.IReference<Windows.UI.Color>, CursorColor);
OBSERVABLE_PROJECTED_SETTING(Int32, HistorySize);
OBSERVABLE_PROJECTED_SETTING(Boolean, SnapOnInput);
OBSERVABLE_PROJECTED_SETTING(Boolean, AltGrAliasing);
OBSERVABLE_PROJECTED_SETTING(Microsoft.Terminal.TerminalControl.CursorStyle, CursorShape);
OBSERVABLE_PROJECTED_SETTING(UInt32, CursorHeight);
OBSERVABLE_PROJECTED_SETTING(Microsoft.Terminal.Settings.Model.BellStyle, BellStyle);
}

runtimeclass ProfilePageNavigationState
{
Windows.Foundation.Collections.IMapView<String, Microsoft.Terminal.Settings.Model.ColorScheme> Schemes;
Microsoft.Terminal.Settings.Model.Profile Profile;
IHostedInWindow WindowRoot; // necessary to send the right HWND into the file picker dialogs.

ProfileViewModel Profile { get; };
};

[default_interface] runtimeclass Profiles : Windows.UI.Xaml.Controls.Page, Windows.UI.Xaml.Data.INotifyPropertyChanged
Expand Down
74 changes: 74 additions & 0 deletions src/cascadia/TerminalSettingsEditor/ViewModelHelpers.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

#include "../inc/cppwinrt_utils.h"

template<typename T>
struct ViewModelHelper
{
public:
winrt::event_token PropertyChanged(::winrt::Windows::UI::Xaml::Data::PropertyChangedEventHandler const& handler)
{
return _propertyChangedHandlers.add(handler);
}

void PropertyChanged(winrt::event_token const& token)
{
_propertyChangedHandlers.remove(token);
}

protected:
void _NotifyChangeCore(const std::wstring_view name)
{
_propertyChangedHandlers(*static_cast<T*>(this), ::winrt::Windows::UI::Xaml::Data::PropertyChangedEventArgs{ name });
}

// template recursion base case: single dispatch
void _NotifyChanges(const std::wstring_view name) { _NotifyChangeCore(name); }

template<typename... Args>
void _NotifyChanges(const std::wstring_view name, Args&&... more)
{
_NotifyChangeCore(name);
_NotifyChanges(std::forward<Args>(more)...);
}

private:
winrt::event<::winrt::Windows::UI::Xaml::Data::PropertyChangedEventHandler> _propertyChangedHandlers;
};

#define _BASE_OBSERVABLE_PROJECTED_SETTING(target, name) \
public: \
auto name() const noexcept { return target.name(); }; \
template<typename T> \
void name(const T& value) \
{ \
if (name() != value) \
{ \
target.name(value); \
_NotifyChanges(L"Has" #name, L#name); \
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this notifies for "HasX" and "X" simultaneously

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} \
} \
bool Has##name() { return target.Has##name(); }

// Defines a setting that reflects another object's same-named
// setting.
#define OBSERVABLE_PROJECTED_SETTING(target, name) \
_BASE_OBSERVABLE_PROJECTED_SETTING(target, name) \
void Clear##name() \
{ \
const auto hadValue{ target.Has##Name() }; \
target.Clear##name(); \
if (hadValue) \
{ \
_NotifyChanges(L"Has" #name, L#name); \
} \
}

// Defines a setting that reflects another object's same-named
// setting, but which cannot be erased.
#define PERMANENT_OBSERVABLE_PROJECTED_SETTING(target, name) \
_BASE_OBSERVABLE_PROJECTED_SETTING(target, name) \
void Clear##name() {}
13 changes: 13 additions & 0 deletions src/cascadia/TerminalSettingsEditor/ViewModelHelpers.idl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) Microsoft Corporation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.idl.h? what the heck

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YOU CAN DEFINE MACROS FOR THE IDL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHAT

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Licensed under the MIT license.

#pragma once

#define OBSERVABLE_PROJECTED_SETTING(Type, Name) \
Type Name \
{ \
get; \
set; \
}; \
Boolean Has##Name { get; }; \
void Clear##Name()