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.

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

Feature Request: Cascading settings (with a hardcoded default.json?) #754

Closed
34 of 42 tasks
DHowett-MSFT opened this issue May 13, 2019 · 17 comments · Fixed by #2515 or #2603
Closed
34 of 42 tasks

Feature Request: Cascading settings (with a hardcoded default.json?) #754

DHowett-MSFT opened this issue May 13, 2019 · 17 comments · Fixed by #2515 or #2603
Assignees
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 13, 2019

Proposal

Include a default profiles.json in the package, allow the user to override it and merge their settings into it.

Questions

  • What if you want to delete a stock profile?
  • Optional features?
  • Turn off defaults? Perhaps {... "thing": null, }?

Child Work Items


Things that need to be done to accomplish Cascading Settings

Each <h3> here is a single PR into master, broken into either sub-PRs or work for me to do as a part of that PR

[x] Don't re-serialize settings on load (PR: #2475)

This should be done first and foremost. This is not really helping anyone. Old settings all migrate nicely, so whatever. Let's just pull it.

[_] Combine a defaults.json with a profiles.json (PR: #2515)

  • Add a defaults.json to the package
  • Create a function for layering a Json::Value on top of an existing object
  • Add tests for layering a profile json blob on top of a profile
  • Add functions to search a list of Profiles for a duplicate, and layer on that dupe or create a new Profile if no such dupe exists.
  • Ensure we can layer each of the following: (WITH TESTS)
    • layer profiles
    • layer color schemes
    • layer keybindings (for a matching key, the user action should override the default action) (NEEDS TESTS)
    • layer globals (NEEDS TESTS)
  • When loading settings, load defaults, then layer user settings on top of default profiles
  • Maintain user settings ordering, not defaults+user ordering
  • Add support for unbinding default keys
    • unbind with "unbound"
    • unbind with null
  • Add support for hiding profiles
  • Ensure that we can reset optional default settings with null (e.g. "iconPath": null)
  • Create the User Settings file if it doesn't exist
  • Stamp the "defaults.json" file into the code, so we can't possibly fail loading it
  • Add support for Alt clicking on "settings" to launch the defaults.json file
  • default construct profiles with scheme "Campbell" to make "scheme" and unnecessary setting in the partial diff

[_] Add Dynamic Profile Generators

  • Add Dynamic Profile Generator for WSL
    • Create Dynamic Profile Generator interface
    • Provide DPG's with GetNamespaceGuid and GetGuidForName functions from App somehow We're not doing this anymore. We'll create the GUID for the profile when it's returned, if it has a GUID of {0}
      • Add a test for generateing a guid based on namespace if GUID is {0}
    • Add layering for dynamic profiles based on guid and source, not just guid
  • Add Dynamic Profile Generator for Powershell Core
  • Add Dynamic Profile Generator for Azure Cloud Shell
  • Ensure that existing WSL, Powershell Core, and Azure profiles gracefully migrate to dynamic profiles
  • Add disabledProfileSources in user settings to hide dynamic profiles by namespace
  • On first launch of the terminal, if Powershell Core exists, make it the default profile
  • When adding this profiles, insert them into our JSON parse tree to save them
    • Only add the delta between the default profile and the dynamic profile's default state to the json. Don't include any user customizations (though at this point there won't be any)
  • Ensure adding a dynamic entry doesn't reformat settings
  • For profiles that were created from a dynamic profile source, they'll have both a guid and source guid that must both match. If a user profile with a source set does not find a matching profile at load time, the profile should be ignored.
  • profile diffs should probably not include "table". Probably don't need to include in the diff keys that are std::nullopt in both the base and derived.
  • Make sure to re-order profiles so they're { user profiles, dynamics not in user profiles, defaults not in user profiles }

[_] Smart Serializing

This would be needed for a Settings UI, so we don't necessarily need it for 1.0

  • Keep the original JSON parse tree around in memory
  • Only serialize things different from defaults
    • profiles
    • schemes
    • keybindings
@DHowett-MSFT DHowett-MSFT added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label May 13, 2019
@DHowett-MSFT DHowett-MSFT added this to the Windows Terminal v1.0 milestone May 13, 2019
@DHowett-MSFT
Copy link
Contributor Author

This will be helped by UUIDv5 profile GUIDs. I'll file that later.

@zadjii-msft zadjii-msft added the Area-Settings Issues related to settings and customizability, for console or terminal label May 15, 2019
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label May 17, 2019
@miniksa miniksa added Product-Terminal The new Windows Terminal. and removed Mass-Chaos labels May 17, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 17, 2019
@rbeesley
Copy link
Contributor

The default profiles.json should also be recreated from the application itself if the user makes a breaking change. They could uninstall and then reinstall, but that has a lot of negative problems as well such as losing any customizations they might have already made. It'd be better to give a path for recovery if the user makes a mistake and there is a chance this could happen if the user is given the chance to modify on their own.

@zadjii-msft zadjii-msft self-assigned this May 31, 2019
zadjii-msft added a commit that referenced this issue Jun 4, 2019
Switch to using jsoncpp as our json library. This lets us pretty-print the json file by default, and lets users place comments in the json file.

We will now only re-write the file when the actual logical structure of the json object changes, not only when the serialization changes.

Unfortunately, this will remove any existing ordering of profiles, and make the order random. We don't terribly care though, because when #754 lands, this will be less painful.

It also introduces a top-level globals object to hold all the global properties, including keybindings. Existing profiles should gracefully upgrade.
@Stanzilla
Copy link
Contributor

I would love to be able to overwrite per profile settings with a global one, let's say I want to use the same color scheme everywhere, setting it in the global section should do that.

zadjii-msft added a commit that referenced this issue Aug 19, 2019
  This is more trouble than it's worth. We had code before to re-serialize
  settings when they changed, to try and gracefully migrate settings from old
  schemas to new ones. This is good in theory, but with #754 coming soon, this
  is going to become a minefield. In the future we'll just always be providing a
  base schema that's reasonable, so this won't matter so much. Keys that users
  have that aren't understood will just be ignored, and that's _fine_.
carlos-zamora pushed a commit that referenced this issue Aug 20, 2019
This is more trouble than it's worth. We had code before to re-serialize
  settings when they changed, to try and gracefully migrate settings from old
  schemas to new ones. This is good in theory, but with #754 coming soon, this
  is going to become a minefield. In the future we'll just always be providing a
  base schema that's reasonable, so this won't matter so much. Keys that users
  have that aren't understood will just be ignored, and that's _fine_.
zadjii-msft added a commit that referenced this issue Aug 21, 2019
@ghost ghost added the In-PR This issue has a related PR label Aug 22, 2019
@adrianghc
Copy link

adrianghc commented Sep 5, 2019

Couldn't WT register a URI scheme that apps could use to add themselves (eg, ms-terminal:///add-profile?guid=&cmdline=&icon=)?

That would avoid having to chase everyone's personal console apps around, as well as trying to locate good icons for them (don't forget VS Dev Prompt, Exchange Online tools, Azure, various databases...). Of course, developers would need to call that URI...

For Store apps, it'd be interesting if a new "Console" capability were available to mediate that discovery, but this isn't necessarily the place for that discussion.

I came to the repo to suggest exactly this, good to see I'm not alone with this.

I think one of the advantages of having the GUID be provided by the shell that wants to register itself instead of generated by WT on call is that uninstalls and subsequent unregistrations of shells would become easier that way.

As for making WT popular, I wonder if one day when WT is far enough in development it could be added to Windows as an optional feature. In that case shell apps could check if WT is present during their install and expect it with a certain probability.

Perhaps we should create a separate issue for this?

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Sep 16, 2019
zadjii-msft added a commit that referenced this issue Sep 16, 2019
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 (#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:
* #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 
* #1398 - This might have honestly been solved by #2475 

## PR Checklist
* [x] Closes #754
* [x] Closes #1378 
* [x] Closes #2566
* [x] I work here
* [x] Tests added/passed
* [x] Requires documentation to be updated - it **ABSOLUTELY DOES**


## Detailed Description of the Pull Request / Additional comments

1. 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. 
2. Next, we add tests for layering properties like that.
3. Now, we add a `defaults.json` to the package. This is the file the users can refer to as our default settings.
4. 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.
5. 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.
6. 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.
7. 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.
8. 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:
* [x] Still need to make Alt+Click work on the settings button
* [x] Need to write some user documentation on how the new settings model works
* [x] Fix the pair of tests I broke (re: Duplicate profiles)


<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

* 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

* 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

* Fix a typo; Update the defaults to match #2378

* Tiny nits

* Some typos, PR nits

* Fix this broken defaults case
zadjii-msft added a commit that referenced this issue 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
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Sep 16, 2019
@ghost
Copy link

ghost commented Sep 24, 2019

🎉This issue was addressed in #2515, which has now been successfully released as Windows Terminal Preview v0.5.2661.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Sep 24, 2019

🎉This issue was addressed in #2603, which has now been successfully released as Windows Terminal Preview v0.5.2661.0.:tada:

Handy links:

@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
10 participants