Add setting for fontSize and lineHeight to the integrated terminal #6998

kisstkondoros · 2016-05-28T19:59:57Z

resolves #6728

mention-bot · 2016-05-28T19:59:57Z

By analyzing the blame information on this pull request, we identified @Tyriar to be a potential reviewer

msftclas · 2016-05-28T20:00:00Z

Hi @kisstkondoros, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

Tyriar · 2016-05-28T20:51:32Z

src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts

+			'type': 'string',
+			'default': ''
+		},
+		'terminal.integrated.charWidth': {


I think #6456 should be figured out before the font size and line height is added to prevent addition the charWidth/charHeight settings.

I also thought about that, but I have no clean solution in mind...
Perhaps measuring a character like X or M with canvas api... smth. like https://msdn.microsoft.com/en-us/library/ff975421(v=vs.85).aspx

Yeah, that's why I haven't tackled it yet 😉

Something along the lines of that is what I was thinking, I think you might be able to do it by creating a div and measuring that.

kisstkondoros · 2016-05-29T18:52:18Z

@Tyriar The mentioned measurement method seems to work quite well, of course after 3fde039 it will need some adjustments.

Tyriar · 2016-05-29T18:57:45Z

@kisstkondoros Awesome! 👍

Tyriar · 2016-05-31T16:15:58Z

@kisstkondoros I merged the PR into master as the 1.2 release was branched off.

Tyriar · 2016-05-31T18:28:08Z

I moved the configuration code into terminalConfigHelper.ts to make it more testable, 775c643

Maybe changing getFontFamily into getFont which returns an ITerminalFont would be best since all 3 values are used together:

export interface ITerminalFont {
    fontFamily: string;
    fontSize: string;
    lineHeight: string;
}

kisstkondoros · 2016-06-02T06:23:13Z

@Tyriar Please have a look, I hope that was the idea! 😉

Tyriar · 2016-06-02T06:45:23Z

src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts

@@ -41,6 +41,14 @@ configurationRegistry.registerConfiguration({
 		'terminal.integrated.fontFamily': {
 			'description': nls.localize('terminal.integrated.fontFamily', "The font family used by the terminal (CSS font-family format), this defaults to editor.fontFamily's value."),
 			'type': 'string'
+		},
+		'terminal.integrated.fontSize': {
+			'description': nls.localize('terminal.integrated.fontSize', "The font size used by the terminal."),


Add: " (in pixels)"

Tyriar · 2016-06-02T07:02:33Z

@kisstkondoros really nice work! Made a bunch of comments, mainly just names and shuffling things around.

resolves microsoft#6728

kisstkondoros · 2016-06-02T20:12:51Z

@Tyriar based on Your comments I've did a refactor, some stuff has been simplified a lot.

Things to note:

Updating the font doesn't triggers layout, I had to add it manually
BoundingClientRect can be 0x0 if the container is not visible
-- if charWidth and / or charHeight is 0 the terminal resize would crash VSCode, so I've added a guard into the layout method

Tyriar · 2016-06-02T20:19:31Z

src/vs/workbench/parts/terminal/electron-browser/terminalConfigHelper.ts

+			this.charMeasureElement = new Builder(this.parentDomElement, true).div().build().getHTMLElement();
+		}
+		let style = this.charMeasureElement.style;
+		style.display = 'inline';


Will this flicker the element on the screen? Should it be positioned absolutely off screen?

I'll check, but this triggers a synchronous layout and then immediatelly removes it I think it is not visible.

Tyriar · 2016-06-02T20:22:31Z

Really nice! Just a couple of little things and good to go, I'm also cloning your branch to check it out.

Tyriar · 2016-06-02T20:33:05Z

src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts

@@ -41,6 +41,14 @@ configurationRegistry.registerConfiguration({
 		'terminal.integrated.fontFamily': {
 			'description': nls.localize('terminal.integrated.fontFamily', "The font family used by the terminal (CSS font-family format), this defaults to editor.fontFamily's value."),
 			'type': 'string'
+		},
+		'terminal.integrated.fontSize': {


Font size should have a default here, otherwise auto-complete does this.

kisstkondoros · 2016-06-02T20:37:18Z

I'll continue with it tomorrow.
Thanks for the feedback! 👍

Tyriar · 2016-06-02T20:37:43Z

src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts

@@ -41,6 +41,14 @@ configurationRegistry.registerConfiguration({
 		'terminal.integrated.fontFamily': {
 			'description': nls.localize('terminal.integrated.fontFamily', "The font family used by the terminal (CSS font-family format), this defaults to editor.fontFamily's value."),
 			'type': 'string'
+		},
+		'terminal.integrated.fontSize': {
+			'description': nls.localize('terminal.integrated.fontSize', "The font size used by the terminal (in pixels), this defaults to editor.fontSize's value."),


Sorry but let's go for consistency with the editor settings for this in commonEditorConfig.ts:

"Controls the font family of the terminal." "Controls the font size of the terminal." "Controls the line height of the terminal."

Tyriar · 2016-06-03T02:03:27Z

Thanks a lot @kisstkondoros, this is awesome! I'm going to refactor terminalPanel.ts for #6458 and didn't want to introduce a bunch of conflicts so I'll make the touch ups mentioned. It should land in the June release! 🎆

kisstkondoros · 2016-06-03T04:27:11Z

Awesome, thanks! 🎉

msftclas added the cla-already-signed label May 28, 2016

Tyriar reviewed May 28, 2016
View reviewed changes

Tyriar added the terminal General terminal issues that don't fall under another label label May 28, 2016

Tyriar self-assigned this May 28, 2016

kisstkondoros force-pushed the terminalfont branch from 58a3bc3 to 3c1103d Compare May 29, 2016 18:49

kisstkondoros force-pushed the terminalfont branch from 3c1103d to 1f3d189 Compare May 29, 2016 19:06

kisstkondoros force-pushed the terminalfont branch 2 times, most recently from a7be30e to 62b0bb1 Compare June 2, 2016 05:13

Tyriar reviewed Jun 2, 2016
View reviewed changes

Tyriar added this to the June 2016 milestone Jun 2, 2016

Add setting for fontSize and lineHeight to the integrated terminal

46ab627

resolves microsoft#6728

kisstkondoros force-pushed the terminalfont branch from 62b0bb1 to 46ab627 Compare June 2, 2016 20:04

Tyriar reviewed Jun 2, 2016
View reviewed changes

Tyriar merged commit 6cbd485 into microsoft:master Jun 3, 2016

Tyriar mentioned this pull request Jun 3, 2016

Determine terminal character size in a font-agnostic way #6456

Closed

kisstkondoros deleted the terminalfont branch June 3, 2016 04:27

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setting for fontSize and lineHeight to the integrated terminal #6998

Add setting for fontSize and lineHeight to the integrated terminal #6998

kisstkondoros commented May 28, 2016

mention-bot commented May 28, 2016

msftclas commented May 28, 2016

Tyriar May 28, 2016 •

edited

Loading

kisstkondoros May 28, 2016

Tyriar May 28, 2016

kisstkondoros commented May 29, 2016

Tyriar commented May 29, 2016

Tyriar commented May 31, 2016

Tyriar commented May 31, 2016

kisstkondoros commented Jun 2, 2016

Tyriar Jun 2, 2016

Tyriar commented Jun 2, 2016

kisstkondoros commented Jun 2, 2016

Tyriar Jun 2, 2016

kisstkondoros Jun 2, 2016

Tyriar commented Jun 2, 2016

Tyriar Jun 2, 2016

kisstkondoros commented Jun 2, 2016

Tyriar Jun 2, 2016

Tyriar commented Jun 3, 2016

kisstkondoros commented Jun 3, 2016

Add setting for fontSize and lineHeight to the integrated terminal #6998

Add setting for fontSize and lineHeight to the integrated terminal #6998

Conversation

kisstkondoros commented May 28, 2016

mention-bot commented May 28, 2016

msftclas commented May 28, 2016

Tyriar May 28, 2016 • edited Loading

Choose a reason for hiding this comment

kisstkondoros May 28, 2016

Choose a reason for hiding this comment

Tyriar May 28, 2016

Choose a reason for hiding this comment

kisstkondoros commented May 29, 2016

Tyriar commented May 29, 2016

Tyriar commented May 31, 2016

Tyriar commented May 31, 2016

kisstkondoros commented Jun 2, 2016

Tyriar Jun 2, 2016

Choose a reason for hiding this comment

Tyriar commented Jun 2, 2016

kisstkondoros commented Jun 2, 2016

Tyriar Jun 2, 2016

Choose a reason for hiding this comment

kisstkondoros Jun 2, 2016

Choose a reason for hiding this comment

Tyriar commented Jun 2, 2016

Tyriar Jun 2, 2016

Choose a reason for hiding this comment

kisstkondoros commented Jun 2, 2016

Tyriar Jun 2, 2016

Choose a reason for hiding this comment

Tyriar commented Jun 3, 2016

kisstkondoros commented Jun 3, 2016

Tyriar May 28, 2016 •

edited

Loading