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

Test: Integrated terminal API #11133

Closed
3 tasks done
Tyriar opened this issue Aug 29, 2016 · 7 comments
Closed
3 tasks done

Test: Integrated terminal API #11133

Tyriar opened this issue Aug 29, 2016 · 7 comments

Comments

@Tyriar
Copy link
Member

Tyriar commented Aug 29, 2016

Test for #9957


An API for the integrated terminal was added. See window.createTerminal and vscode.Terminal. To test this please make sure the API does what it claims and that it fits your expectations and is useful.

Instructions for testing against latest API here: #11111 (comment)

Sample API implementation here: https://github.com/Tyriar/vscode-terminal-api-example

@dbaeumer
Copy link
Member

Sorry for the late assignment but the test plan item wasn't assigned to the August milestone

@octref
Copy link
Contributor

octref commented Aug 30, 2016

Tested in Windows 10, all methods in the API example work as expected. Made a few small changes for your example in Tyriar/vscode-terminal-api-example#3

I'd suggest adding API for getting active terminal and a list of all terminals, though.
Extensions authors might be only interested in using the available terminals and don't want to create new ones.

@rebornix
Copy link
Member

Not sure if it's a problem but Terminal.hide doesn't seem consistent with other methods. When you run Terminal.dispose, corresponding terminal will be disposed and the next terminal in stack will be active. But if you run Terminal.hide, the whole terminal pane hides. This is somewhat confusing as hide should just work on the specific terminal instance, not the whole terminal pane.

Another thing is I don't find the difference between Terminal.show(true); and Terminal.show(false), do you mind describing more about what's the expected behavior of this parameter? @Tyriar

Others are all good.

@octref
Copy link
Contributor

octref commented Aug 31, 2016

@rebornix @Tyriar
My first guess was also Terminal.show(true) would preserve the focus on the editor while showing the terminal. It's a bit confusing.
And if you have 2 Terminals, with the second active. Calling hide() on first one would NOT hide the panel.

I think a better design is to expose

  • showTerminalPanel
  • hideTerminalPanel
  • toggleTerminalPanel

Also expose

  • createTerminal
  • getActiveTerminal
  • getAllTerminals

Then for each terminal, allow it to set itself as active terminal.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 31, 2016

@rebornix Terminal.hide is meant to hide the panel, only if that specific terminal is active, otherwise it does nothing. It wouldn't be useful to move to the next terminal as there could be an arbitrary number of terminals.

I believe Terminal.dispose currently shows the terminal. I think there is a TODO in the code but no associated GH issue (created #11275).

Terminal.show's arg is meant to indicate whether to focus the panel.

@octref it was an intentional design decision to not allow extensions access to terminal that were created by the user via the regular commands. Operations on the terminal panel as a whole may be good, but the API is being kept very slim currently, I want some compelling use cases from extension authors before adding more as it's difficult to change the API once it's in place.

I created #11276 to try clarify the functions some more 👍

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Aug 31, 2016

I agree with @Tyriar on not allowing extensions access to terminals created by users and on starting with slim design with additional features based on community feedback.

Regarding points made by @rebornix, extension authors are bound to have similar confusions, better documentation should help in that regard.

@rebornix
Copy link
Member

I agree that we leave this out of extension api. But we still have two issues (questions) left:

  1. @octref and I don't see the argument of Terminal.show(args) takes effect, the pane gets focused whether it's true or false.
  2. [Question] Current commands for managing terminals are somewhat limited. People who leverage these commands can only manage terminal created by themselves, which means if there are other terminal instances created by others, manually or through API, we have no control on them. Is this by purpose?

About improving the documentation, I think we may want to make sure people are clear about that two dimensions of status of terminal: active/inactive and show/hide.

For community feedback, @Tyriar you reached out to the author of https://marketplace.visualstudio.com/items?itemName=formulahendry.code-runner, I think it might be a good extension that can adopt your new command.

@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.
Projects
None yet
Development

No branches or pull requests

5 participants