Sometimes, saving a setting in the UI doesn't actually apply #9962
Comments
zadjii-msft
added
Issue-Bug
ghost
added
the
Needs-Triage
zadjii-msft
removed
the
Needs-Triage
|
hey @zadjii-msft when I deleted the whole |
|
@Krrishdhaneja That sounds like a totally different issue. Mind filing a new issue? (make sure to include version numbers, which profile you were expecting to see generated, your |
I don't mind making it, but I am rather saying that it might be the solution to that default json profile maybe, by deleting the Can it be it's solution? |
ghost
added
Resolution-Fix-Committed
#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
|
@zadjii-msft Every new profile I generate doesn't have a guid. So if you ignore these profiles you create a new problem. (How will this be fixed on updating to the new release?) |
|
Hey, today I encountered the issue again, and I think what could be possible reason for that is maybe when the modifications are made using SUI, the changes aren't registered in the profile we wanted/made changes for, but rather the SUI creates a completely new profile with those properties. |
|
@Krrishdhaneja Yep, that's basically the root cause of what we fixed in #9964 The "ignore snippets without a guid" mitigation should only apply to snippets without a name or guid. When a profile doesn't have an explicit guid, we generate one for that profile based on the name. in the case of the I also think we didn't end up doing that, so it's not a big deal. |
#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)
The bug that caused #9962 resulted in folks getting profiles written to their settings that didn't contain any identifying information (name or guid), sometimes multiple times. These profiles look (somewhat) like this: ```json { "colorScheme": "Campbell" }, {}, ``` An empty profile serves no purpose -- it shows up in the list as being named "Default", and it only launches CMD (unless the commandline is the thing that the user successfully changed.) We can heal the settings file by simply ignoring those profiles that have *no identifying information* (a guid or a name that can be converted into a guid). Validation ---------- I created a number of profiles that fit this format and made sure that they were ignored on load and destroyed on save.
The bug that caused #9962 resulted in folks getting profiles written to their settings that didn't contain any identifying information (name or guid), sometimes multiple times. These profiles look (somewhat) like this: ```json { "colorScheme": "Campbell" }, {}, ``` An empty profile serves no purpose -- it shows up in the list as being named "Default", and it only launches CMD (unless the commandline is the thing that the user successfully changed.) We can heal the settings file by simply ignoring those profiles that have *no identifying information* (a guid or a name that can be converted into a guid). Validation ---------- I created a number of profiles that fit this format and made sure that they were ignored on load and destroyed on save. ## PR Checklist * [x] Closes an annoyance we discovered after 9962.
The bug that caused #9962 resulted in folks getting profiles written to their settings that didn't contain any identifying information (name or guid), sometimes multiple times. These profiles look (somewhat) like this: ```json { "colorScheme": "Campbell" }, {}, ``` An empty profile serves no purpose -- it shows up in the list as being named "Default", and it only launches CMD (unless the commandline is the thing that the user successfully changed.) We can heal the settings file by simply ignoring those profiles that have *no identifying information* (a guid or a name that can be converted into a guid). Validation ---------- I created a number of profiles that fit this format and made sure that they were ignored on load and destroyed on save. ## PR Checklist * [x] Closes an annoyance we discovered after 9962. (cherry picked from commit 84f6a29)
|
🎉This issue was addressed in #9964, which has now been successfully released as Handy links: |
|
🎉This issue was addressed in #9964, which has now been successfully released as Handy links: |
`SettingsLoader::_parse` used to skip profiles which didn't have either a "guid" or "name" field, due to #9962. This is however wrong for fragment loading, as fragments can alternatively use an "updates" field instead of guid/name. `SettingsLoader::_parse` was updated to allow profiles with this alternative field during fragment loading. ## PR Checklist * [x] Closes #11331 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Wrote the following to `%LOCALAPPDATA%\Microsoft\Windows Terminal\Fragments\test\test.json`: ```json { "profiles": [ { "updates": "{574e775e-4f2a-5b96-ac1e-a2962a402336}", "background": "#FFD700" } ] } ```
The minimal repro:
At this point, the SUI will reload. The scrollbar state didn't apply, and now you've got a mysterious "Default" profile. Also, the
setting.jsonis now corrupted. It's now gotslammed in the profiles list, which is garbage data.
/cc @carlos-zamora.
Now, I'm gonna close all the threads we think are a dupe of this one.
The text was updated successfully, but these errors were encountered: