[PATCH] D67536: [WIP] [clangd] Add support for an inactive regions notification

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D67536#1697696 <https://reviews.llvm.org/D67536#1697696>, @nridge wrote:

> One thing that may be worth considering as well, is that if the client prefers to highlight the text of the line only, it can calculate the length of the line itself. In VSCode for instance, the line lengths are readily available; I imagine other editors are similar since they need that information for many purposes.


So I don't think clients will/should prefer that - for best rendering they should know this is a line highlight.

I think this comes down to how line highlights are represented in the protocol:

- by a separate field: no need to send line length
- by a special token bounds (e.g. [0,0)): no need to send line length
- by a special scope: sending line length is a nice-to-have as it provides graceful degradation for clients that don't understand this extension



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:152
+          // Don't bother computing the offset for the end of the line, just use
+          // zero. The client will treat this highlighting kind specially, and
+          // highlight the entire line visually (i.e. not just to where the text
----------------
hokein wrote:
> sammccall wrote:
> > nridge wrote:
> > > hokein wrote:
> > > > This seems too couple with VSCode client, I would prefer to calculate the range of the line and return to the client.
> > > > 
> > > > Is there any big differences in VSCode between highlighting with the `isWholeLine` and highlighting with the range of the line? 
> > > I took some screenshots to illustrate to difference.
> > > 
> > > Highlighting only to the end of the line of text:
> > > 
> > > {F10158508}
> > > 
> > > Highlighting the whole line:
> > > 
> > > {F10158515}
> > > 
> > > I think the first one looks pretty bad, and is inconsistent with existing practice.
> > > 
> > > Note also that the suggestion is not to special-case the VSCode client specifically; it's to special-case one particular highlighting, which any client can implement.
> > > 
> > > If this special-casing is really unpalatable, we could instead try this suggestion by @sammccall:
> > > 
> > > > Failing that, I'd suggest encoding a list of line-styles on SemanticHighlightingInformation, that should be combined with any tokens on that line.
> > > 
> > > I guess one consideration when evaluating these options is, do we expect to use that "list of line-styles" for anything else in the future? I can't think of anything at the moment, but perhaps there are other uses for it.
> > > 
> > > If not, we could do something slightly simpler, and add a single `isInactive` flag to `SemanticHighlightingInformation`.
> > Three approaches seem feasible here:
> > 1. clients that know about the specific scope can extend it to the whole line. 
> > 2. [0,0) or so indicates "highlight the whole line"
> > 3. use a dedicated property for line styles (vs token styles)
> > 
> > 3 is clearly better than 2 I think, it's more explicit. I don't have a strong opinion of 1 vs 3, but if going with 1 I think it's a good idea to measure the line as Haojian says, so we at least get a basic version of the feature if the client doesn't know about line styles.
> > 
> > > I guess one consideration when evaluating these options is, do we expect to use that "list of line-styles" for anything else in the future? I can't think of anything at the moment
> > Preprocessor directives maybe? (Though these are easy enough for clients to highlight with regex)
> I can't say whether highlighting the line is better than highlighting the range of the line text, but below is the how the inactive TS code is highlighted in VSCode (only the range of text), I personally prefer this style.
> 
> {F10189885}
I think that's an argument for making sure clients clearly distinguish between regular tokens and marking lines: overlapping tokens don't compose well, but we can easily say lines and token styles should compose.

(That particular style is not for me, but it doesn't matter)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67536/new/

https://reviews.llvm.org/D67536