Skip to content

Fix updating with fragments ignoring original profile settings#9293

Merged
1 commit merged intomainfrom
dev/pabhoj/fragments_fix
Feb 25, 2021
Merged

Fix updating with fragments ignoring original profile settings#9293
1 commit merged intomainfrom
dev/pabhoj/fragments_fix

Conversation

@PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Feb 25, 2021 •
edited by DHowett
Loading

Turns out we were adding the fragment source to profiles we update. This
PR fixes it so we keep the original source.

Validation Steps Performed

Existing profile settings are maintained

Closes #9290

@ghost ghost 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) labels Feb 25, 2021
DHowett
DHowett reviewed Feb 25, 2021
View reviewed changes
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
auto childImpl{ matchingProfile->CreateChild() };
childImpl->LayerJson(profileStub);
childImpl->Origin(OriginTag::Fragment);
childImpl->Source(source);
Copy link
Member

@DHowett DHowett Feb 25, 2021

Choose a reason for hiding this comment

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

AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA

Copy link
Contributor Author

@PankajBhojwani PankajBhojwani Feb 25, 2021

Choose a reason for hiding this comment

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

same

@DHowett DHowett requested a review from zadjii-msft February 25, 2021 22:07
@PankajBhojwani PankajBhojwani mentioned this pull request Feb 25, 2021
Merged
carlos-zamora
carlos-zamora requested changes Feb 25, 2021
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Wait. I'm pretty sure I introduced this for SUI Inheritance. So this'll probably break the tooltip there.

Blocking while I look into this a bit more.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 25, 2021
@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented Feb 25, 2021 •
edited
Loading

Wait. I'm pretty sure I introduced this for SUI Inheritance. So this'll probably break the tooltip there.

Lol I just commented on that PR (#9079) asking if we need this call

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 25, 2021
carlos-zamora
carlos-zamora approved these changes Feb 25, 2021
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Tested with SUI Inheritance. Adding the source there wasn't necessary. My bet is that I was using the source to handle fragment-extensions like dynamic profiles, then I added OriginTag::Fragment to not have to do that (and look at the source), and just never removed that source call.

@DHowett
Copy link
Member

DHowett commented Feb 25, 2021

@msftbot merge this in 1 minute

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 25, 2021
@ghost
Copy link

ghost commented Feb 25, 2021

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Thu, 25 Feb 2021 22:43:52 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit f26c246 into main Feb 25, 2021
@ghost ghost deleted the dev/pabhoj/fragments_fix branch February 25, 2021 22:44
DHowett pushed a commit that referenced this pull request Feb 25, 2021
@PankajBhojwani @DHowett
Fix updating with fragments ignoring original profile settings (#9293)
ec89b85 
Turns out we were adding the fragment source to profiles we update. This
PR fixes it so we keep the original source.

## Validation Steps Performed
Existing profile settings are maintained

Closes #9290

(cherry picked from commit f26c246)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DHowett DHowett DHowett approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@zadjii-msft zadjii-msft Awaiting requested review from zadjii-msft

Assignees

No one assigned

Labels

Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Proto-extensions ignore existing profile settings

3 participants

@PankajBhojwani @DHowett @carlos-zamora