fix: Highlighted filenames and links are hard to read in dark mode#329
fix: Highlighted filenames and links are hard to read in dark mode#329jomillerOpen wants to merge 3 commits into
Conversation
jomillerOpen
requested review from
duzitong,
ethanyhou,
jdneo and
xinyi-gong
as code owners
There was a problem hiding this comment.
Pull request overview
This PR improves dark-mode detection in the GitHub Copilot for Eclipse UI by switching from reading a workspace-stored instance preference to querying the active theme via Eclipse’s theme engine, helping ensure dark-theme styling (e.g., highlighted filenames/links) is applied even when theme prefs aren’t persisted in the local workspace.
Changes:
- Updated
UiUtils.isDarkTheme()to useIThemeEngineinstead ofInstanceScopetheme preferences. - Added the required OSGi package import for
org.eclipse.e4.ui.css.swt.themein the UI bundle manifest.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/utils/UiUtils.java | Switches dark-theme detection to the theme engine rather than instance-scope preferences. |
| com.microsoft.copilot.eclipse.ui/META-INF/MANIFEST.MF | Imports the theme engine package needed by the updated dark-theme detection. |
| public static boolean isDarkTheme() { | ||
| Preferences preferences = InstanceScope.INSTANCE.getNode("org.eclipse.e4.ui.css.swt.theme"); | ||
| String themeCssUri = preferences.get("themeid", ""); | ||
| if (themeCssUri.toLowerCase().contains("dark")) { | ||
| return true; | ||
| IThemeEngine engine = PlatformUI.getWorkbench().getService(IThemeEngine.class); | ||
| if (engine != null) { | ||
| String themeCssUri = engine.getActiveTheme().getId(); | ||
| if (themeCssUri.toLowerCase().contains("dark")) { |
| String themeCssUri = preferences.get("themeid", ""); | ||
| if (themeCssUri.toLowerCase().contains("dark")) { | ||
| return true; | ||
| IThemeEngine engine = PlatformUI.getWorkbench().getService(IThemeEngine.class); |
There was a problem hiding this comment.
Instead of adding extra (not needed) dependency, you can use Platform.getPreferencesService(). Original problem was that only workspace scope was considered, preference service, if properly used, can consider other scopes as well.
There was a problem hiding this comment.
The problem I encountered with this is method is that all scopes don't have a value to return. If I do for example:
Preferences preferences1 = InstanceScope.INSTANCE.getNode("org.eclipse.e4.ui.css.swt.theme");
String themeCssUri2 = preferences1.get("themeid", "");
Preferences preferences2 = ConfigurationScope.INSTANCE.getNode("org.eclipse.e4.ui.css.swt.theme");
String themeCssUri3 = preferences2.get("themeid", "");
Preferences preferences3 = DefaultScope.INSTANCE.getNode("org.eclipse.e4.ui.css.swt.theme");
String themeCssUri4 = preferences3.get("themeid", "");
IPreferencesService service = Platform.getPreferencesService();
String themeId = service.getString("org.eclipse.e4.ui.css.swt.theme", "currentThemeId", null, null);
String themeId2 = service.getString("org.eclipse.e4.ui.css.swt.theme", "currentThemeId", null, new IScopeContext[] { InstanceScope.INSTANCE });
String themeId3 = service.getString("org.eclipse.e4.ui.css.swt.theme", "currentThemeId", null, new IScopeContext[] { ConfigurationScope.INSTANCE });
String themeId4 = service.getString("org.eclipse.e4.ui.css.swt.theme", "currentThemeId", null, new IScopeContext[] {DefaultScope.INSTANCE });
All of the scopes return null (the default value if using the platform.getpreferencesservice) or "" (if using the Instancescope method). When I checked with copilot, it seems that the e4 service applies the preference through a different mechanism, so that's why we can't read the preference.
If the preference is saved in that file I mentioned before (such as manually setting/re-setting the preference), then the old code works fine. But if that file/preference isn't saved, then none of the default preference retreival methods are able to see the theme being applied.
There was a problem hiding this comment.
@iloveeclipse Will the newly introduced dependency a concern for you?
There was a problem hiding this comment.
Will the newly introduced dependency a concern for you?
Every added dependency always adds extra work in the future :-)
But if that file/preference isn't saved, then none of the default preference retreival methods are able to see the theme being applied.
I see. I would say this is a bug in Platform UI, you should report it here: https://github.com/eclipse-platform/eclipse.platform.ui/issues. A client doesn't need to have hard dependency to org.eclipse.e4.ui.css.swt.theme bundle just to know whether some preference is set or not.
So your solution seem to be the only possible today.
There was a problem hiding this comment.
@jomillerOpen could you file an issue as @iloveeclipse suggested?
My feeling is that: If the upstream fixes this immediately, then we can assume that we can get the truth via Platform.getPreferencesService(). Users will get the fix automatically as long as they upgrade the Eclipse IDE.
If the upstream has no plan to fix it immediately, I'm ok to have this workaround right now.
Let's see their response first. WDYT?
There was a problem hiding this comment.
Opened a new bug for them: eclipse-platform/eclipse.platform.ui#4168
iloveeclipse
left a comment
iloveeclipse
left a comment
There was a problem hiding this comment.
Please also check that the new code works if CSS themes are disabled via preference (it should work looking at the code, but manual check is always good)
| String themeCssUri = preferences.get("themeid", ""); | ||
| if (themeCssUri.toLowerCase().contains("dark")) { | ||
| return true; | ||
| IThemeEngine engine = PlatformUI.getWorkbench().getService(IThemeEngine.class); |
There was a problem hiding this comment.
Will the newly introduced dependency a concern for you?
Every added dependency always adds extra work in the future :-)
But if that file/preference isn't saved, then none of the default preference retreival methods are able to see the theme being applied.
I see. I would say this is a bug in Platform UI, you should report it here: https://github.com/eclipse-platform/eclipse.platform.ui/issues. A client doesn't need to have hard dependency to org.eclipse.e4.ui.css.swt.theme bundle just to know whether some preference is set or not.
So your solution seem to be the only possible today.
Fixed reading the preferences for the theme if the theme settings aren't saved in the local workspace (under .metadata.plugins\org.eclipse.core.runtime\org.eclipse.e4.ui.css.swt.theme.prefs).
This changes the preference read from the instance scope to using the themeengine's preference retrieval instead. This will return the proper theme string whether the preference is stored in the workspace or not.
Tested with and without that file present. The full string returned when it's in dark mode is "org.eclipse.e4.ui.css.theme.e4_dark".