Skip to content

Fix settings not updating on reload#9289

Merged
DHowett merged 2 commits intomainfrom
dev/pabhoj/settings_reload_fix
Feb 25, 2021
Merged

Fix settings not updating on reload#9289
DHowett merged 2 commits intomainfrom
dev/pabhoj/settings_reload_fix

Conversation

@PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Feb 25, 2021 •
edited
Loading

Summary of the Pull Request

Fix for #9280

In #8602, we started passing a child of the TerminalSettings to the control upon tab initialization, but forgot to do the same when new controls get created on a pane split.

PR Checklist

Validation Steps Performed

Settings reload with multiple panes works

@ghost ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Issue-Samples A request for an example on how we do something. Probably a good docs candidate. Product-Terminal The new Windows Terminal. labels Feb 25, 2021
PankajBhojwani
PankajBhojwani commented Feb 25, 2021
View reviewed changes
src/cascadia/TerminalApp/TerminalPage.cpp
if (debugConnection) // this will only be set if global debugging is on and tap is active
{
TermControl newControl{ settings, debugConnection };
TermControl newControl{ *(winrt::get_self<TerminalSettings>(settings)->CreateChild()), debugConnection };
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.

This change doesn't affect the settings reload bug but I figured we should update this call as well

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.

It actually will impact reload, if the pane is a debug tap pane :D

DHowett
DHowett reviewed Feb 25, 2021
View reviewed changes
src/cascadia/TerminalApp/TerminalPage.cpp
}

TermControl newControl{ controlSettings, controlConnection };
TermControl newControl{ *(winrt::get_self<TerminalSettings>(controlSettings)->CreateChild()), controlConnection };
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.

Oh gosh. Can you file a Code Health task to clean up all the different places in which we create a TerminalControl with its settings? I don't love that we had to make this fix in multiple places.

@carlos-zamora
Copy link
Member

carlos-zamora commented Feb 25, 2021

I think you meant to close #9280?

@ghost ghost added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Severity-Blocking We won't ship a release like this! No-siree. labels Feb 25, 2021
@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented Feb 25, 2021

I think you meant to close #9280?

Yes... thank you for the catch

@PankajBhojwani PankajBhojwani 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 @PankajBhojwani!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 6 hours 13 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@DHowett DHowett merged commit a930aa3 into main Feb 25, 2021
@DHowett DHowett deleted the dev/pabhoj/settings_reload_fix branch February 25, 2021 21:00
DHowett pushed a commit that referenced this pull request Feb 25, 2021
@PankajBhojwani @DHowett
Fix settings not updating on reload (#9289)
e103ca5 
In #8602, we started passing a child of the `TerminalSettings` to the
control upon tab initialization, but forgot to do the same when new
controls get created on a pane split.

## Validation Steps Performed
Settings reload with multiple panes works

Closes #9280

(cherry picked from commit a930aa3)
@zadjii-msft zadjii-msft mentioned this pull request Mar 8, 2021
Open
34 tasks
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 zadjii-msft approved these changes

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. Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Issue-Samples A request for an example on how we do something. Probably a good docs candidate. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Appearance settings in a pane aren't updating on reload

4 participants

@PankajBhojwani @carlos-zamora @DHowett @zadjii-msft