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

Settings UI: Renaming default color scheme applies to each profile instead of defaults section #9094

Closed
mbudnek opened this issue Feb 10, 2021 · 6 comments · Fixed by #9103
Closed

Settings UI: Renaming default color scheme applies to each profile instead of defaults section #9094

mbudnek opened this issue Feb 10, 2021 · 6 comments · Fixed by #9103
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. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.7

Comments

@mbudnek
Copy link

mbudnek commented Feb 10, 2021

Environment

Windows build number: Microsoft Windows [Version 10.0.19042.746]
Windows Terminal version (if applicable): Windows Terminal Preview Version: 1.6.10272.0

Steps to reproduce

  1. Create a custom color scheme named 'foo'
  2. In the "Base Layer" settings page, select 'foo' as the color scheme
  3. Rename the color scheme 'foo' to 'bar'

Expected behavior

The color scheme in the "Base Layer" settings should be updated to be 'bar' and changing it should affect all profiles.

Actual behavior

The color scheme in the "Base Layer" settings changes to "Campbell" and changing it has no effect on other profiles. Looking at the settings file you will see that each profile has had the colorScheme attribute added with the new name, and the colorScheme attribute in the defaults section still has the old name.
@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 10, 2021
@DHowett
Copy link
Member

DHowett commented Feb 10, 2021

@carlos-zamora

@carlos-zamora
Copy link
Member

Oh man. I know exactly what the problem is:

terminal/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

Line 875 in 47f4b41

void CascadiaSettings::UpdateColorSchemeReferences(const hstring oldName, const hstring newName)

We should also update ProfileDefaults (aka "base layer") in this function.

@DHowett
Copy link
Member

DHowett commented Feb 10, 2021

Oof, you know what? I bet the loop here is forcing every profile to override the setting, too.

terminal/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

Lines 879 to 884 in 47f4b41

{
if (profile.ColorSchemeName() == oldName)
{
profile.ColorSchemeName(newName);
}
}

If you set BaseLayer's color scheme first, that check won't force an override ........ but that feels like "works because of circumstances", not "works because of good design"

@carlos-zamora
Copy link
Member

Good call. We need a HasColorSchemeName() check inside the loop too. Make sure that we're only redirecting any explicitly set references.

@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-2 A description (P2) Product-Terminal The new Windows Terminal. labels Feb 10, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 10, 2021
@zadjii-msft zadjii-msft added this to the Terminal v1.7 milestone Feb 10, 2021
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 10, 2021
@DHowett DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Feb 10, 2021
@carlos-zamora carlos-zamora mentioned this issue Feb 10, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Feb 10, 2021
@ghost ghost closed this as completed in #9103 Feb 10, 2021
@ghost ghost removed the In-PR This issue has a related PR label Feb 10, 2021
ghost pushed a commit that referenced this issue Feb 10, 2021
@carlos-zamora
Bugfix: update color scheme references properly (#9103)
4251126 
`CascadiaSettings::UpdateColorSchemeReferences` had two bugs in it:
1. we would never check/update the base layer
2. we would explicitly set the color scheme on a profile referencing the
   old name

This PR fixes both of those issues by checking/updating the base layer,
and ensuring that we check if a profile has an explicit reference before
updating it.

Since the affected code is in TSM, I also created an automated local
test.

## Validation Steps Performed
Bug repro steps.
Specifically tested [DHowett's scenario] too.
Test added.

Closes #9094 

[DHowett's scenario]: #9094 (comment)
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Feb 10, 2021
DHowett pushed a commit that referenced this issue Feb 10, 2021
@carlos-zamora @DHowett
Bugfix: update color scheme references properly (#9103)
c24194a 
`CascadiaSettings::UpdateColorSchemeReferences` had two bugs in it:
1. we would never check/update the base layer
2. we would explicitly set the color scheme on a profile referencing the
   old name

This PR fixes both of those issues by checking/updating the base layer,
and ensuring that we check if a profile has an explicit reference before
updating it.

Since the affected code is in TSM, I also created an automated local
test.

## Validation Steps Performed
Bug repro steps.
Specifically tested [DHowett's scenario] too.
Test added.

Closes #9094

[DHowett's scenario]: #9094 (comment)

(cherry picked from commit 4251126)
@DHowett
Copy link
Member

DHowett commented Feb 11, 2021

Thanks again for the report!

@ghost
Copy link

ghost commented Feb 11, 2021

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

Handy links:

@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Apr 8, 2021
This issue was closed.
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. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.7
Development

Successfully merging a pull request may close this issue.

Bugfix: update color scheme references properly microsoft/terminal
4 participants
@DHowett @mbudnek @carlos-zamora @zadjii-msft