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

Add iconPath and stub color optional properties to TerminalOptions and ExtensionTerminalOptions. #62

Closed
wants to merge 2 commits into from

Conversation

rschnekenbu
Copy link

@rschnekenbu rschnekenbu commented Jan 11, 2023

What it does

Add iconPath optional property to TerminalOptions and ExtensionTerminalOptions
Stub color optional property to TerminalOptions and ExtensionTerminalOptions
This allows creation of terminals from extension with different icons.

Fixes eclipse-theia#11504

Only commit da5f0ce is relevant in this review, the other one from lucas is only a commit missing on our fork.

Contributed on behalf of STMicroelectronics

How to test

  1. Install extension terminal-creation-options-0.0.13.zip. This extension provides several actions to open a new terminal with a different icon in the title and several color options (not supported yet)
  2. Trigger in the command Palette different commands:
  • 'Create terminal with green Theme Icon'. a new terminal with a file icon shall open.
  • 'Create terminal with red URI Icon'. A terminal with default terminal icon shall open as the URIs iconPath is not yet supported.
  • 'Create new terminal' should create a terminal with standard terminal icon.

terminal-colorandicon

Known limitations

  1. Color is not supported but stubbed, so the extension is still supported. The support is strange in vscode, as having a specific color changes also the icon to a kind of cube (at least when I am testing). The main technical reason is that underlying representation Title, coming from phosphorjs library, does not directly support styling. The phosphorjs library is deprecated now, so this may be hard to have a proper fix.
  2. Only ThemeIcon is supported currently in iconPath. URI(s) based icons are not supported, but still do not fail. Same reason for the color management applies here, the limitation comes from Title only accepting iconClass string. I have seen some similar work on webview title, but this includes some dependencies to classes in plugin-ext package (PluginSharedStyle, and some other elements). In Terminal-Widget-impl, we are in terminal package, which can not depend on plugin-ext as it would introduce circular dependencies. So there maybe some other way to implement, but with either some code duplication or a larger refactoring required. This will be part of a following issue.

Review checklist

Reminder for reviewers

lucas-koehler and others added 2 commits January 11, 2023 08:56
…ptions (eclipse-theia#12055)

Fix eclipse-theia#11777

* Extend `TerminalOptions` and `ExtensionTerminalOptions` with `isTransient` property
  according to the VSCode API
* Add terminal preference `enablePersistentSessions` with default value `true`
* Adhere to pref. `enablePersistentSessions` and option `isTransient` in the terminal widget

Contributed on behalf of STMicroelectronics.

Signed-off-by: Lucas Koehler <[email protected]>
in TerminalOptions and ExtensionTerminalOptions
Contributed on behalf of STMicroelectronics

Fixes eclipse-theia#11504
@rschnekenbu rschnekenbu requested a review from sgraband January 11, 2023 15:02
Copy link

@sgraband sgraband left a comment

Choose a reason for hiding this comment

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

Thank you @rschnekenbu! Looks good and works great!

Should we already create a follow up/discussion for the larger refactoring? Or will you open this when the PR is merged? Anyway, i see no reason to not open the real PR.

@rschnekenbu
Copy link
Author

Thanks for this fast review, @sgraband! I opened a new PR on theia side: eclipse-theia#12060. I will open a new issue that finish the implementation once the theia PR is accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vscode] Support optional properties iconPath and color in TerminalOptions and ExtensionTerminalOptions
3 participants