Serialize stub for dynamic profiles by carlos-zamora · Pull Request #9964 · microsoft/terminal

carlos-zamora · 2021-04-27T00:18:25Z

#9962 was caused by a serialization bug. Technically, ToJson works
as intended: if the current layer has any values set, write them out to
the json. However, on first load, the dynamic profile Profile objects
are actually empty (because they inherit from base layer, then the
dynamic profile generator). This means that ToJson writes the dynamic
profiles as empty objects {}. Then, on reload, we see that the dynamic
profiles aren't in the JSON, and we write them again.

To get around this issue, we added a simple check to Profile::ToJson:
if we have a source, make sure we write out the name, guid, hidden, and
source. This is intended to align with Profile::GenerateStub.

Closes #9962

DHowett · 2021-04-27T00:21:51Z

Great! Thanks so much!

qq: why force it during serialization instead of making sure the values are populated when the layer is created? On deserialization, that is.

or: should we just always write out name, guid, hidden, source?

carlos-zamora · 2021-04-27T00:41:30Z

Had an offline discussion:

Technically the only important ones to carry over are guid and source
name is just nice to have because it helps the user orient themselves
Idea: generate guid, source, and (as comment) name (make sure that GenerateStub does the same though)

qq: why force it during serialization instead of making sure the values are populated when the layer is created? On deserialization, that is.

No preference between the two. Figured altering ToJson would be safer but the other would work too. I guess it would mess with finding the source profile object for those 4 properties?

lhecker

I can confirm that this fixes the bug of empty ({}) profiles being created, as well as the SUI save button not working during the first launch.

I personally believe that we should put this logic into Profile::ToJson, as this model class controls how it's serialized/deserialized and "should" probably also control which fields are required in a serialization.
The alternative would be to put this change into CascadiaSettings::_ApplyDefaultsFromUserSettings which I believe might be a less general solution.

zadjii-msft

I'm gonna approve for now because this is such a high hitter, but we really should have a test for this

DHowett

Just a nit. This is a fix that will work, but it is a bit adhoc. To @lhecker's point about things that should be invariant during serialization: these things should be invariant during construction! The object should not be able to be in a bad state, at least for GUID. Name is neither here nor there. 😄

src/cascadia/TerminalSettingsModel/Profile.cpp

lhecker · 2021-04-28T00:30:33Z

these things should be invariant during construction

@DHowett As you might've already heard before the underlying issue is that during the first start we generate dynamic profiles. Due to that you might have two OriginTag::InBox profiles (cmd, powershell) and two OriginTag::Generated ones (pwsh7, wsl). During this initial load all of them get child profiles in CascadiaSettings::_ApplyDefaultsFromUserSettings. At this point we got a problem already: All 4 child profiles have no GUID and no name! The invariant is already violated. So question No. 1:
👉 Would you suggest that CreateChild() copies over the GUID, name, etc. to uphold this "invariant"?

This "issue" of having no GUID (and so on) is fixed for the two inbox profiles right here. Since we load the builtin default-settings.json during load the 2 inbox profiles are stored in _userSettings and that's the two profiles which _GetProfilesJsonObject returns. As such those two inbox profiles are layered in _LayerOrCreateProfile, where those child profiles are assigned the 4 essential properties (GUID, ...) from the default-settings.json.

💣 The dynamic profiles don't get this treatment since they're not part of the _userSettings during the first start of our app. Only on the second start they'll be stored in _userSettings.

ghost · 2021-04-28T16:47:12Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

#9962 was caused by a serialization bug. _Technically_, `ToJson` works as intended: if the current layer has any values set, write them out to the json. However, on first load, the dynamic profile `Profile` objects are actually empty (because they inherit from base layer, then the dynamic profile generator). This means that `ToJson` writes the dynamic profiles as empty objects `{}`. Then, on reload, we see that the dynamic profiles aren't in the JSON, and we write them again. To get around this issue, we added a simple check to `Profile::ToJson`: if we have a source, make sure we write out the name, guid, hidden, and source. This is intended to align with `Profile::GenerateStub`. Closes #9962 (cherry picked from commit 8f93f76)

ghost · 2021-05-25T19:05:36Z

🎉Windows Terminal v1.8.1444.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-05-25T19:08:51Z

🎉Windows Terminal Preview v1.9.1445.0 has been released which incorporates this pull request.:tada:

Handy links:

Serialize stub for dynamic profiles

219edc9

carlos-zamora added zStable-Service-Consider labels Apr 27, 2021

carlos-zamora requested a review from DHowett April 27, 2021 00:18

ghost added Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Apr 27, 2021

lhecker approved these changes Apr 27, 2021

View reviewed changes

zadjii-msft approved these changes Apr 27, 2021

View reviewed changes

zadjii-msft mentioned this pull request Apr 27, 2021

Tests we should maybe write one day #7001

Open

34 tasks

DHowett reviewed Apr 27, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/Profile.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

5d71ca9

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 28, 2021

ghost merged commit 8f93f76 into main Apr 28, 2021

ghost deleted the dev/cazamor/bugfix/9962 branch April 28, 2021 17:59

zadjii-msft mentioned this pull request May 5, 2021

Sometimes, saving a setting in the UI doesn't actually apply #9962

Closed

2 tasks

DHowett removed the zStable-Service-Consider label May 14, 2021

DHowett removed the zPreview-Service-Consider label May 28, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize stub for dynamic profiles#9964

Serialize stub for dynamic profiles#9964
2 commits merged intomainfrom
dev/cazamor/bugfix/9962

carlos-zamora commented Apr 27, 2021 •

edited by DHowett

Loading

Uh oh!

DHowett commented Apr 27, 2021

Uh oh!

carlos-zamora commented Apr 27, 2021

Uh oh!

lhecker left a comment

Uh oh!

zadjii-msft left a comment

Uh oh!

DHowett left a comment

Uh oh!

Uh oh!

lhecker commented Apr 28, 2021 •

edited

Loading

Uh oh!

ghost commented Apr 28, 2021

Uh oh!

ghost commented May 25, 2021

Uh oh!

ghost commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

carlos-zamora commented Apr 27, 2021 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Apr 27, 2021

Uh oh!

carlos-zamora commented Apr 27, 2021

Uh oh!

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lhecker commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 28, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented May 25, 2021

Uh oh!

ghost commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlos-zamora commented Apr 27, 2021 •

edited by DHowett

Loading

lhecker commented Apr 28, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.