search

nvim-tree / nvim-web-devicons

lua `fork` of vim-web-devicons for neovim
MIT License
1.93k stars 182 forks source link

feat: apply light mode color to icon overrides #374

Open ribru17 opened 5 months ago

ribru17 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

alex-courtis commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

ribru17 commented 5 months ago

I like this idea a lot

ribru17 commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

Any updates @ribru17 ?

ribru17 commented 4 months ago

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 commented 4 months ago

Thanks mate, no pressure at all

ribru17 commented 3 months ago

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

alex-courtis commented 3 months ago 
* 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 commented 3 months ago

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 commented 3 months ago
  • 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 commented 3 months ago
  • 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 commented 3 months ago

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.