Skip to content

Provide "terminal.integrated.enableBold" setting connected to #22422#22465

Merged
Tyriar merged 12 commits intomicrosoft:masterfrom
lifez:terminal-enable-bold
Mar 20, 2017
Merged

Provide "terminal.integrated.enableBold" setting connected to #22422#22465
Tyriar merged 12 commits intomicrosoft:masterfrom
lifez:terminal-enable-bold

Conversation

@lifez
Copy link
Contributor

@lifez lifez commented Mar 12, 2017 •
edited by Tyriar
Loading

Fixes #22422

@mention-bot
Copy link

mention-bot commented Mar 12, 2017

@lifez, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima and @Tyriar to be potential reviewers.

@msftclas
Copy link

msftclas commented Mar 12, 2017

@lifez,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link

msftclas commented Mar 12, 2017

@lifez, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@lifez lifez mentioned this pull request Mar 12, 2017
Closed
Tyriar
Tyriar requested changes Mar 12, 2017
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks @lifez, made some comments.

src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts Outdated
Copy link
Member

@Tyriar Tyriar Mar 12, 2017

Choose a reason for hiding this comment

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

This needs to be something like enableBold like how gnome-terminal does it, otherwise this setting will override the terminal's regular bold/normal formatting with what's in this setting.

The logic should then be added as an event listener on the IConfigurationService so it's picked up when the setting is changed, something like this:

set class name 'disable-bold' if `enableBold` setting is not set

I use disable-bold as the class name here because enableBold will be the standard.

Copy link
Contributor Author

@lifez lifez Mar 12, 2017

Choose a reason for hiding this comment

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

You mean set terminal.integrated.enableBold to false

Then add class disable-bold to provide font-weight: normal; right ?

Copy link
Member

@Tyriar Tyriar Mar 13, 2017

Choose a reason for hiding this comment

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

Yes

src/vs/workbench/parts/terminal/electron-browser/terminalConfigHelper.ts Outdated
Copy link
Member

@Tyriar Tyriar Mar 12, 2017

Choose a reason for hiding this comment

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

It's fine to leave out font weight from font measurement as the terminal assumes we're working with a monospace font where bold size is the same as non-bold size.

@Tyriar Tyriar added this to the March 2017 milestone Mar 12, 2017
@lifez
Copy link
Contributor Author

lifez commented Mar 12, 2017 •
edited
Loading

I have rethink about it.It would be great if we can set font-weight of terminal

The reason is when I use vscode on osx with bold font in terminal

the integrate terminal text is not bold

What do you think? @Tyriar

@Tyriar
Copy link
Member

Tyriar commented Mar 13, 2017

So you want all bold text? I wonder if you can set the font family to the be the bold variant? Ie. font-family: "Courier New Bold" or something?

@lifez
Copy link
Contributor Author

lifez commented Mar 13, 2017

Oh, ok I miss this point :)

@lifez
Copy link
Contributor Author

lifez commented Mar 17, 2017

Could you guide me a little about event listener on IConfigurationService I don't see some code about append css ?

@Tyriar
Copy link
Member

Tyriar commented Mar 17, 2017

@lifez sure, you will want to listen to the IConfigurationService within terminalPanel.ts and add the class disable-bold to this._parentDomElement. Listening to config updates can be seen here: https://github.com/Microsoft/vscode/blob/a6280ce6038a251a4deba2d7b33badcde6391b6f/src/vs/workbench/parts/terminal/electron-browser/terminalPanel.ts#L67

Then you will want to add the styles to terminal.css, font-weight: normal !important is probably fine.

@Tyriar
Copy link
Member

Tyriar commented Mar 17, 2017

@lifez I tried to resolve the conflicts using GitHub's web UI for the first time to help you out, hopefully I didn't break compilation or anything 😄

@lifez
Copy link
Contributor Author

lifez commented Mar 17, 2017

@Tyriar Thank you

Then i will do something like this DOM.toggleClass(this._parentDomElement, 'disable-bold') ?

@Tyriar
Copy link
Member

Tyriar commented Mar 17, 2017

@lifez yes, but prefer the explicit value instead of toggle:

DOM.toggleClass(this._parentDomElement, 'disable-bold', !isEnabled);

@Tyriar
Copy link
Member

Tyriar commented Mar 20, 2017

Thanks @lifez, I tweaked things a bit and this is good to go 👍

@Tyriar Tyriar merged commit 8dc5a60 into microsoft:master Mar 20, 2017
@lifez lifez deleted the terminal-enable-bold branch March 22, 2017 11:17
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Tyriar Tyriar Tyriar requested changes

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

March 2017

Development

Successfully merging this pull request may close these issues.

Provide "terminal.integrated.enableBold" setting

4 participants

@lifez @mention-bot @msftclas @Tyriar