doc: Introduce Inheritance Addendum to TSM Spec by carlos-zamora · Pull Request #7876 · microsoft/terminal

carlos-zamora · 2020-10-09T23:25:38Z

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

carlos-zamora · 2020-10-09T23:51:03Z

Idea from Dustin (soon to be added):

Introduce a function similar to LayerJson(Json::Value) that layers a resolved settings object onto another one. This way, when we have profile.defaults resolved to a Profile object, we can just layer that onto other Profile objects easily, rather than re-parsing the profile.defaults json blob.

DHowett

I'm very enamoured with this spec. I love it.

It might be worth calling out that this moves cascading into the object model instead of keeping it as a json-only concept.

I'd also suggest calling out adding an API to CascadiaSettings that gets a reference to the Default profile (profiles.defaults). That'll enable #7414's implementation to spawn incoming commandline app tabs w/ the default profile, for example.

It's clever, putting this in the object model, because any update to a lower layer object will automatically be reflected in objects on top of it, unless they're overriding it already.

Love love love the use of til::coalesce here (thanks @zadjii-msft for splitting it up and putting it in til!)

DHowett · 2020-10-11T02:17:50Z

Incidentally, this will be a path towards fixing #5276 (because clone/inherit/apply is the same as #5276 (comment))

DHowett · 2020-10-12T05:12:52Z

Naming: Clone and Copy are the same thing... so maybe this one needs to be something specific like static Profile Profile::CreateInheritedProfile or static Profile Profile::Inherit. Naming is the hardest part.

zadjii-msft

These are notes from the meeting we just had. Dunno how many you wrote down, but wanted to make sure they were captured

doc/specs/#885 - Terminal Settings Model.md

DHowett

found a fairly large issue w/ deep copy.

DHowett

still blocking - cz may be underestimating how important copy treating parent correctly is. chatted o*line.

DHowett

Looks great!

DHowett · 2020-10-20T19:00:29Z

you may need to add the vsdx file to the spellcheck excluder

miniksa

I'm good with this. Looks like what we discussed at the meeting. My eyes glazed at "directed acyclic graph" captain math, but I trust you there.

doc/specs/#885 - Terminal Settings Model/#885 - Terminal Settings Model.md

…/microsoft/terminal into dev/cazamor/spec/tsm-inheritance

ghost · 2020-10-22T16:23:44Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

## 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

## 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

Introduce Inheritance Addendum to TSM Spec

06d2d38

carlos-zamora force-pushed the dev/cazamor/spec/tsm-inheritance branch from 6d30d5f to 06d2d38 Compare October 9, 2020 23:26

carlos-zamora requested review from DHowett, leonMSFT and zadjii-msft October 9, 2020 23:26

carlos-zamora added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 9, 2020

DHowett reviewed Oct 11, 2020

View reviewed changes

Apply Dustin's feedback

b2fc19a

zadjii-msft self-assigned this Oct 12, 2020

zadjii-msft reviewed Oct 13, 2020

View reviewed changes

carlos-zamora mentioned this pull request Oct 14, 2020

Make Global and Profile settings inheritable #7923

Merged

zadjii-msft removed their assignment Oct 15, 2020

DHowett requested changes Oct 16, 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 Oct 16, 2020

address PR comments

fc9eb71

DHowett requested changes Oct 16, 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 16, 2020

deep copy

d724651

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

DHowett approved these changes Oct 20, 2020

View reviewed changes

miniksa approved these changes Oct 20, 2020

View reviewed changes

zadjii-msft reviewed Oct 21, 2020

View reviewed changes

doc/specs/#885 - Terminal Settings Model/#885 - Terminal Settings Model.md Outdated Show resolved Hide resolved

zadjii-msft assigned carlos-zamora Oct 21, 2020

polish

c1c34ca

carlos-zamora assigned zadjii-msft and unassigned carlos-zamora Oct 21, 2020

zadjii-msft approved these changes Oct 21, 2020

View reviewed changes

microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27

zadjii-msft assigned carlos-zamora and unassigned zadjii-msft Oct 22, 2020

carlos-zamora added 3 commits October 22, 2020 09:21

geeksforgeeks is a word

e9c17e2

Merge branch 'dev/cazamor/spec/tsm-inheritance' of https://github.com…

13f84ca

…/microsoft/terminal into dev/cazamor/spec/tsm-inheritance

Merge branch 'main' into dev/cazamor/spec/tsm-inheritance

d01b2a2

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 22, 2020

carlos-zamora mentioned this pull request Oct 22, 2020

Add Serializer to CascadiaSettings #8018

Merged

1 task

DHowett changed the title ~~Introduce Inheritance Addendum to TSM Spec~~ doc: Introduce Inheritance Addendum to TSM Spec Oct 26, 2020

DHowett merged commit 8700499 into main Oct 26, 2020

DHowett deleted the dev/cazamor/spec/tsm-inheritance branch October 26, 2020 23:22

carlos-zamora mentioned this pull request Oct 29, 2020

Implement inheritance/layering behavior for settings lists/maps #8100

Open

Conversation

carlos-zamora commented Oct 9, 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 Oct 9, 2020

Idea from Dustin (soon to be added):

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Oct 12, 2020

Uh oh!

zadjii-msft 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!

DHowett 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!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett commented Oct 20, 2020

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Oct 22, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlos-zamora commented Oct 9, 2020 •

edited by DHowett

Loading

DHowett commented Oct 11, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.