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

ColorProvider API: Let extensions define the color formats #34366

Closed
aeschli opened this issue Sep 14, 2017 · 12 comments
Closed

ColorProvider API: Let extensions define the color formats #34366

aeschli opened this issue Sep 14, 2017 · 12 comments
Assignees
Labels
api editor-color-picker Editor color picker widget issues

Comments

@aeschli
Copy link
Contributor

aeschli commented Sep 14, 2017

The proposed API for the color provider currently has a knowledge of color formats. Problem is these are language specific. In the case of CSS there are even more formats coming.

Therefore I'd suggest the following API instead of the currently proposed resolveDocumentColor

export interface DocumentColorProvider {
// ...
/**
 * Provide the string representations for a color.
 */
   provideColorPresentations(colorRange: ColorRange):ProviderResult<ColorPresentation[]>;
}
interface ColorPresentation {
   label: string;
   type?: string;
   insertText?: string;
   additionalTextEdits?: TextEdit[];
}

Given a color value (e.g. {red:1, green:0, blue:0}) , the provider returns a number of possible presentations: (#FF0000, rgb(255, 0, 0), hsl(0, 100%, 50%), red) that apply at the given range.

The color picker can let the user toggle through the different presentations.
While the color picker is open and the user looks at different colors, new presentation requests will be sent to the provider for each color value. The color picker label will be updated once the result arrives.

  • the request also contains the range of the color. That's for the case of embedded languages or languages that have location specific presentations.
  • the response has a mandatory label field which is used as label to show to the user in the color picker.
  • if insertText is provided, the insertText will be used if the when the color picker decides to apply that color presentation at the given color range. If no insertText is provided, the label will be used as insertText.
    If there is a need for it, insertText could also support SnippetString.
  • if additionalTextEdits are set, these edits can be additionally applied (e.g. for adding an import statement)
  • the type file is optional. The idea is that the same type of presentation (e.g. hsl) always gets the same type (type: hsl). That way the color picker can stick with the same presentation type while the user browses through different colors.
@vscodebot vscodebot bot added the api label Sep 14, 2017
@aeschli
Copy link
Contributor Author

aeschli commented Sep 14, 2017

Additional suggestion:
Rename ColorRange to ColorInformation. This makes the API consistent with the DocumentProviders API

@aeschli
Copy link
Contributor Author

aeschli commented Sep 14, 2017

@jrieken @rebornix @dbaeumer @joaomoreno Please have a look!

@jrieken
Copy link
Member

jrieken commented Sep 14, 2017

👍 for ColorInformation, also 👍 form more details in ColorInformation, tho unsure about insertText just being a string with a range. Either we make it a TextEdit (similar things we deprecated on the CompletionItem) or we add an optional range (defaults to the information range). Or we just have one array that is just TextEdits...

@dbaeumer
Copy link
Member

I like the proposal but would make everything text edits.

@jrieken
Copy link
Member

jrieken commented Sep 15, 2017

I like the proposal but would make everything text edits.

I like that. We only have to think if we would ever want to support snippet-style-color insertion? E.g one could insert a choice snippet with all color formats?

@joaomoreno
Copy link
Member

I agree that we should not have formats in the API.

I also agree with @jrieken and @dbaeumer.

@rebornix rebornix added the editor-color-picker Editor color picker widget issues label Sep 17, 2017
@rebornix
Copy link
Member

Thanks all for your input on this ❤️

Previously the only concern I have about delegating formatting to extensions is it might lead to potential performance issue (flooding extension host when dragging inside saturation box, re #32235 (comment) ). Since we already reduce document changes with #34001 , I did testing again with functional formatter, the performance is reasonable: my idle CPU usage of Code is 20%, keep dragging inside saturation box will increase the usage to 40-50% on my machine. The number is acceptable as none one keeps dragging quickly forever, even they do so the CPU usage adds 20 - 30 % is still not bad.

Based on @aeschli 's proposal and suggestions from Jo and Joao in #32235 , now I change the API to

export class ColorPresentation {
	/**
	 * The label of this color presentation. It will be shown on the color
	 * picker header. By default this is also the text that is inserted when selecting
	 * this color presentation.
	 */
	label: string;
	/**
	 * An [edit](#TextEdit) which is applied to a document when selecting
	 * this presentation for the color.  When `falsy` the [label](#ColorPresentation.label)
	 * is used.
	 */
	textEdit?: TextEdit;
	/**
	 * An optional array of additional [text edits](#TextEdit) that are applied when
	 * selecting this color presentation. Edits must not overlap with the main [edit](#ColorPresentation.textEdit) nor with themselves.
	 */
	additionalTextEdits?: TextEdit[];
}

export interface DocumentColorProvider {
	provideDocumentColors(document: TextDocument, token: CancellationToken): ProviderResult<ColorInformation[]>;
	provideColorPresentations(colorInfo: ColorInformation, token: CancellationToken): ProviderResult<ColorPresentation[]>;
}

What it covers:

  • For languages like CSS/JSON, a list of label is enough.
  • If extensions want to display different things on the color picker header, they can add a textEdit for color change. For example, we may want to show RGB on header while inserting UIColor() while writing Swift.
  • Extensions can use additionalTextEdits to make additional content change like adding imports.

I separate textEdit and additionalTextEdits as we need to track the primary edit (which is used to change the color) for range update. I went through iOS/Android/UWP color definitions, didn't find a very strong case for snippet so at this moment, we can add support for this in the future.

@rebornix
Copy link
Member

Synced with @aeschli , the provideColorPresentations api is closer to provideDocumentColors instead of resolve.* API, a document parameter is necessary so the latest one is

provideColorPresentations(document: TextDocument, colorInfo: ColorInformation, token: CancellationToken): ProviderResult<ColorPresentation[]>;

@jrieken
Copy link
Member

jrieken commented Sep 22, 2017

I like it but I'd replace textEdit?: TextEdit; with insertText: string; range: Range to be align with what we have completion items. @aeschli Let's discuss on Monday and make this stable

@dbaeumer
Copy link
Member

dbaeumer commented Sep 25, 2017

I always thought that we have insertText and range on a completion item is some sort of backwards compatibility. Wouldn't it make more sense to always express this as a text edit ?

@jrieken
Copy link
Member

jrieken commented Sep 25, 2017

I always thought that we have insertText and range on a completion item is some sort of backwards compatibility.

No, moved range up and deprecated the textEdit to support snippet completions. We did not make the text edit support snippets because a snippet needs an editor and a text edit only needs a model. That's why we have mixed the edit into the completion item.

@aeschli aeschli added this to the September 2017 milestone Sep 26, 2017
@aeschli
Copy link
Contributor Author

aeschli commented Sep 26, 2017

Marking as fixed. API is up-to-date, and extensions and language servers have adopted the new API. We can create new issues for follow-up issues.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api editor-color-picker Editor color picker widget issues
Projects
None yet
Development

No branches or pull requests

5 participants