Improve presentation of inactive region highlight

[HighCommander4]

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.

[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.

[HighCommander4]

@sam-mccall review ping :)

[HighCommander4]

Shopping around for some other reviewers :)

[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?

[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?

I had the same thought in clangd/clangd#132 (comment). Haven't tried to see if it's possible yet.

[Trass3r]

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.

[rolandbernard]

Hi, I just wanted to chime in and say that I implement this in my fork (rolandbernard@4f2c9f3) 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)

[HighCommander4]

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!

[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?

[rolandbernard]

@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.

[HighCommander4]

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).

[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 :)

[sam-mccall]

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:

1. remove inactive regions from syntax highlighting as a failed experiment.
2. keep the implementation and expose it as a separate LSP extension
3. 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)

[HighCommander4]

My conclusion is we should back up and try again:

1. remove inactive regions from syntax highlighting as a failed experiment.

2. 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)

[sam-mccall]

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!)

[HighCommander4]

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

The two reasons I see as most convincing are:

1. Granularity (line vs. token), including the possibility of whole-line styling for inactive regions.
2. 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).

[HighCommander4]

2. 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 :)

[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?)

[Trass3r]

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')
    });

[lua9520]

Any progress about this pr?@HighCommander4

[HighCommander4]

Fixed a typo in package.json

[HighCommander4]

Thank you for your work on this. While we wait for the review and pull, would you mind sharing pictures of "old presentation" along with how the new options look like?

Sure. Before this patch:

After this patch (background highlight option):

After this patch (opacity option):

[hokein]

Thanks, the implementation looks good in general.

I have a question about the rendering theme, see my comment.

[HighCommander4]

Addressed review comments, except for the one about editorUnresolvedOpacity which I couldn't figure out how to use (details in comment reply)

[HighCommander4]

@hokein, perhaps we could proceed with the current patch for now? If at some point we figure out how to make use of vscode's editorUnnecessaryCode.opacity instead of having our own setting, we can always switch to that in a follow-up change.

[hokein]

The code looks great (sorry for taking it so long). Let's ship it!

[HighCommander4]

Rebased and addressed review comments.

[miguno]

Thanks everyone for completing this!

[khlebnikov]

Thank you for making this happen! I do wonder when is this feature be released? IIUC, this change was merged in May, and the latest release was in April. Is it on the horizon or would you recommend just building the extension from source?

[GitMensch]

and... why should one build from source - is there a "nightly" / "ci" version that may be used?

[khlebnikov]

I don't quite understand. I can use the weekly builds of clangd itself, but this is a change to the VS Code extension and I don't see something similar for the extension. If I'm looking in the wrong place - please let me know.

[HighCommander4]

I sent out a PR to release a new version: #516.

I don't believe we currently have any sort of automatically created preview/snapshot releases for vscode-clangd. I agree that this would be nice to have. I'm not sure that "nightly" is the appropriate frequency, given that the project gets commits much less often than that (there have been a total of 6 commits in 2023 so far). Perhaps it would make sense to have every commit trigger a snapshot release? If someone has the familiarity with Github Actions to write a patch to the yaml files in .github/workflows that accomplishes this, that would be very neat.

[HighCommander4]

I don't believe we currently have any sort of automatically created preview/snapshot releases for vscode-clangd. I agree that this would be nice to have. I'm not sure that "nightly" is the appropriate frequency, given that the project gets commits much less often than that (there have been a total of 6 commits in 2023 so far). Perhaps it would make sense to have every commit trigger a snapshot release? If someone has the familiarity with Github Actions to write a patch to the yaml files in .github/workflows that accomplishes this, that would be very neat.

Filed #517 to keep any further discussion of this in a dedicated place.

[kon72]

Hello @HighCommander4,

The highlighting appears to be ineffective in certain instances, especially on Windows.
I've sent out a fix in #515. Can you please review it?

[menash134]

@HighCommander4 Thanks for the cool work. Could somebody point out the documentation for highlighting the disabled code. Iam using Nvim with clangd, which option needs to be enabled

[HighCommander4]

@HighCommander4 Thanks for the cool work. Could somebody point out the documentation for highlighting the disabled code. Iam using Nvim with clangd, which option needs to be enabled

I've implemented the server-side of the inactive region highlighting in clangd, and the client-side support in the vscode-clangd plugin.

Client-side support in other clients like neovim needs to be implemented separately in the respective client / plugin. I don't know whether anyone has done that for neovim yet.