Skip to content

Add a helper function to initialize TermControls in TerminalPage#9296

Merged
3 commits merged intomainfrom
dev/pabhoj/control_init
Mar 12, 2021
Merged

Add a helper function to initialize TermControls in TerminalPage#9296
3 commits merged intomainfrom
dev/pabhoj/control_init

Conversation

@PankajBhojwani
Copy link
Contributor

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

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 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
@PankajBhojwani PankajBhojwani changed the title Implement and use helper function to initialize TermControls in TerminalPage Implement and use a helper function to initialize TermControls in TerminalPage Feb 25, 2021
DHowett
DHowett requested changes Feb 26, 2021
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I'd like to see if there's a little more we can do here. There must be more code that's duplicated between these functions 😄

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 26, 2021
@carlos-zamora carlos-zamora mentioned this pull request Feb 26, 2021
Merged
@ghost ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Mar 5, 2021
@ghost
Copy link

ghost commented Mar 5, 2021

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@DHowett
Copy link
Member

DHowett commented Mar 11, 2021

blah

@ghost ghost removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Mar 11, 2021
@DHowett
Copy link
Member

DHowett commented Mar 11, 2021

I can't remember what made me balk at this -- I tried to talk about considering terminal control creation as a "data flow" pipeline:

Profile -----------\
                    ----> Settings -------------> Control
NewTerminalArgs ---/         \                /
                              `-> Connection-/

and then Pankaj convinced me that that's pretty much what we already had with this PR.

@DHowett DHowett changed the title Implement and use a helper function to initialize TermControls in TerminalPage Add a helper function to initialize TermControls in TerminalPage Mar 12, 2021
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 12, 2021
@ghost
Copy link

ghost commented Mar 12, 2021

Hello @DHowett!

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.

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 reviewed Mar 12, 2021
View reviewed changes
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 12, 2021
DHowett
DHowett requested changes Mar 12, 2021
View reviewed changes
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 12, 2021
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 12, 2021
@DHowett DHowett added AutoMerge Marked for automatic merge by the bot when requirements are met and removed AutoMerge Marked for automatic merge by the bot when requirements are met labels Mar 12, 2021
@ghost ghost merged commit b76087f into main Mar 12, 2021
@ghost ghost deleted the dev/pabhoj/control_init branch March 12, 2021 20:08
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

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

Assignees

No one assigned

Labels

Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Code health: update every call of initializing TermControl with settings

3 participants

@PankajBhojwani @DHowett @zadjii-msft