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

OSC 4/10/11/12 color handling #3524

Merged
merged 27 commits into from
Dec 22, 2021
Merged

OSC 4/10/11/12 color handling #3524

merged 27 commits into from
Dec 22, 2021

Conversation

jerch
Copy link
Member

@jerch jerch commented Oct 22, 2021

Shall implement OSC10/11 color handling, and extend OSC 4 by the query notation.

Fixes #3487. Also closes #3549.

Beside OSC 4/10/11 there are more color commands, but I am not quite sure yet, how they are supposed to work (esp. OSC 5 with its "special colors" is a mystery to me). Not sure if the PR will deal with those as well in the end, they have no priority on my side. Most of OSC 12 -19 is def. out of scope for xterm.js.

TODO:

@jerch
Copy link
Member Author

jerch commented Oct 23, 2021

@Tyriar Figured that terminals properly implementing the OSC color commands support named colors, e.g. you can set there the background with:

$> echo -e '\x1b]11;ForestGreen\x07'

which would switch to some greenish background. Currently I hesitate to add support for those named colors in xterm.js, because the name definitions are really big for very little benefit.

X11 defines 753 color names in rgb.txt for 8-bit RGB (filesize 17kB). I was able to pack those definitions in a string of 10kB - thats the package size penalty if we want full X11 color name support as in other TEs.
133 of these colors map 1:1 to browser CSS colors (tested on Chrome), thus could be replaced by a getComputedStyle shim on the browser with some alias trickery. The remaining 475 color names form 373 genuine colors, that cannot be shimmed from browser side, thus are either unknown or we'd need an additional mapping for them. Such a mapping still would eat ~9kB (with aliases), just for having those color names defined.

I think we should not support named colors at all, as it has little benefit for 8 - 10kB more on the package. First the OSC color commands are a niche use case, second ppl can always use the RGB channel notation to get them. At least we should state this in the sequence docs. Thoughts? Do we care for those 10kB?

Edit: An interesting finger exercise might be to reduce the storage needs by finding a perfect hash function for those color names (should get us <4kB).

Edit2: Created a PHF version, which takes ~5kB for the data (see commit below).

@Tyriar
Copy link
Member

Tyriar commented Oct 25, 2021

I know 10kb is small in related to the current ~320kb of the package but it's already too big imo. At the same point in time we shouldn't be creating addons for everything as that makes consuming the package harder.

What about expecting an rgb.json to be served from the module directory or something. So we would only request it when a named color is encountered, then using the async handler infrastructure request the file from the directory. We may need an option to customize the directory, for example when using ASAR. Thoughts?

@jerch
Copy link
Member Author

jerch commented Oct 25, 2021

I know 10kb is small in related to the current ~320kb of the package but it's already too big imo.

Yes I second your concern regarding the overall package size. I am not sure by what changes the major shifts happened, thats what I see when asking CDNs for various versions:

  • v2.0 50kB
  • v3.0 120kB
  • v3.14 220kB
  • v4.14 330kB

Ofc big part of the deal are tech extensions, that are rather code expensive, like the whole canvas renderer, the reflow logic is a big block of code and also the parser/buffer rewrites created several code modules, that did not exist before. But is a 6 times bigger package justified by that functionality? I'd say yes, still 6 times is scary, esp. when considering the fact, that we are still lacking several cores features (like the PR here).
When dealing with packaging of the image-addon I kinda stumbled over several aspects, that are subpar with TS builds. I was able to shrink its package output from >110kB to ~50kB by doing some ES6 tree shaking in node-sixel, furthermore TS privates are not name-mangled by default. Not going into details here, but I think we can reduce package size by at least 25% with some more clever bundle processing. Do we need an issue to explicitly deal with the bundling output size?

What about expecting an rgb.json to be served from the module directory or something. So we would only request it when a named color is encountered, then using the async handler infrastructure request the file from the directory. We may need an option to customize the directory, for example when using ASAR. Thoughts?

Yes I think we need to solve the "foreign data problem" for xterm.js in a uniform way, as things like proper unicode v13/14 handling with grapheme support alone would pull several 10kB of "blob" data, that are not easy/nice to integrate in the normal code units. The way it is done currently for wcwidth with its big table inline in the code is not very size friendly.
I am a bit concerned about the JSON way, as it does not fit nicely into the different loader/bundler use cases - while nodejs' loader can handle it on the fly, ES6 cannot (this is currently in draft). webpack currently simply embeds imported JSON resources in its outputs, if loaded via TS' import statement (maybe that can be "asyncified" when marked as external resource, idk).

So I have no clear answer to that yet. Maybe we can learn from others, how they deal with blob data.

@mofux
Copy link
Contributor

mofux commented Oct 25, 2021

Wouldn't a dynamic import (e.g. const { xcolors } = await import('./xcolors')) at the right spot be enough to support our use-case? Webpack will create a separate chunk for those imports and will request the chunk on demand.

https://webpack.js.org/guides/code-splitting/#dynamic-imports

@Eugeny
Copy link
Member

Eugeny commented Oct 25, 2021

I'm not sure how well that works with other bundlers (Rollup etc.), and even with Webpack, that takes away the user's control over the chunk name (which is set through a magic comment inside the import() call).

IMHO it would be much more flexible to just add a field/method for the user to set the color data and then they can decide whether to import() it dynamically, or to import statically so that it ends up bundled within the same chunk.

@jerch
Copy link
Member Author

jerch commented Oct 25, 2021

@mofux I think for that we have to switch to ESM first, as in CommonJS top level awaits are not allowed? But switching to full ESM removes support for import 'some_data.json', as it has not landed yet.

The whole story behind CommonJS vs ESM is kinda frustrating. I tried mixed CJS/ESM package support with node-sixel but failed. From my investigations it seems, that this is not really supported and basically needs 2 different package outputs. There is a very big thread started by @sindresorhus about that for TS builds (cannot find the link atm) - it is full of non working examples for this or that reason. A nightmare currently.

Edit:
@Eugeny Not sure, how you mean that:

... then they can decide whether to import() it dynamically, or to import statically so that it ends up bundled within the same chunk.

You mean like a compile/bundling switch (because that would have to be eval'ed at building time)? Thats a thing I kinda miss around TS - I dont see a nice way to "macro" things or create compile time settings (thats why the create_module.js script of the last commit does it all inline and must be executed manually, pretty ugly if you ask me).

@jerch
Copy link
Member Author

jerch commented Oct 25, 2021

@Tyriar, @mofux, @Eugeny

To not stall this PR too much creating another eternity PR, I suggest we just dump the named color support for now, until we have solved the "additional data loading issue" (created #3530 for that). While my PHF solution works, it still adds 5kB to the package size for no good reason, thus gonna remove it.

@jerch jerch marked this pull request as ready for review November 8, 2021 20:02
@jerch
Copy link
Member Author

jerch commented Nov 8, 2021

@Tyriar Done with this PR. I postponed several aspects and created follow-up issues, see the PR todo list at the top.

@jerch jerch requested a review from Tyriar November 8, 2021 20:04
@jerch
Copy link
Member Author

jerch commented Nov 10, 2021

@Tyriar Now done for real. 😸

@jerch jerch changed the title OSC 4/10/11 color handling OSC 4/10/11/12 color handling Nov 10, 2021
@@ -197,6 +206,7 @@ export namespace rgba {
return (fgR << 24 | fgG << 16 | fgB << 8 | 0xFF) >>> 0;
}

// FIXME: Move this to channels NS?
Copy link
Member

Choose a reason for hiding this comment

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

The namespace was meant to represent the source, so rgba.toChannels = rgba format to channels format

@@ -4,6 +4,9 @@
*/

import { IColor } from 'browser/Types';
import { IColorRGB } from 'common/Types';

// FIXME: Move Color.ts lib to common?
Copy link
Member

Choose a reason for hiding this comment

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

Color.ts lives in browser historically as it's only required when a renderer is active, things have changed a little with this PR where InputHandler knows more about color. Sounds like a good change if XParseColor ends up using these utils, but if not it would be best to keep in browser as then it's not included unnecessarily in xterm-headless.

@Tyriar Tyriar added this to the 4.16.0 milestone Dec 22, 2021
@Tyriar Tyriar merged commit ae6f575 into xtermjs:master Dec 22, 2021
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.

implement OSC 104 | 110 | 111 Implement OSC 10/11, prolly other OSC color commands as well
4 participants