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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
