Repository navigation
Conversation
|
Do we have SVG icons using fill="currentColor" in platform / simrel ? |
No, because support was broken before. |
c54d7bf to
2989f31
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Early window images can retain black rasterization, and concurrent initialization can overwrite the session color.
2 open findings
What changed in this PR
Adds theme-controlled SVG currentColor support to improve icon visibility in the dark theme.
Changes:
- Registers a Shell CSS property with once-per-session application.
- Sets the dark-theme color to
#CCCCCCand freezes it across theme switches. - Updates dependencies and adds property and theme-switch tests.
| File | Description |
|---|---|
| tests/org.eclipse.e4.ui.tests.css.swt/src/org/eclipse/e4/ui/tests/css/swt/ThemeTest.java | Tests color freezing across theme switches. |
| tests/org.eclipse.e4.ui.tests.css.swt/src/org/eclipse/e4/ui/tests/css/swt/ShellTest.java | Tests one-time application and system-property precedence. |
| bundles/org.eclipse.ui.themes/css/dark/e4-dark_globalstyle.css | Sets the dark-theme SVG color. |
| bundles/org.eclipse.e4.ui.css.swt/src/org/eclipse/e4/ui/css/swt/properties/custom/CSSPropertySVGCurrentColorHandler.java | Implements SVG color application and freezing. |
| bundles/org.eclipse.e4.ui.css.swt/plugin.xml | Registers the Shell property handler. |
| bundles/org.eclipse.e4.ui.css.swt/META-INF/MANIFEST.MF | Updates SWT requirements and package access. |
| bundles/org.eclipse.e4.ui.css.swt.theme/src/org/eclipse/e4/ui/css/swt/internal/theme/ThemeEngine.java | Freezes SVG color when switching themes. |
| bundles/org.eclipse.e4.ui.css.swt.theme/META-INF/MANIFEST.MF | Updates the CSS bundle requirement. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4497f2d to
962ce41
Compare
SVG icons using fill="currentColor" are always rasterized black, which makes them unreadable in the dark theme. The new Shell property swt-svg-current-color passes the theme color to SWT once per session, before the workbench window contents are rasterized, so currentColor icons in workbench window contents follow the theme without per-theme icon copies. Product window icons are initialized before theme application and are outside the scope of this startup recoloring. A theme switch freezes the color, also when the starting theme did not set one, so cached and new rasterizations never mix colors before the restart. An explicit swt.svg.currentColor system property still takes precedence. Assisted-by: multiple AI agents and layers of automated tooling 🤖
962ce41 to
4aaaeef
Compare
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
Do I understand correctly that this change will only work if SVGFileFormat.setCurrentColor() (non-API) will never be changed and if no one else uses that method to set a (different) current color? I am not sure if it's a good idea to make such assumptions.
And isn't this usage even contradictory to how currentColor in SVGs is supposed to be used? Shouldn't the surrounding CSS foreground color or something like that be used for it?
|
Not sure I understand your concern. The css engine frequency calls internal SWT API as a lot of code does in platform and always has in the last 20 years. We are usually the first and only consumer of internal platform API Also this is the first step of better support for flexible colors in SVG. Currently SVG is hard code to black. Support in platform for customizing that would be nice for my RCP clients. |
|
Okay, let's drop the point regarding internal API usage. My main concerns are as follows:
That's why this currently looks more like a shortcut/hack to me than something that does a first step for for future extension. |
|
I think your example it is similar to the shell color which can be set via css and via a method call. If someone sets the color via method call I would not say it is broken. And currently the currentColor in the SVG implementation can have only one value. Without this css option is is always black and with this change it can be configured which is better than always black. Once the color could be set by widget we could move this to make it more flexible. |
SVG icons using
fill="currentColor"are always rasterized black, which makes them unreadable in the dark theme. This adds the CSS propertyswt-svg-current-coloronShell, which passes the theme color to SWT's newSVGFileFormat.setCurrentColor(eclipse-platform/eclipse.platform.swt#3552) once per session, before the workbench window contents are rasterized. The dark theme sets it to#CCCCCC, while the light theme keeps SWT's default black. A single icon in the workbench window contents then works in both themes without per-theme copies, and icons with explicit colors are unaffected. Product window icons are initialized before theme application and are outside the scope of this startup recoloring. Since already rasterized icons keep their color, a theme switch takes effect after a restart.