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

highlight_groups now gives option to set new opts while respecting defaults. #171

Merged
merged 2 commits into from
Jul 15, 2023

Conversation

NiamhFerns
Copy link
Contributor

You now need to opt out of respect_default_hightlight_groups if you'd like highlight_group settings to be fully overwritten (even for options you didn't touch).

Previously, setting only one option for a highlight_group would reset the highlight group completely and only apply that option. This is really frustrating if all you want to do is update which options are, for example, italic or bold.

Default rose-pine highlight groups now live in a table that gets updated with the users config before being applied (hence the huge amount of lines updated in theme.lua).

…faults.

You now need to opt out of respect_default_hightlight_groups if you'd like
highlight_group settings to be fully overwritten (even for options you
didn't touch).
@mvllow
Copy link
Member

mvllow commented Jul 13, 2023

Thank you for the PR! I’m happy to merge as is but want to first know your thoughts on having this be more automatic to avoid another option:

If the user overrides a format only option, e.g. italic or bold, merge those. Fg and bg colors would never be merged.

Looking forward to discuss implementation details or if this sounds too magical let me know :)

@NiamhFerns
Copy link
Contributor Author

So if a user were to override a fg/bg/sp colour, that would also reset the format only options?

Say you have a colour group such that by default { fg == "pine", italic == true} and you set it to { italic = false} in your config (only overriding whether it's italic). The actual contents of that colour group after the merge would be { fg == "pine", italic == false}.

Then say you have a colour group such that by default { fg == "pine", italic == true } and you set it to { fg = "foam"} in your config (only overriding the fg colour). The actual contents of that colour group after the merge would be { fg == "foam, italic == nil }.

Am I understanding correctly?

@mvllow
Copy link
Member

mvllow commented Jul 14, 2023

That’s exactly how I imagined it.

Although, if that is too magical, perhaps a key in each colour group could be used for even finer grain control:

Comment = { italic = true, clear = false }

Not in love with the name “clear” but just thinking out loud.

@NiamhFerns
Copy link
Contributor Author

I quite like the idea of having it be a key per group. That way users could say "Yeah for these I want it to be only what I put." but the default behaviour would be to just append/merge them. I don't really mind if it merges or overwrites by default.

Group = { ..., no_defaults = true },

My main concern with the other option is that it's not super explicit. A user may miss that part in the readme or whatever and then there's magical behaviour that doesn't fully make sense. "Why does it only merge sometimes?" type thing.

Implementing shouldn't be an issue. Can do that when I get some free time today. c:

@mvllow
Copy link
Member

mvllow commented Jul 14, 2023

Sounds great, if you don't mind implementing the key per group then let's do that. I think merging options by default will match user's expectations.

Not to bikeshed but more ideas for the option name: inherit or extend (both default to true). Feel free to choose whatever name you like though & thank you for working on this, much appreciated!

@NiamhFerns
Copy link
Contributor Author

There we go, that's been implemented with inherit key. Very good word for it actually, thanks.

Also removed some unneeded comments I left in by accident.

@mvllow mvllow merged commit 76cae45 into rose-pine:main Jul 15, 2023
@mvllow
Copy link
Member

mvllow commented Jul 15, 2023

Thank you :)

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.

None yet

2 participants