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.

Sign up for GitHub

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

Jump to bottom

Make Global and Profile settings inheritable #7923

Merged
31 commits merged into from
Oct 27, 2020
Merged

Make Global and Profile settings inheritable #7923

31 commits merged into from
Oct 27, 2020

Conversation

carlos-zamora
Copy link
Member

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

@carlos-zamora
Copy link
Member Author

carlos-zamora commented Oct 14, 2020 •
edited
Loading

TODO for this PR

  • Update IDL with Has... functions
  • Runtime error when _globals->CreateChild() is being assigned (only when loading settings.json, fine otherwise)
  • Update spec and propagate changes from spec
  • Profile has a few settings we handle manually (i.e. BI alignment). I need to hook those up properly too
  • (Bonus Points) maybe make these settings observable (we'll need that soon anyways) Nope

(new items!)

(new items 2!)

  • fix/add tests
  • remove IInspectable crap
  • fix/test Copy

(new items 3!)

  • really do get rid of IReference internally

DHowett
DHowett reviewed Oct 14, 2020
View reviewed changes
src/inc/til/coalesce.h Outdated
// Method Description:
// - Base case provided to throw an assertion if you call coalesce_value(opt, opt, opt)
template<typename T>
T coalesce_value(const winrt::Windows::Foundation::IReference<T>& base)
Copy link
Member

Choose a reason for hiding this comment

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

til cannot have an unguarded reference to IReference. til is conceptually below things that use winrt.

Copy link
Member

Choose a reason for hiding this comment

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

oh i see, you.. did the thing i did in JsonUtils. Clever.

DHowett
DHowett reviewed Oct 14, 2020
View reviewed changes
src/cascadia/TerminalSettingsModel/Profile.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IInheritable.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IInheritable.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IInheritable.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IInheritable.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IInheritable.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/Profile.cpp Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft self-assigned this Oct 15, 2020
@carlos-zamora

This comment has been minimized.

Sign in to view
@DHowett DHowett requested a review from zadjii-msft October 21, 2020 02:55
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 23, 2020
@DHowett
Copy link
Member

DHowett commented Oct 23, 2020 •
edited by carlos-zamora
Loading

must-fix:

  • default profile fails to resolve on empty settings case

pleasant to have:

  • if the p.d block is empty, don't even make a node for it!

DHowett
DHowett reviewed Oct 26, 2020
View reviewed changes
src/cascadia/TerminalSettingsModel/GlobalAppSettings.h

com_ptr<KeyMapping> _keymap;
std::vector<SettingsLoadWarnings> _keybindingsWarnings;

Windows::Foundation::Collections::IMap<hstring, Model::ColorScheme> _colorSchemes;
Windows::Foundation::Collections::IMap<hstring, Model::Command> _commands;

std::optional<hstring> _getUnparsedDefaultProfileImpl() const;
Copy link
Member

Choose a reason for hiding this comment

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

how is this different from GETSET_SETTING?

Copy link
Member Author

Choose a reason for hiding this comment

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

The setter for UnparsedDefaultProfile needs to set _validDefaultProfile to false. We need this to validate that the provided "defaultProfile" value actually maps to something.

DHowett
DHowett approved these changes Oct 26, 2020
View reviewed changes
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.

Let's do this.

src/cascadia/TerminalSettingsModel/Profile.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/Profile.cpp
Comment on lines 258 to 265
// Special case the legacy dynamic profiles here. In this case,
// `this` is a dynamic profile with a source, and our _source is one
// of the legacy DPG namespaces. We're looking to see if the other
// json object has the same guid, but _no_ "source"
if (_Source == WslGeneratorNamespace ||
_Source == AzureGeneratorNamespace ||
_Source == PowershellCoreGeneratorNamespace)
if (mySource == WslGeneratorNamespace ||
mySource == AzureGeneratorNamespace ||
mySource == PowershellCoreGeneratorNamespace)
{
Copy link
Member

Choose a reason for hiding this comment

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

FYI @carlos-zamora this is the code that makes "source" optional for the pre-0.5 dynamic generators

zadjii-msft
zadjii-msft approved these changes Oct 27, 2020
View reviewed changes
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.

Good work landing all this, it's a complicated change in a complicated codebase, but looks like it should work. I'm sure we'll find out very fast if it doesn't 😄

src/cascadia/TerminalSettingsModel/Profile.h
@@ -73,10 +69,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
const Windows::UI::Xaml::VerticalAlignment BackgroundImageVerticalAlignment() const noexcept;
void BackgroundImageVerticalAlignment(const Windows::UI::Xaml::VerticalAlignment& value) noexcept;

GETSET_SETTING(guid, Guid);
GETSET_SETTING(guid, Guid, _GenerateGuidForProfile(Name(), Source()));
Copy link
Member

Choose a reason for hiding this comment

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

I'm shocked that this works, but I get why it does

@zadjii-msft zadjii-msft removed their assignment Oct 27, 2020
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 27, 2020
@ghost
Copy link

ghost commented Oct 27, 2020

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.

@ghost ghost merged commit b603929 into main Oct 27, 2020
@ghost ghost deleted the dev/cazamor/tsm/inheritance branch October 27, 2020 17:35
@carlos-zamora carlos-zamora restored the dev/cazamor/tsm/inheritance branch October 27, 2020 17:35
@carlos-zamora carlos-zamora deleted the dev/cazamor/tsm/inheritance branch October 27, 2020 17:44
@carlos-zamora carlos-zamora mentioned this pull request Oct 29, 2020
Open
@ghost
Copy link

ghost commented Nov 11, 2020

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

Handy links:

ghost pushed a commit that referenced this pull request Nov 17, 2020
@carlos-zamora
Add Serializer to CascadiaSettings (#8018)
6b503ba 
##  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
@carlos-zamora carlos-zamora mentioned this pull request Nov 30, 2020
Merged
3 tasks
ghost pushed a commit that referenced this pull request Dec 1, 2020
@carlos-zamora
Always create and link profiles.defaults object (#8445)
6213172 
The Settings UI exposes the `profiles.defaults` (PD) object. Today, we
remove PD if there's nothing in it. However, that causes problems with
the Settings UI, because we have no `Profile` object to bind to
(resulting in a crash). Rather than making the Settings UI create a PD,
and link it in the inheritance tree, it's much easier to just _always_
create and link the PD object.

## References
#1564 - Settings UI (fixes a crash for this)
#7923 - Introduces inheritance

## PR Checklist
* [X] Tests added/passed

## Validation Steps Performed
* [x] repro steps for crash in Settings UI (copied changes over to that
      branch for testing)
* [x] tests passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@DHowett DHowett DHowett approved these changes

Assignees
No one assigned
Labels
Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carlos-zamora @DHowett @zadjii-msft