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

terminal: fix terminal theming #7280

Closed
vince-fugnitto opened this issue Mar 5, 2020 · 5 comments · Fixed by #7325
Closed

terminal: fix terminal theming #7280

vince-fugnitto opened this issue Mar 5, 2020 · 5 comments · Fixed by #7325
Labels
bug bugs found in the application help wanted issues meant to be picked up, require help terminal issues related to the terminal theming issues related to theming

Comments

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Mar 5, 2020

Description

terminal theming from VS Code theming extensions does not work properly.
For example, the vscode-builtin-theme-tomorrow-night-blue theme contributes colors to the terminal:

"terminal.ansiBlack": "#111111",
"terminal.ansiRed": "#ff9da4",
"terminal.ansiGreen": "#d1f1a9",
"terminal.ansiYellow": "#ffeead",
"terminal.ansiBlue": "#bbdaff",
"terminal.ansiMagenta": "#ebbbff",
"terminal.ansiCyan": "#99ffff",
"terminal.ansiWhite": "#cccccc",
"terminal.ansiBrightBlack": "#333333",
"terminal.ansiBrightRed": "#ff7882",
"terminal.ansiBrightGreen": "#b8f171",
"terminal.ansiBrightYellow": "#ffe580",
"terminal.ansiBrightBlue": "#80baff",
"terminal.ansiBrightMagenta": "#d778ff",
"terminal.ansiBrightCyan": "#78ffff",
"terminal.ansiBrightWhite": "#ffffff"

Theia

image

VS Code

image

@vince-fugnitto vince-fugnitto added help wanted issues meant to be picked up, require help terminal issues related to the terminal theming issues related to theming labels Mar 5, 2020
@vince-fugnitto
Copy link
Member Author

It looks like we have logic already to handle the foreground colors but they do not seem to be applied properly:

export const terminalAnsiColorMap: { [key: string]: { index: number, defaults: ColorDefaults } } = {

@akosyakov
Copy link
Member

It should be already supported:

export const terminalAnsiColorMap: { [key: string]: { index: number, defaults: ColorDefaults } } = {

Maybe some regression.

@vince-fugnitto
Copy link
Member Author

Maybe some regression.

I thought it might have been caused by the xterm update (#7121) but it was not the case.
I verified with the commit c5569a4

@vince-fugnitto vince-fugnitto changed the title terminal: add support for terminal theming terminal: fix terminal theming Mar 5, 2020
@vince-fugnitto vince-fugnitto added the bug bugs found in the application label Mar 5, 2020
@ghost
Copy link

ghost commented Mar 11, 2020

One problem that I noticed is that, when we do id.substring(13) here , we get Black instead of black expected by xterm.js. I am doing that conversion in my local to make my hard-coded theme (from color map above) to load up correctly. Does that make sense? But we need to see if that is applicable to loading theme from VS Code plugins and user settings. I don't see any code related to either feature in the code. VS Code takes a slightly different approach to some of these problems: terminaColorRegistry.ts and terminalInstance.ts.

@ghost
Copy link

ghost commented Mar 12, 2020

Here is a simple correction that can be applied here to make it load from the theme correctly:

const colorKey = id.substring(13);
const colorName = colorKey.charAt(0).toLowerCase() + colorKey.substring(1);

vince-fugnitto added a commit that referenced this issue Mar 12, 2020
Fixes #7280

This commit fixes the issue where VS Code terminal theming was not
applied correctly. Since the VS Code theming options for terminals
and the `xterm` ITheme differed (lower-case), the terminal themes
were not applied properly.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
vince-fugnitto added a commit that referenced this issue Mar 12, 2020
Fixes #7280

This commit fixes the issue where VS Code terminal theming was not
applied correctly. Since the VS Code theming options for terminals
and the `xterm` ITheme differed (lower-case first letter), the terminal themes
were not applied properly.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
vince-fugnitto added a commit that referenced this issue Mar 12, 2020
Fixes #7280

This commit fixes the issue where VS Code terminal theming was not
applied correctly. Since the VS Code theming options for terminals
and the `xterm` ITheme differed (lower-case first letter), the terminal themes
were not applied properly.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application help wanted issues meant to be picked up, require help terminal issues related to the terminal theming issues related to theming
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants