Add updates concerning dynamic profile generation #1321

zadjii-msft · 2019-06-19T13:59:53Z

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.

I'm making a PR for this element of the design into the other spec. It seemed like a pretty substantial change to the other spec that should almost be reviewed on its own. If we agree on this bit of it, I'll merge it in to be reviewed with the rest.

@DHowett-MSFT

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.

DHowett-MSFT

I've sat on this for a couple weeks, and I actually like it more than I thought I did. I have some language clarifications and an idea, though.

DHowett-MSFT · 2019-07-08T03:02:44Z

doc/cascadia/Cascading-Default-Settings.md

-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


It shouldn't attempt to find any profiles that exist -- right now, during the "early phase," there shouldn't be any profiles that exist except the ones in the defaults. The ones from the json file have been read from disk, but not yet turned into profile objects.

The generator will create a bunch of basics, which are then strictly overlaid with the user's json json tree. The generator doesn't ever need to query the user's store.

I think there's one other step you missed. We need to make a first pass on the user's settings to make sure all the profiles end up in the right order. We'll do nothing in that pass other than instantiate a list of profiles with matching GUIDs. When we generate profiles dynamically, their settings will be applied to the profile objects that already exist, as if only to maintain the ordering.

That doesn't really matter, we can put profiles in order once we load the user's settings. We're using a vector right now but that's an implementation detail.

DHowett-MSFT · 2019-07-08T03:05:16Z

doc/cascadia/Cascading-Default-Settings.md

@@ -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


This is required for things like "I uninstalled my WSL distribution, why is it still hanging around" to work, anyway. Maybe if we bang on the language a bit to change "generator" to "source"? A "profile source" can be disabled, and that will remove all of the profiles that originated from that source.

I think the discussion around "profile generators can be slow" should be resolved by "don't do that, you are a bad plugin author" 😄

This also opens us up to "default.json parser is just another type-of-source", which might be a safe mental model?

DHowett-MSFT · 2019-07-08T03:08:17Z

doc/cascadia/Cascading-Default-Settings.md

@@ -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


Now here is a thought. If we force the GUID, we can encode source: '{guid-of-source}' in the user copy of the profile. If a profile has a source, but that source didn't early-generate a profile of the same guid, THEN we just fail to apply. We don't need any logic to say "if the command line is blank, skip it"!

DHowett-MSFT · 2019-07-08T03:08:30Z

doc/cascadia/Cascading-Default-Settings.md

@@ -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


Above source-guid comment might help address this!

DHowett-MSFT · 2019-07-08T03:13:27Z

The flow becomes:

load default.json from disk -> DefaultsJson
load profiles.json from disk -> UserJson
parse DefaultsJson into Profile objects -> new Profile objects 1
run <all generators>::Generate -> new Profile objects 2
union all Profile objects 1 and 2 -> Profile set 3
- Set 3 contains only profiles with a total {guid, source} tuple.
overlay UserJson.globals.defaults on all Profiles in set 3
overlay UserJson. on all Profiles in set 3
1. If a profile with a source is not attested by that source (check set 3), we can't overlay it (!) so it is never created, so it never needs to be deleted.

Done. User "global defaults" override all generator settings, profile specific settings override all settings, and generators have 0 reliance on UserJson.

…ie/s/754-dynamic-profiles

zadjii-msft · 2019-07-31T17:59:40Z

@DHowett-MSFT how about:

When we load the settings, we'll perform the following steps, each mentioned in greater detail below:

Load from disk the defaults.json (the default settings) -> DefaultsJson

Load from disk the profiles.json (the user settings) -> UserJson

Perform a preliminary scan of the user settings, and create all the profiles in order they appear in the user settings file. Fill in their guid and source properties (if they exist).

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.

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.

[Not covered in this spec] Apply the UserJson.globals.defaults settings to every profile, including default, generated and user profiles. (This will override generated settings for generated profiles).

Layer all user settings upon the existing settings models.

Validate the settings.

If necessary, write the modified settings back to profiles.json.

I think functionally it's the same, with an added step to make sure the profiles appear in the order they appear in the user settings file

DHowett-MSFT · 2019-07-31T18:02:45Z

I guess I just don't get why 4, 5 need to happen before 3. The total order can be enforced during 7, when we're "applying the user's settings". Before that, they have no order. 😄

@DHowett-MSFT

…t's all consistent but it's worth reviewing @DHowett-MSFT

DHowett-MSFT

THis is solid.

DHowett-MSFT · 2019-08-01T01:34:58Z

doc/cascadia/Cascading-Default-Settings.md

+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.


this wording still suggests that the generator knows-about the user profile object and updates it.

DHowett-MSFT · 2019-08-01T01:35:27Z

doc/cascadia/Cascading-Default-Settings.md

+added to the user settings, all that will be added is the `name`, `guid`, and
+`source`.
+
+<!-- If a dynamic profile generator has determined that a profile should no longer be


nix commented code when merging

@DHowett-MSFT

* Start working on drafting this spec * Really add a LOT of notes * More spec updates. * Remove `hiddenProfiles` in favor of `profile.hidden` * Add info on how layering will work * add more powershell core info * Finish remaining TODO sections * Apply suggestions from code review Fix simple typos Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com> * Lots of feedback from PR * Try and make dynamic settings a bit clearer * more clearly call out serializing only what's different from a default- constructed `Profile` * Add more goals * add a blurb for user-default profile objects * Add updates concerning dynamic profile generation (#1321) * 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. * Add some initial updates from discussion * 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 * Some minor updates from Dustin * Fix a bunch of slightly more minor points in the spec * Move "Profile Ordering" to "Future considerations" * Add some notes on migrating profiles, GUID generation, de-duping profiles, and O R A N G E * Fix the indenting here * Update powershell core to be a dynamic profile, don't even mention other options. * Remaining PR feedback * Apply suggestions from code review Co-Authored-By: Michael Niksa <miniksa@microsoft.com> * remove a dead comment

zadjii-msft requested a review from DHowett-MSFT June 19, 2019 13:59

DHowett-MSFT mentioned this pull request Jun 24, 2019

Feature Request: Allow users to re-scan for profiles #1518

Closed

DHowett-MSFT reviewed Jul 8, 2019

View reviewed changes

This was referenced Jul 20, 2019

Bug Report - Ubuntu not listed as new tab option #2037

Closed

Bug Report (Don't display ubuntu terminal) #2023

Closed

Merge branch 'dev/migrie/s/754-cascading-settings-spec' into dev/migr…

43ce0d9

…ie/s/754-dynamic-profiles

zadjii-msft added 2 commits July 31, 2019 14:44

Add some initial updates from discussion

abef53b

Pushing some updates here. I haven't given it a once over to ensure i…

6eb4f6b

…t's all consistent but it's worth reviewing @DHowett-MSFT

DHowett-MSFT approved these changes Aug 1, 2019

View reviewed changes

Some minor updates from Dustin

5a16c1c

zadjii-msft merged commit dd2c4db into dev/migrie/s/754-cascading-settings-spec Aug 1, 2019

zadjii-msft deleted the dev/migrie/s/754-dynamic-profiles branch August 1, 2019 12:44

zadjii-msft mentioned this pull request Aug 1, 2019

Draft Spec for Cascading Default + User Settings #1258

Merged

3 tasks

zadjii-msft mentioned this pull request May 1, 2020

Azure Cloud Shell is readded to the profiles.json file after I remove it and restart terminal. #3231

Closed

zadjii-msft mentioned this pull request Nov 16, 2020

Default profiles appear even if removed from the config.json #8270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add updates concerning dynamic profile generation #1321

Add updates concerning dynamic profile generation #1321

zadjii-msft commented Jun 19, 2019

DHowett-MSFT left a comment

DHowett-MSFT Jul 8, 2019

zadjii-msft Jul 31, 2019

DHowett-MSFT Jul 31, 2019

DHowett-MSFT Jul 8, 2019

DHowett-MSFT Jul 8, 2019

DHowett-MSFT Jul 8, 2019

DHowett-MSFT Jul 8, 2019

DHowett-MSFT Jul 8, 2019

DHowett-MSFT commented Jul 8, 2019 •

edited

Loading

zadjii-msft commented Jul 31, 2019 •

edited

Loading

DHowett-MSFT commented Jul 31, 2019

DHowett-MSFT left a comment

DHowett-MSFT Aug 1, 2019

DHowett-MSFT Aug 1, 2019

Add updates concerning dynamic profile generation #1321

Add updates concerning dynamic profile generation #1321

Conversation

zadjii-msft commented Jun 19, 2019

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Jul 8, 2019 • edited Loading

zadjii-msft commented Jul 31, 2019 • edited Loading

DHowett-MSFT commented Jul 31, 2019

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Jul 8, 2019 •

edited

Loading

zadjii-msft commented Jul 31, 2019 •

edited

Loading