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: apply light mode color to icon overrides #374

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ribru17
Copy link
Contributor

@ribru17 ribru17 commented Jan 6, 2024

This PR makes it so that icon overrides automatically adjust to their light-mode color counterparts upon background change. It also fixes a bug that I found where override highlights would still be reset to white if only the cterm_color value was present.

It does this using the help of the color darkening function from the scripts file (which unfortunately cannot be imported directly as it is not in the lua directory) and a gist documenting conversions, which I linked in a comment.

This commit also fixes an issue where override colors would be reset if
only the cterm_color value was present.
@ribru17 ribru17 changed the title Light mode overrides feat: apply light mode color to icon overrides Jan 6, 2024
@alex-courtis
Copy link
Member

This is a fantastic change @ribru17 however this adds too much runtime complexity. We explicitly decided to do any such calculations at compile time.

Stepping back and looking at the problem:

  1. Only one override may be present
  2. Override applies to dark and light mode
  3. Override highlights cterm bug

We could resolve 1 and 2 by providing a mechanism to specify multiple overrides: dark and light. The user can pre-determine the colours they desire.

We can't change the API however we can add. Perhaps a light boolean, default false, in the table for each override.

@ribru17
Copy link
Contributor Author

ribru17 commented Jan 7, 2024

Sounds good, this is a better way to do it. I have a question: would a value of light = false default to applying to both light and dark modes (for backwards compatibility) while a value of light = true means it only applies to light mode (presumably after the dark mode version was applied, to override it)?

@alex-courtis
Copy link
Member

Sounds good, this is a better way to do it. I have a question: would a value of light = false default to applying to both light and dark modes (for backwards compatibility) while a value of light = true means it only applies to light mode (presumably after the dark mode version was applied, to override it)?

Yes, that's a bit confusing.

How about an enum: mode = "light" and mode = "dark" ?

@ribru17
Copy link
Contributor Author

ribru17 commented Jan 7, 2024

Cool! And if not present, default to both I assume? Trying to make sure I understand fully before amending the PR

@alex-courtis
Copy link
Member

alex-courtis commented Jan 7, 2024

Cool! And if not present, default to both I assume? Trying to make sure I understand fully before amending the PR

Sorry, must remain compatible. Missing is dark only.

Edit: You're right. That is current behaviour.

@ribru17
Copy link
Contributor Author

ribru17 commented Jan 7, 2024

I'm realizing an issue: say I want to override_by_extension a scm file. With this proposed approach I basically need two values mapped to the same key (['scm']), one with my mode = 'light' configuration and one for dark. Without changing the API I think this cannot work. Perhaps we can add a suffix like _light to all of the override types and then conditionally apply these overrides only in light mode? Or is there another way to get the previous version to work do you think?

Note: If we go with the second way, I think it would be sufficient to add just the _light suffix (e.g. override_by_extension_light), since omitting it will work as always, and if we only want a light mode override we can specify it only in the suffixed version. If we only want a dark mode one, we can use the non-suffixed version and then have a blank one in the light mode suffixed version. Unless you think it is still better to also add a dark mode suffix.

@alex-courtis
Copy link
Member

With this proposed approach I basically need two values mapped to the same key (['scm']), one with my mode = 'light' configuration and one for dark.

Yes, there's no way around storing two overrides.

Perhaps we can add a suffix like _light to all of the override types and then conditionally apply these overrides only in light mode? Or is there another way to get the previous version to work do you think?

If we go with the second way, I think it would be sufficient to add just the _light suffix

I think you're right, it is far more intuitive and clear than a mode value. Light versions of API methods and _light entries in the setup table will be easier to use as they are more copy/pastable.

@gegoune
Copy link
Collaborator

gegoune commented Jan 7, 2024

Could we maybe make color key also able to take a table? If table first value would be what it is now and second would be a value for other mode. If single string just continue with current somewhat broken experience. (Not at my pc to check code in more detail.)

@gegoune
Copy link
Collaborator

gegoune commented Jan 7, 2024

More so, we could even merge two large tables internally and use two colours side by side.

@ribru17
Copy link
Contributor Author

ribru17 commented Jan 7, 2024

I like this idea a lot

@ribru17
Copy link
Contributor Author

ribru17 commented Jan 11, 2024

Could we maybe make color key also able to take a table? If table first value would be what it is now and second would be a value for other mode. If single string just continue with current somewhat broken experience. (Not at my pc to check code in more detail.)

@alex-courtis What do you think about this?

@alex-courtis
Copy link
Member

Could we maybe make color key also able to take a table? If table first value would be what it is now and second would be a value for other mode. If single string just continue with current somewhat broken experience. (Not at my pc to check code in more detail.)

@alex-courtis What do you think about this?

That could work. It would not break existing API (we'd keep both versions) and would allow a better user experience.

Let's see how it looks...

@alex-courtis
Copy link
Member

Any updates @ribru17 ?

@ribru17
Copy link
Contributor Author

ribru17 commented Mar 2, 2024

Sorry, I haven't taken a look at this in a while, I've been getting a bit busy sadly. I will have time in 3 weeks to take an extended go at this but I am also fine if someone else wants to pick it up

@alex-courtis
Copy link
Member

Thanks mate, no pressure at all

@ribru17
Copy link
Contributor Author

ribru17 commented Mar 15, 2024

Some things to think about after examining the code for some time:

  • For something like get_icon_colors, do we return the table of both colors (kind of a breaking change) or dynamically detect the bg of the user to give the right color, or maybe give dark by default with some parameter to specify we want light color?
  • Other functions that return the full icon object will have to have some sort of breaking change probably, or maybe we can use a strategy mentioned above
  • Does anyone know of a good way to merge the two icon tables? Seems very tedious and I fear my scripting skills aren't up to par...

@alex-courtis
Copy link
Member

* Does anyone know of a good way to merge the two icon tables? Seems very tedious and I fear my scripting skills aren't up to par...

icons = vim.tbl_extend("keep", {}, icons_by_filename, icons_by_file_extension, icons_by_operating_system)

@ribru17
Copy link
Contributor Author

ribru17 commented Mar 15, 2024

I mean we could try and merge them this way, but I meant in a meta-programming way where we can just merge the two icons files, getting rid of one and then we just have one source of truth containing both the light and dark colors. I am trying to think maybe of a macro that could do this easily but it seems difficult

@alex-courtis
Copy link
Member

  • For something like get_icon_colors, do we return the table of both colors (kind of a breaking change) or dynamically detect the bg of the user to give the right color, or maybe give dark by default with some parameter to specify we want light color?

That family is currently returning only the dark or light based on the vim.o.background == "light" test.

We shouldn't need to break that, however we can add a boolean or enum to opts to allow the user to specify which.

@alex-courtis
Copy link
Member

  • Other functions that return the full icon object will have to have some sort of breaking change probably, or maybe we can use a strategy mentioned above

I'm optimistic: we can do this without breaking anything.

@alex-courtis
Copy link
Member

I mean we could try and merge them this way, but I meant in a meta-programming way where we can just merge the two icons files, getting rid of one and then we just have one source of truth containing both the light and dark colors. I am trying to think maybe of a macro that could do this easily but it seems difficult

Oh I see... #414 should cover that.

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