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

Compatibility third party themes #8536

Closed
Leon0402 opened this issue Sep 21, 2020 · 12 comments · Fixed by #8964
Closed

Compatibility third party themes #8536

Leon0402 opened this issue Sep 21, 2020 · 12 comments · Fixed by #8964
Labels
help wanted issues meant to be picked up, require help theming issues related to theming vscode issues related to VSCode compatibility

Comments

@Leon0402
Copy link

Bug Description:

It seems like there is a change in theia, causing some themes to break? The editor is not colored correctly as you can see from the screenshots:

image

image

As you can see it will use for editor whatever theme has been selected before.

Steps to Reproduce:

  1. Install the Atom One Dark Extension (and possibly others?)
  2. Select it from the theme

I also checked that the version of the theme is the same as in VsCode (2.2.2), so I assume it's not a bug in the theme itself.

I tried it with another theme (Material) and couldn't reproduce it though, might be trying it out with a couple more themes.

Additional Information

  • Operating System: Arch Linux
  • Theia Version: 1.5.0
@vince-fugnitto vince-fugnitto added help wanted issues meant to be picked up, require help theming issues related to theming vscode issues related to VSCode compatibility labels Sep 21, 2020
@Leon0402
Copy link
Author

I found the following error. The theme uses css colors by name sometimes (afaik) instead of hexcodes. Could theia support them as well?

root ERROR Color 'white' is NOT normalized, it must have 6 positions.
root ERROR Error: Illegal value for token color: #inheri
    at e.getId (file:///usr/lib/theia-electron/lib/vs/editor/editor.main.js:614:741)
    at r (file:///usr/lib/theia-electron/lib/vs/editor/editor.main.js:614:415)
    at Function.e.createFromParsedTokenTheme (file:///usr/lib/theia-electron/lib/vs/editor/editor.main.js:615:230)
    at Function.e.createFromRawTokenTheme (file:///usr/lib/theia-electron/lib/vs/editor/editor.main.js:615:144)
    at e.get [as tokenTheme] (file:///usr/lib/theia-electron/lib/vs/editor/editor.main.js:2334:409)
    at t.setTheme (file:///usr/lib/theia-electron/lib/vs/editor/editor.main.js:2336:829)
    at Object.Q [as setTheme] (file:///usr/lib/theia-electron/lib/vs/editor/editor.main.js:2529:896)
    at e.updateTheme (file:///usr/lib/theia-electron/lib/bundle.js:12:559136)
    at file:///usr/lib/theia-electron/lib/bundle.js:12:558473
    at file:///usr/lib/theia-electron/lib/bundle.js:1:65902

@danarad05
Copy link
Contributor

FYI: @amiramw @vince-fugnitto @Leon0402 > #8964

@vince-fugnitto
Copy link
Member

I found the following error. The theme uses css colors by name sometimes (afaik) instead of hexcodes. Could theia support them as well?

Just wanted to mention that this is a backwards compatibility support, it is no longer possible to support colors by name.

image

@danarad05
Copy link
Contributor

danarad05 commented Jan 19, 2021

I found the following error. The theme uses css colors by name sometimes (afaik) instead of hexcodes. Could theia support them as well?

Just wanted to mention that this is a backwards compatibility support, it is no longer possible to support colors by name.

Thanks for your comment @vince-fugnitto, before this PR, color names where replaced with 'undefined' if color name was less than 6 chars. if it were 6 chars or more, it would be truncated to 6 chars and eventually would error out in ColorMap.prototype.getId function for not conforming to it's regexp (/^#?([0-9A-Fa-f]{6})([0-9A-Fa-f]{2})?$/).

Now, with this PR, color names are truncated to 6 chars and if they are not conforming with above regexp than they would also be replaced with 'undefined' - and thus preventing exception in getId function.

@vince-fugnitto
Copy link
Member

Now, with this PR, color names are truncated to 6 chars and if they are not conforming with above regexp than they would also be replaced with 'undefined' - and thus preventing exception in getId function.

@danarad05 do you understand why we do the truncation to 6 characters? By color names do you mean both hex colors and colors by string? I'm wondering if for instance if the valid hex value #000 is supported.

@danarad05
Copy link
Contributor

@vince-fugnitto
I do not know why the truncation to 6 chars is done - if there was an issue or reason for that - because of that I didn't want to alter current logic but implement the smallest of impact on current code.

On VS Code's side - getId function supports up to 8 chars w/o "#" in beginning (/^#?([0-9A-Fa-f]{6})([0-9A-Fa-f]{2})?$/) - it doesn't support color names and not #000 as they do not conform the regexp - so we can not support it on Theia until it does - IMO.

@vince-fugnitto
Copy link
Member

@vince-fugnitto
I do not know why the truncation to 6 chars is done - if there was an issue or reason for that - because of that I didn't want to alter current logic but implement the smallest of impact on current code.

No problem, just wanted to know if you understand why it was done this way. I believe the extra 2 characters (to 8) is to control the alpha transparency of the color.

it doesn't support color names and not #000 as they do not conform the regexp

When creating a color theme, I do see warnings about using color strings, but #000 is valid for me (as a shorthand notation).
Such short hand notations are also tested in their test-suite (parseHex method):

@danarad05
Copy link
Contributor

@vince-fugnitto

When creating a color theme, I do see warnings about using color strings, but #000 is valid for me (as a shorthand notation).
Such short hand notations are also tested in their test-suite (parseHex method):

You are correct - #000 is a valid css shorthand but on VSCode side (specifically the ColorMap.getId) will not pass the regex validation (and throw an exception) so maybe that is why on Theia side we replace it with undefined.

@danarad05
Copy link
Contributor

@vince-fugnitto @Leon0402 #8964 awaiting review
cc: @amiramw

@vince-fugnitto
Copy link
Member

The pull-request #8964 does not successfully address the issue, the following is still broken: #8890

bundle.js:135275 root ERROR TypeError: Cannot read property 'length' of null
    at Object.parseHex (file:///home/evinfug/workspaces/theia/examples/electron/lib/vs/editor/editor.main.js:1305:38)
    at Function.Color.fromHex (file:///home/evinfug/workspaces/theia/examples/electron/lib/vs/editor/editor.main.js:1072:37)
    at StandaloneTheme.getColors (file:///home/evinfug/workspaces/theia/examples/electron/lib/vs/editor/editor.main.js:127144:50)
    at StandaloneTheme.getColor (file:///home/evinfug/workspaces/theia/examples/electron/lib/vs/editor/editor.main.js:127159:30)
    at MonacoColorRegistry.push.../../packages/monaco/lib/browser/monaco-color-registry.js.MonacoColorRegistry.getCurrentColor (file:///home/evinfug/workspaces/theia/examples/electron/lib/66.bundle.js:216:56)
    at MonacoColorRegistry.../../packages/core/lib/browser/color-registry.js.ColorRegistry.getCurrentCssVariable (file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:102998:26)
    at _loop_1 (file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:102815:46)
    at ColorApplicationContribution.../../packages/core/lib/browser/color-application-contribution.js.ColorApplicationContribution.update (file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:102826:21)
    at file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:102800:79
    at file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:134612:33

Steps to reproduce:

  1. install the dracula at night theme
  2. change the theme to dracula at night
  3. the theme is broken, and there are many errors in the console

The bug makes the application almost unusable.

@danarad05
Copy link
Contributor

danarad05 commented Feb 21, 2021

The pull-request #8964 does not successfully address the issue, the following is still broken: #8890

bundle.js:135275 root ERROR TypeError: Cannot read property 'length' of null
    at Object.parseHex (file:///home/evinfug/workspaces/theia/examples/electron/lib/vs/editor/editor.main.js:1305:38)
    at Function.Color.fromHex (file:///home/evinfug/workspaces/theia/examples/electron/lib/vs/editor/editor.main.js:1072:37)
    at StandaloneTheme.getColors (file:///home/evinfug/workspaces/theia/examples/electron/lib/vs/editor/editor.main.js:127144:50)
    at StandaloneTheme.getColor (file:///home/evinfug/workspaces/theia/examples/electron/lib/vs/editor/editor.main.js:127159:30)
    at MonacoColorRegistry.push.../../packages/monaco/lib/browser/monaco-color-registry.js.MonacoColorRegistry.getCurrentColor (file:///home/evinfug/workspaces/theia/examples/electron/lib/66.bundle.js:216:56)
    at MonacoColorRegistry.../../packages/core/lib/browser/color-registry.js.ColorRegistry.getCurrentCssVariable (file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:102998:26)
    at _loop_1 (file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:102815:46)
    at ColorApplicationContribution.../../packages/core/lib/browser/color-application-contribution.js.ColorApplicationContribution.update (file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:102826:21)
    at file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:102800:79
    at file:///home/evinfug/workspaces/theia/examples/electron/lib/bundle.js:134612:33

Steps to reproduce:

  1. install the dracula at night theme
  2. change the theme to dracula at night
  3. the theme is broken, and there are many errors in the console

The bug makes the application almost unusable.

@vince-fugnitto

Opened PR #9097 as it originates from different issue than #8536

@vince-fugnitto
Copy link
Member

Closed thanks to #9097.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted issues meant to be picked up, require help theming issues related to theming vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants