Vscode: Color Picker: Finalize Color Picker API

[joaomoreno]

There are two APIs we need to finalize: Monaco and VSCode.

Both APIs are identical except in terms of color formatters: VSCode supports a declarative format for color formats, while Monaco supports more generic function-style formatters.

Discussion Points

• [ ] Color value ranges - We have a very peculiar Color class internally, which has predefined value ranges for color components. RGB is stored in ranges of [0-255], for example. I am more in favor of using floating point [0-1] ranges for all color coordinates in the API. This is what is currently implemented. We just need consensus on the approach.
• [ ] Color.fromHex - I suggest we keep this in proposed state until #32295 is fixed, at which point it can be completely removed.
• [ ] Color Insertion - The current API works great with colors which are already in the document. Both the API and the color picker widget don't work really well when the user wants to _insert_ a new color. This needs thought. I think we can surface this as a feature request so we can get the API out the door asap.

[zbandhan]

Color Picker should be appeared after clicking on rectangle color symbol (box) like chrome if there is a existed color value otherwise it should be as soon as # sign typed. It is appearing with hovered action now.

[rebornix]

@joaomoreno considering the inline box pr is still under review, I'd like to share some thoughts of API finalization of both VSCode and Monaco.

VSCode

• Color Ranges. Right now extTypes is already using float numbers, IMHO it's already in a good shape.
• As we are suggesting users to use Float number instead of more preferred integer, I'd like to keep fromHex, fromHSLA, etc in the exposed Color class for now, which can help generate correct rgba value.
• Code insertion is still not there, but it's not a blocker.

I would expect VSCode API to be like what's in https://github.com/Microsoft/vscode/pull/32864 .

Monaco

Exposing this feature to Monaco requires more work, in order to get a consistent API declaration. Currently the API is like

export interface IColorFormatter {
    readonly supportsTransparency: boolean;
    format(color: Color): string;
}

export interface IColorRange {
    range: IRange;
    color: IColor;
    formatters: IColorFormatter[];
}
export interface IColor {
    readonly red: number;
    readonly green: number;
    readonly blue: number;
    readonly alpha: number;
}

It's a bit confusing as the relationship between IColor and Color is not Interface and Implementation. IColor is more likely RGBA. To make it consistent with VSCode API and use float number always, we can change the signature of IColorFormatter to

export interface IColorFormatter {
    readonly supportsTransparency: boolean;
    format(color: IColor): string;
}

and cast IColor to Color internally, in which way we don't need to expose Color/RGBA/etc and extensions build their own formatters on top of IColor (which is RGBA actually). The catch is we need to do the casing almost everywhere in contrib/colorPicker and tests.

Another way is drop IColor and use RGBA directly

export interface IColorRange {
    range: IRange;
    color: RGBA;
    formatters: IColorFormatter[];
}

This way, we need to expose all Color types Color RGBA HSLA HSVA. They might be more useful to extensions but it makes the API heavier.

[jrieken]

fyi - before this all becomes API we need to discuss the color format. it looks like a pretty big stunt and if we want something like that we should at least pick a syntax that is closer to what text mate snippet its variables and transforms look like

[joaomoreno]

It's a bit confusing as the relationship between IColor and Color is not Interface and Implementation.

Yeah, IColorFormatter should work in terms of IColor. That was an oversight.

Another way is drop IColor and use RGBA directly

Definitely interesting, but not yet convinced.

[rebornix]

@jrieken based on my understanding of TextMate snippet, we can do slight changes to current format to make it closer to TextMate snippet and share those concepts.

Our current format rgb({red:d[0-255]}, {green:d[0-255]}, {blue:d[0-255]}) is declarative, red, green and blue are Variables and we'll replace these placeholders with corresponding channel. While a typical Variable in TextMate snippet is like \textbf{${TM_SELECTED_TEXT:no text was selected}}.

Their similarities:

• surrounded by curly braces {}
• variable name and value/format are separated by :

Differencies

• TextMate variables are all upper case.
• If we use TM variable directly, we don't wrap the name with {} otherwise when we need to set its default value and do formatting, wrap it with {}. Our color format is always surrounded by {}
• We don't use $

To make it closer to TM, we can define our color format in following way:

• A color format contains Variables.
• Available variables: RED, GREEN, BLUE, HUE, SATURATION, LUMINANCE and ALPHA.
• We use variables in color format by prefixing the variable name with $, e.g., rgb($RED, $GREEN, $BLUE).
• Each variable has its default number format and range, e.g., RED's default number format is Integer and default range is [0-255].
• We can change the variable number format and range by using this syntax ${«variable»:«new format»«new range»}, e.g. ${RED:f[0-1]}
• Available number format
  
  
  
  • f, float with 2 decimal points.
  
  
  • Xf, float with X
  
  
  • d, decimal.
  
  
  • x, X, hexadecimal.
  
  
• Range format is [LOW:HIGH]

Several samples with this new syntax

• rgb(${RED}, ${GREEN}, ${BLUE}, ${ALPHA:d[0-255]})
• #${RED:X}${GREEN:X}${BLUE:X}

As we only convert color value from its default format/range to new format/range, a declarative format «new format»«new range» is good enough for now. If ppl have complex feature requests, we can then add Transformations to this. @jrieken look forward to your comments on above proposal.

[jrieken]

Great, I think we are on the right path with this. Some more thoughts...

• I don't think that there is rule saying TextMate variables must be upper case, it just happens to be like that for most dynamic variables, similar to environment variables
• The notion ${var_name:default value} is just one aspect of it and I don't know if it's the best fit because the semantics are: "Insert the resolved value of var_name and if that isn't possible insert default value. In our case we do have values for $RED, $GREEN etc. and I think we need something else.

In order to stay closer to the semantics and features of TextMate I can think of the following

1. We have more variables to statically cover number formats $RED_HEX, $RED_FLOAT, $RED_DEC etc. Next is number ranges and I believe the most common color ranges are [0-1], [0-100], [0-255], and [0-360]. I might be mistaken but could add more variables, e.g $HUE_DEC_360.
   
   
   
   1. We can fiddle with the variable names and make them 'carry information', e.g $RED_HEX[0-255] which then will be interpreted as todays {blue:X[0-255]}. The challenge is that it's not well defined what a variable name is. From my experience it is [a-zA-Z_][_a-zA-Z0-9]* so it needs some tweaking or a different format.
   
   
   2. The big hammer is something like snippet-transforms. They allow to manipulate the resolved value of a variable before insertion. The general format is ${«variable»/«regexp»/«format»/«options»} A real world sample that inserts your user name with the first latter capitalised is this: Hello ${USER/./${0:/upcase}/}. So, in original text mate the format-string supports built-in modifiers, like /upcase etc. I am looking at those modifiers and we could have something in the same spirit, e.g ${RED/.*/${0:/toDecimal(0,255)}/} which admittedly looks very complex...
   
   

To conclude, my preferences are option 1 and 2 or something in-between them. Our snippet parser and controller support all of this already today and it just needs another snippet variable resolver. I personally need to get a better understanding of the color formats because I don't understand red:d[0-255] when I can only create a Color object with numbers between 0 and 1. That makes me believe the 0 isn't actually needed in the formats. Please enlighten me @joaomoreno.

[joaomoreno]

0 just serves as a minimum, as you could also have red:d[-128:128] although that is a very far fetched example... so I think we could drop the minimum and just go with red:d[255] or similar.

[jrieken]

Ok, so my proposal would be dynamic variables ala $RED_HEX_FF. That would allows us to dump all custom parsing logic, make the API use the SnippetString and make the implementation use the SnippetController. So, I believe the format for color variable names would be

color    -> '$_' name ('_' format)?
name   ->  'red' | 'green' | 'blue' | 'hue' | 'saturation' | 'luminance' | 'alpha'
format -> ('d' | 'x' | 'f' | 'f' decimal) ('_' decimal){0-2}

Which allows for $red_x_ff (upper bound) or $red_x_00_ff (lower/upper bound) etc. This isn't the prettiest and bends a little the meaning of variables names but if we want to ship this month we have little slack for other experiments.

Now, seeing these names I might prefer something more transform-like, e.g ${RED/toDecimal(0,255)} which isn't adhering to the text mate snippet syntax but maybe something we can invent...

[jrieken]

Alternatively to the discussions above I'd propose we stop the effort in trying to describe all possible color representations on this planet and simply delegate this to the extension in question. So, instead of a template string, we simply ask the provider for the insert string of a color. It will adding to the provider something like this

interface ColorProvider {
  resolveColor(color: Color): ProviderResult<string>
}

So, while the color changes (or on commit) we ask the provider to turn it into a string. Yeah, more chatty but more conservative, less API exposure etc.

[joaomoreno]

That was the original approach, of course. I'll write down the missing drawbacks:

• The label in the UI can possibly be laggy due to the roundtrip
• That lag will be aggravated when we start throttling the calls, which we must
• There won't be any label when the extension host is busy, which it can bee since while we pick a new color, we update the model, which can trigger extensions to do some work

With formats, we're obviously optimising towards responsiveness. I'm still convinced it is the way to go, especially considering that the argument against it is purely not being sure about the format syntax.

I personally don't have an opinion on the syntax itself. We can align it better with TextMate, but I doubt we'll change the experience for anyone for the better. Developers will still need to read the documentation to learn how to use them, no matter how many snippets they've written in the past.

[jrieken]

There won't be any label when the extension host is busy, which it can bee since while we pick a new color, we update the model, which can trigger extensions to do some work

Well, with todays implementation we keep it very busy by sending hundreds of document change events while dragging around in the picker UI. Also something we don't want and something we need to throttle.

The general truth is that the editor doesn't work well when the extension host is busy. However, that also applies to format on type, code lens, resolving completions, typing etc. So, I wonder if you have actually implemented and tested that or is this merely a feeling?

[joaomoreno]

It's really just a feeling... it would actually be cool to see it live.

[rebornix]

resolveColor

I don't have a strong opinion on this. When we ask provideDocumentColors, extensions need to convert to color from whatever to RGBA. Considering they have the toolset to do the transformation, resolveColor can convert RGBA to the target format with the same toolset, e.g. parse-color.

To support multiple formats, resolveColor should better return string[] which does toString for all formats.

The label in the UI can possibly be laggy due to the roundtrip
That lag will be aggravated when we start throttling the calls, which we must

This pushes me more to declarative format as we need throttling and we don't want to make it laggy.

with todays implementation we keep it very busy by sending hundreds of document change events while dragging around in the picker UI

@alexandrudima pointed this out to me in this morning as well and I did some basic testing. I didn't check whether it makes extensions laggy but dragging around for a few seconds, the CPU usage increases 50% and even 100% sometimes (well if you keep dragging it for 5 seconds, you can see a 100+% Code Helper).

No one complains about it yet but I'd like to have it fix to avoid another HN post.

I'll add more info ...

[rebornix]

Current format

const CSSColorFormats = {
    Hex: '#{red:X}{green:X}{blue:X}',
    RGB: {
        opaque: 'rgb({red:d[0-255]}, {green:d[0-255]}, {blue:d[0-255]})',
        transparent: 'rgba({red:d[0-255]}, {green:d[0-255]}, {blue:d[0-255]}, {alpha})'
    },
    HSL: {
        opaque: 'hsl({hue:d[0-360]}, {saturation:d[0-100]}%, {luminance:d[0-100]}%)',
        transparent: 'hsla({hue:d[0-360]}, {saturation:d[0-100]}%, {luminance:d[0-100]}%, {alpha})'
    }
};

Dynamic TextMate Snippet Variable

const CSSColorFormats = {
    Hex: '#${red_x}${green_x}${blue_x}',
    RGB: {
        opaque: 'rgb(${red_d_0_255}, ${green_d_0_255}, ${blue_d_0_255})',
        transparent: 'rgba(${red_d_0_255}, ${green_d_0_255}, ${blue_d_0_255}, ${alpha})'
    },
    HSL: {
        opaque: 'hsl(${hue_d_0_360}, ${saturation_d_0_100}%, ${luminance_0_100}%)',
        transparent: 'hsla(${hue_d_0_360}, ${saturation_d_0_100}%, ${luminance_d_0_100}%, ${alpha})'
    }
};

[jrieken]

Trying to organise and structure my thoughts here... I have two main concerns, one is regarding the declarative nature and one is the constantly changing document

Declarative Color Format

It definitely looks appealing and is cleverly implemented however it's not free of worry to me. These are my concerns

• The API is asymmetric: we execute code to find colours but use a declarative approach for printing. It makes we wonder why we can express the printing of a color in declarative terms but not the discovery?
• It increases the API surface by another dimension, even if we use a snippet syntax we need custom transformer functions like toDecimal
• I doubt that we cover all color formats. In C# there is Color.FromArgb(int) with special byte order for that int. We might be able to express that but it might get ugly with un-padded values (F vs 0F). Then there are also well-known constants for colors, e.g. #FFF0F8FF is Color.AliceBlue and I don't see how that is expressed in the color format string.

Document Change Event Avalanche

This isn't really an API concern but an implementation concern. Today, while dragging around the model is being updated, these changes are forwarded to the extension host and language servers. I am unsure how we cope with that. I believe this is comparable to previewing IntelliSense suggestions on focus vs. inserting them on select and I'd feel better if we make this a UI thing and not a model change.

In short, my suggestion would be to execute code that transforms a color into one or many strings or text edits. Unsure how often we call that, personally I wouldn't feel uncomfortable if the document only changes on commit. Then I don't feel like I am loosing my current state. Updating the label inside the widget is another story, but we could also show two labels. A fixed color format (RGBA) and the 'preview' change that the provider will do.

[jrieken]

@rebornix fyi - I have one or two little nits we should discuss around the new API but we can put that in a different issue

[octref]

Was verifying this, but the API still lives in the proposed dts? I thought the plan is to move it to stable during Sep.

[jrieken]

no