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

Cterm colors #93

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Cterm colors #93

wants to merge 3 commits into from

Conversation

oncomouse
Copy link

I was hoping to generate cterm colors (ctermfg and ctermbg) in a theme I'm working on. This should be everything needed to get it working.

@rktjmp
Copy link
Owner

rktjmp commented Jun 24, 2022

Thanks for the PR!

I am a bit split on this, as we originally intentionally ignored support for cterm* because it added key ambiguity. Now that we support a looser attribute list, more in-line with nvim_set_hl, it feels like the support should be there.

If we add support for ctermfg/bg we should also support the cterm attribute map. It's unclear what the correct behaviour is when provided separate bold = true attributes in this case. Should they be merged into a cterm key in the exporter? How can we know when to include that value and when to not (as most of the time people probably dont want it generating noise, and defaulting it to cterm=NONE is also probably incorrect.)

I think, probably we just say cterm, ctermfg and ctermbg are not "officially supported" but the key-values are passed through as a kindness if the user provides them.

Can you also add:

  • doc updates

    • accepted keys to "direct definition" with the not "officially supported" caveat.
  • test updates

@@ -28,7 +28,8 @@ local function allowed_option_keys()
-- could be any value
"fg", "bg", "sp",
"gui", -- should be a string
"blend", -- integer
-- integers
"blend", "ctermfg", "ctermbg",
Copy link
Owner

Choose a reason for hiding this comment

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

Can you confirm this is only ints? I think they can be "red" etc too?

@ruler501
Copy link
Contributor

The import script should probably be updated to go along with this change as well.

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.

3 participants