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

VHDL highlights.scm improvement #10845

Merged
merged 3 commits into from
Jun 29, 2024

Conversation

Chris44442
Copy link
Contributor

@Chris44442 Chris44442 commented May 28, 2024

Hi, the highlighting for the VHDL language is very basic and is missing a lot of features. My changes fixes that. I'd like to think these are sane defaults and also minimalist.

@teburd

@teburd
Copy link
Contributor

teburd commented May 29, 2024

Ultimately good vhdl highlighting needs more context than what I think a tree sitter grammar can provide. Frankly its a language that could do well with an lsp highlighter imo after looking at this as it requires a deeper understanding of the context than simply grammatical rules around whats valid/invalid. I found no one in the helix matrix chat could help me improve the queries/highlights :( If you manage to do better, by all means do so!

Many things in vhdl are modules but are effectively so common as to be builtins. Treating them as builtins though is wrong.

I compared your changes to helix master, to emacs vhdl major mode. Emacs major mode is still significantly better at highlighting but I think this PR does make things look a bit nicer.

Missing from helix that I can see in comparison to emacs vhdl major mode...

  • bit value highlights ('0', '1', 'Z', etc)
  • builtin function highlights
  • function highlights
  • differentiable process and entity name highlights

Emacs vhdl major mode left side, helix with your changes right side show neorv32_fifo.vhd (stnolting's sweet open source riscv core)

Screenshot from 2024-05-29 08-15-06

@Chris44442
Copy link
Contributor Author

Chris44442 commented May 29, 2024

My guess is that we will not have an LSP based VHDL highlighting in the near future. For now we will have to use tree-sitter-vhdl.

There are of course a few problems. The main one being that the maintainer has disappeared years ago. Second the "function_call" problem, which partly the VHDL language is to blame, and what you said. The words I filed under function.builtin are a workaround, because I can't fix the root problem in the parser. Thirdly there is the error highlighting, which should not be done by a tree-sitter parser as most agree. And lastly there are some problems in the code and also with the "offical" highlighting.scm file. This is why nvim-treesitter has rejected tree-sitter-vhdl so far (a parser rewrite is being discussed right now). Either way: Helix uses it.

In your screenshot you are using the default theme with no customization. Tbh I am not sure why so many constructs highlighting are missing from the themes by default. Anyway you need to set operator, keyword.operator, number, character, string and maybe one or two more in the theme settings. Like so:

character = { fg = "light-green" }
number = { fg = "red" }
operator = { fg = "green" }

I tried to mimic the Verilog highlights.scm from the official tree-sitter repo. Due to the issues that I mentioned above there was only so much I could do. Not all of the functions are highlighted, with no way of fixing it in the .scm file sadly. This is obviously subjective but imho the highlighting looks a lot better with the PR than without it. With my theme settings this is the comparison between the current highlights.scm (top) and my PR (bottom). Imho there is not too much difference from my PR to the Emacs screenshot.

Screenshot_20240529_202026

Screenshot_20240529_201924

@teburd
Copy link
Contributor

teburd commented May 29, 2024

Looks nice, now why is the default theme so meh then, send another PR :)

@Chris44442
Copy link
Contributor Author

Chris44442 commented May 29, 2024

Looks nice, now why is the default theme so meh then, send another PR :)

Mh yeah I am a bit hesitant because it literally concerns all themes. At first I thought it was a deliberate choice.

@the-mikedavis the-mikedavis added the A-language-support Area: Support for programming/text languages label Jun 3, 2024
runtime/queries/vhdl/highlights.scm Outdated Show resolved Hide resolved
runtime/queries/vhdl/highlights.scm Outdated Show resolved Hide resolved
runtime/queries/vhdl/highlights.scm Outdated Show resolved Hide resolved
runtime/queries/vhdl/highlights.scm Outdated Show resolved Hide resolved
runtime/queries/vhdl/highlights.scm Outdated Show resolved Hide resolved
runtime/queries/vhdl/highlights.scm Outdated Show resolved Hide resolved
runtime/queries/vhdl/highlights.scm Outdated Show resolved Hide resolved
@Chris44442
Copy link
Contributor Author

@archseer

I have addressed all the requested changes. Can we merge?

@archseer archseer merged commit c6dbb9c into helix-editor:master Jun 29, 2024
6 checks passed
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
mxxntype pushed a commit to mxxntype/helix that referenced this pull request Aug 14, 2024
kyruzic pushed a commit to kyruzic/helix that referenced this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants