feat: apply light mode color to icon overrides

[ribru17]

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.

[alex-courtis]

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]

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]

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]

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

[alex-courtis]

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]

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]

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]

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]

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

[ribru17]

I like this idea a lot

[ribru17]

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]

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]

Any updates @ribru17 ?

[ribru17]

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]

Thanks mate, no pressure at all

[ribru17]

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]

* 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...

https://github.com/nvim-tree/nvim-web-devicons/blob/0db6a106d0fba1b54ff2b7f9db108e7505a4c26f/lua/nvim-web-devicons.lua#L36

[ribru17]

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]

• 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]

• 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]

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.