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

Don't explode name-only profiles #2789

Merged
merged 8 commits into from
Sep 17, 2019

Conversation

zadjii-msft
Copy link
Member

Summary of the Pull Request

When the user adds a name-only profile, we shouldn't re-add that to their list of profiles each time they launch the terminal.

References

Introduced in #2515 or #2603

PR Checklist

  I was thinking maybe we have the stubs have a GUID included. I like that less though I think. That would mean that DPGs would always have the GUID generated for them, even if the DPG doesn't specify a GUID. I guess that's fine though. No DPG's _aren't_ generating names now so this shouldn't change anything.
  If the profile doesn't have a guid, it's a name-only profile.
  During validation, we'll generate a GUID for the profile, but
  validation occurs after this. We should ignore these types of
  profiles.
  If a dynamic profile was generated _without_ a GUID, we also
  don't want it serialized here. The first check in
  Profile::ShouldBeLayered checks that the profile hasa guid. For a
  dynamic profile without a GUID, that'll _never_ be true, so it
  would be impossible to be layered.
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Sep 17, 2019

const auto settings0Json = VerifyParseSucceeded(settings0String);

CascadiaSettings settings;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this test need to parse the defaults and validate them? it seems extraneous, but idk

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Sep 17, 2019
@ghost ghost requested review from miniksa and carlos-zamora September 17, 2019 16:09
@zadjii-msft zadjii-msft merged commit c30ef6d into master Sep 17, 2019
@ghost
Copy link

ghost commented Sep 24, 2019

🎉Windows Terminal Preview v0.5.2661.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autoinsertion of profiles causes exponential explosion when you add a {name:}-only profile
3 participants