Add support for OSC 4 ; color ; color spec #3163

slawekzachcial · 2020-11-03T17:55:10Z

This pull request is an attempt to add support for OSC 4, as per #3036.

The code is far from complete. I don't yet understand enough the architecture of the library and so the modified classes may be wrong, or the way they have been modified is wrong (e.g. should update interface rather than coding directly into implementation, should leverage existing helpers to parse colors, etc...). Also I have not created any tests yet - I need to better understand first the right way to modify the code and will add the corresponding tests then. Finally, there are a few console.log calls I'm using to help me understand the flow - those will be removed and/or replaced by calls to _logService.

To test the output I'm using base16-shell colortest script as my original intent has been to make xterm.js work with Base16.

Before my updates, the output of the script in the Demo console looked like this:

After my updates, the output looks like this:

It seems that all the background colors are handled properly.

However, for foreground it only works for colors 1-16 but not above (see the shades of blue on the left). I'm not able to figure out what else needs to be modified to make it work.

Hopefully somebody could provide me some guidance as to how to make it work, and then how to clean up this PR so that it is aligned with the design and the spirit of this project.

Thanks!

slawekzachcial · 2020-11-03T20:02:00Z

For the blue shades issue I raised above it is something related to my system settings - so no need to worry about it.

I was able to run it successfully in node:current docker container.

Here is the test bash snippet I used in the Demo terminal (same colors as base16-shell colortest above:

printf "\x1b]4;0;rgb:18/18/18\x1b\\x1b]4;1;rgb:ab/46/42\x1b\\x1b]4;2;rgb:a1/b5/6c\x1b\\x1b]4;3;rgb:f7/ca/88\x1b\\x1b]4;4;rgb:7c/af/c2\x1b\\x1b]4;5;rgb:ba/8b/af\x1b\\x1b]4;6;rgb:86/c1/b9\x1b\\x1b]4;7;rgb:d8/d8/d8\x1b\\x1b]4;8;rgb:58/58/58\x1b\\x1b]4;9;rgb:ab/46/42\x1b\\x1b]4;10;rgb:a1/b5/6c\x1b\\x1b]4;11;rgb:f7/ca/88\x1b\\x1b]4;12;rgb:7c/af/c2\x1b\\x1b]4;13;rgb:ba/8b/af\x1b\\x1b]4;14;rgb:86/c1/b9\x1b\\x1b]4;15;rgb:f8/f8/f8\x1b\\x1b]4;16;rgb:dc/96/56\x1b\\x1b]4;17;rgb:a1/69/46\x1b\\x1b]4;18;rgb:28/28/28\x1b\\x1b]4;19;rgb:38/38/38\x1b\\x1b]4;20;rgb:b8/b8/b8\x1b\\x1b]4;21;rgb:e8/e8/e8\x1b\\r\n"
for i in {0..21}; do printf "\x1b[38;5;%dmcolor%02d   \x1b[48;5;%dm_______\x1b[0m\r\n" "$i" "$i" "$i"; done

And here is the output:

Tyriar

Haven't tested it out but the PR looks good, comments are mainly just shuffling a few things around.

src/browser/Terminal.ts

src/common/InputHandler.ts

slawekzachcial · 2020-11-04T08:01:22Z

Thanks for the review! Let me rework it.

slawekzachcial · 2020-11-04T21:32:49Z

I implemented the changes but I do notice that there is something weird going on with foreground colors with indices equal or higher to 16. If I run the shell snippet twice - it works fine the first time but the second time foreground colors above 15 become the shades of blue, as if their original values were somehow cached. Background colors seem to work fine.

Tyriar

it works fine the first time but the second time foreground colors above 15 become the shades of blue, as if their original values were somehow cached. Background colors seem to work fine.

Colors 16+ are a little different as historically they're currently readonly even to themes. I'd try test the different renderers by changing the rendererType and checking the webgl addon box to narrow down which ones have problems. We may need to tweak the renderer(s) too.

slawekzachcial · 2020-11-06T22:54:34Z

I poked around the code some more (without really understanding the details ;-) ) and here is what I found:

renderer/atlas/DynamicCharAtlas.ts would return DEFAULT_ANSI_COLORS if requested index is bigger than what's configured there.
The configuration is generated in browser/renderer/atlas/CharAtlasUtils and there it is limited to 16 colors only
I locally replaced the line ansi: colors.ansi.slice(0, 16) with ansi: colors.ansi and now my re-re-rendering works as expected

But since this 16-color limitation was put there I have no idea what else this will break.

I also noticed in the CharAtlasUtils the following comment:

// For the static char atlas, we only use the first 16 colors, but we need all 256 for the
// dynamic character atlas.
ansi: colors.ansi.slice(0, 16)

... and still the config passed to DynamicCharAtlas has only 16 colors.

slawekzachcial · 2020-11-07T20:46:44Z

I added the new event type and unit tests. I also updated CharAtlasUtils to use ALL Ansi colors and not just the first 16. Even though all tests pass on my workstation I'm conscious that this may break other things and certainly requires more ad-hoc testing.

slawekzachcial · 2020-11-07T20:48:26Z

If all of this worked, I guess the remaining would be to update documentation.

slawekzachcial · 2020-11-08T09:27:02Z

After all the changes it's now possible to run base16-shell colortest multiple times and the colors for foreground and background are consistently correct.

slawekzachcial · 2020-11-14T10:12:47Z

@Tyriar I'm sure you must be busy with other work. Do you think there is anything else I should be looking at in this PR?

src/common/InputHandler.test.ts

src/common/InputHandler.ts

jerch

Sorry for taking us so long to get it merged.

I still have a few questions / remarks (also see below), mainly I wonder how to go about OSC 104 later on, which is meant to restore initial values after changes made from OSC 4. I think we would need some sort of stored default colors in the color manager, prolly pulled from the initial theme settings.

jerch · 2020-12-06T20:36:50Z

src/browser/Terminal.ts

+  private _changeAnsiColor(event: IAnsiColorChangeEvent): void {
+    const color = rgba.toColor(event.red, event.green, event.blue);
+
+    this._colorManager!.colors.ansi[event.colorIndex] = color;
+    this._renderService?.setColors(this._colorManager!.colors);
+    this.viewport?.onThemeChange(this._colorManager!.colors);
+  }


An additional method on Terminal is an unlucky choice, as we try to get rid of helpers on terminal in favor of more direct service usage. Can we get this moved to the color manager directly? Not sure if it is properly service'ified yet, if not we can keep it here and move it later on...

I dont like the ! much, as it might lead to runtime errors in case the manager is indeed not there for whatever reason. Lets better hide the access behind an initial condition test, something like if (!this._colorManager) { return; }...

Yes, if (!this._colorManager) { return; } is a good idea here.

@jerch The function now exists early if _colorManager is undefined.

jerch · 2020-12-06T20:40:14Z

src/browser/renderer/atlas/CharAtlasUtils.ts

-    ansi: colors.ansi.slice(0, 16)
+    // ansi: colors.ansi.slice(0, 16)
+    // TODO: Using entire array to support OSC 4; can this break anything?
+    ansi: colors.ansi


Hmm yes, it basically increases the atlas size from 16 * 256 pixmaps to 256 * 256. Not sure thought if it will cause problems in terms of browser limits. This needs some serious testing by someone with more atlas insights than me.

@Tyriar Is it safe to expand the atlas here that way?

Looking at the code it seems like this should be fine, I think the slice is there as previously it was static but it ends up falling back to DEFAULT_ANSI_COLORS in ColorManager.

jerch · 2020-12-06T20:42:37Z

src/common/InputHandler.test.ts

+  describe('OSC', () => {
+    it('should parse correct Ansi color change data', () => {
+      // this is testing a private method
+      const parseAnsiColorChange = inputHandler['_parseAnsiColorChange'];


Haha - thats a quite hacky access, but okish for tests. I normally subclass things to be tested with privates exposed in test files to still get the type checking done (which will be completely skipped with your way).

Yes, that's for tests only. Was suggested by @kena0ki as I was using as any. Could you point me to an example of subclassing? I thought for a subclass to get access to the private parent members some sort of a hack like the above would also be required?

I thought for a subclass to get access to the private parent members some sort of a hack like the above would also be required?

Yes thats true. Either you can go with protected in the original class and just expose that particular property from the subclass, or do the the any trick in the subclass, but with proper typing of the result (here the function type decl).

Could you point me to an example of subclassing?

I did something similar with protected and subclassing in the parser tests here:

xterm.js/src/common/parser/EscapeSequenceParser.test.ts

Line 60 in f8f7e7e

class TestEscapeSequenceParser extends EscapeSequenceParser {

👍 this is the recommended way as then TS refactors will just work automatically instead of causing a test runtime error.

Thanks. Done. I did not realize there was already TestInputHandler. I made the method protected in the base class and exposed it as public in the test class. Most of other fields in the test class used the same approach.

jerch · 2020-12-06T20:45:34Z

src/common/InputHandler.ts

@@ -250,6 +250,8 @@ export class InputHandler extends Disposable implements IInputHandler {
  public get onScroll(): IEvent<number> { return this._onScroll.event; }
  private _onTitleChange = new EventEmitter<string>();
  public get onTitleChange(): IEvent<string> { return this._onTitleChange.event; }
+  private _onAnsiColorChange = new EventEmitter<IAnsiColorChangeEvent>();


Whats the idea for a custom event here? Shall this be interceptable later on from outside?

I'm not sure 😊 I modeled it after "title change".

Thinking about it some more, using the event is the way that InputHandler can notify the Terminal that the colors have changes, so that the Terminal can then update _colorManager colors.

Yes sounds good that way. I still wonder, if we would need a stricter coupling of the color manager to the input handler, esp. for those ? sequences. For those the input handler needs to query the colors.

Hi, @slawekzachcial! Thanks for the great PR!

Would it be possible to also make onAnsiColorChange a part of a public API? Just like onTitleChange is done.

jerch · 2020-12-06T22:16:36Z

src/common/InputHandler.ts

@@ -2711,6 +2714,38 @@ export class InputHandler extends Disposable implements IInputHandler {
    this._iconName = data;
  }

+  private _parseAnsiColorChange(data: string): IAnsiColorChangeEvent | null {
+    // example data: 5;rgb:aa/bb/cc
+    const regex = /(\d+);rgb:([0-9a-fA-F]{2})\/([0-9a-fA-F]{2})\/([0-9a-fA-F]{2})/;


I think this does not enough for OSC 4. As far as I get it from the xterm spec it should be capable to handle arbitrary long index ; spec pairs, also case should not matter. Thus this should be a valid sequence:

OSC 4 ; 1 ; rgb:00/00/00 ; 2 ; rgb:ff/00/00 ; 3 ; RGB:00/FF/00 BEL

Furthermore xterm supports a ? instead of a valid color spec and would respond with a sequence to set this color register with the current registered value:

echo -e '\x1b]4;255;?\a' --> sends OSC 4;255;rgb:eeee/eeee/eeee BEL

(also note the double width of the response for the color spec, not sure if we have to support that as well for input)

Yes indeed OSC 4 allows to set multiple colors in the same command. This can be easily fixed I think IAnsiColorChangeEvent could carry an array of colors instead of one right now.

I'll admit I did not understand the ? comment. Would that mean that if it comes as ? it can be simply ignored as there will be no change?

Interestingly enough, in your ? example you note the 4-digit hex numbers. Looking at xparsecolor (see "Color Names" section) this can come a either 1, 2, 3 or 4-digit hex number. But I did not quite understand what does "the value scaled in N bits" mean so left it at 2 for now.

@jerch The latest commits allow to pass multiple color values in the same OSC 4 call.

The ? means, that the application wants to get the current color from terminal, thus the terminal should respond with the color currently set.

Idc much if we dont support that yet, we can add it by a later PR. In the end we should be capable to handle OSC 4 as xterm would, otherwise we have a partly broken implementation.

slawekzachcial · 2020-12-07T21:00:18Z

I'll try to look at OSC 104 - I was only after OSC 4 here 😉. Thanks for the review. Let me rework few pieces.

slawekzachcial · 2020-12-09T21:40:18Z

@jerch with respect to the issue with adding a new method to the Terminal, how otherwise could InputHandler interact with ColorManager instance?

jerch · 2021-01-02T18:52:44Z

@jerch with respect to the issue with adding a new method to the Terminal, how otherwise could InputHandler interact with ColorManager instance?

Yes thats where the nasty part begins. Imho we need a closer coupling between input handler and color manager to get full support for OSC 4 with ? as well as for OSC 104 later on. Closer coupling means - the color manager would need to get servicified and given to the input handler at the ctor. Not sure if we should aim for that in this PR or postpone it to a later code shift when getting down to ? and OSC 104.

@Tyriar Your thoughts in the regard?

Tyriar

Yes thats where the nasty part begins. Imho we need a closer coupling between input handler and color manager to get full support for OSC 4 with ? as well as for OSC 104 later on. Closer coupling means - the color manager would need to get servicified and given to the input handler at the ctor. Not sure if we should aim for that in this PR or postpone it to a later code shift when getting down to ? and OSC 104.

@Tyriar Your thoughts in the regard?

What is the problem we're trying to solve here? If InputHandler needs to tell ColorManager something, an event should work. If we need something else/more extensive I think passing an IColorManager to InputHandler should be fine, it's important not to introduce cyclic dependencies though.

slawekzachcial · 2021-01-09T15:55:39Z

If I may chime in... If deeper refactoring is required to get InputHandler and ColorManager more tightly together, that's certainly a topic for a different PR as its impact will be much broader than the OSC 4 implementation. I would keep this PR focused on OSC4 - if there are other things related to this feature missing, please let me know and I'll fix it.

Tyriar

Great work @slawekzachcial! Sorry about the delay, I've had very patchy availability for the last couple of months.

Add rudimenatary and not fully working support of OSC 4

5c7644a

slawekzachcial changed the title ~~Add support OSC 4 ; color ; color spec~~ Add support for OSC 4 ; color ; color spec Nov 3, 2020

Merge branch 'master' into add-osc-4

1c3da7e

Tyriar requested changes Nov 3, 2020

View reviewed changes

src/browser/Terminal.ts Outdated Show resolved Hide resolved

src/browser/Terminal.ts Outdated Show resolved Hide resolved

src/browser/Terminal.ts Outdated Show resolved Hide resolved

src/common/InputHandler.ts Outdated Show resolved Hide resolved

Tyriar added this to the 4.10.0 milestone Nov 3, 2020

Move color parsing from Terminal to InputHandler

b22d7f1

Tyriar reviewed Nov 4, 2020

View reviewed changes

slawekzachcial added 2 commits November 5, 2020 08:51

Fix TODO comment

b127436

Merge branch 'master' into add-osc-4

712f0ae

slawekzachcial added 3 commits November 7, 2020 21:36

Add IAnsiColorChangeEvent type

b898e01

Add IAnsiColorChangeEvent related tests

d5f4ae5

Use all and not only 16 Ansi colors in CharAtlas config

bc86b78

slawekzachcial requested a review from Tyriar November 7, 2020 20:46

Add terminal sequence doc

77ed5ba

slawekzachcial marked this pull request as ready for review November 8, 2020 09:19

slawekzachcial added 2 commits November 8, 2020 22:05

Update OSC 4 description to match xtermjs#3036

6535995

Clarify color spec description

e6917d1

Merge branch 'master' into add-osc-4

4c6dc45

kena0ki reviewed Nov 28, 2020

View reviewed changes

src/common/InputHandler.test.ts Outdated Show resolved Hide resolved

src/common/InputHandler.ts Outdated Show resolved Hide resolved

slawekzachcial added 3 commits November 29, 2020 16:30

Make InputHandler parseAnsiColorChange private

52b64be

Fix quotes

f4a5638

Add Ansi color change positive test

5a74ce2

jerch reviewed Dec 6, 2020

View reviewed changes

slawekzachcial added 2 commits December 7, 2020 23:20

Make safe references to colorManager in _changeAnsiColor

2804bfa

Implement OSC 4 for list of colors

7d3f537

slawekzachcial mentioned this pull request Dec 10, 2020

Add support for setting colors using "Operation System Command" sequence. #1493

Closed

Tyriar reviewed Jan 4, 2021

View reviewed changes

Make _parseAnsiColorChange available for tests in TestInputHandler

5c33e40

Merge branch 'master' into add-osc-4

676aabc

slawekzachcial requested a review from Tyriar January 9, 2021 15:58

Remove resolved TODO

19c14b0

Tyriar approved these changes Jan 11, 2021

View reviewed changes

Tyriar enabled auto-merge January 11, 2021 15:42

Tyriar merged commit ea1fcc6 into xtermjs:master Jan 11, 2021

This was referenced Mar 15, 2021

Bump xterm from 4.9.0 to 4.11.0 httpsgithu/hyper#92

Closed

Bump xterm from 4.7.0 to 4.11.0 quaardvark/hyper#111

Closed

Bump xterm from 4.7.0 to 4.11.0 PLSV/hyper#112

Closed

Bump xterm from 4.6.0 to 4.11.0 Qstar/lens#18

Closed

Add support for OSC 4 ; color ; color spec #3163

Add support for OSC 4 ; color ; color spec #3163

Conversation

slawekzachcial commented Nov 3, 2020

slawekzachcial commented Nov 3, 2020

Tyriar left a comment

Choose a reason for hiding this comment

slawekzachcial commented Nov 4, 2020

slawekzachcial commented Nov 4, 2020

Tyriar left a comment

Choose a reason for hiding this comment

slawekzachcial commented Nov 6, 2020 • edited Loading

slawekzachcial commented Nov 7, 2020

slawekzachcial commented Nov 7, 2020

slawekzachcial commented Nov 8, 2020

slawekzachcial commented Nov 14, 2020

jerch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerch Dec 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slawekzachcial commented Dec 7, 2020

slawekzachcial commented Dec 9, 2020

jerch commented Jan 2, 2021 • edited Loading

Tyriar left a comment

Choose a reason for hiding this comment

slawekzachcial commented Jan 9, 2021

Tyriar left a comment • edited Loading

Choose a reason for hiding this comment

slawekzachcial commented Nov 6, 2020 •

edited

Loading

jerch Dec 6, 2020 •

edited

Loading

jerch commented Jan 2, 2021 •

edited

Loading

Tyriar left a comment •

edited

Loading