vscode-clangd Improve presentation of inactive region highlight

This patch intercepts the semantic tokens requests using middleware, extracts the inactive code ranges, and uses setDecorations() to provide whole-line background highlighting for them.

May 25 '21 07:05 HighCommander4

@sam-mccall I wonder what you think of this patch.

On the one hand, it may seem like a lot of effort for a small cosmetic improvement. (But I've already done the effort :-) )

On the other hand, I believe visualizing inactive code with a background highlight is a common C++ IDE trope that may be worth emulating. This patch is also a good building block for the future enhancement described here (basically, to omit the semantic foreground color for inactive tokens altogether, such that the client-side highlightings (e.g. bold keywords) are retained and combined with the semantic background in inactive code).

The approach taken in this patch to implement the background highlight is in line with Microsoft's suggestion to use editor decorations for this purpose until a more specific API greared towards inactive code is added.

Aug 16 '21 06:08 HighCommander4

@sam-mccall review ping :)

Sep 13 '21 07:09 HighCommander4

Shopping around for some other reviewers :)

Oct 04 '21 06:10 HighCommander4

Seems to work. Still relies on the comment semantic token type, which seems to override the default syntax highlighting. Is it perhaps technically possible to remove the semantic token after processing it so you get a faded region with basic syntax highlighting?

Dec 20 '21 16:12 Trass3r

Seems to work. Still relies on the comment semantic token type, which seems to override the default syntax highlighting. Is it perhaps technically possible to remove the semantic token after processing it so you get a faded region with basic syntax highlighting?

I had the same thought in https://github.com/clangd/clangd/issues/132#issuecomment-864222394. Haven't tried to see if it's possible yet.

Dec 20 '21 20:12 HighCommander4

The thing is that if you don't specify color in createTextEditorDecorationType nothing gets applied and the code is colored green. I think there was a vscode issue about that.

Dec 20 '21 22:12 Trass3r

Hi, I just wanted to chime in and say that I implement this in my fork (https://github.com/rolandbernard/vscode-clangd/commit/4f2c9f3f7aa65f129b7e7b424719a7c4f4eec548) because it bugged me too much. I wanted it to look like in the official Microsoft C++ extension, i.e., the region only has a lower opacity and still uses the default syntax highlighting. So I replace all the comment semantic tokens before returning them.

(This is how it looks with my changes)

Feb 13 '22 22:02 rolandbernard

Hi, I just wanted to chime in and say that I implement this in my fork (rolandbernard@4f2c9f3)

This is really neat!

It's the same high-level idea that I was after: combine a presentational style indicating "inactive", with client-side (TextMate) token colors. The only difference is the choice of presentation style for "inactive" (in my case it's a grey background, in yours it's lower opacity). Anyways, I believe your patch demonstrates that this sort of combination is possible!

Feb 14 '22 07:02 HighCommander4

@rolandbernard I looked through your patch. One thing I'm curious about is replaceInactiveRanges(): is the purpose of changing the token type to inactiveCodeTokenTypeReplaceIndex that we know that vscode will not have any style associated with that token type, so it'll just ignore it? And it's simpler to overwrite the token type with this no-op token type, than to erase the token from the encoded data altogether?

Feb 14 '22 07:02 HighCommander4

@rolandbernard I looked through your patch. One thing I'm curious about is replaceInactiveRanges(): is the purpose of changing the token type to inactiveCodeTokenTypeReplaceIndex that we know that vscode will not have any style associated with that token type, so it'll just ignore it? And it's simpler to overwrite the token type with this no-op token type, than to erase the token from the encoded data altogether?

Yes, I choose inactiveCodeTokenTypeReplaceIndex to be outside the range of token types that are in capabilities.semanticTokensProvider.legend.tokenTypes. It seems vscode ignores them, but I have no idea if that is something one can or should rely on. I think it would also be possible to remove the tokens from the data altogether, but this was, as you say, simpler to implement.

Feb 14 '22 09:02 rolandbernard

Yes, I choose inactiveCodeTokenTypeReplaceIndex to be outside the range of token types that are in capabilities.semanticTokensProvider.legend.tokenTypes. It seems vscode ignores them, but I have no idea if that is something one can or should rely on. I think it would also be possible to remove the tokens from the data altogether, but this was, as you say, simpler to implement.

Thanks, that makes sense.

The other trick in your implementation that hadn't occurred to me was that the provideDocumentSemanticTokensEdits middleware is allowed to pass a full set of tokens (not a delta) on to vscode (I thought I'd have to compute a modified delta that would apply to the modified tokens, which seemed involved).

Feb 15 '22 07:02 HighCommander4

@rolandbernard I updated the PR to make use of the techniques from your patch to preserve the client-side coloring, hope that's cool (I added a comment to credit you).

The styling in my patch currently remains a background highlight, but it's very easy to change to opacity if that's preferred, I don't really feel strongly about it.

Now, we just need someone kind enough to review and consider merging this improvement :)

Feb 15 '22 08:02 HighCommander4

I'm sorry this has taken a long time. I bounced off this many times because IMO neither "yes" nor "no" are good answers...

The behavior looks great (especially preserving the client-side highlights). And our current behavior (coloring disabled code as comments) seems to be worse than nothing based on bug reports.

On the other hand, the design we've landed on (in general, not this patch) isn't good.

client would be much simpler if inactive regions were separate from tokens. (The implementation is impressive but scary).
server would also be simpler
client and server would be less coupled
MS wants these separate in the protocol
practical effect of mixing them (coloring as comments) is undesirable
this design can't accommodate if constexpr properly

My conclusion is we should back up and try again:

remove inactive regions from syntax highlighting as a failed experiment.
keep the implementation and expose it as a separate LSP extension
add client-side support using exactly the techniques here, but without the complicated demuxing

WDYT?

(I'm happy to put together a patch for 1+2 if that's helpful, there are some questions about such an LSP extension to work out)

Feb 18 '22 10:02 sam-mccall

My conclusion is we should back up and try again:

remove inactive regions from syntax highlighting as a failed experiment.

keep the implementation and expose it as a separate LSP extension

Square one, it's nice to see you again :)

This is the very first approach I took for inactive regions (modulo the "combining client-side tokens" part), in Sept 2019:

Server-side patch (looking at the first version of the diff in particular, as later versions reformulate it on top of semantic highlighting) Client-side patch (likewise)

Feb 18 '22 18:02 HighCommander4

Wow, I'd forgotten. That's excruciating to read, I'm sorry.

Still, s/a mistake/my mistake/, how do you think we should proceed?

(If you prefer to go ahead with this version, that seems reasonable. I'd love to know whether you think it's not a mistake vs not worth fixing vs too frustrating to touch anymore would be useful!)

Feb 18 '22 19:02 sam-mccall

I agree that conceptually, a separate protocol for inactive regions makes more sense.

The two reasons I see as most convincing are:

Granularity (line vs. token), including the possibility of whole-line styling for inactive regions.
The usefulness of combining the inactive style with token colors. (Maybe, in the distant future when we can get clangd to build an AST for all preprocessor branches, even with semantic token colors.)

So, I think a reasonable way forward would be to resurrect the patches which add a new protocol, rebase them, and tweak them to preserve the client-side TextMate tokens.

I'm happy to do that (probably after finishing up a couple of other in-progress things, so if you'd like to beat me to it, please feel free).

Feb 18 '22 22:02 HighCommander4

The usefulness of combining the inactive style with token colors. (Maybe, in the distant future when we can get clangd to build an AST for all preprocessor branches, even with semantic token colors.)

Actually, maybe even in the not-so-distant future by sending pseudo-parser based heuristic semantic tokens for code inside inactive regions :)

Feb 18 '22 22:02 HighCommander4

I guess, for completeness, it's worth noting that a downside of the dedicated protocol is that we lose the ability of other clients to have some styling of inactive code without implementing the new protocol. (Perhaps we could mitigate that by still sending the comment semantic tokens if the client doesn't indicate capability for the new protocol?)

Feb 19 '22 08:02 HighCommander4

The styling in my patch currently remains a background highlight, but it's very easy to change to opacity if that's preferred, I don't really feel strongly about it.

The opacity really makes the difference in making it clear the code is inactive. But actually combining it with the background is even better.

    vscode.window.createTextEditorDecorationType({
      isWholeLine: true,
      opacity: config.get<number>('clangd.inactiveRegions.opacity').toString(),
      backgroundColor:
          new vscode.ThemeColor('clangd.inactiveRegions.background')
    });

Feb 24 '22 23:02 Trass3r

Any progress about this pr?@HighCommander4

Jun 25 '22 17:06 lua9520

Any progress about this pr?@HighCommander4

It's on my list to implement the approach described in this comment but I haven't gotten to it yet. Hope to get to it in the coming weeks, or if someone would like to take it over, please feel free.

Jun 25 '22 18:06 HighCommander4

Would really like to see this implemented, or at least an option to disable the comment semantic on inactive regions.

We have many build configs for the same source code base and all variants needs to be maintained, but the majority appear as unreadable comments, effectively making it impossible to use this plugin with the current behaviour 😢

I definitely wouldn't mind a pragmatic approach for now, and then a more architecturally clean approach later. I don't know how to apply the patches above to an actual vscode install that is running this extension.

Jul 26 '22 09:07 thomasthorsen

Sorry folks, I haven't gotten to this, and I'm not likely to for several more weeks as I'll be travelling.

If someone would like to take this over, you're very welcome to.

If someone would like to pursue a quicker/easier fix, one that could be considered to add an option to the server to not send inactive region tokens altogether (or to the client to ignore them?)

Jul 27 '22 03:07 HighCommander4

@thomasthorsen Here are steps to apply the patch:

Following from https://code.visualstudio.com/api/working-with-extensions/publishing-extension#installation, download/install Node.js then run npm install -g vsce. Clone this repo, checkout the latest tag, and apply the patch given by @rolandbernard. I also had to run npm install esbuild and npm install before creating the package with vsce package.

I had to add the line getState(): vscodelc.FeatureState { return {kind: 'static'}; } right above the line with dispose in it in inactive-regions.ts for it to install correctly due to updates since the patch was created.

Now you can copy the extension that ends in .vsix to a remote server and use the Install from VSIX... command to install it.

Aug 29 '22 15:08 jk000

Hoping for resolved conflicts and possible integration.

Dec 06 '22 23:12 GitMensch

Would really like to see this implemented, or at least an option to disable the comment semantic on inactive regions. (https://github.com/clangd/vscode-clangd/pull/193#issuecomment-1195225221)

+1

I, too, have run into the problem of not being able to disable semantic highlighting via clangd for inactive regions, for example, when writing .h header files that define short-named aliases for long-named functions:

/* myheader.h */

extern int my_super_long_function(int a, int b);

#ifdef MY_SHORT_NAMES
#define short_fn my_super_long_function
#endif

Today, the last three lines (ifdef to endif) are always grayed out by clangd as if they were comments, which makes maintaining such code sections quite cumbersome:

Generally speaking, I do like the graying-out of inactive regions. But there are some situations like the one I described above in which the current behavior is inconvenient.