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.

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

How to determine if a Terminal created using new API has been closed by the user? #10925

Closed
DonJayamanne opened this issue Aug 25, 2016 · 17 comments
Assignees
Labels
api feature-request Request for new features or functionality terminal Integrated terminal issues

Comments

@DonJayamanne
Copy link
Contributor

  • VSCode Version: Insider Build

I'm using the code sample put together by @Tyriar

Steps to Reproduce:

  1. Run the above sample
  2. Create a terminal using the new API
  3. Send text to this new terminal
  4. Close the new terminal using the icon on the top right of the terminal panel
    (NOTE: Terminal has now been disposed, but extension is not aware of this)
  5. Next try to send text to previously created terminal (that no longer exists)
    A new terminal window is opened.

Is it possible for the extension to be notified of the fact that the terminal has been closed?
Either via:

  • A callback (method or event)
  • Or extension needs to be able to query this
@Tyriar
Copy link
Member

Tyriar commented Aug 25, 2016

@jrieken what's the common/recommended way of communicating this sort of event via the API?

@jrieken
Copy link
Member

jrieken commented Aug 26, 2016

We do this with the global events ala window.onDidCloseTerminal like this active editor event.

@Tyriar One question regarding hide vs dispose. Are they the same? So when hiding a terminal its like killing it? Iff so we should revisit the API around that and remove either hide or dispose.

@Tyriar
Copy link
Member

Tyriar commented Aug 26, 2016

@jrieken I should flesh this out in the jsdoc if it wasn't clear, but hide will close the panel if that terminal is currently active, dispose kills the terminal but keeps the panel up.

@jrieken
Copy link
Member

jrieken commented Aug 29, 2016

Close the new terminal using the icon on the top right of the terminal panel
(NOTE: Terminal has now been disposed, but extension is not aware of this)

@Tyriar I am a little confused because of that. I assume that is the same as calling hide where ctrl+c is like dispose

@Tyriar
Copy link
Member

Tyriar commented Aug 29, 2016

The workbench.action.terminal.kill command (bin icon) will kill the terminal instance, currently the API only goes one way so an API terminal does not know that a terminal has been disposed via the command. I'll look into making an window.onDidCloseTerminal similar to https://github.com/Microsoft/vscode/blob/1f412b5a1e52c661eb60f203fbdc43507430d5df/src/vs/vscode.d.ts#L3321

@DonJayamanne
Copy link
Contributor Author

@Tyriar , will the proposed window.onDidCloseTerminal be available in the next version (1.5) of VS Code or is this something that would probably be added in a subsequent build?
If it is planned for a later version, then I can work around this without having to wait for it.

@Tyriar Tyriar added this to the September 2016 milestone Sep 2, 2016
@Tyriar Tyriar added the feature-request Request for new features or functionality label Sep 2, 2016
@Tyriar
Copy link
Member

Tyriar commented Sep 2, 2016

@DonJayamanne looking at this for 1.6

@DonJayamanne
Copy link
Contributor Author

@Tyriar , thanks

@Tyriar Tyriar added the api label Sep 14, 2016
@Tyriar Tyriar closed this as completed in e3831ee Sep 15, 2016
Tyriar added a commit that referenced this issue Sep 15, 2016
Tyriar added a commit to microsoft/vscode-extension-samples that referenced this issue Sep 15, 2016
@daviwil
Copy link
Contributor

daviwil commented Sep 15, 2016

@Tyriar Awesome, nice work!

@Tyriar
Copy link
Member

Tyriar commented Sep 15, 2016

This will land in tomorrow's insiders. Note that currently the event will only fire for terminals created with the API. This can be easily be changed in a backwards compatible way to include all terminals if an window.onDidOpenTerminal is added.

It should fire when terminals are closed both through the API and through user interaction (command, button).

@jrieken
Copy link
Member

jrieken commented Sep 19, 2016

@Tyriar What is the definition of close? kill/dispose or not visible anymore?

@Tyriar
Copy link
Member

Tyriar commented Sep 19, 2016

@jrieken kill/dispose, I'd prefer to name this onDidDisoseTerminal to be consistent.

@Tyriar Tyriar reopened this Sep 19, 2016
@jrieken
Copy link
Member

jrieken commented Sep 19, 2016

👎 We already have this and it is also about disposing

        /**
         * An event that is emitted when a [text document](#TextDocument) is disposed.
         */
        export const onDidCloseTextDocument: Event<TextDocument>;

@Tyriar
Copy link
Member

Tyriar commented Sep 19, 2016

So just improve the jsdoc that it's about disposing, not hiding?

@Tyriar
Copy link
Member

Tyriar commented Sep 19, 2016

Also onDidCloseTextDocument is a little different isn't it? Dispose on that could indicate deleting the file.

@Tyriar Tyriar closed this as completed in 639d7de Sep 19, 2016
@Tyriar
Copy link
Member

Tyriar commented Sep 19, 2016

Changed close to dispose in the jsdoc, reopen if there's anything else you think should change.

@jrieken
Copy link
Member

jrieken commented Sep 20, 2016

lgtm

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api feature-request Request for new features or functionality terminal Integrated terminal issues
Projects
None yet
Development

No branches or pull requests

5 participants