Fix for issue #2906 (semantic highlighting styles overruled by TM scopes)

[HighCommander4]

The idea for the fix comes from the observation that there are VSCode
plugins that implement semantic highlighting using
TextEditor.setDecorations(), and do not suffer from this problem.

We don't want to use setDecorations() itself because it's not really
suited for incremental updates to the highlighting (which the LSP
protocol extension that Theia implements allows for).

However, setDecorations() is implemented in terms of deltaDecorations()
(which is what Theia uses), and changing Theia's use of
deltaDecorations() to be more like the implementation of
setDecorations() seems to fix this bug.

Specifically, instead of getting an inlineClassName directly from the
TokenMetadata (which seems to produce the problematic inlineClassName
that coflicts with TM), we:

• Get the token color from the TokenMetadata

• Construct an IDecorationRenderOptions from the token color
  (as if we were going to call setDecorations())

• Use ICodeEditorService.registerDecorationType() and
  ICodeEditorService.resolveDecorationOptions() to massage the
  IDecorationRenderOptions into an IModelDecorationOptions. This
  appears to cause monaco to allocate a new inlineClassName for
  the color which doesn't conflict with TM.

• Call deltaDecorations() with IModelDecorationOptions obtained in
  this way.

[HighCommander4]

@kittaakos, what do you think about this alternative approach for fixing #2906?

The fix is not complete (e.g. it's missing code to update the decoration options when the theme changes), but I'm curious what do you think of the general approach.

[kittaakos]

@HighCommander4, can you please provide us an example so that we can try out your changes against the master state? Thanks!

[HighCommander4]

I tested this with the C++ language server, clangd. This can be built from source, or installed from https://apt.llvm.org/ (though note that, at this time, only the Debian Buster, Debian sid, and Ubuntu Disco packages are new enough to include the semantic highlighting feature).

Example steps to demonstrate the problem:

• Install clangd and ensure it's in your PATH. (If installing via the apt.llvm.org packages, note that the name of the binary they install is suffixed with the version, e.g. clangd-9. You can either symlink that into your PATH as clangd, or run Theia with CPP_CLANGD_COMMAND=clangd-9 in the environment).
• Apply my patch to enable semantic highlighting in Theia's C++ language client.
• Run Theia, create a file with extension .cpp, and paste the following code:

namespace problem {
    void foo();
}
int main() {
    problem::foo();
}

Without this patch, on line 5, the tokens problem and foo have the same color (which is wrong). If you look in devtools, the token problem has both the mtk1 and mtk12 class names, but mtk12 overrides mtk1 (which is the one we want).

With this patch, the token problem has a different color. In devtools, we can see it has a class name ced-DecorationType8-1, which correctly takes precedence over mtk12.

[kittaakos]

Works very nicely! I have tried with the YANG data modeling language in Yangster.

I add a few screenshots to show how nicely your contribution works:

Before:

After:

For instance, although we had a special rule for coloring descriptions, it did not work previosuly, as the String rule overruled our customization. Now it works. 🎉

I have manually verified the CSS classes, and with your changes there are no mtkX styles anymore. Please continue with your approach and get rid of the TODOs you added to the source code.

[HighCommander4]

I fixed the TODOs, addressed the other comments, and cleaned up the patch a bit. Should be ready for a proper review now. Thanks!

[akosyakov]

@kittaakos please have a look when you have a chance 🙏

[kittaakos]

I am verifying the changes with the YANG language.

[kittaakos]

We do not update the highlighted tokens when changing the theme.

Steps to reproduce:

• open a file and see we have coloring for a certain name, for instance, aio. ✅
• change the theme. aio is not highlighted.
• change back the theme. aio is still not highlighted.
• close and reopen the editor, aio is highlighted again.

[kittaakos]

Unfortunately, it does not work if I want to decorate a keyword. For instance, in YANG, we decorate the key keyword if it is nested in a leaf. The previous implementation could color keywords, it does not work with your changes. I do not know why. See:

Before:

After:

I can see the ranges with the corresponding tokens from the LS, it seems to be correct, but the final CSS class is missing.
Test DSL:

module d {
    leaf key {
        type string;
    }
}

Input from the LS:

After conversion:

But the CSS class is missing:

It has to be similar to any other colored range:

Edit: typos.

[HighCommander4]

Unfortunately, it does not work if I want to decorate a keyword. For instance, in YANG, we decorate the key keyword if it is nested in a leaf.

@kittaakos Could you share some instructions for testing this with YANG? Should I download the YANG language server from here? Is there any setup required on the Theia side?

[kittaakos]

some instructions for testing

It is a hack, but I created a PR in yangster to use your patched semantic highlighting capabilities in Theia. You can check the README how to clone and build Yangster. The rest, starting, debugging should be Theia specific. If you spot a problem with the way I am "patching" the code or have a better idea, let me know.

[HighCommander4]

You can check the README how to clone and build Yangster.

@kittaakos, I tried following these steps, but at the yarn rebuild:electron step, I get the following:

$ yarn rebuild:electron
yarn run v1.12.3
$ theia rebuild:electron
Processing @theia/node-pty
Processing nsfw
Processing native-keymap
Processing find-git-repositories
Uncaught Exception:  Error: Unable to find electron-prebuilt's version number, either install it or specify an explicit version
Error: Unable to find electron-prebuilt's version number, either install it or specify an explicit version
    at Object.<anonymous> (/home/nr/dev/projects/ericsson-misc/yangster/node_modules/electron-rebuild/lib/src/cli.js:81:19)
    at Generator.next (<anonymous>)
    at /home/nr/dev/projects/ericsson-misc/yangster/node_modules/electron-rebuild/lib/src/cli.js:8:71
    at new Promise (<anonymous>)
    at __awaiter (/home/nr/dev/projects/ericsson-misc/yangster/node_modules/electron-rebuild/lib/src/cli.js:4:12)
    at /home/nr/dev/projects/ericsson-misc/yangster/node_modules/electron-rebuild/lib/src/cli.js:70:8
    at Object.<anonymous> (/home/nr/dev/projects/ericsson-misc/yangster/node_modules/electron-rebuild/lib/src/cli.js:146:4)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)

What should I do to get past this?

(Apologies if this is a basic question; I'm new to the world of javascript development. I did try googling and ran some npm commands to try and get past this, but I'm pretty sure they just made things worse and borked my checkout altogether.)

[kittaakos]

Apologies if

You do not have to.

Processing find-git-repositories
Uncaught Exception:  Error: Unable to find electron-prebuilt's version number, either install it or specify an explicit version
Error: Unable to find electron-prebuilt's version number, either install it or specify an explicit version

I have never seen this before. I look into it later today and give you an update.

[kittaakos]

@HighCommander4, what OS did you use building yangster?
I have seen an issue with find-git-repositories in a completely different project.

[HighCommander4]

@kittaakos I am using Debian 9. (Is that relevant? My understanding was that the build process doesn't really use the OS's package repositories.)

[HighCommander4]

@kittaakos Hi, just wanted to ping here. Any suggestions for how I can get Yangster building, or another way to reproduce the issue you observed with the patch? I'd like to reproduce the issue and fix it.

[kittaakos]

Any suggestions for how I can get Yangster building,

I do not have. I could build Yangster without any issues.

[HighCommander4]

Is there perhaps a different language which is affected by the issue?

[kittaakos]

Is there perhaps a different language which is affected by the issue?

I am not aware of it. I can create a simple DSL to simplify the context, but I do not have much time dealing with this right now.

Is the LS-based semantic highlighting feature that urgent for you? I mean, MS has not even accepted the proposal yet. I would expect we have to rewrite it from scratch by the time there is an update on the specification.

[HighCommander4]

I was able to get yangster to build. The missing step was:

yarn add -W @theia/electron@0.7.0-next.e147cf39

After that, yarn rebuild:electron succeeds.

Unfortunately, the electron app fails to start, with the following error:

~/dev/yangster/yangster-app-electron$ yarn start
yarn run v1.17.3
$ theia start --root-dir=../workspace
/home/nr/dev/yangster/node_modules/electron/dist/electron --root-dir=../workspace /home/nr/dev/yangster/yangster-app-electron/src-gen/frontend/electron-main.js: symbol lookup error: /home/nr/dev/yangster/node_modules/native-keymap/build/Release/keymapping.node: undefined symbol: _ZN2v816FunctionTemplate3NewEPNS_7IsolateEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEENS_5LocalIS4_EENSA_INS_9SignatureEEEiNS_19ConstructorBehaviorENS_14SideEffectTypeE
Done in 0.67s.

[HighCommander4]

Unfortunately, the electron app fails to start, with the following error:

I was able to get past this by deleting the repository and starting from scratch. What appears to have made the difference is running yarn add -W @theia/electron@0.7.0-next.e147cf39 before even attempting to run yarn rebuild:electron.

[HighCommander4]

And I can confirm that I can reproduce the undesirable rendering.

[HighCommander4]

I found the problem, it's a simple mistake in my patch.

I had a check of the form if (scope) where scope was number | undefined. I meant for the check to fail for undefined, but in fact it also failed for 0. As a result, the semantic highlighting with kind 0 was not applied.

You can tell I'm a JS newbie...

We didn't notice this in the C++ language server because the semantic highlighting with kind 0 was "variable", and its color in the theme matched the regex-based highlighting color, and the regex-based highlighting was already coloring them right for the most part.

[HighCommander4]

We do not update the highlighted tokens when changing the theme.

I can reproduce this as well, investigating.

[HighCommander4]

I was able to get yangster to build. The missing step was:

yarn add -W @theia/electron@0.7.0-next.e147cf39

To clarify, I got this package name and version from the output of the yarn command, which included the following line:

yangster-app-electron: please install @theia/electron@0.7.0-next.e147cf39 as a runtime dependency

[HighCommander4]

We do not update the highlighted tokens when changing the theme.

I can reproduce this as well, investigating.

The reason we're not updating the highlightings when the theme changes, is that we don't reuse CSS class names between different themes, we create new ones, but nothing updates the class names currently used in the editor.

I'll try to revise the patch so that we reuse the class names, as we did before this patch.

[HighCommander4]

The updated patch addresses the issue with changing the theme as well.

I believe that's all the issues raised so far, and the patch is ready to continue to be reviewed.

[kittaakos]

Unfortunately, the keywords are still not highlighted. See my comment here, key has to have a different color if it is a leaf.

Besides that, changing the theme messes up the colors. See the original STRING color for the namespace value:

This contribution looks super cool, let me know if you need me for another review.

[HighCommander4]

@kittaakos Are you sure you were testing with the latest patch here (including patching it into yangster)? It works for me locally, both the leaf key coloring and changing themes.

[HighCommander4]

For convenience, I updated the code to patch yangster in theia-ide/yangster#82.

[kittaakos]

It works great, thank you for your help @HighCommander4 👍🥇

I have verified it on macOS in Chrome with the YANG language.