Add workbench.fontAliasing configuration option #30628
Conversation
Add simple boolean configuration option that controls how fonts are being rendered in workbench. Enabling this option in configuration results with more native feel on macOS.
|
@priithaamer, It will cover your contributions to all Microsoft-managed open source projects. |
|
@priithaamer, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
| @@ -175,6 +175,11 @@ let workbenchProperties: { [path: string]: IJSONSchema; } = { | |||
| 'type': 'boolean', | |||
| 'description': nls.localize('closeOnFileDelete', "Controls if editors showing a file should close automatically when the file is deleted or renamed by some other process. Disabling this will keep the editor open as dirty on such an event. Note that deleting from within the application will always close the editor and that dirty files will never close to preserve your data."), | |||
| 'default': true | |||
| }, | |||
| 'workbench.fontAliasing': { | |||
There was a problem hiding this comment.
Since this setting only seems to have an impact on macOS, you should check if your are on macOS and only contribute this setting on macOS (platform.isMacintosh).
There was a problem hiding this comment.
As i am just learning the code base -- are plaform dependent configuration options common? This has one side effect when using settings-sync to syncronize configuration between mac and windows machines, it'll mark "unknown" configuration properties as invalid on another platform.
There was a problem hiding this comment.
@priithaamer I am still trying to understand if this CSS property does anything on other OS. When I tried on Windows and Linux (through VM) I could not see a visual difference.
| @@ -175,6 +175,11 @@ let workbenchProperties: { [path: string]: IJSONSchema; } = { | |||
| 'type': 'boolean', | |||
| 'description': nls.localize('closeOnFileDelete', "Controls if editors showing a file should close automatically when the file is deleted or renamed by some other process. Disabling this will keep the editor open as dirty on such an event. Note that deleting from within the application will always close the editor and that dirty files will never close to preserve your data."), | |||
| 'default': true | |||
| }, | |||
| 'workbench.fontAliasing': { | |||
| 'type': 'boolean', | |||
There was a problem hiding this comment.
Would it not make more sense to expose three options as enumeration here? none, subpixel-antialiased and antialiased? I find it a bit weird to say workbench.fontAliasing: true because this implies that by default there is no font smoothing happening, which is not true. The default in Chrome seems to be subpixel-antialiased
There was a problem hiding this comment.
Thank you for the review! That makes sense. However, I'd hide this implementation detail (direct mapping to CSS property values) from the configuration. So it is just a matter of naming the values. I'll try to come up with the names!
| @@ -910,6 +916,11 @@ export class Workbench implements IPartService { | |||
| this.workbenchLayout.layout(); | |||
| } | |||
|
|
|||
| private setFontAliasing(enabled: boolean) { | |||
| this.fontAliasingEnabled = enabled; | |||
| this.workbench.style('-webkit-font-smoothing', enabled ? 'antialiased' : ''); | |||
There was a problem hiding this comment.
I think it would be better to apply this on the <body> element to make sure that really everything is covered. E.g. there are things that can draw outside of the workbench (the tweet a smile feedback dialog for example).
There was a problem hiding this comment.
What would be the correct approach here -- should it access body element from the workbench class by going up the dom tree or should i move the logic entirely to another class? As said above, i'm just getting familiar with the code -- suggestions are welcome.
There was a problem hiding this comment.
@priithaamer I think it is OK to do document.body to access it.
Support both additional "antialiased" and "none" methods on font antialiasing. Leaving subpixel-antialiased as default See microsoft#2577
Instead of using only workbench element, apply font aliasing setting globally on entire window. See microsoft#2577
| @@ -202,6 +204,7 @@ export class Workbench implements IPartService { | |||
| private editorsVisibleContext: IContextKey<boolean>; | |||
| private inZenMode: IContextKey<boolean>; | |||
| private hasFilesToCreateOpenOrDiff: boolean; | |||
| private fontAliasing: string; | |||
There was a problem hiding this comment.
You can make this type: private fontAliasing: 'default' | 'antialiased' | 'none'
|
LGTM, thanks 👍 |
Adds new
workbench.fontAliasingconfiguration option. This sets-webkit-font-smoothing: antialiasedon entire workbench in order to improve font rendering on retina screens. See the discussion in #2577