Initial contribution of @theia/external-terminal #9186

DucNgn · 2021-03-11T16:55:05Z

What it does

Introduces a new extension: @theia/external-terminal. The extension adds support for spawning external terminals from Electron applications. In the future, the extension can be extended to support features such as spawning external terminals during debug sessions.
(Browser applications should not pull this extension).

Checklist for merging:

CQ registered and approved
core: lazy preference proxy #9169 needs to be merged first since this PR depends on the ability for late binding in preference proxy.

Command:

Closes terminal: add support for opening external terminal windows #8712 : Supports opening native terminals by calling Open New External Terminal from the command palette.

Preferences:

The PR contributes the following preferences:
- terminal.external.windowsExec
- terminal.external.osxExec
- terminal.external.linuxExec
  to allow users to customize the desired terminal application to run on their current platform.
On startup, the backend determines the default terminal on the host machine, then sets the default field in the preference schema accordingly.
Changing the Exec preference of other OS than the current one won't have any effect. (i.e: Changing osxExec while running on Windows doesn't have any effect).

How to test:

Start Theia Electron application
Verify the default exec of host OS is displaying the correct terminal.

Calling command Open New External Terminal has different behaviors, depending on the current workspace status:

No workspaces opened and no current active editor -> Spawn the terminal at the user home directory.

no-ws-no-editors.mp4

No workspaces opened but has an active editor -> Spawn the terminal at the parent folder of the active editor file.

no-ws-active-editor.mp4

Only one workspace opened -> Spawn the terminal at the root of the current workspace.

singleWS.mp4

Multi-root workspace opened -> Displays a quick pick to let users choose which workspace to spawn the terminal.

multiWS.mp4

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

Signed-off-by: Duc Nguyen duc.a.nguyen@ericsson.com

vince-fugnitto · 2021-03-11T17:33:16Z

I'll file the appropriate CQs for the changes.

Edit: added a link in the pull-request description: CQ.

packages/terminal-external/src/electron-browser/terminal-external-preference.ts

vince-fugnitto · 2021-03-16T17:28:28Z

The CQ has been approved 👍:

We have completed a high level triage of the attachment and you may now check the content into your project’s source code repository...

paul-marechal

LGTM, works well on Windows!

I just have minor comments.

packages/terminal-external/src/common/terminal-external.ts

packages/terminal-external/src/electron-browser/terminal-external-preference.ts

paul-marechal · 2021-03-17T19:45:19Z

Please rebase on latest master, I just merged #9169 :)

colin-grant-work

Seems to be working for me. Some minor comments on the code and some even more minor comments on the comments. I do wonder why the name terminal-external rather than external-terminal. The former sounds like it's things that happen outside of the terminal, rather than terminals that appear outside the application.

packages/terminal-external/README.md

packages/terminal-external/src/common/terminal-external.ts

packages/terminal-external/src/electron-browser/terminal-external-contribution.ts

packages/terminal-external/src/electron-browser/terminal-external-preference.ts

packages/terminal-external/src/electron-node/terminal-external-service.ts

colin-grant-work · 2021-03-18T16:11:59Z

packages/terminal-external/src/electron-node/terminal-external-service.ts

+                    } else if (process.env.DESKTOP_SESSION === 'kde-plasma') {
+                        r('konsole');
+                    } else if (process.env.COLORTERM) {
+                        r(process.env.COLORTERM);


This seems to fail for me (on RedHat Linux) at this branch. It seems that the COLORTERM in my environment is truecolor, but when that's used to spawn a terminal, the openTerminal request fails.

root ERROR Request openTerminal failed with error: spawn truecolor ENOENT Error: spawn truecolor ENOENT at Process.ChildProcess._handle.onexit (internal/child_process.js:264:19) at onErrorNT (internal/child_process.js:456:16) at processTicksAndRejections (internal/process/task_queues.js:81:21)

VSCode suffers the same failure, though, so perhaps we shouldn't expect to do better, and once I set the preference to the correct path, I do get a new terminal as expected.

https://www.gnu.org/software/gettext/manual/html_node/The-TERM-variable.html

Not sure what the actual semantic of TERM and COLORTERM is, but IIUC one can use them to tell programs like xterm to configure some kind of feature support, or in the case of COLORTERM it would be set by the terminal application itself for processes spawned by it to detect features like truecolor support. So trying to use it to get an executable sounds weird, given that?

Being as bad as VS Code is acceptable, but if someone is able to tell for sure that this is bogus then we can fix that logic.

vince-fugnitto · 2021-03-18T16:16:27Z

I do wonder why the name terminal-external rather than external-terminal.

@colin-grant-work I believe it was mainly to group terminal functionality together on the filesystem:

I agree however that external-terminal reads better.

colin-grant-work · 2021-03-18T22:19:48Z

On a side note, I ❤️ the desktop background in the screencasts in the description.

DucNgn · 2021-03-22T20:00:37Z

@colin-grant-work

Thank you for such a detailed review, super appreciate it. I updated the PR to address your comments 👍

Side note: my razer desktop background 😄

vince-fugnitto

The updated changes look good to me, and are well described.
We can always update the extension in the future to add additional features, and/or refactorings if necessary.

The changes also work well, I verified on macOS and Linux 👍

packages/terminal-external/src/electron-browser/terminal-external-contribution.ts

packages/external-terminal/src/electron-node/linux-external-terminal-service.ts

packages/external-terminal/src/common/external-terminal.ts

packages/external-terminal/src/electron-browser/external-terminal-preference.ts

packages/external-terminal/src/electron-browser/external-terminal-contribution.ts

packages/external-terminal/src/electron-node/linux-external-terminal-service.ts

The following commit adds a new extension `@theia/terminal-external`. The extension adds support for spawning external terminals from Electron applications. + Supports opening native terminals by calling `Open New External Terminal` from the command palette. + The commit contributes the following preferences: - `terminal.external.windowsExec` - `terminal.external.osxExec` - `terminal.external.linuxExec` to allow users to customize the desired terminal application to run on their current OS. Co-authored-by: Paul <paul.marechal@ericsson.com> Co-authored-by: vince-fugnitto <vincent.fugnitto@ericsson.com> Signed-off-by: Duc Nguyen <duc.a.nguyen@ericsson.com>

DucNgn · 2021-03-23T13:24:40Z

@kittaakos Thanks for the review! Just wanted to clarify that this extension targets Electron applications only.

Other than that, I updated the PR to address your comments.

kittaakos · 2021-03-23T14:06:11Z

Just wanted to clarify that this extension targets Electron applications only.

👍 Can we enforce it? I am curious if this is something Theia can do. What happens when a downstream adds the new @theia/external-terminal extension to the browser app? Thanks!

kittaakos

Very nice! Thank you for the new feature

DucNgn · 2021-03-23T14:28:14Z

What happens when a downstream adds the new @theia/external-terminal extension to the browser app?

I believe adding the extension to a browser app won't break anything, it simply will just be a no-op.

kittaakos · 2021-03-23T14:36:11Z

I believe adding the extension to a browser app won't break anything, it simply will just be a no-op.

Of course, you're right. Thank you!

theia/packages/external-terminal/package.json

Lines 14 to 17 in db38e00

    
           { 
        
             "backendElectron": "lib/electron-node/external-terminal-backend-module", 
        
             "frontendElectron": "lib/electron-browser/external-terminal-frontend-module" 
        
           }

vince-fugnitto · 2021-03-23T16:13:48Z

I'll merge the pull-request tomorrow so it can be added to the upcoming release.
Since the new extension is pretty self-contained (no other extension depends on it) there isn't a potential risk of breaking anyone.

Thank you for the work @dukengn 👍

DucNgn force-pushed the dn/external-terminal branch from ff65621 to 5d61df6 Compare March 11, 2021 16:56

vince-fugnitto added electron issues related to the electron target terminal issues related to the terminal CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) labels Mar 11, 2021

paul-marechal reviewed Mar 11, 2021

View reviewed changes

packages/terminal-external/src/electron-browser/terminal-external-preference.ts Outdated Show resolved Hide resolved

paul-marechal reviewed Mar 17, 2021

View reviewed changes

packages/terminal-external/src/common/terminal-external.ts Outdated Show resolved Hide resolved

packages/terminal-external/src/electron-browser/terminal-external-preference.ts Outdated Show resolved Hide resolved

DucNgn force-pushed the dn/external-terminal branch from 5d61df6 to e6af6d0 Compare March 17, 2021 18:41

paul-marechal marked this pull request as ready for review March 17, 2021 19:44

DucNgn force-pushed the dn/external-terminal branch from e6af6d0 to 3ace0ae Compare March 17, 2021 19:58

vince-fugnitto requested review from vince-fugnitto, kittaakos and colin-grant-work March 18, 2021 13:14

colin-grant-work reviewed Mar 18, 2021

View reviewed changes

DucNgn force-pushed the dn/external-terminal branch from 3ace0ae to 4c4a7df Compare March 22, 2021 16:20

DucNgn changed the title ~~Initial contribution of @theia/terminal-external~~ Initial contribution of @theia/external-terminal Mar 22, 2021

DucNgn force-pushed the dn/external-terminal branch from 5f7d24a to a3aa8d9 Compare March 22, 2021 19:38

DucNgn requested review from paul-marechal and colin-grant-work March 22, 2021 20:00

vince-fugnitto approved these changes Mar 22, 2021

View reviewed changes

kittaakos reviewed Mar 23, 2021

View reviewed changes

DucNgn force-pushed the dn/external-terminal branch from a3aa8d9 to 8f08be0 Compare March 23, 2021 13:18

DucNgn force-pushed the dn/external-terminal branch from 8f08be0 to db38e00 Compare March 23, 2021 13:19

kittaakos approved these changes Mar 23, 2021

View reviewed changes

colin-grant-work approved these changes Mar 23, 2021

View reviewed changes

paul-marechal merged commit eb83e0e into eclipse-theia:master Mar 24, 2021

vince-fugnitto deleted the dn/external-terminal branch March 24, 2021 17:12

vince-fugnitto added this to the 1.12.0 milestone Mar 25, 2021

marcdumais-work mentioned this pull request Mar 29, 2021

Bump dependencies to v1.12.1, add new Theia extensions eclipse-theia/theia-ide#60

Merged

1 task

vince-fugnitto mentioned this pull request Sep 3, 2021

Support vscode's titleBarStyle #10044

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial contribution of @theia/external-terminal #9186

Initial contribution of @theia/external-terminal #9186

DucNgn commented Mar 11, 2021 •

edited

Loading

vince-fugnitto commented Mar 11, 2021 •

edited

Loading

vince-fugnitto commented Mar 16, 2021 •

edited

Loading

paul-marechal left a comment

paul-marechal commented Mar 17, 2021 •

edited

Loading

colin-grant-work left a comment •

edited

Loading

colin-grant-work Mar 18, 2021

paul-marechal Mar 22, 2021 •

edited

Loading

vince-fugnitto commented Mar 18, 2021 •

edited

Loading

colin-grant-work commented Mar 18, 2021

DucNgn commented Mar 22, 2021

vince-fugnitto left a comment

DucNgn commented Mar 23, 2021

kittaakos commented Mar 23, 2021

kittaakos left a comment

DucNgn commented Mar 23, 2021

kittaakos commented Mar 23, 2021

vince-fugnitto commented Mar 23, 2021

Initial contribution of @theia/external-terminal #9186

Initial contribution of @theia/external-terminal #9186

Conversation

DucNgn commented Mar 11, 2021 • edited Loading

What it does

Checklist for merging:

Command:

Preferences:

How to test:

No workspaces opened and no current active editor -> Spawn the terminal at the user home directory.

No workspaces opened but has an active editor -> Spawn the terminal at the parent folder of the active editor file.

Only one workspace opened -> Spawn the terminal at the root of the current workspace.

Multi-root workspace opened -> Displays a quick pick to let users choose which workspace to spawn the terminal.

Review checklist

Reminder for reviewers

vince-fugnitto commented Mar 11, 2021 • edited Loading

vince-fugnitto commented Mar 16, 2021 • edited Loading

paul-marechal left a comment

Choose a reason for hiding this comment

paul-marechal commented Mar 17, 2021 • edited Loading

colin-grant-work left a comment • edited Loading

Choose a reason for hiding this comment

colin-grant-work Mar 18, 2021

Choose a reason for hiding this comment

paul-marechal Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

vince-fugnitto commented Mar 18, 2021 • edited Loading

colin-grant-work commented Mar 18, 2021

DucNgn commented Mar 22, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

DucNgn commented Mar 23, 2021

kittaakos commented Mar 23, 2021

kittaakos left a comment

Choose a reason for hiding this comment

DucNgn commented Mar 23, 2021

kittaakos commented Mar 23, 2021

vince-fugnitto commented Mar 23, 2021

DucNgn commented Mar 11, 2021 •

edited

Loading

vince-fugnitto commented Mar 11, 2021 •

edited

Loading

vince-fugnitto commented Mar 16, 2021 •

edited

Loading

paul-marechal commented Mar 17, 2021 •

edited

Loading

colin-grant-work left a comment •

edited

Loading

paul-marechal Mar 22, 2021 •

edited

Loading

vince-fugnitto commented Mar 18, 2021 •

edited

Loading