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

Add Spec for winrt TerminalSettings #6904

Merged
merged 12 commits into from
Oct 9, 2020

Conversation

carlos-zamora
Copy link
Member

Summary of the Pull Request

This introduces a spec for (what I like to call) winrt TerminalSettings. Basically, we need to move over some of the code that resides in TerminalApp that relates to the settings model, then expose some of the settings objects as winrt objects. Doing so will allow us to access/modify settings across different project layers (a must-have for the Settings UI).

References

#885 - winrt Terminal Settings issue
#1564 - spec for most of the backend work for Settings UI

@carlos-zamora carlos-zamora added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Jul 13, 2020
@carlos-zamora carlos-zamora requested a review from DHowett July 13, 2020 21:14
@zadjii-msft
Copy link
Member

Gonna throw this out there without reading the spec yet - AppSettings. That way we avoid the redundant Terminal - Microsoft.Terminal.AppSettings

@DHowett
Copy link
Member

DHowett commented Jul 13, 2020

should the namespace and DLL be Microsoft.Terminal.Settings.Model for completeness' sake?

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I have thoughts, so let's discuss. I think the winrt-ification of the app settings is important, but I think we can do that without mucking up the layering too much.

doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
@ghost 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 Jul 14, 2020
@DHowett
Copy link
Member

DHowett commented Jul 15, 2020

I think I'm fine with this. It's a very small spec ;P

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

chatted; i'd like this spec to flesh out the model API a little more. How does the app get a settings object? How does it save it? Who provides the json files? Who listens for json file changes?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 17, 2020
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/spec/settings-ui-architecture branch from 6e3af87 to 5cb8f6e Compare July 18, 2020 02:28
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 18, 2020
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 20, 2020
@DHowett DHowett removed their assignment Jul 20, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 20, 2020
ghost pushed a commit that referenced this pull request Aug 3, 2020
Move TerminalSettings object from TerminalSettings project
(Microsoft.Terminal.Settings) to TerminalApp project. `TerminalSettings`
specifically operates as a bridge that exposes any necessary information
to a TerminalControl.

Closes #7139 
Related Epic: #885
Related Spec: #6904

## PR Checklist
* [X] Closes #7139 
* [X] CLA signed
* [X] Tests ~added~/passed (no additional tests necessary)
* [X] ~Documentation updated~
* [X] ~Schema updated~

## Validation Steps Performed
Deployed Windows Terminal and opened a few new tabs.
ghost pushed a commit that referenced this pull request Aug 7, 2020
## Summary of the Pull Request
Move `ICoreSettings` and `IControlSettings` from the TerminalSettings project to the TerminalCore and TerminalControl projects respectively. Also entirely removes the TerminalSettings project.

The purpose of these interfaces is unchanged. `ICoreSettings` is used to instantiate a terminal. `IControlSettings` (which requires an `ICoreSettings`) is used to instantiate a UWP terminal control.

## References
Closes #7140 
Related Epic: #885 
Related Spec: #6904 

## PR Checklist
* [X] Closes #7140 
* [X] CLA signed
* [X] Tests ~added~/passed (no additional tests necessary)
* [X] ~Documentation updated~
* [X] ~Schema updated~

## Detailed Description of the Pull Request / Additional comments
A lot of the work here was having to deal with winmd files across all of these projects. The TerminalCore project now outputs a Microsoft.Terminal.TerminalControl.winmd. Some magic happens in TerminalControl.vcxproj to get this to work properly.

## Validation Steps Performed
Deployed Windows Terminal and opened a few new tabs.
ghost pushed a commit that referenced this pull request Aug 10, 2020
## Summary of the Pull Request

Adds the `setColorScheme` action, to change the color scheme of the active control to one given by the `name` parameter. `name` is required. If `name` is not the name of a color scheme, the action does nothing.

## References

* Being done as a stepping stone to #6689 

## PR Checklist
* [x] Closes #5401
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Technically, the action is being done by changing the settings of the current `TerminalSettings` of the `TermControl`. Frankly, it should be operating on a copy of the `TermControl`'s `IControlSettings`, then updating the control's settings, or the Control should just listen for changes to it's setting's properties, and update in real time (without a manual call to `UpdateSettings`. However, both those paths are somewhere unknowable beyond #6904, so we'll just do this for now.

## Validation Steps Performed

* tested manually with a scheme that exists
* tested manually with a scheme that doesn't exist
@carlos-zamora
Copy link
Member Author

The section on keybindings was discussed with Griese. The presented code is an excerpt from that discussion.

@carlos-zamora carlos-zamora requested a review from a team August 31, 2020 21:35
@carlos-zamora carlos-zamora self-assigned this Sep 10, 2020
@carlos-zamora
Copy link
Member Author

Next step: Add a section on how to identify where a setting comes from (and exposing that info to the TerminalSettingsEditor).

Sample Scenario The TerminalSettingsEditor displays the following color scheme for each profile object
    "defaults": "Solarized", // profiles.defaults
    "A": "Raspberry", // profile A
    "B": "Tango", // profile B
    "C": "Solarized" // profile C

If profiles.defaults gets changed to "Tango", the following occurs

    "defaults": "Tango",
    "A": "Raspberry",
    "B": "Tango",
    "C": "Tango"

If profiles.defaults then gets changed to "Campbell", the following occurs

    "defaults": "Campbell",
    "A": "Raspberry",
    "B": "Tango", // Tango or Campbell?
    "C": "Campbell" // Tango or Campbell?

If you look at the configuration of settings.json, this is easy to figure out:

{
    "profiles":
    {
        "defaults": {
            // Scenarios:
            //  1. Solarized
            //  2. Tango
            //  3. Campbell
            "colorScheme": "Tango"
        },
        "list": [
            {
                "profile": "A",
                "colorScheme": "Raspberry"
            },
            {
                "profile": "B",
                "colorScheme": "Tango"
            },
            {
                "profile": "C"
            }
        ]
    }
}

But we need the TerminalSettingsEditor to indicate the source of the fallback value.

NOTE: recording fallback source is important for the following scenarios

  • profiles.defaults
  • copyFormatting keybinding arg
  • Theming
  • Keybindings

@carlos-zamora
Copy link
Member Author

Next step: Add a section on how to identify where a setting comes from (and exposing that info to the TerminalSettingsEditor).

Sample Scenario
NOTE: recording fallback source is important for the following scenarios

  • profiles.defaults
  • copyFormatting keybinding arg
  • Theming
  • Keybindings

This should be an addendum. And I don't want to hold up #7667.

This shouldn't be news to most of the team since I presented it a while back.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I don't believe anything I have here rises above a nit. I'm not interested in bogging this spec down in the bureaucracy of perfection any longer, let's get 'er moving

doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
doc/specs/#885 - winrt Terminal Settings.md Outdated Show resolved Hide resolved
carlos-zamora added a commit that referenced this pull request Oct 6, 2020
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:
1. 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.
2. `enum LaunchMode` was moved from TerminalApp to TSM
3. `AzureConnectionType` and `TelnetConnectionType` were moved from the
   profile generators to their respective TerminalConnections
4. CascadiaSettings' `SettingsPath` and `DefaultSettingsPath` are
   exposed as `hstring` instead of `std::filesystem::path`
5. `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.
6. The localization resources had to be split into two halves.
   - Resource file linked in init.cpp. Verified at runtime thanks to the
     StaticResourceLoader.
7. Added constructors to some `ActionArgs`
8. 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
Copy link
Member Author

Discussed offline that we'll merge this one in with 2 approvals. I'll create addendums to this spec.

@carlos-zamora carlos-zamora merged commit f0b8875 into master Oct 9, 2020
@carlos-zamora carlos-zamora deleted the dev/cazamor/spec/settings-ui-architecture branch October 9, 2020 18:19
skyline75489 pushed a commit to skyline75489/terminal that referenced this pull request Oct 10, 2020
## Summary of the Pull Request
This introduces a spec for (what I like to call) winrt TerminalSettings. Basically, we need to move over some of the code that resides in TerminalApp that relates to the settings model, then expose some of the settings objects as winrt objects. Doing so will allow us to access/modify settings across different project layers (a must-have for the Settings UI).

## References
microsoft#885 - winrt Terminal Settings issue
microsoft#1564 - spec for most of the backend work for Settings UI
DHowett pushed a commit that referenced this pull request Oct 26, 2020
This introduces an addendum to the Terminal Settings Model spec that
covers inheritance and fallback. Basically, settings objects will now
have a reference to a parent object. If the settings object does not
have a setting defined, it will ask its parent to resolve the value. A
parent is set using the `Clone()` function. `Copy()` is used to copy the
value and structure of the settings model, whereas `Clone()` is used to
copy a reference to the settings model and build an inheritance tree.

## References
#6904 - Terminal Settings Model Spec
#1564 - Settings UI
ghost pushed a commit that referenced this pull request Oct 27, 2020
## Summary of the Pull Request
Introduces `IInheritable` as an interface that helps move cascading settings into the Terminal Settings Model. `GlobalAppSettings` and `Profile` both are now `IInheritable`. `CascadiaSettings` was updated to `CreateChild()` for globals and each profile when we are loading the JSON data.

IInheritable does most of the heavy lifting. It introduces a two new macros and the interface. The macros help implement the fallback functionality for nullable and non-nullable settings.

## References
#7876 - Spec Addendum
#6904 - TSM Spec
#1564 - Settings UI

#7876 - `Copy()` needs to be updated to include _parent
ghost pushed a commit that referenced this pull request Nov 17, 2020
##  Summary of the Pull Request
This adds `ToJson` functions to `Profile`, `GlobalAppSettings`, and `ColorScheme`. They are used in `CascadiaSettings` to completely serialize an instance of the settings model. Thanks to #7923, all of the settings are `std::optional`, and `JsonUtils` only writes out values that are actually populated.

`CascadiaSettings::WriteSettingsToDisk` serializes the current settings and writes them to the settings.json. A backup file is created with your old contents.

#### Limitations:
- all of the color schemes are serialized regardless of them coming from defaults.json or settings.json
- keybindings/actions are copied/pasted

## References
#1564 - Settings UI
TSM Specs (#6904 and #7876)

## PR Checklist
* [x] Tests added/passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants