Add Cascading User + Default Settings#2515
Merged
zadjii-msft merged 54 commits intomasterfrom Sep 16, 2019
Merged
Conversation
* Moves tests into individual classes * adds support for layering a colorscheme on top of another
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.
zadjii-msft
added
Area-Settings
DHowett-MSFT
approved these changes
Sep 10, 2019
This was referenced Sep 10, 2019
…yer-settings # Conflicts: # doc/user-docs/UsingJsonSettings.md
carlos-zamora
approved these changes
Sep 13, 2019
| const auto guid = guidOrder.at(gIndex); | ||
| for (size_t pIndex = gIndex; pIndex < _profiles.size(); pIndex++) | ||
| { | ||
| auto profileGuid = _profiles.at(pIndex).GetGuid(); |
Member
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // Function Description: | ||
| // - Gets the object in the given JSON obejct under the "profiles" key. Returns |
Member
There was a problem hiding this comment.
I think you missed this one haha (mispelled object)
DHowett-MSFT
approved these changes
Sep 13, 2019
miniksa
reviewed
Sep 16, 2019
|
|
||
| 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)); |
Member
There was a problem hiding this comment.
Why wasn't this THROW_IF_WIN32_BOOL_FALSE?
miniksa
approved these changes
Sep 16, 2019
zadjii-msft
added a commit
that referenced
this pull request
Sep 16, 2019
_**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. <!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## 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 <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> ## References <!-- Please review the items on the PR checklist before submitting--> ## 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 <!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here --> ## 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 <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## 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 <[email protected]> * PR nits
This was referenced Sep 17, 2019
|
🎉 Handy links: |
ghost
mentioned this pull request
This was referenced Sep 25, 2019
zadjii-msft
added a commit
that referenced
this pull request
Oct 4, 2019
## 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.
4 tasks
4 tasks
ghost
pushed a commit
that referenced
this pull request
Dec 11, 2019
## 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_
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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:
defaults.jsonthat ships with the package (the "default settings")profiles.jsonwith 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:
PR Checklist
Detailed Description of the Pull Request / Additional comments
FromJsonfunctions in Profile, ColorScheme, Globals, etc, and converting them toLayerJsonmethods. These are effectively the same, with the change that instead of building a new object, they are simply layering the values on top ofthisobject.defaults.jsonto the package. This is the file the users can refer to as our default settings.defaults.jsonand stamp it into an auto generated.hfile, so we can use it's data without having to worry about reading it from disk.LoadAllfunction inCascadiaSettings. Now, the function does two loads - one from the defaults, and then a second load from theprofiles.jsonfile, layering the settings from each source upon the previous values.profiles.jsonfile doesn't exist, we'll create it from a hardcodeduserDefaults.json, which is stamped in similar to howdefaults.jsonis.defaults.json, but the user doesn't want to be bound to anything.TODO: