Introduce TerminalSettingsModel project by carlos-zamora · Pull Request #7667 · microsoft/terminal

carlos-zamora · 2020-09-18T05:38:20Z

Introduces a new TerminalSettingsModel (TSM) project. This project is
responsible for (de)serializing and exposing Windows Terminal's settings
as WinRT objects.

References

#885: TSM epic
#1564: Settings UI is dependent on this for data binding and settings access
#6904: TSM Spec

In the process of ripping out TSM from TerminalApp, a few other changes
were made to make this possible:

AppLogic's ApplicationDisplayName and ApplicationVersion was
moved to CascadiaSettings
- These are defined as static functions. They also no longer check if
  AppLogic::Current() is nullptr.
enum LaunchMode was moved from TerminalApp to TSM
AzureConnectionType and TelnetConnectionType were moved from the
profile generators to their respective TerminalConnections
CascadiaSettings' SettingsPath and DefaultSettingsPath are
exposed as hstring instead of std::filesystem::path
Command::ExpandCommands() was exposed via the IDL
- This required some of the warnings to be saved to an IVector
  instead of std::vector, among some other small changes.
The localization resources had to be split into two halves.
- Resource file linked in init.cpp. Verified at runtime thanks to the
  StaticResourceLoader.
Added constructors to some ActionArgs
Utils.h/cpp were moved to cascadia/inc. JsonKey() was moved to
JsonUtils. Both TermApp and TSM need access to Utils.h/cpp.

A large amount of work includes moving to the new namespace
(TerminalApp --> Microsoft::Terminal::Settings::Model).

Fixing the tests had its own complications. Testing required us to split
up TSM into a DLL and LIB, similar to TermApp. Discussion on creating a
non-local test variant can be found in #7743.

Closes #885

carlos-zamora · 2020-09-18T05:42:00Z

Note for Reviewers

This PR is still in draft because I need to fix all of the tests. That shouldn't be too difficult. Aside from that, this PR is ready to go!

EDIT: Since the tests need access to the implementation types, I'll have to break up TSM into 2 projects: TSM and TSMLib.

leonMSFT

I'm curious about what you said about how the "localization resources had to be split into two halves".

I took a look at init.cpp but wasn't quite sure how it's linking the resource file, could you briefly explain?
What if TerminalSettingsEditor also needed to have its own localization resources, would it have to do something similar?

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalSettingsModel/TerminalSettingsModel.vcxproj

DHowett · 2020-09-18T19:11:59Z

src/cascadia/TerminalSettingsModel/TerminalSettingsModel.vcxproj

+<?xml version="1.0" encoding="utf-8"?>
+<Project DefaultTargets="Build" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <PropertyGroup Label="Globals">
+    <ProjectGuid>{CA5CAD1A-d7ec-4107-b7c6-79cb77ae2907}</ProjectGuid>


i hope this is unique.

This used to be TerminalSettings' guid. Is that a problem?

DHowett

96/102

src/cascadia/TerminalSettingsModel/pch.h

src/cascadia/TerminalSettingsModel/init.cpp

src/cascadia/TerminalSettingsModel/Profile.cpp

src/cascadia/TerminalSettingsModel/KeyMapping.h

src/cascadia/TerminalSettingsModel/KeyChordSerialization.h

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

src/cascadia/TerminalSettingsModel/ActionArgs.h

src/cascadia/TerminalApp/Tab.cpp

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

src/cascadia/TerminalApp/TerminalAppLib.vcxproj

zadjii-msft

Okay, I'm done with all 133 in this PR, and my only concern is over the same utils.cpp thing Dustin brought up

DHowett

hitting submit b/c queued comments?

carlos-zamora · 2020-09-29T19:10:16Z

Changes for the next commit(s):

Try to kill JsonKey (or move to JsonUtils)
Move VisualizeControlCodes into TIL
Refactor Command to expose icon like Profile does (loading image happens outside of settings model)

carlos-zamora · 2020-09-30T00:34:54Z

Try to kill JsonKey (or move to JsonUtils)

Oh...I already had it in JsonUtils.

Move VisualizeControlCodes into TIL

Moved and added a test. Had to remove a noexcept because of audit mode.

Refactor Command to expose icon like Profile does (loading image happens outside of settings model)

CommandPalette data binds itself to the IconSource of Command. Refactoring such that that isn't that case felt a bit more risky than I'd like it to be for this PR. I still moved RefreshIcon() from Command into TerminalPage, which should be good enough tbh. I'll open a follow-up item if you feel strongly about this though.

EDIT: yeah, that needs to happen. Just a bit big for this PR. Filed #7784 as a follow-up.

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp

src/cascadia/LocalTests_TerminalApp/TabTests.cpp

src/cascadia/TerminalSettingsModel/Profile.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalSettingsModel/ColorScheme.h

DHowett · 2020-10-01T21:44:47Z

src/cascadia/TerminalSettingsModel/KeyChordSerialization.h

+{
+    struct KeyChordSerialization
+    {
+        KeyChordSerialization() = default;


I don't recall this conversation coming to a close. Did you try it without both the factory and the default ctor? There must be a way to do this.,

src/cascadia/TerminalSettingsModel/dll/Microsoft.Terminal.Settings.Model.vcxproj

src/cascadia/TerminalSettingsModel/dll/pch.h

carlos-zamora · 2020-10-02T03:43:17Z

Urgh. Technically, y'all should be reading/merging #6904 first. So go read that * grumble *

src/cascadia/TerminalApp/DebugTapConnection.cpp

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

src/cascadia/TerminalSettingsModel/KeyMappingSerialization.cpp

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

#7796 and #7667 were being implemented concurrently. As a part of #7667, Command was moved from TermApp to TSM. This just applies that change to a line we missed in #7796 and fixes the build break.

## Summary of the Pull Request Introduce the `IconPathConverter` to `TerminalApp`. `Command` and `Profile` now both return the unexpanded icon path. `IconPathConverter` is responsible for expanding the icon path and retrieving the appropriate icon source. This also removes `Profile`'s expanded icon path and uses the `IconPathConverter` when necessary. This allows users to set profile icons to emoji as well. However, emoji do not appear in the jumplist. ## References Based on #7667 ## PR Checklist * [X] Closes #7784 * [x] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [x] Schema updated. ## Validation Steps Performed Deploy succeeded.

microsoft#7796 and microsoft#7667 were being implemented concurrently. As a part of microsoft#7667, Command was moved from TermApp to TSM. This just applies that change to a line we missed in microsoft#7796 and fixes the build break.

## Summary of the Pull Request Introduce the `IconPathConverter` to `TerminalApp`. `Command` and `Profile` now both return the unexpanded icon path. `IconPathConverter` is responsible for expanding the icon path and retrieving the appropriate icon source. This also removes `Profile`'s expanded icon path and uses the `IconPathConverter` when necessary. This allows users to set profile icons to emoji as well. However, emoji do not appear in the jumplist. ## References Based on microsoft#7667 ## PR Checklist * [X] Closes microsoft#7784 * [x] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [x] Schema updated. ## Validation Steps Performed Deploy succeeded.

## Summary of the Pull Request This implements the `Copy` function for `CascadiaSettings`. Copy performs a deep copy of a `CascadiaSettings` object. This is needed for data binding in the Terminal Settings Editor. The `Copy` function was basically implemented in every settings model object. This was mostly just repetitive work. ## References #7667 - TSM #1564 - Settings UI ## PR Checklist * [X] Tests added/passed

ghost · 2020-11-11T18:11:14Z

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Scenario Product-Terminal The new Windows Terminal. labels Sep 18, 2020

carlos-zamora force-pushed the dev/cazamor/set/terminal-settings-model branch from 183cff4 to 99d3d4c Compare September 18, 2020 05:41

leonMSFT reviewed Sep 18, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

DHowett reviewed Sep 18, 2020

View reviewed changes

src/cascadia/TerminalSettingsModel/TerminalSettingsModel.vcxproj Outdated Show resolved Hide resolved

DHowett reviewed Sep 18, 2020

View reviewed changes

DHowett requested changes Sep 18, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 18, 2020

carlos-zamora mentioned this pull request Sep 25, 2020

Convert LocalTests_SettingsModel into UnitTests_SettingsModel #7743

Open

carlos-zamora marked this pull request as ready for review September 25, 2020 22:41

carlos-zamora added 4 commits September 25, 2020 15:42

Introduce TerminalSettingsModel project

e939039

PR Comments - Dustin

da06885

break up TSM into LIB + DLL

212bac7

Fix and break up local tests

99b126c

carlos-zamora force-pushed the dev/cazamor/set/terminal-settings-model branch from 14f405c to 99b126c Compare September 25, 2020 22:43

DHowett reviewed Sep 26, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalAppLib.vcxproj Outdated Show resolved Hide resolved

zadjii-msft reviewed Sep 29, 2020

View reviewed changes

DHowett reviewed Sep 29, 2020

View reviewed changes

carlos-zamora added 3 commits September 29, 2020 14:29

move VisualizeControlCodes to TIL

905de27

move inc/utils to TermApp

0247a48

fix audit mode

9bb94d6

fix build + remove GUID comparison from Utils

3630e11

zadjii-msft mentioned this pull request Oct 1, 2020

Add a setting to configure the audible bell #7793

Merged

4 tasks

zadjii-msft approved these changes Oct 1, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

leonMSFT approved these changes Oct 1, 2020

View reviewed changes

DHowett requested changes Oct 1, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 1, 2020

Address Dustin's PR Comments

ed979dd

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 2, 2020

carlos-zamora assigned DHowett Oct 2, 2020

carlos-zamora mentioned this pull request Oct 2, 2020

Add Spec for winrt TerminalSettings #6904

Merged

add a comment for KeyChordSerialization 'unnecessary' constructor

8ed84b0

DHowett reviewed Oct 5, 2020

View reviewed changes

src/cascadia/TerminalApp/DebugTapConnection.cpp Outdated Show resolved Hide resolved

DHowett reviewed Oct 5, 2020

View reviewed changes

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp Outdated Show resolved Hide resolved

DHowett reviewed Oct 5, 2020

View reviewed changes

src/cascadia/TerminalSettingsModel/KeyMappingSerialization.cpp Outdated Show resolved Hide resolved

remove inc/Utils references

bdeb44a

carlos-zamora mentioned this pull request Oct 5, 2020

Introduce IconConverter #7830

Merged

3 tasks

DHowett reviewed Oct 5, 2020

View reviewed changes

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp Show resolved Hide resolved

DHowett approved these changes Oct 5, 2020

View reviewed changes

carlos-zamora merged commit 2608e94 into master Oct 6, 2020

carlos-zamora deleted the dev/cazamor/set/terminal-settings-model branch October 6, 2020 16:57

carlos-zamora restored the dev/cazamor/set/terminal-settings-model branch October 6, 2020 16:57

carlos-zamora deleted the dev/cazamor/set/terminal-settings-model branch October 6, 2020 16:57

carlos-zamora mentioned this pull request Oct 6, 2020

Feature Request - Implement Settings UI #1564

Closed

34 tasks

mpela81 mentioned this pull request Oct 7, 2020

Add Close... option to Tab context menu #7728

Merged

6 tasks

carlos-zamora mentioned this pull request Oct 8, 2020

Fix old reference to TermApp::Command #7863

Merged

carlos-zamora mentioned this pull request Oct 10, 2020

Implement CascadiaSettings::Copy() #7877

Merged

1 task

ghost mentioned this pull request Nov 11, 2020

Replace TerminalSettings runtimeclasses with interfaces only #885

Closed

4 tasks

Conversation

carlos-zamora commented Sep 18, 2020 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Uh oh!

carlos-zamora commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note for Reviewers

Uh oh!

leonMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DHowett Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlos-zamora commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett Oct 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carlos-zamora commented Oct 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Nov 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlos-zamora commented Sep 18, 2020 •

edited by DHowett

Loading

carlos-zamora commented Sep 18, 2020 •

edited

Loading

carlos-zamora commented Sep 29, 2020 •

edited

Loading

carlos-zamora commented Sep 30, 2020 •

edited

Loading