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! render highlights over rulers #3599

Conversation

AlexanderBrevig
Copy link
Contributor

@AlexanderBrevig AlexanderBrevig commented Aug 29, 2022

Bug pointed out here: #3587 (comment)

This fix is so easy I'm thinking I may have missed something?
EDIT: Ref ChrHorn below; themes that use ui.ruler.fg will not work anymore.

@AlexanderBrevig AlexanderBrevig mentioned this pull request Aug 29, 2022
22 tasks
@ChrHorn
Copy link
Contributor

ChrHorn commented Aug 29, 2022

Was about to do the same.

With this fix we loose the ability to set fg for the ruler. Although currently only the solarized themes and base16_transparent use it explicitly. Other themes might only set ui.virtual.fg so they also would have to be changed, this includes the base theme.

@AlexanderBrevig
Copy link
Contributor Author

Was about to do the same.

With this fix we loose the ability to set fg for the ruler. Although currently only the solarized themes and base16_transparent use it explicitly. Other themes might only set ui.virtual.fg so they also would have to be changed, this includes the base theme.

Spot on!
This is a regression for themes that use fg for rulers.
If we want to support it I guess the cleanest option is to extract cursor from general highlights, and draw it last?

@AlexanderBrevig AlexanderBrevig changed the title fix: render highlights over rulers fix! render highlights over rulers Aug 31, 2022
@kirawi kirawi added A-helix-term Area: Helix term improvements A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 13, 2022
@luetage
Copy link
Contributor

luetage commented Jan 18, 2023

I stumbled upon the same thing earlier and found the same flawed fix. One thing to note is that with the current code, themes which set the primary cursor color modifiers to »reversed« are not affected and don’t need the fix. But the majority of themes does. I wonder whether it’s even worth supporting a foreground color for rulers, as mentioned few themes use it and traditionally a colorcolumn/ruler is just that, a change in background color. Might be easier to just fix the few themes using it than to work around it. Another thing is that cursorline and cursorcolumn “disappear” behind the ruler, don’t know whether that’s on purpose and what was intended originally.

@joshbainbridge
Copy link
Contributor

My two cents, this PR might well fix more themes than it breaks. Currently the character under the curser disappears entirely, or becomes hard to read, under many themes (see for example gruvbox). This is because when just the ruler background is set, it can often match or be very close to the cursor foreground colour.

@luetage
Copy link
Contributor

luetage commented Mar 12, 2023

@joshbainbridge This should just be merged. It’s been 7 months now and there is no reason this bug should endure.

@pascalkuthe
Copy link
Member

clsoing this PR as stale, the rendering code has been entirely replaced. The text rendingr has been split into base and overlay highlights. The rulers should be rendered after the base highlights but before the overlay highlights.

Thank you for contributing!

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 A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants