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

feat #7892: support vsc ext create pseudo terminal #7925

Merged
merged 1 commit into from
Jun 4, 2020
Merged

feat #7892: support vsc ext create pseudo terminal #7925

merged 1 commit into from
Jun 4, 2020

Conversation

datou0412
Copy link
Contributor

@datou0412 datou0412 commented May 29, 2020

Signed-off-by: 二凢 dingyu.wdy@alibaba-inc.com

What it does

Support API vscode.window.createTerminal with option ExtensionTerminalOptions

  • In plugin process, store _pseudoTerminals in memory
  • TerminalServiceMainImpl listen to terminal widget events and call _pseudoTerminals&widget methods through proxy
  • TerminalWidgetImpl fire events and expose methods.
  • Create a pseudo terminal process named asPseudoPty in backend.

Plugin Process <--proxy--> TerminalServiceMain <--Events&Methods--> TerminalWidget <--proxy--> Shell Process

How to test

Install any extension which use the api vscode.window.createTerminal with option ExtensionTerminalOptions

  • package this extension with vsce and move the .vsix file to plugins folder
  • yarn start:browser
  • cmd+p -> Extension Terminal Sample: Create

Just input anything
image

Resize your window
image

Review checklist

Reminder for reviewers

@akosyakov akosyakov added process issues related to the process extension terminal issues related to the terminal vscode issues related to VSCode compatibility labels May 29, 2020
@akosyakov
Copy link
Member

Install an extension which use the api vscode.window.createTerminal with option ExtensionTerminalOptions

It would be good to have something more concrete steps for reviewers.

Maybe this one can be used to test

Maybe sounds like you did not test your changes. How to test section should show that you know how to test and share it with others.

@datou0412
Copy link
Contributor Author

Install an extension which use the api vscode.window.createTerminal with option ExtensionTerminalOptions

It would be good to have something more concrete steps for reviewers.

Maybe this one can be used to test

Maybe sounds like you did not test your changes. How to test section should show that you know how to test and share it with others.

I have tested my changes with the extension. Maybe i should remove Maybe :p

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

Code looks quite good. I hope it works as good too, someone has to test it.

@akosyakov
Copy link
Member

@datou0412 Could you add fix marker in the description for issues which will be resolved with it? see https://help.github.com/en/enterprise/2.16/user/github/managing-your-work-on-github/closing-issues-using-keywords

@datou0412
Copy link
Contributor Author

Code looks quite good. I hope it works as good too, someone has to test it.

I have fixed all conversation issues, re-review is welcomed

@datou0412 datou0412 requested a review from akosyakov June 1, 2020 12:49
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Code LGTM, I'll let Anton review the widget-api/storage aspect.

packages/process/src/node/pseudo-pty.ts Outdated Show resolved Hide resolved
packages/process/src/node/pseudo-pty.ts Outdated Show resolved Hide resolved
packages/process/src/node/terminal-process.ts Show resolved Hide resolved
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

tested with the sample vscode extension and it behaves in the same way as in VS Code

Please get rid of commits fixing issues introduced by the PR. One commit will be enough. Please see: https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#checklist-commit-history

Signed-off-by: 二凢 <dingyu.wdy@alibaba-inc.com>
@akosyakov akosyakov merged commit 3079416 into eclipse-theia:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process issues related to the process extension terminal issues related to the terminal vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants