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

Fragments with more than one "updates" profile may fail to load with a duplicate GUID warning #11597

Closed
lhecker opened this issue Oct 24, 2021 · 1 comment · Fixed by #11598
Closed

Fragments with more than one "updates" profile may fail to load with a duplicate GUID warning #11597

lhecker opened this issue Oct 24, 2021 · 1 comment · Fixed by #11598
Labels
Area-Settings Issues related to settings and customizability, for console or terminal 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.

Comments

@lhecker
Copy link
Member

lhecker commented Oct 24, 2021 •
edited
Loading

Windows Terminal version (or Windows build number)

1.12.2931.0

Steps to reproduce

As reported at #11325 (comment)

  • Create this file at %LOCALAPPDATA%\Microsoft\Windows Terminal\Fragments\jan-config\wsl.json: 
    {
    "$schema": "https://aka.ms/terminal-profiles-schema",
    "profiles": [
        {
            "updates": "{58ad8b0c-3ef8-5f4d-bc6f-13e4c00f2530}",
            //"name": "Debian",
            "colorScheme": "Nord",
            "hidden": false,
            "cursorShape": "underscore",
            "fontFace": "FantasqueSansMono Nerd Font",
            "fontSize": 16,
            "backgroundImage": "C:/Users/jantari/Openlogo-debianV2.svg - Kopie.png",
            "backgroundImageStretchMode": "none",
            "backgroundImageOpacity": 0.3,
            "backgroundImageAlignment": "bottomRight",
            "useAcrylic": false,
            "acrylicOpacity": 0.86,
            "source": "Windows.Terminal.Wsl"
        },
        {
            "updates": "{07b52e3e-de2c-5db4-bd2d-ba144ed6c273}",
            //"name": "Ubuntu 20.04",
            "colorScheme": "Nord",
            "hidden": false,
            "cursorShape": "underscore",
            "fontFace": "FantasqueSansMono Nerd Font",
            "fontSize": 16,
            "backgroundImage": "%LOCALAPPDATA%/WSL_ICONS/wt-wsl-ubuntu-logo.png",
            "backgroundImageStretchMode": "none",
            "backgroundImageOpacity": 0.3,
            "backgroundImageAlignment": "bottomRight",
            "useAcrylic": true,
            "acrylicOpacity": 0.86,
            "source": "Windows.Terminal.Wsl"
        }
    ]
}

Expected Behavior

Settings load successfully.

Actual Behavior

image

Cause & Solution

The "updates" key is an alternative "guid" key for fragment profiles.
But SettingsLoader::_appendProfile stores and deduplicates profiles according to their "guid" only. We need to modify the function to optionally store profiles by their "updates" key instead, otherwise multiple fragment profile without "guid" might collide as they produce the same default GUID.
@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 Oct 24, 2021
@lhecker lhecker added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Oct 24, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 24, 2021
This was referenced Oct 24, 2021
Merged
Merged
@ghost ghost added the In-PR This issue has a related PR label Oct 24, 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 Oct 26, 2021
@ghost ghost closed this as completed in #11598 Oct 27, 2021
ghost pushed a commit that referenced this issue Oct 27, 2021
@lhecker
Fix loading of fragments that update multiple profiles (#11598)
fe26a6e 
The "updates" key is an alternative "guid" key for fragment profiles.
But SettingsLoader::_appendProfile stores and deduplicates profiles according
to their "guid" only. We need to modify the function to optionally store
profiles by their "updates" key as well, otherwise multiple fragment
profiles without "guid" might collide as they produce the same default GUID.

## PR Checklist
* [x] Closes #11597
* [x] I work here
* [ ] Tests added/passed
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Validation Steps Performed
* Unit tests pass ✔️
* Issue #11597 doesn't reproduce anymore ✔️
@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 Oct 27, 2021
DHowett pushed a commit that referenced this issue Dec 13, 2021
@lhecker @DHowett
Fix loading of fragments that update multiple profiles (#11598)
c9cde49 
The "updates" key is an alternative "guid" key for fragment profiles.
But SettingsLoader::_appendProfile stores and deduplicates profiles according
to their "guid" only. We need to modify the function to optionally store
profiles by their "updates" key as well, otherwise multiple fragment
profiles without "guid" might collide as they produce the same default GUID.

## PR Checklist
* [x] Closes #11597
* [x] I work here
* [ ] Tests added/passed
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Validation Steps Performed
* Unit tests pass ✔️
* Issue #11597 doesn't reproduce anymore ✔️

(cherry picked from commit fe26a6e)
@ghost
Copy link

ghost commented Dec 14, 2021

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

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Settings Issues related to settings and customizability, for console or terminal 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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix loading of fragments that update multiple profiles microsoft/terminal
2 participants
@lhecker @zadjii-msft