Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SettingsUI removes "disabledProfileSources" #9032

Closed
vefatica opened this issue Feb 4, 2021 · 5 comments · Fixed by #9038
Closed

SettingsUI removes "disabledProfileSources" #9032

vefatica opened this issue Feb 4, 2021 · 5 comments · Fixed by #9038
Assignees
@carlos-zamora
Labels
Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-DataLoss A bug that causes the user's data to be lost, unintentionally
Milestone
Terminal v1.7

Comments

@vefatica
Copy link

vefatica commented Feb 4, 2021

Using the SettingsUI removes my "disabledProfileSources" customization. I wish it didn't do that. Perhaps it clobbers other customizations too.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Feb 4, 2021
@zadjii-msft
Copy link
Member

Huh. That's no good.

@zadjii-msft zadjii-msft added this to the Terminal v1.7 milestone Feb 4, 2021
@zadjii-msft zadjii-msft added Area-Settings UI 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. Severity-DataLoss A bug that causes the user's data to be lost, unintentionally labels Feb 4, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 4, 2021
@WSLUser

This comment has been minimized.

Sign in to view
@zadjii-msft
Copy link
Member

Woah now - removes a setting != reformats the file. In general, the file will be reformatted, yes. It should NOT, in any circumstances, be removing settings that are otherwise set. Unless the user modifies a setting, it should remain untouched (or in the case of floats, equivalent).

If the setting is getting lost, then there's a good chance that it's being bound incorrectly. That's my theory at least.

@vefatica
Copy link
Author

vefatica commented Feb 4, 2021

I read #8991 when it was new but didn't appreciate what @WSLUser was saying until this issue arose. The new format is shocking at first but completely rational and consistent across the whole file. I'll have no problem getting used to it. I miss the comments that gave hints about the use of the file's major sections; "keybindings" is the only section that still has such/any comments. Hopefully, someday soon, I won't have to look at the file at all.

@carlos-zamora carlos-zamora mentioned this issue Feb 4, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Feb 4, 2021
@ghost ghost removed the In-PR This issue has a related PR label Feb 5, 2021
DHowett pushed a commit that referenced this issue Feb 5, 2021
@carlos-zamora
Serialize 'disabledProfileSources' (#9038)
230fad5 
"disabledProfileSources" is saved to `CascadiaSettings` _not_
`GlobalAppSettings` (and, even then, it's only read when it's used,
never saved). This PR specifically detects if it was defined in
settings.json, and copies it over when the settings are serialized.

## Validation Steps Performed
1. Added "disabledProfileSources" to settings.json, then serialized. -->
   "disabledProfileSources" is now maintained.
2. Updated `CascadiaSettings` serialization test

Closes #9032
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Feb 5, 2021
DHowett pushed a commit that referenced this issue Feb 5, 2021
@carlos-zamora @DHowett
Serialize 'disabledProfileSources' (#9038)
9444617 
"disabledProfileSources" is saved to `CascadiaSettings` _not_
`GlobalAppSettings` (and, even then, it's only read when it's used,
never saved). This PR specifically detects if it was defined in
settings.json, and copies it over when the settings are serialized.

## Validation Steps Performed
1. Added "disabledProfileSources" to settings.json, then serialized. -->
   "disabledProfileSources" is now maintained.
2. Updated `CascadiaSettings` serialization test

Closes #9032

(cherry picked from commit 230fad5)
@ghost
Copy link

ghost commented Feb 11, 2021

🎉This issue was addressed in #9038, which has now been successfully released as Windows Terminal Preview v1.6.10412.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-DataLoss A bug that causes the user's data to be lost, unintentionally
Projects
None yet
Milestone
Terminal v1.7
Development

Successfully merging a pull request may close this issue.

Serialize 'disabledProfileSources' microsoft/terminal
4 participants
@carlos-zamora @zadjii-msft @WSLUser @vefatica