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

Code health: update every call of initializing TermControl with settings #9292

Closed
PankajBhojwani opened this issue Feb 25, 2021 · 1 comment · Fixed by #9296
Closed

Code health: update every call of initializing TermControl with settings #9292

PankajBhojwani opened this issue Feb 25, 2021 · 1 comment · Fixed by #9296
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@PankajBhojwani
Copy link
Contributor

PankajBhojwani commented Feb 25, 2021 •
edited
Loading

In #8602, we started passing a child of the TerminalSettings to the control upon tab initialization, we need to make sure that we are doing this for every call of initializing a new TermControl (we initially forgot to do this for controls initialized during pane split, which led to #9280).

EDIT: Ideally, we do this by having a helper that handles the creation of new TermControls, see DHowett's comment below.
@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 Feb 25, 2021
@PankajBhojwani PankajBhojwani added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Feb 25, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 25, 2021
@PankajBhojwani PankajBhojwani removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 25, 2021
@DHowett
Copy link
Member

DHowett commented Feb 25, 2021

I'd rather us use this bug for making it so we don't have so many places. Like, "Unify TerminalControl creation in TerminalApp to reduce duplication and regression risk"

@PankajBhojwani PankajBhojwani mentioned this issue Feb 25, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Feb 25, 2021
@ghost ghost closed this as completed in #9296 Mar 12, 2021
@ghost ghost removed the In-PR This issue has a related PR label Mar 12, 2021
ghost pushed a commit that referenced this issue Mar 12, 2021
@PankajBhojwani
Add a helper function to initialize TermControls in TerminalPage (#9296)
b76087f 
Since #8602 merged, we need to pass a child of the settings object to
the TermControl upon initializing it. Since this happens in a few places
in `TerminalPage`, its probably best to use a helper. 

Closes #9292
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Mar 12, 2021
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-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. 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.

Add a helper function to initialize TermControls in TerminalPage microsoft/terminal
2 participants
@DHowett @PankajBhojwani