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

Fix settings not updating on reload #9289

Merged
merged 2 commits into from
Feb 25, 2021
Merged

Fix settings not updating on reload #9289

merged 2 commits into from
Feb 25, 2021

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
@@ -876,7 +876,7 @@ namespace winrt::TerminalApp::implementation

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

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

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
@@ -1840,7 +1840,7 @@ namespace winrt::TerminalApp::implementation
return;
}

TermControl newControl{ controlSettings, controlConnection };
TermControl newControl{ *(winrt::get_self<TerminalSettings>(controlSettings)->CreateChild()), controlConnection };
Copy link
Member

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

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

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 DHowett added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. and removed zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. labels Feb 25, 2021
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

@zadjii-msft zadjii-msft zadjii-msft approved these changes

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

@DHowett DHowett DHowett 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