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

feat(themes): add rulers for themes missing them #10309

Merged
merged 13 commits into from
Apr 18, 2024

Conversation

RoloEdits
Copy link
Contributor

As talked about here, I went through and searched for any ui.virtual.ruler using fg.

rg -iN '\"ui.virtual.ruler\"?\s=?\s\{?\sfg?\s=?\s\"\w+"?\s\}' runtime/themes/

After these changes the above grep yields no results.

Along the way, I addressed other issues brought up in #5721, and added rulers to themes that didn't have one. For those where the ruler was invisible, there is now one that shows up, and for those where the ruler was bright red, it should now match the theme better.

default:
default_ruler

solarized_dark:
solarized_dark_ruler

solarized_light:
solarized_light_ruler

horizon-dark:
horizon-dark_ruler

mellow:
mellow_ruler

poimandres:
poimandres_ruler

poimandres-storm:
poimandres-storm_ruler

varua:
varua_ruler

vim_dark_high_contrast:
vim_dark_high_constrast_ruler

base16_default:
base16_default_ruler

base16_default_dark:
base16_default_dark_ruler

base16_default_light:
base16_default_light_ruler

base16_default_terminal:
base16_default_terminal_ruler

I also took the liberty and normalized the change in #10261 for solzarized_light

Closes: #5721

@RoloEdits
Copy link
Contributor Author

I don't use these themes myself, so unsure how an 8 hour coding session will affect contrast perception for some of these. But at least now there is a config option they can check and modify if its not working.

@pascalkuthe pascalkuthe added A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. labels Apr 9, 2024
@RoloEdits
Copy link
Contributor Author

RoloEdits commented Apr 9, 2024

Also, what seemed to be holding up #3599 was the breaking of fg rulers? With these changes removing all fg rulers, perhaps this could be an opportunity to adopt a different rendering order.(Not any fix in particular, but I know there was a few issues of precedence with the rulers and selections, etc.)

@pascalkuthe
Copy link
Member

Also, what seemed to be holding up #3599 was the breaking of fg rulers? With these changes removing all fg rulers, perhaps this could be an opportunity to adopt a different rendering order.(Not any fix in particular, but I know there was a few issues of precedence with the rulers and selections, etc.)

Yeah if we merge this that could be ok. But I wanted a better solution where rulers get rendered after the base highlights but before overlays like selections. We made some changes to the rendering system since and it's now possible to render in between those two so that would be a better solution.

@the-mikedavis
Copy link
Member

\cc @archseer as this affects the default theme

@the-mikedavis the-mikedavis requested a review from archseer April 9, 2024 14:09
@the-mikedavis the-mikedavis merged commit 34291f0 into helix-editor:master Apr 18, 2024
6 checks passed
@RoloEdits RoloEdits deleted the theme-ruler-normalization branch April 18, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Not all themes display rulers
3 participants