-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Serialize stub for dynamic profiles #9964
Conversation
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? |
Had an offline discussion:
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna approve for now because this is such a high hitter, but we really should have a test for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄
@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 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 💣 The dynamic profiles don't get this treatment since they're not part of the |
Hello @DHowett! Because this pull request has the 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 (
|
#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)
#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)
🎉 Handy links: |
🎉 Handy links: |
#9962 was caused by a serialization bug. Technically,
ToJson
worksas intended: if the current layer has any values set, write them out to
the json. However, on first load, the dynamic profile
Profile
objectsare actually empty (because they inherit from base layer, then the
dynamic profile generator). This means that
ToJson
writes the dynamicprofiles as empty objects
{}
. Then, on reload, we see that the dynamicprofiles 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