Skip to content
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.

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

feat(semantic_tokens): link readonly variables to Constant #331

Closed
wants to merge 1 commit into from

Conversation

amaanq
Copy link
Contributor

@amaanq amaanq commented Mar 8, 2023

Forgot one :)

@folke
Copy link
Owner

folke commented Mar 8, 2023

Can you use @constant instead?

And probably better to drop @lsp.mod.readonly?

@folke
Copy link
Owner

folke commented Mar 8, 2023

hmm, I'm not actually sure about your suggested change. Can you show me a before/after of what you are trying to fix?

In typescript for example ReadOnly<string> is not the same as a constant

@amaanq
Copy link
Contributor Author

amaanq commented Mar 8, 2023

Can you use @constant instead?

Sure

And probably better to drop @lsp.mod.readonly?

Tbh, I'm not super familiar with the new semantic_tokens but I found a gist that seems to give me a rough idea, agreeing with what you said:

Then, if you want everything which is read-only to be italic:

hi @lsp.mod.readonly gui=italic
If you only want parameters which are read-only to be italic:

hi @lsp.typemod.parameter.readonly gui=italic

gist is here: https://gist.github.com/swarn/fb37d9eefe1bc616c2a7e476c0bc0316

So, I agree with removing the @lsp.mod.readonly as that seems to indicate any identifiers with modifiers that are readonly

The problem I want to fix (before and after):
image
image

I applied your suggestions now, let me know how ReadOnly<string> looks after that

@folke folke closed this in 8da3aab Mar 8, 2023
@folke
Copy link
Owner

folke commented Mar 8, 2023

I see what you mean. But the actual bug is actualy that the @variable doesn't have a proper fg. Check the Inspect output below:
image

Fixed it by setting the fg prop for @variable. Won't be orange now, but white as it should be. Otherwise emost things would have been orange in typescript :)

@amaanq
Copy link
Contributor Author

amaanq commented Mar 8, 2023

Wow! That fixed another bug I always wondered about but didn't bother to look into:

image

Before your fix, variables would just be highlighted to whatever was the capture above it if any (white by default usually as it wouldn't be inside a capture, just a node). So before, var would be green like the string, which I didn't notice happen in other themes.

However...

image

Builtin vars such as console in js lose their highlights...tragically

Highlighting @lsp.typemod.variable.defaultLibrary to @variable.builtin should work :)

@folke
Copy link
Owner

folke commented Mar 8, 2023

image

console doesn't seem to get the proper semantic tokens it seems

@folke
Copy link
Owner

folke commented Mar 8, 2023

I had to do my earlier change a bit different, since it broke in some places.

I now simply define @lsp.type.variable as an empty hl, so it will use the treesitter highlights instead

@folke
Copy link
Owner

folke commented Mar 8, 2023

console highlighting (treesitter) is now back, but so is that initial thing you wanted to fix.

tbh, I kinda like how that highlights. The const declaration is shown as a constant and using it variable shows it as variable. It's actually better that it's two colors. Makes it easier to read.

@amaanq
Copy link
Contributor Author

amaanq commented Mar 8, 2023

console highlighting (treesitter) is now back, but so is that initial thing you wanted to fix.

tbh, I kinda like how that highlights. The const declaration is shown as a constant and using it variable shows it as variable. It's actually better that it's two colors. Makes it easier to read.

In fact, I'd like your opinion here then.. as I introduced that separate highlighting change (I am not a big JS/TS user)

nvim-treesitter/nvim-treesitter#4466

riley-martine pushed a commit to riley-martine/tokyonight.nvim that referenced this pull request Dec 7, 2023
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.

None yet

2 participants