domtronn / all-the-icons.el

A utility package to collect various Icon Fonts and propertize them within Emacs.
MIT License
1.48k stars 177 forks source link

[svg] handle theme changes #368

Closed Stebalien closed 1 month ago

Stebalien commented 1 year ago

The svg branch caches re-colored svgs so changing the theme won't actually change the icon colors. I'd suggest:

wyuenho commented 1 month ago

Does 87924c6ce56c408682764c5b22e8fda1a5c49b07 solve your problem? You still won't see the foreground color refreshed when you adjusts any all-the-icons faces, but at least the background can pass through.

Stebalien commented 1 month ago

I haven't looked at this in a long time but it looks like it was fixed in 22d163f5103dc07d8ba7b694bfc37284a1a1d968.

Unfortunately, 87924c6ce56c408682764c5b22e8fda1a5c49b07 re-introduces the issue because the it hard-codes the colors instead of pulling them from the faces (which means changing the faces doesn't change the icon colors).

wyuenho commented 1 month ago

I think 87924c6 does the exact opposite of what you described. 22d163f shouldn't affect you since before this commit it was simply setting the foreground and background twice to the icon.

Stebalien commented 1 month ago

https://github.com/domtronn/all-the-icons.el/commit/22d163f5103dc07d8ba7b694bfc37284a1a1d968 fixed the issue because it removed the explicit setting of the foreground instead relying entirely on the text's face property, which gets recalculated when changing themes.

https://github.com/domtronn/all-the-icons.el/commit/87924c6ce56c408682764c5b22e8fda1a5c49b07 re-breaks it by setting the face with a hard-coded foreground. If/when the underlying face's foreground changes, the change won't be reflected in the text (i.e., the icon) because the the face set on that property is (:foreground "#color") instead of some-face.

Stebalien commented 1 month ago

Note: I've tested with https://github.com/domtronn/all-the-icons.el/commit/87924c6ce56c408682764c5b22e8fda1a5c49b07 and with https://github.com/domtronn/all-the-icons.el/commit/87924c6ce56c408682764c5b22e8fda1a5c49b07 reverted.

wyuenho commented 1 month ago

Ah I see, in this case I think we should define an all-the-icons-default-face which only inherits the default face's foreground, and we'll use this custom default face instead of the global default face.

Stebalien commented 1 month ago

Where? The fix for this issue is just to revert https://github.com/domtronn/all-the-icons.el/commit/87924c6ce56c408682764c5b22e8fda1a5c49b07 unless that commit was fixing another issue.

Using the default face is fine as long as we're using faces themselves instead of extracting colors from them.