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

incorrect colors on keys/variables #189

Open
drishal opened this issue Sep 2, 2024 · 12 comments
Open

incorrect colors on keys/variables #189

drishal opened this issue Sep 2, 2024 · 12 comments

Comments

@drishal
Copy link

drishal commented Sep 2, 2024

as per the catppuccin docs, the keys/properties are supposed to be blue right?
for eg on nix for vscode, we get the blue color:
image

however on emacs we get normal white color like this:
image

as per catppuccin style guide it should be blue
image

another example with json:
vscode:
image

Emacs:
image

infact even the brackets dont seem to be themed correctly

I already have the ts mode enabled for both as well

One thing I am confused with,is the ts-modes somehow causing issues?

@drishal
Copy link
Author

drishal commented Sep 2, 2024

so interestingly, after a bit of trying to fiddle with the theme myself it seems as though setting
font-lock-variable-name-face :foreground ,ctp-blue I can finally get the keys to be blue, but then it ends up themeing all kinds of variables to blue, not just keys

@jtbx
Copy link
Member

jtbx commented Sep 2, 2024

Hey thanks for reporting this! The face used for colouring the attributes in nix-ts-mode is font-lock-variable-name-face. This is a tricky decision because if we were to set :foreground ,ctp-blue on font-lock-variable-face it would make all variable definitions blue in all languages. That might work for expression languages like Nix but for other languages it can be very disconcerting seeing that much colour all the time. To be clear this is not an issue with treesitter major modes.

Here's an example of some C code with :foreground ,ctp-blue on font-lock-variable-name-face:

ball

There are two issues here: first, being that the Catppuccin theme does not style property faces (font-lock-property-name-face inherits from font-lock-variable-name-face); second, being the abuse of faces by modes. Faces such as font-lock-property-name-face exist, but in modes such as nix-ts-mode font-lock-variable-name-face is used instead. Modes should use the designated face where possible, and when they don't things can quite easily look oversaturated or have an unintended colour.

Essentially on our side we should patch in support for font-lock-property-name-face and font-lock-property-use-face and modes such as nix-ts-mode should be patched to use these faces where there is too much or not enough colour.

@drishal
Copy link
Author

drishal commented Sep 3, 2024

Hey thanks for reporting this! The face used for colouring the attributes in nix-ts-mode is font-lock-variable-name-face. This is a tricky decision because if we were to set :foreground ,ctp-blue on font-lock-variable-face it would make all variable definitions blue in all languages. That might work for expression languages like Nix but for other languages it can be very disconcerting seeing that much colour all the time. To be clear this is not an issue with treesitter major modes.

Here's an example of some C code with :foreground ,ctp-blue on font-lock-variable-name-face:

ball

There are two issues here: first, being that the Catppuccin theme does not style property faces (font-lock-property-name-face inherits from font-lock-variable-name-face); second, being the abuse of faces by modes. Faces such as font-lock-property-name-face exist, but in modes such as nix-ts-mode font-lock-variable-name-face is used instead. Modes should use the designated face where possible, and when they don't things can quite easily look oversaturated or have an unintended colour.

Essentially on our side we should patch in support for font-lock-property-name-face and font-lock-property-use-face and modes such as nix-ts-mode should be patched to use these faces where there is too much or not enough colour.

yes, I also noticed this thing with a lot of other ts modes as well, observed this in both json ts mode and also toml ts mode as well
however interestingly if I switch to js-json-mode instead of the treesitter one keys are correctly themed to ctp-blue
but in case of toml mode both normal and ts mode are themed incorrectly with ctp-text instead 🤔

@drishal
Copy link
Author

drishal commented Sep 3, 2024

also yes I do agree that having :foreground ,ctp-blue is probably a bad idea for other languages

@jtbx
Copy link
Member

jtbx commented Sep 3, 2024

yes, I also noticed this thing with a lot of other ts modes as well, observed this in both json ts mode and also toml ts mode as well however interestingly if I switch to js-json-mode instead of the treesitter one keys are correctly themed to ctp-blue but in case of toml mode both normal and ts mode are themed incorrectly with ctp-text instead 🤔

I'd have to look into what faces those modes are using, it's especially difficult when different modes use faces for different things

@gs-101
Copy link
Contributor

gs-101 commented Dec 9, 2024

Essentially on our side we should patch in support for font-lock-property-name-face and font-lock-property-use-face [...]

Hello, how's is progress on this? I just saw this issue and I think I'll open up a PR on nix-ts-mode. I'll set font-lock-property-name-face to some random value first to see if it looks right.

Nix

On Emacs:

scratch 09:58 AM 09-12-2024

On Kate:

Untitled * — Kate 10:07 AM 09-12-2024

(It's rosewater pink?)

JSON

On Emacs:

scratch 10:00 AM 09-12-2024

On Kate:

Untitled * — Kate 10:04 AM 09-12-2024

@drishal
Copy link
Author

drishal commented Dec 10, 2024

Essentially on our side we should patch in support for font-lock-property-name-face and font-lock-property-use-face [...]

Hello, how's is progress on this? I just saw this issue and I think I'll open up a PR on nix-ts-mode. I'll set font-lock-property-name-face to some random value first to see if it looks right.

Nix

On Emacs:

On Kate:

JSON

On Emacs:

scratch 10:00 AM 09-12-2024

On Kate:

Untitled * — Kate 10:04 AM 09-12-2024

Hmm interesting, in emacs the keys are red 🤔

@gs-101
Copy link
Contributor

gs-101 commented Dec 10, 2024

Hmm interesting, in emacs the keys are red 🤔

I just set it to red for a quick demonstration of fontification (setting a face to red in Emacs just requires a quick set-face-attribute 'FACE nil :foreground "red"), if I were to make a change I'd set it to blue like it is suggested in the style guide.

It looks perfect for JSON already, because they already use font-lock-property-face. For Nix I think I'd need another editor, because in Kate it's incorrect.

@gs-101
Copy link
Contributor

gs-101 commented Dec 10, 2024

Emacs vs. VSCodium

08:35 AM 10-12-2024

For Nix, there would still be a long way to go (although, this comparison might be inaccurate, since VSCode doesn't use tree-sitter, let me know if VSCodium in my screenshot is also incorrect).

@drishal
Copy link
Author

drishal commented Dec 12, 2024

Emacs vs. VSCodium

For Nix, there would still be a long way to go (although, this comparison might be inaccurate, since VSCode doesn't use tree-sitter, let me know if VSCodium in my screenshot is also incorrect).

that seems to look pretty close

edit: Also I wonder, would it be possible to theme brackets as well

@gs-101
Copy link
Contributor

gs-101 commented Dec 12, 2024

edit: Also I wonder, would it be possible to theme brackets as well

If it's defined in the grammar, then yes (I saw entries for parentheses and curly brackets), but it would require some time to get it right, as I'd need to do a more thorough reading of the grammar.

EDIT: the brackets seem to be colored based on depth, so you can just use rainbow-delimiters instead.

@drishal
Copy link
Author

drishal commented Dec 13, 2024

edit: Also I wonder, would it be possible to theme brackets as well

If it's defined in the grammar, then yes (I saw entries for parentheses and curly brackets), but it would require some time to get it right, as I'd need to do a more thorough reading of the grammar.

EDIT: the brackets seem to be colured based on depth, so you can just use rainbow-delimiters instead.

yes rainbow-delimiters work for now

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

No branches or pull requests

3 participants