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

Editor decoration with mtk inlineClassName #1070

Closed
kittaakos opened this issue Sep 19, 2018 · 11 comments
Closed

Editor decoration with mtk inlineClassName #1070

kittaakos opened this issue Sep 19, 2018 · 11 comments

Comments

@kittaakos
Copy link

Hi There,

In Eclipse Theia, we are trying to support language server (LS)-based semantic highlighting with editor decorations.

How does it work now:

  • On LS initialize, we send back a bunch of TextMate (TM) scopes supported by the LS.
  • On textDocument/didOpen and textDocument/didChange, the LS calculates the highlighted positions and sends the data back to the client.
  • The client converts the TM scopes into the internal metadata number using the TokenTheme.
  • We transform the metadata into an inlineClassName via the getClassNameFromMetadata function. This will result in an mtk${NUMBER} string.
  • We create an editor decoration with an option: { inlineClassName }.

It works perfectly unless the on the fly generated inlineClassName for the semantic highlighting has a higher number than the inlineClassName for the TM tokenizer.

Problem:

This Monaco Playground Example shows that the editor decoration has limitations if one tries to reuse the mtk inline CSS class names. The trailing ${NUMBER} does matter in the class name.

const code = [
    'function foo() { /* `unc` has the same color as `foo` */ }',
    'function bar() { /* `unc` does not have the same color as `bar` :( */ }'
].join('\n');

const editor = monaco.editor.create(document.getElementById('container'), {
    value: code,
    language: 'javascript'
});

// The generated CSS class name for the keywords is `mtk6`.
// If the `inlineClassName` for the editor decoration has a higher number, the editor decoration wins.
// Otherwise, the CSS class generated from the TM tokenization wins.
const decorations = editor.deltaDecorations([], [
    { range: new monaco.Range(1, 2, 1, 5), options: { inlineClassName: 'mtk7' }},
    { range: new monaco.Range(1, 10, 1, 13), options: { inlineClassName: 'mtk7' }},
    { range: new monaco.Range(2, 2, 2, 5), options: { inlineClassName: 'mtk4' }},
    { range: new monaco.Range(2, 10, 2, 13), options: { inlineClassName: 'mtk4' }},
]);

Question:

Is it possible to overcome this limitation? I have looked into the code a bit, and it seems, the _applyInlineDecorations function is responsible for merging the inline decorations with the tokens line by line. Is it possible to customize it via a custom service? If no, what would be the recommended way to prefer the inline decoration over the tokens?

Thank you!

@mofux
Copy link
Contributor

mofux commented Sep 19, 2018

IMHO the whole approach of using decorations for this feature isn't ideal:

  • decorations are not being drawn to the minimap, which means that the tokens that come from the semantic highlighting won't be shown in the minimap
  • mixing mtk{number} classes on the same element is error prone as rules with a higher {number} will overrule rules with a lower {number} because they appear later in the generated css (note: it has got to do with the order they are generated and written to the css, not the name of the class).

I think a cleaner approach would be to update the line tokens themselves after the semantic highlighting comes back.

@kittaakos
Copy link
Author

Thanks for the feedback, @mofux!

decorations are not being drawn to the minimap

I was not aware of this.

I think a cleaner approach would be to update the line tokens themselves after the semantic highlighting comes back.

👍

@kittaakos
Copy link
Author

I think a cleaner approach would be to update the line tokens themselves after the semantic highlighting comes back.

@mofux, this raises a question; if two separate rules highlight the same document range, how can this be done by modifying the line tokens only. For instance, a field has to be highlighted as italic (because it is a static Java field) and also has to be colored differently (because it is a deprecated Java field) then how one can get the "combined" metadata for the two separate TM scopes? It is feasible with the mtk inlineClassName approach via the editor decorations. Thank you!

@mofux
Copy link
Contributor

mofux commented Sep 25, 2018

@kittaakos
I've just checked how VSCode does it, and they also add an inlineClassName decoration for the token, instead of issuing an extra TM Scope for it:

screen shot 2018-09-25 at 11 26 17

Their decoration is also not drawn in the minimap, but they don't seem to care 😁

My initial thought was to provide an extra Textmate token that describes the semantic state, like unused.variable.parameter.js, which you could then style using the Textmate Theme file, but I guess going the inlineClassName approach is simpler. You still should not re-use the mtk classes for semantic highlighting but create your own ones -- just like VSCode does, so you avoid conflicts and can force your class styles to take precedence over the mtk class styles.

@kittaakos
Copy link
Author

My initial thought was to provide an extra Textmate token that describes the semantic state, like unused.variable.parameter.js, which you could then style using the Textmate Theme file,

This is not an option. That would mean, each time, one adds support for a new language server (LS) in the application, the theme files have to be adjusted with the styles supported by the LS. (Let me know if this need additional clarification.)

just like VSCode does, so you avoid conflicts and can force your class styles to take precedence over the mtk class styles.

This.

Cool, you have already answered my next question. Then my plan is the following, what do you think, could this work?

  • Read all mtk CSS classes from the document.
  • Copy them and create new CSS classes with a custom class name prefixes on the fly. (mtk${NUMBER} -> my-mtk${NUMBER})
  • Attach the customized CSS classes to the document.
  • Receive the TM scopes from the LS.
  • Map the scopes to the metadata.
  • Get the built-in mtk class name for the TM scope.
  • Map the mtk class name to the custom prefixed class name. (my-mtk)
  • Use the EditorDecoration with the inlineClassName property.
  • Repeat the steps on theme change events.

Thank you!

@mofux
Copy link
Contributor

mofux commented Sep 25, 2018

@kittaakos Yes I guess your approach would work. I've studied the docs in microsoft/vscode-languageserver-node#367 trying to understand how your semantic highlighting is working. From what I understand, semantic tokens identified by the language server can only add additional highlighting information, but they cannot undo tokens that were previously identified by Textmate (vscode-textmate in your case) - is that correct?

I'm a little bit confused about the the usage of the term scope in that documentation. Does it refer to language scope name (e.g. source.js), or does it refer to the resolved TM token scope names (e.g. variable.parameter.js)? Can you point me to the source of a LS implementation that already supports the semanticHighlightingCapabilities?

What I'm trying to understand is how for example the TypeScript LS would provide the highlight for an unused variable, which you'll want to fade-out in the editor (just like VSCode does).

@kittaakos
Copy link
Author

Yes I guess your approach would work.

👍

the language server can only add additional highlighting information, but they cannot undo tokens that were previously identified by Textmate (vscode-textmate in your case) - is that correct?

Correct. The LS can only add styles, but the additional styles must overrule (extend) the styles from TM.

I'm a little bit confused about the the usage of the term scope in that documentation.

scope is an array of TM scopes "encoded" into an unsigned integer. On LS startup the LS sends back the available scopes as an string[][]. For instance, in Java, the TM scopes for static final fields is the following array:

{
  scope: ["storage.modifier.static.java", "storage.modifier.final.java", "variable.other.definition.java", "meta.definition.variable.java", "meta.class.body.java", "meta.class.java", "source.java"]
}

but it is sent as:

{
  scope: 0
}

The client code resolves the scope number into the TM scopes array, and gets the narrowest TM scope for the styles:
https://github.com/theia-ide/theia/blob/bbcc0da97014bf76dd60a612163af4a189b0eb01/packages/monaco/src/browser/monaco-semantic-highlighting-service.ts#L127-L137

@mofux
Copy link
Contributor

mofux commented Sep 25, 2018

Got it. Guess the approach of creating your own class names that take precedence over the build-in mtk classes is the way to go then. Thanks for your clarifications.

@kittaakos
Copy link
Author

No no, thank you!

I would like to leave the issue open and ask for assistance if required. Once I am ready with the reference implementation, I am happy to link it here and will close the issue.

@HighCommander4
Copy link

HighCommander4 commented Aug 6, 2019

Note that vscode-cquery (and soon clangd-vscode) have semantic highlighting implementations that don't appear to suffer from this problem.

The API they use to set highlightings is TextEditor.setDecorations(). Is that something we could use in Theia, in preference to deltaDecorations()?

@kittaakos
Copy link
Author

I am going to close this as it is fixed in Theia. The credit goes to @HighCommander4. 👍

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants