From fbcd941b1c55819af4b8e3319352b62903425baf Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 19 Jun 2019 08:57:22 -0500 Subject: [PATCH 1/4] Add updates concerning dynamic profile generation This is based on discussion with @dhowett-msft we had o*line. We're trying to work through a way to prevent dynamic profiles from roaming to machines the dynamic profiles might not exist on. After writing this up, I'm not totally sure that it's a better design. --- doc/cascadia/Cascading-Default-Settings.md | 110 +++++++++++++++++++-- 1 file changed, 100 insertions(+), 10 deletions(-) diff --git a/doc/cascadia/Cascading-Default-Settings.md b/doc/cascadia/Cascading-Default-Settings.md index 2122110fdce..27dcc815110 100644 --- a/doc/cascadia/Cascading-Default-Settings.md +++ b/doc/cascadia/Cascading-Default-Settings.md @@ -64,6 +64,18 @@ the terminal re-generate the default settings. This is fairly annoying to the end-user, so ideally we'll find a way to be able to prevent this scenario. +### Goal: Prevent Roaming Settings from Failing +Another problem currently is that when settings roam to another machine, it's +possible that the second machine doesn't have the same applications installed as +the first, and some profiles might be totally invalid on the second machine. +Take for example, profiles for WSL distros. If you have and Ubuntu profile on +your first machine, and roam that profile to a second machine without Ubuntu +installed, then the Ubuntu profile would be totally broken on the second +machine. + +While we won't be able to non-destructively prevent all failures of this case, +we should be able to catch it in certain scenarios. + ## Solution Design The settings are now composed from two files: a "Default" settings file, and a @@ -71,17 +83,15 @@ The settings are now composed from two files: a "Default" settings file, and a When we load the settings, we'll perform the following steps, each mentioned in greater detail below: -1. Load the `defaults.json` (the default settings) -1. Load the `profiles.json` (the user settings) +1. Load from disk the `defaults.json` (the default settings) +1. Load from disk the `profiles.json` (the user settings) 1. Perform a preliminary scan of the user settings, and create all the profiles in order they appear in the user settings file. 1. Layer all settings from the defaults on the existing profiles, and create the default color schemes, as well as the default global settings. +1. Generate any dynamically generated profiles, if necessary. 1. Layer all user settings upon the existing settings models. -1. Generate any dynamically generated profiles, if necessary. This step might - request the user settings are saved out. -1. Validate the settings. This step might request the user settings are saved - out. +1. Validate the settings. 1. If necessary, write the modified settings back to `profiles.json`. ### Default Settings @@ -219,15 +229,33 @@ profiles. The generator will return some sort of result indicating that it wants a save operation to occur. The app will then save the `profiles.json` file, including these new profiles. -If a dynamic profile generator has determined that a profile should no longer be +We'll generate these dynamic profiles immediately after applying the defaults. +When a generator runs, it'll be able to create unique profile GUIDs for each +source it wants to generate a profile for. It'll attempt find any profiles that +exist in the list of profiles with a matching GUID, and apply the settings to +that profile if found, or it'll create a new profile to apply the settings to. + +When we're serializing the settings, instead of comparing a dynamic profile to +the default-constructed `Profile`, we'll compare it to the state of the +`Profile` after the dynamic profile generator created it. It'd then only +serialize settings that are different from the auto-generated version. It will +also always make sure that the GUID of the dynamic profile is included in the +user settings file, as a point for the user to add customizations to the dynamic +profile to. + +We'll need to keep the state of these dynamically generateed profiles around in +memory during runtime to be able to ensure the only state we're serializing is +that which is different from the initially generated dynamic profile. + + -Should a dynamic profile generator try to create a profile that's already + Additionally, a user might not want a dynamic profile generator to always run. They might want to keep their Azure connections visible in the list of profiles, @@ -239,6 +267,22 @@ like `autoloadWslProfiles` and `autoloadAzureConnections`. These will be set to true in the default settings, but the user can override them to false if they so chose. +#### What if a dynamic profile is in the user setttings, but wasn't generated? + +After a dynamic profile is generated, we'll leave behind an entry with it's GUID +in the profiles list in the user settings. However, if the user's settings roam +to another machine where that dynamic profile is no longer generated, what +should we do with this profile? On the second machine, it'll be a profile with +various settings customizations, but some of the important information that's +been dynamically generated, like the commandline or the name, won't exist. It +won't have a valid commandline, and the target that it _should_ be pointing at +doesn't exist. + +We'll need an additional validation step to find all profiles like this, and +remove them from the list of profiles. The default-constructed profile doesn't +have a commandline, and for any profiles that were originally dynamically +generated like this, they won't either. + #### What if a dynamic profile is removed, but it's the default? We should probably add an extra validation step at the end of serializing to @@ -444,6 +488,39 @@ the `profiles.json` file. Fortunately though, users should be able to remove much of the boilerplate from their `profiles.json` files, and trim it down just to their modifications. +### Dynamic Profile Generators Need to be Enabled +With the current proposal, profiles that are generated by a dynamic profile +generator _need_ that generator to be enabled for the profile to appear in the +list of profiles. If the generator isn't enabled, then the important parts of +the profile (name, commandline) will never be set, and the profile's settings +from the user settings will be ignored at runtime. + +For generators where the generation of profiles might be a lengthy process, this +could negatively impact startup time. Take for example, some hypothetical +generator that needs to make web requests to generate dynamic profiles. Because +we need the finalized settings to be able to launch the terminal, we'll be stuck +loading until that generator is complete. + +However, if the user disables that generator entirely, we'll never display that +profile to the user, even if they've done that setup before. + +So the trade-off with this design is that non-existent dynamic profiles will +never roam to machines where they don't exist and aren't valid, but the +generators _must_ be enabled to use the dynamic profiles. + + + ## Future considerations * It's possible that a very similar layering loading mechanism could be used to layer per-machine settings with roaming settings. Currently, there's only one @@ -458,6 +535,15 @@ to their modifications. behave correctly. It's possible that we could abstract our implementation into a WinRT interface that extensions could implement, and be triggered just like other dynamic profile generators. + - Additionally, we could expand the dynamic profile generator contract to + force it to use a "namespace GUID". We'd then have the generator ask us to + generate a profile GUID for some static string, given it's namespace GUID. + The generator then wouldn't need to always generate unique GUIDs for its + profiles, but it would need to generate unique strings. We could then allow + the user to disable profile generators solely by namespace guid, so they + could disable the generator from a certain extension easily, without the + extension needing to implement its own `autloadProfiles` + setting. * **Multiple settings files** - This could enable us to place color schemes into a seperate file (like `colorschemes.json`) and put keybindings into their own file as well, and reduce the number of settings in the user's `profiles.json`. @@ -474,6 +560,10 @@ to their modifications. that as the point of comparison to check if a setting's value has changed. There may be more unknowns with this proposal, so I leave it for a future feature spec. + - How do these play with dynamically generated profiles? If the user sets a + `commandline` for their global default profile, then the dynamic profile + validation step, where we remove profiles without a `commandline`, won't + work any longer. ## Resources N/A From abef53b2598ca6386547c231adf5ed5adebd7002 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 31 Jul 2019 14:44:55 -0500 Subject: [PATCH 2/4] Add some initial updates from discussion --- doc/cascadia/Cascading-Default-Settings.md | 29 +++++++++++++++------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/doc/cascadia/Cascading-Default-Settings.md b/doc/cascadia/Cascading-Default-Settings.md index 27dcc815110..8f0c95b7358 100644 --- a/doc/cascadia/Cascading-Default-Settings.md +++ b/doc/cascadia/Cascading-Default-Settings.md @@ -1,7 +1,7 @@ --- author: Mike Griese @zadjii-msft created on: 2019-05-31 -last updated: 2019-06-13 +last updated: 2019-07-31 issue id: 754 --- @@ -83,13 +83,22 @@ The settings are now composed from two files: a "Default" settings file, and a When we load the settings, we'll perform the following steps, each mentioned in greater detail below: -1. Load from disk the `defaults.json` (the default settings) -1. Load from disk the `profiles.json` (the user settings) -1. Perform a preliminary scan of the user settings, and create all the profiles - in order they appear in the user settings file. +1. Load from disk the `defaults.json` (the default settings) -> DefaultsJson +1. Load from disk the `profiles.json` (the user settings) -> UserJson + 1. Layer all settings from the defaults on the existing profiles, and create the default color schemes, as well as the default global settings. -1. Generate any dynamically generated profiles, if necessary. + - To layer: if a profile exists already in the list of profiles with a + matching `guid` and `source`, apply the settings _to that Profile object_. + Otherwise, if there is no match, append it to the end of the list of + profiles. +1. Generate any dynamically generated profiles, if necessary. Layer them on top + of existing profiles. If any profiles were appended in this manner, indicate + that we'll need to save the updated settings. +1. [Not covered in this spec] Apply the UserJson.globals.defaults settings to + every profile, including default, generated and user profiles. 1. Layer all user settings upon the existing settings models. 1. Validate the settings. 1. If necessary, write the modified settings back to `profiles.json`. @@ -243,10 +252,12 @@ also always make sure that the GUID of the dynamic profile is included in the user settings file, as a point for the user to add customizations to the dynamic profile to. -We'll need to keep the state of these dynamically generateed profiles around in +We'll need to keep the state of these dynamically generated profiles around in memory during runtime to be able to ensure the only state we're serializing is that which is different from the initially generated dynamic profile. +When the generator is first run, and determines that a new profile has been added + profiles, but it would need to generate unique strings. We could then allow the user to disable profile generators solely by namespace guid, so they could disable the generator from a certain extension easily, without the - extension needing to implement its own `autloadProfiles` + extension needing to implement its own `autoloadProfiles` setting. * **Multiple settings files** - This could enable us to place color schemes into a seperate file (like `colorschemes.json`) and put keybindings into their own From 6eb4f6b0136896d58e45e66f407919b4391e5295 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 31 Jul 2019 16:44:35 -0500 Subject: [PATCH 3/4] Pushing some updates here. I haven't given it a once over to ensure it's all consistent but it's worth reviewing @dhowett-msft --- doc/cascadia/Cascading-Default-Settings.md | 110 ++++++++++++--------- 1 file changed, 62 insertions(+), 48 deletions(-) diff --git a/doc/cascadia/Cascading-Default-Settings.md b/doc/cascadia/Cascading-Default-Settings.md index 8f0c95b7358..885397d2515 100644 --- a/doc/cascadia/Cascading-Default-Settings.md +++ b/doc/cascadia/Cascading-Default-Settings.md @@ -85,21 +85,30 @@ When we load the settings, we'll perform the following steps, each mentioned in greater detail below: 1. Load from disk the `defaults.json` (the default settings) -> DefaultsJson 1. Load from disk the `profiles.json` (the user settings) -> UserJson - -1. Layer all settings from the defaults on the existing profiles, and create the - default color schemes, as well as the default global settings. - - To layer: if a profile exists already in the list of profiles with a - matching `guid` and `source`, apply the settings _to that Profile object_. - Otherwise, if there is no match, append it to the end of the list of - profiles. -1. Generate any dynamically generated profiles, if necessary. Layer them on top - of existing profiles. If any profiles were appended in this manner, indicate - that we'll need to save the updated settings. -1. [Not covered in this spec] Apply the UserJson.globals.defaults settings to - every profile, including default, generated and user profiles. -1. Layer all user settings upon the existing settings models. +1. Parse DefaultsJson to create all the default profiles, schemes, keybindings. +1. [Not covered in this spec] Check the UserJson to find the list of dynamic + profile sources that should run. +1. Run all the _enabled_ dynamic profile generators. Those profiles will be + added to the set of profiles. + - During this step, check if any of the profiles added here don't exist in + UserJson. If they _don't_, the generator created a profile that didn't + exist before. Return a value indicating the user settings should be + re-saved (with the new profiles added). +1. [Not covered in this spec] Layer the UserJson.globals.defaults settings to + every profile in the set, both the defaults, and generated profiles. +1. Apply the user settings from UserJson. Layer the profiles on top of the + existing profiles if possible (if both `guid` and `source` match). If a + profile from the user settings does not already exist, make sure to apply the + UserJson.globals.defaults settings first. Also layer Color schemes and + keybindings. + - If a profile with a `source` does not exist, don't create a new Profile + object for it. That generator didn't run, so we'll effectively hide the + profile. +1. Re-order the list of profiles, to match the ordering in the UserJson. If a + profile doesn't exist in UserJson, it should follow all the profiles in the + UserJson. If a profile listed in UserJson doesn't exist, we can skip it + safely in this step (the profile will be a dynamic profile that didn't get + populated.) 1. Validate the settings. 1. If necessary, write the modified settings back to `profiles.json`. @@ -128,11 +137,13 @@ set of setings in it. ### Layering settings -When we load the settings, we'll do it in two stages. First, we'll deserialize the -default settings that we've hardcoded. Then, we'll intelligently layer the -user's setting upon those we've already loaded. Some objects, like the default -profiles, we'll need to make sure to load from the user settings into the -existing objects we created from the default settings. +When we load the settings, we'll do it in three stages. First, we'll deserialize +the default settings that we've hardcoded. We'll then generate any profiles that +might come from dynamic profile sources. Then, we'll intelligently layer the +user's setting upon those we've already loaded. If a user wants to make changes +to some objects, like the default profiles, we'll need to make sure to load from +the user settings into the existing objects we created from the default +settings. * We'll need to make sure that any profile in the user settings that has a GUID matching a default profile loads the user settings into the object created @@ -143,6 +154,11 @@ existing objects we created from the default settings. instead. * For any color schemes who's name matches the name of a default color scheme, we'll need to apply the user settings to the existing color scheme. +* 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 will + be ignored. See more details in the [Dynamic Profiles](#dynamic-profiles) + section. ### Hiding Default Profiles @@ -202,6 +218,13 @@ we'll compare the value of the setting with the value of the _default constructed_ `Profile` object. This will help ensure that each profile in the user's settings file maintains the minimal amount of info necessary. +When we're adding profiles due to their generation in a dynamic profile +generator, we'll need to serialize them, then insert them back into the +originally parsed json object to be serialized. We don't want the automatic +creation of a new profile to automatically trigger re-writing the entire user +settings file, but we do want newly created dynamic profiles to have an entry +the user can easily edit. + ### Dynamic Profiles Sometimes, we may want to auto-generate a profile on the user's behalf. Consider @@ -232,6 +255,11 @@ generator runs, it can check if that profile source already has a profile associated with it, and do nothing (as to not create many duplicate "Ubuntu" profiles, for example). +Additionally, each dynamic profile generator **must have a unique source guid** +to associate with the profile. When a dynamic profile is generated, the source's +guid will be added to the profile, to make sure the profile is correlated with +the source it came from. + When a dynamic profile generator runs, it will determine what new profiles need to be added to the user settings, and it can append those to the list of profiles. The generator will return some sort of result indicating that it wants @@ -248,15 +276,20 @@ When we're serializing the settings, instead of comparing a dynamic profile to the default-constructed `Profile`, we'll compare it to the state of the `Profile` after the dynamic profile generator created it. It'd then only serialize settings that are different from the auto-generated version. It will -also always make sure that the GUID of the dynamic profile is included in the +also always make sure that the `guid` of the dynamic profile is included in the user settings file, as a point for the user to add customizations to the dynamic -profile to. +profile to. Additionally, we'll also make sure the `source` is always serialized +as well, to keep the profile correlated with the generator that created it. We'll need to keep the state of these dynamically generated profiles around in memory during runtime to be able to ensure the only state we're serializing is that which is different from the initially generated dynamic profile. -When the generator is first run, and determines that a new profile has been added +When the generator is run, and determines that a new profile has been added, +we'll need to make sure to add the profile to the user's settings file. This +will create an easy point for users to customize the dynamic profiles. When +added to the user settings, all that will be added is the `name`, `guid`, and +`source`. force it to use a "namespace GUID". We'd then have the generator ask us to generate a profile GUID for some static string, given it's namespace GUID. The generator then wouldn't need to always generate unique GUIDs for its - profiles, but it would need to generate unique strings. We could then allow + profiles, but it would need to generate unique strings. We already allow the user to disable profile generators solely by namespace guid, so they - could disable the generator from a certain extension easily, without the - extension needing to implement its own `autoloadProfiles` - setting. + could disable the generator from a certain extension easily. * **Multiple settings files** - This could enable us to place color schemes into a seperate file (like `colorschemes.json`) and put keybindings into their own file as well, and reduce the number of settings in the user's `profiles.json`. From 5a16c1ceb5e8c057d850262d15fb766b09b8593e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 1 Aug 2019 07:44:12 -0500 Subject: [PATCH 4/4] Some minor updates from Dustin --- doc/cascadia/Cascading-Default-Settings.md | 35 ++++------------------ 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/doc/cascadia/Cascading-Default-Settings.md b/doc/cascadia/Cascading-Default-Settings.md index 885397d2515..6a3834d917d 100644 --- a/doc/cascadia/Cascading-Default-Settings.md +++ b/doc/cascadia/Cascading-Default-Settings.md @@ -260,18 +260,18 @@ to associate with the profile. When a dynamic profile is generated, the source's guid will be added to the profile, to make sure the profile is correlated with the source it came from. -When a dynamic profile generator runs, it will determine what new profiles need -to be added to the user settings, and it can append those to the list of -profiles. The generator will return some sort of result indicating that it wants -a save operation to occur. The app will then save the `profiles.json` file, -including these new profiles. - We'll generate these dynamic profiles immediately after applying the defaults. When a generator runs, it'll be able to create unique profile GUIDs for each source it wants to generate a profile for. It'll attempt find any profiles that exist in the list of profiles with a matching GUID, and apply the settings to that profile if found, or it'll create a new profile to apply the settings to. +After a dynamic profile generator runs, we will determine what new profiles need +to be added to the user settings, so we can append those to the list of +profiles. We'll store some sort of result indicating that we want a save +operation to occur. After the rest of the deserializing is done, the app will +then save the `profiles.json` file, including these new profiles. + When we're serializing the settings, instead of comparing a dynamic profile to the default-constructed `Profile`, we'll compare it to the state of the `Profile` after the dynamic profile generator created it. It'd then only @@ -291,16 +291,6 @@ will create an easy point for users to customize the dynamic profiles. When added to the user settings, all that will be added is the `name`, `guid`, and `source`. - - - - Additionally, a user might not want a dynamic profile generator to always run. They might want to keep their Azure connections visible in the list of profiles, even if its no longer a valid target. Or they might want to not automatically @@ -535,19 +525,6 @@ So the trade-off with this design is that non-existent dynamic profiles will never roam to machines where they don't exist and aren't valid, but the generators _must_ be enabled to use the dynamic profiles. - - ## Future considerations * It's possible that a very similar layering loading mechanism could be used to layer per-machine settings with roaming settings. Currently, there's only one