-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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.
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
Fix TermControl initialization to pre-seed working dir #9397
Conversation
@skyline75489 - can you please take a look that I am not ruining stuff? 😊 |
That's a good one-liner fix. I'm OK. Just want to make sure if this is @DHowett wants. |
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? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion
@DHowett - I am afraid I failed to follow the part with directory handler. My understanding wast that |
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. |
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. |
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! 💃 |
There was a problem hiding this 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 😄
There was a problem hiding this 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
Hello @zadjii-msft! Because this pull request has the 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 (
|
## 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)
## 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)
🎉 Handy links: |
🎉 Handy links: |
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed