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

Fix precedence of ui.virtual.whitespace #8733

Closed

Conversation

chtenb
Copy link
Contributor

@chtenb chtenb commented Nov 6, 2023

This fixes the first half of #5675, namely the whitespace case.
The idea is to move the whitespace highlighting logic to the same place as the other highlighting logic, such that it's precedence can be precisely controlled.

This is the place where the other highlighting scopes are also computed,
such that the precedence of each scope can be correctly defined.
@chtenb chtenb changed the title Move whitespace highlighting to EditorView Fix precedence of ui.virtual.whitespace Nov 6, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

I donüt think we should implement it this way. That kind of highlighting is fairly inefficient since it requires collection to a vec (its fine for something rare like the cursor but not something common like whitespace). Even worse it requires a secondary transversal of all visible text. The text transversal is already the rendering bottleneck so doing it twice is not a good idea.

I am still not sure what the best way to handle this would be without causing too much overhead. One way could be to maintain two highlgt iterators. One for sytnax highlighting and one for overlays. The we could have the option to patch highlights before the syntax highlts, after the syntax highlights but before the overlay (like whitespace and indent guides) and then finally after the overlays.

That also should be pretty easy to implement.

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.

2 participants