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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Proposal TerminalExitStatus.reason #152833

Merged
merged 11 commits into from Jul 11, 2022
Merged

Proposal TerminalExitStatus.reason #152833

merged 11 commits into from Jul 11, 2022

Conversation

jeanp413
Copy link
Contributor

This PR fixes #130231

Implements API proposal TerminalExitStatus.reason to address issue #130231
Added Shutdown(main motivation for #130231) and Unknown(i.e. any other reasons) reasons

Side note, after working on this, I would say that firing vscode.window.onDidCloseTerminal on shutdown is more of a bug than a feature, if some extension terminal logic is required to execute on shutdown then using context.subscriptions would be preferred I think but changing onDidCloseTerminal behavior would be a breaking change 馃

cc @Tyriar

jeanp413
jeanp413 commented Jun 22, 2022
View reviewed changes
src/vs/workbench/contrib/terminal/browser/terminal.ts
*/
dispose(immediate?: boolean): void;
Copy link
Contributor Author

@jeanp413 jeanp413 Jun 22, 2022 •
edited

Choose a reason for hiding this comment

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

Is immediate deprecated? There was only one place that used it (in terminalQuickAccess.ts) but I changed it to terminalService.safeDisposeTerminal so it's the same as other calls.
If not I'll revet this change.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not totally sure what the current state of this is, but it was always a bit of a hack to add a param to dispose like this so I'd want to move away from it if we were to change things in this area.

@akosyakov
Copy link
Contributor

@Tyriar Does proposed change make sense to you? We would really appreciate such API like stable though to unblock our customers. 馃檹

Tyriar
Tyriar reviewed Jun 24, 2022
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

The idea seems reasonable, I'll make a note to bring this to the next API sync which will be in 2 weeks.

src/vscode-dts/vscode.proposed.terminalExitReason.d.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/terminal/browser/terminal.ts
*/
dispose(immediate?: boolean): void;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not totally sure what the current state of this is, but it was always a bit of a hack to add a param to dispose like this so I'd want to move away from it if we were to change things in this area.

@Tyriar Tyriar added this to the July 2022 milestone Jun 24, 2022
@akosyakov akosyakov mentioned this pull request Jul 4, 2022
Closed
@akosyakov
Copy link
Contributor

@Tyriar thank you! Is API sync internal or we can follow it somehow as well?

@meganrogge
Copy link
Contributor

@akosyakov it is internal and we will post a summary after that happens

@loujaybee loujaybee mentioned this pull request Jul 7, 2022
Closed
@meganrogge
Copy link
Contributor

Discussed at the API sync and we think the shape looks good. Just the one question above

@jeanp413 @Tyriar
Update src/vs/workbench/contrib/terminal/browser/terminalInstance.ts
4ccbed7 
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
@jeanp413
Copy link
Contributor Author

jeanp413 commented Jul 8, 2022 •
edited

I forgot to ask this, how long does it take for a proposal API to become stable? I just realized that even if this is merged we won't be able to use it in our extension and publish it to the marketplace 馃槄

@Tyriar
Copy link
Member

Tyriar commented Jul 8, 2022

It needs to stay proposed for at least a month, this one's not complex so 1 month seems right

@akosyakov
Copy link
Contributor

akosyakov commented Jul 11, 2022 •
edited

Discussed at the API sync and we think the shape looks good. Just the one question above

@Tyriar @meganrogge Thank you a lot 馃檹 Is it good to land? I believe @jeanp413 addressed the feedback.

Tyriar
Tyriar previously approved these changes Jul 11, 2022
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Looks good, will merge after a full test pass

Tyriar
Tyriar previously approved these changes Jul 11, 2022
View reviewed changes
lramos15
lramos15 previously approved these changes Jul 11, 2022
View reviewed changes
@Tyriar Tyriar dismissed stale reviews from lramos15 and themself via fcbef68 July 11, 2022 14:48
@Tyriar Tyriar enabled auto-merge July 11, 2022 14:53
@Tyriar Tyriar merged commit 678e59a into microsoft:main Jul 11, 2022
@jeanp413 jeanp413 deleted the proposal-130231 branch July 11, 2022 15:09
@github-actions github-actions bot locked and limited conversation to collaborators Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@meganrogge meganrogge meganrogge left review comments

@Tyriar Tyriar Tyriar approved these changes

@lramos15 lramos15 lramos15 approved these changes

Assignees

@Tyriar Tyriar

Labels
None yet
Projects
None yet
Milestone
July 2022
Development

Successfully merging this pull request may close these issues.

expose pty terminal api to detect when it is explicitly killed by a user
5 participants
@jeanp413 @akosyakov @meganrogge @Tyriar @lramos15