Repository navigation
Let themes declare that they are dark - #4354
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new defaults regress legacy ID-based dark-theme behavior for existing ITheme implementations and two-argument Theme construction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds explicit dark-theme metadata while retaining legacy ID-based detection and persisting appearance information for early startup styling.
Changes:
- Adds
isDarkThemeextension metadata andITheme.isDark(). - Uses and persists appearance metadata throughout theme selection.
- Avoids restart prompts when switching between themes of the same appearance.
File summaries
| File | Description |
|---|---|
ThemeTest.java |
Tests metadata parsing and persistence. |
plugin.xml (tests) |
Declares test themes. |
css/testTheme.css |
Supplies test stylesheet. |
build.properties |
Packages test resources. |
DefaultThemePreference.java |
Centralizes default-theme persistence. |
ViewsPreferencePage.java |
Uses appearance-aware restart behavior. |
org.eclipse.ui.themes/plugin.xml |
Marks platform dark themes explicitly. |
IDEApplication.java |
Reads persisted appearance during startup. |
ITheme.java |
Adds the appearance API. |
ThemeEngine.java |
Parses, applies, persists, and restores appearance. |
Theme.java |
Stores explicit appearance state. |
org.eclipse.e4.ui.css.swt.theme.exsd |
Documents the extension attribute. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
445499d to
ffbd198
Compare
ffbd198 to
5cd65b6
Compare
|
PR updated with the proposals, except one which was IMHO just a misunderstanding caused by wrong Javadoc, fixed now. |
The theme extension point gains an "isDarkTheme" attribute, so a theme states its appearance instead of the platform guessing it from the theme id. Themes without the attribute keep the id based classification. ThemeEngine uses ITheme.isDark() for the SWT appearance preference and when picking a dark theme to inherit the operating system setting, so a product shipping its own dark theme works too. The appearance preferences recommend a restart only when switching between light and dark. The flag is persisted next to the theme id, which lets IDEApplication style the workspace selection dialog before the theme engine exists. Assisted-by: multiple AI agents and layers of automated tooling 🤖
5cd65b6 to
b863876
Compare
|
Thanks @BeckerWdf for the review |
eclipse-platform/eclipse.platform.ui#4354 adds an isDarkTheme attribute to the theme extension point for 2026-12. Set it to true on the five dark themes and to false on One Light, so the platform no longer has to guess from the id. Older releases ignore the attribute and keep using the substring match, which the ids still satisfy. Assisted-by: multiple AI agents and layers of automated tooling 🤖 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157cPD5VyXHbCQEUbiLZzvA
|
@vogella in #4356 (comment) you said that with the PR merge a restart is not longer needed when switching between light and dark. But I still see the "restart" dialog when switching between dark and light: |
I said no restart is required for different dark themes (dark -> dark) or different light themes (light -> light). You can test by installing additional themes, e.g. from https://github.com/vogellacompany/eclipse-themes |
Ah ok. So then I mis-interpreted your text:
Thanks for clarification. |
|
Btw: What would be needed to be fixed so that we no longer need to trigger a restart after a switch from light to dark theme? |
Themes can now declare their appearance with an
isDarkThemeattribute on the theme extension point instead of the platform guessing it from the theme id. Themes that do not set it keep the old id based classification, so existing contributions behave as before.ITheme.isDark()is added as a default method, so implementors are not broken. The theme engine uses it for the SWT appearance preference and when picking a dark theme to inherit the operating system setting, which means a product shipping its own dark theme is now handled instead of onlyorg.eclipse.e4.ui.css.theme.e4_dark. The appearance preference page only recommends a restart when the switch actually crosses between light and dark, so switching between two dark themes no longer interrupts with a dialog. The flag is persisted next to the theme id, which letsIDEApplicationstyle the workspace selection dialog from the recorded value, the only thing it has before the workbench and the theme engine exist.This supersedes #2808 by @BeckerWdf, which had the same idea and where the
isDarkThemeattribute name comes from. That PR no longer applies since the platform specific dark theme processors it patches have been removed in the meantime.