Add Cascading User + Default Settings #2515

zadjii-msft · 2019-08-22T22:33:46Z

Summary of the Pull Request

This PR represents the start of the work on Cascading User + default settings, #754.

Cascading settings will be done in two parts:

Layered Default+User settings (this PR)
Dynamic Profile Generation (Add Dynamic Profile Generators #2603).

Until both are done, _neither are going in. The dynamic profiles PR will target this PR when it's ready, but will go in as a separate commit into master.

This PR covers adding one primary feature: the settings are now in two separate files:

a static defaults.json that ships with the package (the "default settings")
a profiles.json with the user's customizations (the "user settings)

User settings are layered upon the settings in the defaults settings.

References

Other things that might be related here:

Try to flow together JSON serialization for keybindings #1378 - This seems like it's definitely fixed. The default keybindings are much cleaner, and without the save-on-load behavior, the user's keybindings will be left in a good state
Float values in settings are too precise (especially for people who don't know IEEE854) #1398 - This might have honestly been solved by Just never save the settings #2475

PR Checklist

Closes Feature Request: Cascading settings (with a hardcoded default.json?) #754
Closes Try to flow together JSON serialization for keybindings #1378
Closes Automatically merge in new keybindings #2566
I work here
Tests added/passed
Requires documentation to be updated - it ABSOLUTELY DOES

Detailed Description of the Pull Request / Additional comments

We start by taking all of the FromJson functions in Profile, ColorScheme, Globals, etc, and converting them to LayerJson methods. These are effectively the same, with the change that instead of building a new object, they are simply layering the values on top of this object.
Next, we add tests for layering properties like that.
Now, we add a defaults.json to the package. This is the file the users can refer to as our default settings.
We then take that defaults.json and stamp it into an auto generated .h file, so we can use it's data without having to worry about reading it from disk.
We then change the LoadAll function in CascadiaSettings. Now, the function does two loads - one from the defaults, and then a second load from the profiles.json file, layering the settings from each source upon the previous values.
If the profiles.json file doesn't exist, we'll create it from a hardcoded userDefaults.json, which is stamped in similar to how defaults.json is.
We also add support for unbinding keybindings that might exist in the defaults.json, but the user doesn't want to be bound to anything.
We add support for hiding a profile, which is useful if a user doesn't want one of the default profiles to appear in the list of profiles.

TODO:

Still need to make Alt+Click work on the settings button
Need to write some user documentation on how the new settings model works
Fix the pair of tests I broke (re: Duplicate profiles)

* Moves tests into individual classes * adds support for layering a colorscheme on top of another

…yer-settings

must have committed without staging this change, uh oh. Not like those tests actually work so nbd

This is like 80% of #754. Needs tests.

* add support to unbind a key with `null` or `"unbound"` or `"garbage"`

In the end, I think we need to ask _was this worth it_

Add a MsBuild target to auto-generate a header with the defaults.json as a string in the file. That way, we can _always_ load the defaults. Literally impossible to not.

…yer-settings

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

src/cascadia/TerminalApp/GlobalAppSettings.cpp

src/cascadia/TerminalApp/defaults.json

src/cascadia/ut_app/JsonTests.cpp

…yer-settings # Conflicts: # doc/user-docs/UsingJsonSettings.md

carlos-zamora · 2019-09-12T22:56:29Z

src/cascadia/TerminalApp/CascadiaSettings.cpp

+        const auto guid = guidOrder.at(gIndex);
+        for (size_t pIndex = gIndex; pIndex < _profiles.size(); pIndex++)
+        {
+            auto profileGuid = _profiles.at(pIndex).GetGuid();


I think we don't expect that many profiles so it shouldn't be a big concern for now. I'm fine with this being a separate PR. Just attach a GitHub issue to it.

carlos-zamora · 2019-09-12T23:22:13Z

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

+}
+
+// Function Description:
+// - Gets the object in the given JSON obejct under the "profiles" key. Returns


I think you missed this one haha (mispelled object)

src/cascadia/LocalTests_TerminalApp/ColorSchemeTests.cpp

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

tools/GenerateHeaderForJson.ps1

miniksa · 2019-09-16T16:18:37Z

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

    THROW_LAST_ERROR_IF(fileSize == INVALID_FILE_SIZE);

    auto utf8buffer = std::make_unique<char[]>(fileSize);

    DWORD bytesRead = 0;
-    THROW_LAST_ERROR_IF(!ReadFile(hFile.get(), utf8buffer.get(), fileSize, &bytesRead, nullptr));
+    THROW_LAST_ERROR_IF(!ReadFile(hFile, utf8buffer.get(), fileSize, &bytesRead, nullptr));


Why wasn't this THROW_IF_WIN32_BOOL_FALSE?

_**This PR targets the #2515 PR**_. It does that for the sake of diffing. When this PR and #2515 are both ready, I'll merge #2515 first, then change the target of this branch, and merge this one.  ## Summary of the Pull Request This PR adds support for "dynamic profiles", in accordance with the [Cascading Settings Spec](https://github.com/microsoft/terminal/blob/master/doc/cascadia/Cascading-Default-Settings.md#dynamic-profiles). Currently, we have three types of default profiles that fit the category of dynamic profile generators. These are profiles that we want to create on behalf of the user, but require runtime information to be able to create correctly. Because they require runtime information, we can't ship a static version of these profiles as a part of `defaults.json`. These three profile generators are: * The Powershell Core generator * The WSL Distro generator * The Azure Cloud Shell generator  ## References  ## PR Checklist * [x] Closes #754 * [x] I work here * [x] look at all these **Tests** * [x] Requires documentation to be updated - This is done as part of the parent PR  ## Detailed Description of the Pull Request / Additional comments We want to be able to enable the user to edit dynamic profiles that are generated from DPGs. When dynamic profiles are added, we'll add entries for them to the user's `profiles.json`. We do this _without re-serializing_ the settings. Instead, we insert a partial serialization for the profile into the user's settings. ### Remaining TODOs: * Make sure that dynamic profiles appear in the right place in the order of profiles -> #2722 * [x] don't serialize the `colorTable` key for dynamic profiles. * [x] re-parse the user settings string if we've changed it. * Handle changing the default profile to pwsh if it exists on first launch, or file a follow-up issue -> #2721  ## Validation Steps Performed <hr> * Create profiles by layering them * Update test to layer multiple times on the same profile * Add support for layering an array of profiles, but break a couple tests * Add a defaults.json to the package * Layer colorschemes * Moves tests into individual classes * adds support for layering a colorscheme on top of another * Layer an array of color schemes * oh no, this was missed with #2481 must have committed without staging this change, uh oh. Not like those tests actually work so nbd * Layer keybindings * Read settings from defaults.json + profiles.json, layer appropriately This is like 80% of #754. Needs tests. * Add tests for keybindings * add support to unbind a key with `null` or `"unbound"` or `"garbage"` * Layer or clear optional properties * Add a helper to get an optional variable for a bunch of different types In the end, I think we need to ask _was this worth it_ * Do this with the stretch mode too * Add back in the GUID check for profiles * Add some tests for global settings layering * M A D W I T H P O W E R Add a MsBuild target to auto-generate a header with the defaults.json as a string in the file. That way, we can _always_ load the defaults. Literally impossible to not. * When the user's profile.json doesn't exist, create it from a template * Re-order profiles to match the order set in the user's profiles.json * Add tests for re-ordering profiles to match user ordering * Add support for hiding profiles using `"hidden": true` * Use the hardcoded defaults.json for the exception->"use defaults" case * Somehow I messed up the git submodules? * woo documentation * Fix a Terminal.App.Unit.Tests failure * signed/unsigned is hard * Use Alt+Settings button to open the default settings * Missed a signed/unsigned * Start dynamically creating profiles * Give the inbox generators a namespace and generally hack this a lot less * Some very preliminary PR feedback * More PR feedback Use the wil helper for the exe path Move jsonutils into their own file kill some dead code * Add templates to these bois * remove some code for generating defaults, reorder defaults.json a tad * Make guid a std::optional * Large block of PR feedback * Remove some dead code * add some comments * tag some todos * stl is love, stl is life * Serialize the source key * Make the Azure cloud shell a dynamic profile * Make the built-in namespaces public * Add a mechanism for quick-diffing a profile This will be used to generate the json snippets for dynamically generated profiles. * Generate partial serializations of dynamic profiles _not_ in the user settings * Start writing tests for generating dyn profiles * dyn profiles generate GUIDs based on _source * we won't run DPGs when they'd disabled? * Add more DPG tests - TestDontRunDisabledGenerators * Don't layer profiles with a source that's also different * Add another test, DoLayerUserProfilesOnDynamicsWhenSourceMatches * Actually insert new dynamic profiles into the file * Minor cleanup of `Profile::ShouldBeLayered` * Migrate legacy profiles gracefully * using namespace winrt::Windows::UI::Xaml; * _Only_ layer dynamic profiles from user settings, never create * Write a test for migrating dynamic profiles * Comments for dayssssss * add `-noprofile` * Fix the crash that dustin found * -Encoding ASCII * Set a profile's default scheme to Campbell * Fix the tests I regressed * Update UsingJsonSetting.md to reflect that changes from these PRs * Change how GenerateGuidForProfile works * Make AppKeyBindings do its own serialization * Remove leftover dead code from the previous commit * Fix up an enormous number of PR nits * Don't layer a profile if the json doesn't have a GUID * Fix a test I unfixed * get rid of extraneous bois{}; * Piles of PR feedback * Collection of PR nits * PR nits * Fix a typo; Update the defaults to match #2378 * Tiny nits * In-den-taition! * Some typos, PR nits * Fix this broken defaults case * Apply suggestions from code review Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com> * PR nits

ghost · 2019-09-24T16:27:06Z

🎉Windows Terminal Preview v0.5.2661.0 has been released which incorporates this pull request.:tada:

Handy links:

## Summary of the Pull Request Layer the `globals` globals on top of the root globals. ## PR Checklist * [x] Closes #2906 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments We added the ability for the root to be used as the globals object in #2515. However, if you have a globals object, then the settings in the root will get ignored. That's bad. We should layer them.

## Summary of the Pull Request _This is attempt 2 at this feature_. The original PR can be found at #3369. These are settings that apply to _every_ profile, before user customizations. If the user wants to add "default profile settings", they can make the `"profiles"` property an _object_, instead of a list, and add `"defaults"` key underneath that object. The users list of profiles should then be under the `list` property of the `profiles` object. ## References #2515, #2603, #3369, #3569 ## PR Checklist * [x] Closes #2325 * [x] I work here * [x] Tests added/passed * [x] schema, docs updated ## Detailed Description of the Pull Request / Additional comments ~~Discussion in #2325 itself serves as the "spec" for this task. I thought we'd need more discussion on the topic, but it ended up being pretty straightforward.~~ I should not have said that in the original PR. We've had a better spec review now that I think we're happier with. ## Validation Steps Performed _ran the tests_

zadjii-msft added 25 commits August 20, 2019 09:51

Create profiles by layering them

858341f

Update test to layer multiple times on the same profile

2074ac2

Add support for layering an array of profiles, but break a couple tests

deb135f

Add a defaults.json to the package

e524ea7

Layer colorschemes

8157fd6

* Moves tests into individual classes * adds support for layering a colorscheme on top of another

Layer an array of color schemes

16336bb

Merge remote-tracking branch 'origin/master' into dev/migrie/f/754-la…

870fed3

…yer-settings

oh no, this was missed with #2481

22e375d

must have committed without staging this change, uh oh. Not like those tests actually work so nbd

Layer keybindings

4e3c3c3

Read settings from defaults.json + profiles.json, layer appropriately

2c2fa14

This is like 80% of #754. Needs tests.

Add tests for keybindings

9bcf0c5

* add support to unbind a key with `null` or `"unbound"` or `"garbage"`

Layer or clear optional properties

c6b80b9

Add a helper to get an optional variable for a bunch of different types

c040356

In the end, I think we need to ask _was this worth it_

Do this with the stretch mode too

51bcf11

Add back in the GUID check for profiles

8b938ff

Add some tests for global settings layering

aec4c51

M A D W I T H P O W E R

1d662ca

Add a MsBuild target to auto-generate a header with the defaults.json as a string in the file. That way, we can _always_ load the defaults. Literally impossible to not.

When the user's profile.json doesn't exist, create it from a template

9a14dab

Re-order profiles to match the order set in the user's profiles.json

82b57f8

Add tests for re-ordering profiles to match user ordering

2084f89

Add support for hiding profiles using "hidden": true

e07c1ba

Use the hardcoded defaults.json for the exception->"use defaults" case

3ab0f25

Merge remote-tracking branch 'origin/master' into dev/migrie/f/754-la…

39b7fe1

…yer-settings

Somehow I messed up the git submodules?

9b6a2ee

woo documentation

be8e858

zadjii-msft requested review from DHowett-MSFT, carlos-zamora and miniksa August 23, 2019 13:52

zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Aug 23, 2019

zadjii-msft requested review from miniksa and DHowett-MSFT September 10, 2019 22:54

DHowett-MSFT approved these changes Sep 10, 2019

View reviewed changes

This was referenced Sep 10, 2019

On first launch, if Powershell Core is installed, set it as the default profile #2721

Closed

_ReorderProfilesToMatchUserSettingsOrder could almost certainly not be O(N^2) #2723

Closed

zadjii-msft added 3 commits September 11, 2019 15:07

Merge remote-tracking branch 'origin/master' into dev/migrie/f/754-la…

86aabba

…yer-settings # Conflicts: # doc/user-docs/UsingJsonSettings.md

Fix a typo; Update the defaults to match #2378

edb1ead

Tiny nits

fed6492

zadjii-msft requested a review from carlos-zamora September 12, 2019 16:59

carlos-zamora approved these changes Sep 13, 2019

View reviewed changes

DHowett-MSFT approved these changes Sep 13, 2019

View reviewed changes

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

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

tools/GenerateHeaderForJson.ps1 Outdated Show resolved Hide resolved

zadjii-msft added 2 commits September 13, 2019 09:14

Some typos, PR nits

d1e98b6

Fix this broken defaults case

e454753

miniksa reviewed Sep 16, 2019

View reviewed changes

miniksa approved these changes Sep 16, 2019

View reviewed changes

zadjii-msft merged commit 8ba8f35 into master Sep 16, 2019

This was referenced Sep 17, 2019

Don't explode name-only profiles #2789

Merged

Add keyboard shortcuts to increase and decrease font size #2700

Merged

ghost mentioned this pull request Sep 24, 2019

Try to flow together JSON serialization for keybindings #1378

Closed

This was referenced Sep 25, 2019

The globals object should be layered on the root settings object #2906

Closed

Windows Terminal: Open terminal in current folder via single command #620

Closed

Layer the globals globals on top of the root globals #3031

Merged

zadjii-msft mentioned this pull request Oct 29, 2019

Add support for "User Default" settings #3369

Closed

4 tasks

zadjii-msft mentioned this pull request Dec 9, 2019

Add support for "User Default" settings II #3892

Merged

4 tasks

zadjii-msft deleted the dev/migrie/f/754-layer-settings branch January 31, 2020 14:21

zadjii-msft mentioned this pull request Feb 17, 2022

Settings editor saves settings.json lines with trailing whitespace #10887

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Cascading User + Default Settings #2515

Add Cascading User + Default Settings #2515

zadjii-msft commented Aug 22, 2019 •

edited

Loading

carlos-zamora Sep 12, 2019

carlos-zamora Sep 12, 2019

miniksa Sep 16, 2019

ghost commented Sep 24, 2019

Add Cascading User + Default Settings #2515

Add Cascading User + Default Settings #2515

Conversation

zadjii-msft commented Aug 22, 2019 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

TODO:

carlos-zamora Sep 12, 2019

Choose a reason for hiding this comment

carlos-zamora Sep 12, 2019

Choose a reason for hiding this comment

miniksa Sep 16, 2019

Choose a reason for hiding this comment

ghost commented Sep 24, 2019

zadjii-msft commented Aug 22, 2019 •

edited

Loading