-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
doc: Introduce Inheritance Addendum to TSM Spec #7876
Conversation
6d30d5f
to
06d2d38
Compare
Idea from Dustin (soon to be added):Introduce a function similar to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!)
Incidentally, this will be a path towards fixing #5276 (because clone/inherit/apply is the same as #5276 (comment)) |
Naming: Clone and Copy are the same thing... so maybe this one needs to be something specific like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are notes from the meeting we just had. Dunno how many you wrote down, but wanted to make sure they were captured
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found a fairly large issue w/ deep copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still blocking - cz may be underestimating how important copy
treating parent
correctly is. chatted o*line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
you may need to add the vsdx file to the spellcheck excluder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
Hello @carlos-zamora! Because this pull request has the 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 (
|
## 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
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 thevalue and structure of the settings model, whereas
Clone()
is used tocopy a reference to the settings model and build an inheritance tree.
References
#6904 - Terminal Settings Model Spec
#1564 - Settings UI