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 TermControl initialization to pre-seed working dir #9397

Merged
2 commits merged into from
Mar 8, 2021
Merged

Fix TermControl initialization to pre-seed working dir #9397

2 commits merged into from
Mar 8, 2021

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Mar 5, 2021

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Mar 5, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 5, 2021

@skyline75489 - can you please take a look that I am not ruining stuff? 😊

@Don-Vito Don-Vito changed the title 8969 preseed starting dir Fix TermControl initialization to pre-seed working dir Mar 5, 2021
@skyline75489
Copy link
Collaborator

That's a good one-liner fix. I'm OK. Just want to make sure if this is @DHowett wants.

@DHowett
Copy link
Member

DHowett commented Mar 5, 2021

I think this is the wrong way around— we’re telling TermControl that an OSC 9;9 occurred using its internal API so that it calls TermControl’s working directory handler to trigger the working directory event. Can TerminalApp just handle this at the top layer? :)

DHowett
DHowett requested changes Mar 5, 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.

Discussion

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

Don-Vito commented Mar 5, 2021

Discussion

@DHowett - I am afraid I failed to follow the part with directory handler. My understanding wast that _workingDirectory in TerminalApi is just a container for the source of truth.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 5, 2021
@Don-Vito Don-Vito requested a review from DHowett March 5, 2021 23:30
@skyline75489
Copy link
Collaborator

I think what Dustin says is to add new event for "working directory change" and a callback for that event, like we did for title change ("pfnTitleChanged") and the other things.

@skyline75489
Copy link
Collaborator

skyline75489 commented Mar 5, 2021 •
edited
Loading

Well, due to my laziness, there's no such event at the moment. Or I'm misunderstanding Dustin. I'm still in the bed probably not really thinking straight 😅

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 5, 2021

Well, due to my laziness, there's no such event at the moment. Or I'm misunderstanding Dustin. I'm still in the bed probably not really thinking straight 😅

There is no such event, and actually I am also confused.

We could probably manage _workingDirectory on a higher level than in Terminal, in the first place, i.e., to introduce some event in Terminal and forward it to the TerminalApp. In this case the source of truth would reside in the app. However currently it is in Terminal.

@zadjii-msft
Copy link
Member

Yea I'm confused too. This is exactly how I would've done it... But Dustin is out for the next couple days, so we'll see what he has to say

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 8, 2021

Yea I'm confused too. This is exactly how I would've done it... But Dustin is out for the next couple days, so we'll see what he has to say

Let's leverage Dustin's absence to push some shitty code! 💃

DHowett
DHowett approved these changes Mar 8, 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.

Thanks for the explanation! Chester is right- I thought that we had an event that fired out of the control to alert the application.

In general, I find myself unhappy with the number of places we use TerminalSettings/CoreSettings/ControlSettings to communicate state from TerminalApp back to TerminalApp. Know what I mean?

Is the StartingDirectory a Core setting, or is it a control setting? If it’s a core setting, there is probably a different place we should be propagating it in. Using a Control setting to have Control tell Core something seems like it’s also the wrong speed.

None of these are complaints or concerns about the work you’ve done here, @Don-Vito! Just musing 😄

zadjii-msft
zadjii-msft approved these changes Mar 8, 2021
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Oh okay that makes sense then

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 8, 2021
@ghost
Copy link

ghost commented Mar 8, 2021

Hello @zadjii-msft!

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.

@ghost ghost merged commit 19bd0c9 into microsoft:main Mar 8, 2021
@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. labels Mar 8, 2021
DHowett pushed a commit that referenced this pull request Mar 15, 2021
@Don-Vito @DHowett
Fix TermControl initialization to pre-seed working dir (#9397)
86fa001 
## PR Checklist
* [x] Closes #8969
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

(cherry picked from commit 19bd0c9)
@DHowett DHowett removed the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label Mar 15, 2021
DHowett pushed a commit that referenced this pull request Mar 15, 2021
@Don-Vito @DHowett
Fix TermControl initialization to pre-seed working dir (#9397)
c7dc350 
## PR Checklist
* [x] Closes #8969
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

(cherry picked from commit 19bd0c9)
@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Mar 16, 2021
@zadjii-msft zadjii-msft mentioned this pull request Apr 7, 2021
Closed
@ghost
Copy link

ghost commented Apr 14, 2021

🎉Windows Terminal v1.7.1033.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Apr 14, 2021
Closed
@ghost
Copy link

ghost commented Apr 14, 2021

🎉Windows Terminal Preview v1.8.1032.0 has been released which incorporates this pull request.:tada:

Handy links:

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

@DHowett DHowett

Labels
Area-User Interface Issues pertaining to the user interface of the 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-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Don-Vito @skyline75489 @DHowett @zadjii-msft