Skip to content

Conversation

kjeremy
Copy link
Contributor

@kjeremy kjeremy commented Feb 25, 2020

No description provided.

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

LGTM!

r=me with nits fxed

};

for event in root.preorder_with_tokens() {
match event {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also add a quick

if element.text_range().intersection(range_to_highlight).is_none() { continue }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where would you like that? On the LSP side the server is allowed to return all highlights for the file if it can't do the range for whatever reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

For every event: if parent happens to be a big node, like an impl, we might process unnecessary children

@matklad
Copy link
Contributor

matklad commented Feb 25, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 25, 2020

@bors bors bot merged commit ae0aeb1 into rust-lang:master Feb 25, 2020
@lnicola
Copy link
Member

lnicola commented Feb 25, 2020

I haven't really followed the semantic highlighting changes. Will we now (after the client changes) have faster highlighting thanks to this?

@kjeremy kjeremy deleted the semantic-ranges branch February 25, 2020 21:27
@kjeremy
Copy link
Contributor Author

kjeremy commented Feb 25, 2020

@lnicola

If I run this branch on insiders with the necessary changes to the client (this uses the textDocument/semantcTokens request) it definitely colors faster than on stable but I'm not sure what accounts for that.

We still use the machinery in syntax_highlighting so at the moment generating tokens will stay the same.

If the client sends textDocument/semanticTokens/range requests for a viewport (this PR) it could potentially be faster since we wouldn't need to generate or transmit tokens for the whole file. There is also the textDocument/semanticTokens/edits request which is supposed to efficiently report deltas between document revisions. I have yet to see either of these requests in the insiders build when we advertise the features.

@matklad
Copy link
Contributor

matklad commented Feb 25, 2020 via email

@lnicola
Copy link
Member

lnicola commented Feb 25, 2020

But I haven't seen a highlighting bug since I disabled the rainbow thingy last year 😆.

@matklad
Copy link
Contributor

matklad commented Feb 25, 2020 via email

@kjeremy
Copy link
Contributor Author

kjeremy commented Feb 25, 2020

@matklad Sooooooo I'm actually seeing that with API as well. We get syntax highlighting but occasionally semantic highlighting (AND decorations... maybe that's a hint?) won't kick in until we edit a file. I think what's happening is that we get a "content modified" error and those requests are not tried again for some reason.

EDIT: Nevermind I think this is because of a panic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants