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

[vscode] Add iconPath and stub color optional properties to TerminalOptions and ExtensionTerminalOptions #12060

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

rschnekenbu
Copy link
Contributor

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 #11504

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 extension does 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. Terminal package 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 once this PR is merged.

Review checklist

Reminder for reviewers

@vince-fugnitto
Copy link
Member

@rschnekenbu the APIs added are already optional and shouldn't affect the extension from loading or starting. Given that, why should we stub and not just implement the API properly when ready?

@vince-fugnitto vince-fugnitto added terminal issues related to the terminal vscode issues related to VSCode compatibility labels Jan 11, 2023
@rschnekenbu
Copy link
Contributor Author

@vince-fugnitto, I think this PR still makes sense for 2 points: the support for the icon is already be partially availlable for the incoming release and the API is clearly marked as stubbed, not simply ignored. Second point would mean we could step up the vscode API compatibility version with some remaining work clearly tracked.
Thanks in any case to have commented this PR!

@vince-fugnitto
Copy link
Member

@rschnekenbu If we want to stub I'm mainly wondering if we should instead mark both iconPath and color as stubbed given they are implemented partially or not at alll. Currently we have (compatibility report with this branch):

image

image

It is misleading to mark iconPath as fully compatible, perhaps partial or stubbed is more representative. The color property for ExtensionTerminalOptions should be marked as stubbed as well.

in TerminalOptions and ExtensionTerminalOptions
Contributed on behalf of STMicroelectronics

Fixes eclipse-theia#11504
@rschnekenbu
Copy link
Contributor Author

@vince-fugnitto, indeed, that can be misleading for iconPath. I updated the contribution removing the URIs on iconPath.
I also added 'stubbed' marker to ExtensionTerminalOptions.color.

Here is the report on TerminalOptions and ExtensionTerminalOptions now:
TerminalOptions
ExtensionTerminalOptions

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I think it's better now that we represent the actual support, and we can track compatibility.

@JonasHelming JonasHelming merged commit 315f2e0 into eclipse-theia:master Jan 12, 2023
@paul-marechal paul-marechal added this to the 1.34.0 milestone Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

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