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: allow setting terminal color #28

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

iRoachie
Copy link

Addresses #10 (comment).

Note that color is not supported for split terminals (other than the first) as there's no changeColor API that accepts args similar to renameWithArg

@BetterToAutomateTheWorld

Hi,

Please merge it, this feature is much appreciated !

@cjatoba
Copy link

cjatoba commented Aug 23, 2022

Very good this feature, as I usually change the color currently I need to do it manually, it would help a lot to automate that way.

Copy link

@PumpedSardines PumpedSardines left a comment

Choose a reason for hiding this comment

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

If we're gonna implement this feature, shouldn't we support split terminals from the start? This seems like a weird implementation

Comment on lines +73 to +75
if (terminalWindow.splitTerminals[0]?.color) {
color = TerminalColor[terminalWindow.splitTerminals[0].color];
}

Choose a reason for hiding this comment

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

You need to check that terminalWindow.splitTerminals[0]?.color is an actual color and not something random the user has inputed. Otherwise color will be undefined.

Copy link
Author

Choose a reason for hiding this comment

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

In any case, it falls back to white

term = vscode.window.createTerminal({
name: name,
color: new vscode.ThemeColor(color),

Choose a reason for hiding this comment

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

I'm not sure what happens if you call vscode.ThemeColor constructor with undefined, but it's better to ensure that color never is white instead of undefined.

Comment on lines +1 to +10
export enum TerminalColor {
"black" = "terminal.ansiBlack",
"red" = "terminal.ansiRed",
"green" = "terminal.ansiGreen",
"yellow" = "terminal.ansiYellow",
"blue" = "terminal.ansiBlue",
"magenta" = "terminal.ansiMagenta",
"cyan" = "terminal.ansiCyan",
"white" = "terminal.ansiWhite",
}

Choose a reason for hiding this comment

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

Why put this in an enum if you're gonna use it like a map? Imo it's better to do a readonly object.

@@ -96,7 +96,7 @@
"@types/mocha": "^7.0.2",
"@types/node": "^13.11.0",
"@types/text-encoding": "^0.0.35",
"@types/vscode": "^1.46.0",
"@types/vscode": "^1.67.0",

Choose a reason for hiding this comment

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

Is it required to update the version? because this could possibly be breaking otherwise.

@mattveraldi mattveraldi mentioned this pull request Oct 7, 2022
@EthanSK
Copy link
Owner

EthanSK commented Oct 20, 2022

merged icons, pls fix conflicts

@iRoachie
Copy link
Author

@EthanSK conflicts fixed

@kdubb1337
Copy link

@iRoachie more conflicts popped up.

@EthanSK
Copy link
Owner

EthanSK commented Nov 5, 2022

I tried getting icons to work, couldn't. VScode API is very buggy, so will have to wait sadly...

@RutsuKun
Copy link

any update?

@laneschmidt
Copy link

I don't think it works? Tried adding "color": "red" to one of my terminals but still white (default color).

Also tried "terminal.ansiRed" to no avail.

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.

8 participants