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.

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

Renaming a color scheme used by a profile should properly update the profile #8756

Closed
carlos-zamora opened this issue Jan 12, 2021 · 1 comment · Fixed by #8793
Closed

Renaming a color scheme used by a profile should properly update the profile #8756

carlos-zamora opened this issue Jan 12, 2021 · 1 comment · Fixed by #8793
Assignees
Labels
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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@carlos-zamora
Copy link
Member

Steps to reproduce

  1. Open Settings UI
  2. Have a profile ("Profile A") refer to a custom color scheme ("Color Scheme A")
  3. Go to the Color Schemes page
  4. Select "Color Scheme A"
  5. Rename it to something else ("Color Scheme B")
  6. Navigate back to the profile that referred to this color scheme ("Profile A")

Expected behavior

"Profile A" should now reference "Color Scheme B".

Actual behavior

"Profile A" refers to "Campbell"

Additional details

The TerminalSettingsModel stores Profile.ColorScheme as a string reference to the name of the ColorScheme.
In TerminalSettingsEditor (Profiles.cpp), if a ColorScheme with that name isn't found, we fallback to "Campbell".

Perhaps a better architecture would be to reference a ColorScheme object in the TerminalSettingsModel? Or at least reference a ColorScheme object in the ViewModel in TerminalSettingsEditor.

@carlos-zamora carlos-zamora added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-1 A description (P1) Area-Settings UI Anything specific to the SUI labels Jan 12, 2021
@carlos-zamora carlos-zamora added this to the Terminal v1.6 milestone Jan 12, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 12, 2021
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 12, 2021
@carlos-zamora carlos-zamora self-assigned this Jan 14, 2021
@carlos-zamora
Copy link
Member Author

So, I'm conflicted. There's the "brute-force" approach and the "elegant" way to do this:

  • brute-force approach:
    • When rename is accepted, iterate over all profiles and if profile.ColorSchemeName() == oldName, update the profile w/ profile.ColorSchemeName(newName).
  • elegant approach:
    • Idea: use ColorScheme references so that renaming a ColorScheme automatically updates whoever references it
    • Implementation:
      • ProfileViewModel holds a reference to a ColorScheme object (the one that the String ColorSchemeName refers to)
      • (the hard/annoying part) Since ColorScheme.Name() is not observable, we need to create a ColorSchemeViewModel to wrap each ColorScheme, which makes Name observable.
      • On a NameChanged event, ProfileViewModel would update Profile.ColorSchemeName
    • Concerns:
      • As I was writing this, I realized that that won't work either. We're creating the ProfileViewModel as we're navigating to the profile page. So the reference to the ColorScheme object would be created after the rename operation was completed.
      • To get around that issue, we would have to wrap all of the settings model objects with their respective view model objects when the Settings UI is opened.

@DHowett I'm going with the brute-force approach right now. Not too happy with it but it'll at least fix the bug for now. We should discuss what the overall architecture should look like, then file an issue to perform the refactor.

@ghost ghost added the In-PR This issue has a related PR label Jan 14, 2021
@ghost ghost closed this as completed in #8793 Jan 22, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 22, 2021
ghost pushed a commit that referenced this issue Jan 22, 2021
## Summary of the Pull Request
This fixes a bug where renaming/deleting a color scheme would not update profiles that referenced it.

This also adds detection for renaming a color scheme to a name that is already in use, and adds appropriate UI for that.

## References
#6800 - Settings UI Epic

## PR Checklist
* [X] Closes #8756 

## Detailed Description of the Pull Request / Additional comments
`Model::CascadiaSettings` was updated to have a `UpdateColorSchemeReferences()` function that updates all profiles referencing the newly renamed color scheme.

`Editor::ColorSchemesPageNavigationState` now takes and exposes a `Model::CascadiaSettings`.

When a color scheme is renamed or deleted, we use `CascadiaSettings` to update our list of color schemes appropriately, then call `UpdateColorSchemeReferences()` to update the profiles.

The tricky part is that `Profile` does not store a direct reference to `ColorScheme`, but rather the name of the color scheme. See [this tread](#8756 (comment)) for a discussion on this topic.

## Validation Steps Performed
Repro steps from #8756 when renaming/deleting a referenced color scheme.

## Demo
![Scheme Name Already In Use Demo](https://user-images.githubusercontent.com/11050425/105431427-6e023980-5c0a-11eb-894a-42152fc77f05.gif)
mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
## Summary of the Pull Request
This fixes a bug where renaming/deleting a color scheme would not update profiles that referenced it.

This also adds detection for renaming a color scheme to a name that is already in use, and adds appropriate UI for that.

## References
microsoft#6800 - Settings UI Epic

## PR Checklist
* [X] Closes microsoft#8756 

## Detailed Description of the Pull Request / Additional comments
`Model::CascadiaSettings` was updated to have a `UpdateColorSchemeReferences()` function that updates all profiles referencing the newly renamed color scheme.

`Editor::ColorSchemesPageNavigationState` now takes and exposes a `Model::CascadiaSettings`.

When a color scheme is renamed or deleted, we use `CascadiaSettings` to update our list of color schemes appropriately, then call `UpdateColorSchemeReferences()` to update the profiles.

The tricky part is that `Profile` does not store a direct reference to `ColorScheme`, but rather the name of the color scheme. See [this tread](microsoft#8756 (comment)) for a discussion on this topic.

## Validation Steps Performed
Repro steps from microsoft#8756 when renaming/deleting a referenced color scheme.

## Demo
![Scheme Name Already In Use Demo](https://user-images.githubusercontent.com/11050425/105431427-6e023980-5c0a-11eb-894a-42152fc77f05.gif)
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant