Theme-aware plugin icons via an optional -dark or -light variant of the icon file - #2722
Conversation
…variant A plugin may place 'icon-dark.png' and/or 'icon-light.png' next to its 'icon.png'; the variant matching the active theme is served instead. Which themes count as dark comes from the existing ThemeHelper::isDarkTheme(), so black and gray get -dark and white and azure get -light. Applied at the four places that render a plugin supplied .png icon: the nav and sidebar utility buttons, the Settings and Tools tiles, the page and tab title, and the Plugins page list. In each case the helper runs after the existing lookup has picked a file, so the existing resolution order and fallbacks are unchanged. Backward compatible: with no matching variant present the icon renders exactly as before, and glyph icons return early and never reach the check.
WalkthroughPNG plugin and VM icons now support light and dark theme variants. The new ChangesTheme-aware plugin icons
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant IconRenderer
participant theme_icon
participant ThemeHelper
IconRenderer->>theme_icon: pass PNG icon path
theme_icon->>ThemeHelper: resolve active theme
ThemeHelper-->>theme_icon: return theme
theme_icon-->>IconRenderer: return themed or original path
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Same theme_icon() helper from the plugin-icon PR, wired into the three places a VM's icon renders: the VMs tab list, the Dashboard widget (and its VM-usage tile, which shares the same resolved icon), and the Add/Edit VM form's preview + icon-chooser grid. The chooser grid also skips -dark/-light files as their own entries, since theme_icon() already serves the matching variant for its canonical icon. VM icons ship as first-party files in this repo (unlike Docker container icons, which come from third-party template URLs with no dark/light concept), so a future -dark/-light pair for any of them works without further changes -- no such pairs exist yet, so this is a no-op today.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emhttp/plugins/dynamix.vm.manager/include/VMMachines.php`:
- Line 54: Update the icon handling around $lv->domain_get_icon_url($res) and
theme_icon() so absolute filesystem paths are normalized to the theme resolver’s
expected path before variant lookup. Preserve existing URL and relative-path
behavior while ensuring absolute VM icon paths still resolve their themed
variants.
In `@emhttp/plugins/dynamix/include/DashboardApps.php`:
- Line 179: Update theme_icon() call sites to normalize absolute /boot/config
paths returned or used by domain_get_icon_url() before resolution. Apply this in
emhttp/plugins/dynamix/include/DashboardApps.php lines 179-179,
emhttp/plugins/dynamix.vm.manager/include/VMedit.php lines 129-129, and
emhttp/plugins/dynamix.vm.manager/include/VMedit.php lines 139-143; preserve
relative-path handling and ensure themed variants resolve for user-installed
icons.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c2ed5261-83cc-4edb-9ee3-3bd6c5888b9f
📒 Files selected for processing (3)
emhttp/plugins/dynamix.vm.manager/include/VMMachines.phpemhttp/plugins/dynamix.vm.manager/include/VMedit.phpemhttp/plugins/dynamix/include/DashboardApps.php
| $autostart = $lv->domain_get_autostart($res) ? 'checked' : ''; | ||
| $state = $lv->domain_state_translate($dom['state']); | ||
| $icon = $lv->domain_get_icon_url($res); | ||
| $icon = theme_icon($lv->domain_get_icon_url($res)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve theme resolution for absolute VM icon paths.
domain_get_icon_url() returns the raw $strIcon when is_file($strIcon) succeeds. theme_icon() then checks the variant under $docroot/$variant. For an absolute path such as /boot/config/.../icon.png, this checks the wrong filesystem location, so the themed variant is skipped. Normalize filesystem paths before calling theme_icon(), or update theme_icon() to resolve them correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@emhttp/plugins/dynamix.vm.manager/include/VMMachines.php` at line 54, Update
the icon handling around $lv->domain_get_icon_url($res) and theme_icon() so
absolute filesystem paths are normalized to the theme resolver’s expected path
before variant lookup. Preserve existing URL and relative-path behavior while
ensuring absolute VM icon paths still resolve their themed variants.
| } | ||
| $menu = sprintf("onclick=\"addVMContext('%s','%s','%s','%s','%s','%s','%s','%s','%s','%s','%s','%s','%s')\"", addslashes($vm), addslashes($uuid), addslashes($template), $state, addslashes($vmrcurl), strtoupper($vmrcprotocol), addslashes($log),addslashes($fstype), $vmrcconsole,false,addslashes(str_replace('"',"'",$WebUI)),$pcierror,$srioverror); | ||
| $icon = $lv->domain_get_icon_url($res); | ||
| $icon = theme_icon($lv->domain_get_icon_url($res)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Fix the shared path contract for /boot/config icons.
theme_icon() cannot resolve variants when callers pass absolute /boot/config/... paths because it prepends $docroot before file_exists(). User-installed VM icons therefore keep the base image instead of switching themes.
emhttp/plugins/dynamix/include/DashboardApps.php#L179: handle absolute paths returned bydomain_get_icon_url().emhttp/plugins/dynamix.vm.manager/include/VMedit.php#L129: handle absolute paths used by the edit preview.emhttp/plugins/dynamix.vm.manager/include/VMedit.php#L139-L143: handle absolute paths used by the icon chooser.
📍 Affects 2 files
emhttp/plugins/dynamix/include/DashboardApps.php#L179-L179(this comment)emhttp/plugins/dynamix.vm.manager/include/VMedit.php#L129-L129emhttp/plugins/dynamix.vm.manager/include/VMedit.php#L139-L143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@emhttp/plugins/dynamix/include/DashboardApps.php` at line 179, Update
theme_icon() call sites to normalize absolute /boot/config paths returned or
used by domain_get_icon_url() before resolution. Apply this in
emhttp/plugins/dynamix/include/DashboardApps.php lines 179-179,
emhttp/plugins/dynamix.vm.manager/include/VMedit.php lines 129-129, and
emhttp/plugins/dynamix.vm.manager/include/VMedit.php lines 139-143; preserve
relative-path handling and ensure themed variants resolve for user-installed
icons.
|
CodeRabbit's
The only branch of |
1 participant
A plugin can now ship a per-theme variant of its
.pngicon. Next toicon.pngit may placeicon-dark.pngand/oricon-light.png, and the webgui serves the variant matching the active theme.Plugins that ship a single icon are untouched: with no matching variant file present the icon renders exactly as it does today, and glyph icons (
icon-*and FontAwesome) are not affected at all.Closes #2704.
Why
Glyph icons already follow the theme, because they render as
<b class="fa fa-... system">and inherit the theme text color. Image icons do not. A.pngrenders as a static<img>, so a full-color logo can be unreadable on some themes: a dark logo disappears on the black theme, a light one washes out on the white theme.The only theme-adaptive option for an image icon today is a single-color inline SVG using
currentColor, which forces the logo to be monochrome and loses its real colors. In practice plugin authors work around this in the artwork instead, for example by drawing concentric dark and light rings so that one of them always contrasts with whatever background the icon lands on. That constrains the logo rather than fixing the problem.How
One helper in
Helpers.php:It takes the docroot-relative path of an already resolved icon and returns either the theme variant or the same path unchanged. It returns early for anything that is not a
.png, so glyph icons never reach the file check.Which themes count as dark is not a new decision. The helper asks the existing
ThemeHelper::isDarkTheme(), soblackandgrayget-dark,whiteandazureget-light, and any theme added later is classified in one place. The suffix is resolved once per request and cached in a static, so a page with many icons costs at most one extrafile_existsper icon.The theme comes from
$display['theme']on a normal page load.ShowPlugins.phpis fetched standalone over AJAX and has no$display, so the helper falls back toparse_plugin_cfg('dynamix',true)there. That keeps the helper self-contained instead of changing the variable scope of that endpoint.The helper is applied at the four places that render a plugin-supplied
.pngicon:.pageIcon=, from<root>/icons/).pageIcon=, from<root>/images/or<root>/)<root>/icons/).plgicon, fromplugins/<name>/images/orplugins/<name>/)In each case the helper runs after the existing lookup has decided which file to use, so the existing resolution order and the existing fallbacks are unchanged.
Deliberately not covered: Docker container and VM icons, which come from user-editable templates and remote URLs rather than from files a plugin ships. Also SVG, because a
.svgvalue inIcon=is not handled as an image today (it falls through to the FontAwesome branch), so supporting it would be a separate change.This implements option A from the issue (naming convention, no manifest change), because it needs no new field in either the
.pageor the.plgformat and works the same for both.Testing
php -lis clean on all changed files under PHP 8.4.23 withshort_open_tag=On.theme_icon()was also exercised directly against the realThemeHelperusing a sandbox docroot:blackandgraypickicon-dark.pngwhen it existswhiteandazurepickicon-light.pngwhen it existsrocketandicon-appall come back unchanged$displayunset, the config fallback resolves the theme correctly and raises nothing atE_ALLFiles
emhttp/plugins/dynamix/include/Helpers.phpemhttp/plugins/dynamix/include/PageBuilder.phpemhttp/plugins/dynamix/include/DefaultPageLayout/MainContent.phpemhttp/plugins/dynamix/include/DefaultPageLayout/Navigation/Main.phpemhttp/plugins/dynamix.plugin.manager/include/ShowPlugins.phpSummary by CodeRabbit