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

Syntax highlighting in file picker for unopened files #609

Closed
heliostatic opened this issue Aug 18, 2021 · 9 comments
Closed

Syntax highlighting in file picker for unopened files #609

heliostatic opened this issue Aug 18, 2021 · 9 comments
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-help-wanted Call for participation: Extra attention is needed E-medium Call for participation: Experience needed to fix: Medium / intermediate

Comments

@heliostatic
Copy link
Contributor

I assume that because the syntax highlighting happens via tree-sitter, files that have never been opened don't have syntax data/an ast to highlight in the file picker? In any case, it's sort of weird in practice--although I couldn't find any issues, open or closed, about it, so perhaps I have something configured wrong?

CleanShot.2021-08-17.at.21.10.59.-.01.mp4
@kirawi
Copy link
Member

kirawi commented Aug 18, 2021

The file picker preview is a new addition, and for now it doesn't do highlighting on other files for performance reasons, I believe.

@heliostatic
Copy link
Contributor Author

The file picker preview is a new addition, and for now it doesn't do highlighting on other files for performance reasons, I believe.

Thanks, read the linked PR, makes sense. I'll keep an eye on any PRs in that code area.

@pickfire pickfire added C-enhancement Category: Improvements A-helix-term Area: Helix term improvements E-hard Call for participation: Experience needed to fix: Hard / a lot labels Aug 18, 2021
@pickfire
Copy link
Contributor

pickfire commented Aug 18, 2021

Since we don't have an issue for this I think we can keep this open.

The performance issue is due to syntax highlighting is currently synchronous which will be slow when we are using the file picker. The implementation that we think would be better is first show the version without syntax highlighting and at the same time getting syntax highlighting for the file asynchronously. I think it requires some changes to make the picker run async stuff which is non-trivial so the original PR removed the syntax highlighting feature.

@heliostatic heliostatic reopened this Aug 18, 2021
@heliostatic
Copy link
Contributor Author

Gitui has a nice approach to async syntax highlighting: https://github.com/extrawurst/gitui/blob/master/src/ui/syntax_text.rs

@kirawi kirawi added E-medium Call for participation: Experience needed to fix: Medium / intermediate E-help-wanted Call for participation: Extra attention is needed and removed E-hard Call for participation: Experience needed to fix: Hard / a lot labels Oct 10, 2021
@lykahb
Copy link
Contributor

lykahb commented Sep 6, 2022

Would tree-sitter performance be a problem if helix passes it a constant-size chunk of file, comparable to the size of a preview?

A few experiments with the tree-sitter playground show that it returns a tree rich enough to show highlighting. It seems to work better on the chunks from the beginning of the file - the use case for file picker. But it is usually good enough when parsing a chunk from the middle - the use case for global search.

A few differences from the regular output of tree-sitter:

  • some sub-trees may be under the ERROR nodes
  • for expressions where either opening or closing element is missing, it may be classified in a wrong way (e.g, array as sequence_expression or function_declaration as expression_statement with function).

So far I haven't seen the tree output that may make highlighting misleading - usually the wrong named node would be similar in meaning, or a chunk of text would have no named node (and so no highlighting) at all.

@the-mikedavis
Copy link
Member

Passing part of the file would reduce the time taken to parse the syntax tree but query analysis can also take quite some time for languages like Elixir or Swift (it's a sub-secord but noticeable hang).

I'm very skeptical that passing only a constant-size chunk works acceptably well across languages though. I would find incorrect highlights more distracting than no highlights

@archseer
Copy link
Member

a85e386

@pickfire
Copy link
Contributor

I noticed it just highlight the previews once then forgets, should be cache the result so that we don't have to highlight again?

@gabydd
Copy link
Member

gabydd commented Dec 18, 2022

should this be closed(trying to clean out some old issues)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-help-wanted Call for participation: Extra attention is needed E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
Development

No branches or pull requests

7 participants