Skip to content

v1.1 semantic highlight feature, introduced in specification 3.16.0 #788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 24 commits into from
Closed

v1.1 semantic highlight feature, introduced in specification 3.16.0 #788

wants to merge 24 commits into from

Conversation

@FelipeLema
Copy link

@FelipeLema FelipeLema commented Apr 17, 2021 •
edited
Loading

Continuation of #740 (fixed TODOs, sprinkled some using and lexicographical comparisons)

I originally sent this as a PR to @jlahoda, but they seem to have lost interest? anyway, I'm willing to take care of this until merged to main branch

  • textDocument/semanticTokens/range
  • textDocument/semanticTokens/full
  • textDocument/semanticTokens/full/delta

jlahoda and others added 9 commits December 25, 2020 11:22
@jlahoda
First attempt to include the semantic highlight feature, introduced i…
503ac95 
…n specification 3.16.0.
@jlahoda
Removing unnecessary include.
013b655
@FelipeLema
name types for (trace|read)ability
a4de46b
@FelipeLema
use tuple for lexicographical comparison
ca51f9e
@FelipeLema
handle TODOs, imprve readbility
554833a
@FelipeLema
fix type
c986030
@FelipeLema
fix compilation
661870a
@FelipeLema
textDocument/semanticTokens/range
84a00f6 
- add overhead code for new method
- move code from "semantic tokens for full document" to
  "semantic tokens for range in document"
- add range delimitation to function
- make "full document" use "range that covers all document"
@FelipeLema
fix debug build
700c1ff
@MaskRay MaskRay force-pushed the master branch 9 times, most recently from 94ba2b3 to c018bce Compare May 9, 2021 18:34
@FelipeLema FelipeLema mentioned this pull request May 10, 2021
Closed
@FelipeLema
Copy link
Author

this is ready to be merged (deltas are not mandatory). I've tested this using Emacs+lsp-mode and it seems to work Ok.

I will come back to work in deltas in a few weeks

FelipeLema added 2 commits May 28, 2021 13:22
@FelipeLema
use SemanticTokensWithId internally
ab53802 
- Move SemanticTokens to QueryFile
- Add SemanticTokensId (though still returning SemanticTokens)
- Save latest SemanticTokensId in QueryFile (latest semantic token sent
is associated to a file)
@FelipeLema
indent fix
e702d33
@FelipeLema
Copy link
Author

I just cancelled deltas because I don't see much support for it in the wild, and it looks like quite the work to do.

This PR is ready to be... uhm... pulled.

MaskRay
MaskRay requested changes Jun 6, 2021
View reviewed changes
@FelipeLema
Copy link
Author

re deltas: prabirshrestha/vim-lsp#633 (comment)

@FelipeLema
correct usage of "empty (list) of params"
03324a1 
lsp-mode was complaining that generated json was not properly parsed
@MaskRay
Copy link
Owner

MaskRay commented Jun 23, 2021 •
edited
Loading

The biggest problem for me is that I cannot test this.

Do you know how to test lsp-mode semantic tokens? I dropped the ccls-style semantic highlighting locally in emacs-ccls but cannot observe any fontlock color differences even if I have set (setq lsp-semantic-tokens-mode t).

-- i/ccls.el
+++ w/ccls.el
@@ -162,4 +162,3 @@ DIRECTION can be \"D\", \"L\", \"R\" or \"U\"."
   :notification-handlers
-  (lsp-ht ("$ccls/publishSkippedRanges" #'ccls--publish-skipped-ranges)
-          ("$ccls/publishSemanticHighlight" #'ccls--publish-semantic-highlight))
+  (lsp-ht ("$ccls/publishSkippedRanges" #'ccls--publish-skipped-ranges))
   :initialization-options (lambda () ccls-initialization-options)

Please don't use //!. Just keep the original comments.

The commits should better be squashed.

MaskRay
MaskRay reviewed Jun 23, 2021
View reviewed changes
src/messages/initialize.cc
"macro"
};

const char * SEMANTIC_MODIFIERS[] = {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-format should make it const char *SEMANTIC_MODIFIERS[] =

The preferred style is kSemanticModifiers

src/messages/textDocument_semanticToken.cc
using namespace clang;

namespace {

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single blank line

@MaskRay
Copy link
Owner

MaskRay commented Jun 23, 2021

remove $ccls/publishSemanticHighlight

This is not good. $ccls/publishSemanticHighlight provides rainbow colors which I don't think semantic tokens can do.

@FelipeLema
Copy link
Author

Do you know how to test lsp-mode semantic tokens?
I'm using latest lsp-mode with lsp-semantic-tokens-mode set to t. Do mind that the text properties differ between lsp-semantic-tokens.el and ccls.el differ.

$ccls/publishSemanticHighlight provides rainbow colors which I don't think semantic tokens can do.

will replace with something like $ccls/publishRainbowHighlight

@FelipeLema FelipeLema mentioned this pull request Jun 24, 2021
Closed
@FelipeLema
Copy link
Author

I'm looking to decouple the rainbowing from the ccls Emacs package now.

See discussion after this comment

@FelipeLema
Copy link
Author

can anyone walk me through the "rainbowing process"? any pics / video samples of what end users will see?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MaskRay MaskRay MaskRay requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@FelipeLema @MaskRay @jlahoda