Skip to content

Provide Explicit "isDarkTheme" Attribute #2808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BeckerWdf
Copy link
Contributor

@BeckerWdf BeckerWdf commented Feb 20, 2025

Up to now we had some heuristics to check if a theme is a dark theme. This now makes this explicit.

  • Add attribute to extension point
  • Fill this attribute for the platforms dark theme
  • Remove heuristics and now check this explicit flag

See also #2797 (comment)

@BeckerWdf BeckerWdf requested a review from sratz February 20, 2025 08:54
@BeckerWdf BeckerWdf self-assigned this Feb 20, 2025
@akurtakov
Copy link
Member

My main concern with such "api" is what happens with e.g. gray #808080 are they dark or light? What about some dark(-ish) "blue"? It becomes a very thin line to decide with such boolean.

@sratz
Copy link
Member

sratz commented Feb 20, 2025

I think it's pretty common for themes to be either dark or light. At least all major operating systems also have this kind of primary boolean distinction.

@akurtakov
Copy link
Member

@sratz I agree that OSes(GTK I know has it for sure) have such boolean distinction and I am not against this addition, just raising some concern about theming in general (not only in Eclipse) for which no one found better solution yet.

Copy link
Contributor

github-actions bot commented Feb 20, 2025

Test Results

 1 823 files   -  1   1 823 suites   - 1   1h 27m 36s ⏱️ - 4m 16s
 7 922 tests + 4   7 693 ✅ + 3  228 💤 ± 0  1 ❌ +1 
23 808 runs   - 33  23 069 ✅  - 24  738 💤  - 10  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 257c857. ± Comparison against base commit 037c5d7.

♻️ This comment has been updated with latest results.

@BeckerWdf BeckerWdf force-pushed the isDarkTheme branch 2 times, most recently from 97bb557 to b93d493 Compare February 20, 2025 10:50
@BeckerWdf BeckerWdf force-pushed the isDarkTheme branch 2 times, most recently from 2b8766b to 3030140 Compare February 28, 2025 09:54
@eclipse-platform-bot
Copy link
Contributor

eclipse-platform-bot commented Feb 28, 2025

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.e4.ui.css.swt.theme/META-INF/MANIFEST.MF
bundles/org.eclipse.e4.ui.swt.gtk/META-INF/MANIFEST.MF
bundles/org.eclipse.e4.ui.swt.win32/META-INF/MANIFEST.MF
bundles/org.eclipse.e4.ui.workbench.renderers.swt.cocoa/META-INF/MANIFEST.MF
bundles/org.eclipse.ui.themes/META-INF/MANIFEST.MF
tests/org.eclipse.e4.ui.tests.css.swt/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From 854143c9a63bdee4996e6fb25480987763afa801 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Fri, 11 Apr 2025 11:37:33 +0000
Subject: [PATCH] Version bump(s) for 4.36 stream


diff --git a/bundles/org.eclipse.e4.ui.css.swt.theme/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.css.swt.theme/META-INF/MANIFEST.MF
index f140ef150e..ddf1170b83 100644
--- a/bundles/org.eclipse.e4.ui.css.swt.theme/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.ui.css.swt.theme/META-INF/MANIFEST.MF
@@ -1,7 +1,7 @@
 Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-SymbolicName: org.eclipse.e4.ui.css.swt.theme;singleton:=true
-Bundle-Version: 0.14.500.qualifier
+Bundle-Version: 0.14.600.qualifier
 Bundle-Name: %pluginName
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
diff --git a/bundles/org.eclipse.e4.ui.swt.gtk/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.swt.gtk/META-INF/MANIFEST.MF
index c8a8100502..30a21c7959 100644
--- a/bundles/org.eclipse.e4.ui.swt.gtk/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.ui.swt.gtk/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %fragmentName
 Bundle-SymbolicName: org.eclipse.e4.ui.swt.gtk;singleton:=true
-Bundle-Version: 1.2.200.qualifier
+Bundle-Version: 1.2.300.qualifier
 Fragment-Host: org.eclipse.e4.ui.css.swt.theme;bundle-version="0.10.0"
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Bundle-Localization: fragment-gtk
diff --git a/bundles/org.eclipse.e4.ui.swt.win32/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.swt.win32/META-INF/MANIFEST.MF
index 187d89e9d4..a08a75878f 100644
--- a/bundles/org.eclipse.e4.ui.swt.win32/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.ui.swt.win32/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %fragmentName
 Bundle-SymbolicName: org.eclipse.e4.ui.swt.win32;singleton:=true
-Bundle-Version: 1.2.300.qualifier
+Bundle-Version: 1.2.400.qualifier
 Fragment-Host: org.eclipse.e4.ui.css.swt.theme;bundle-version="0.10.0"
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Bundle-Localization: fragment-win32
diff --git a/bundles/org.eclipse.e4.ui.workbench.renderers.swt.cocoa/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.workbench.renderers.swt.cocoa/META-INF/MANIFEST.MF
index 843b08f8fc..811e91bffb 100644
--- a/bundles/org.eclipse.e4.ui.workbench.renderers.swt.cocoa/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.ui.workbench.renderers.swt.cocoa/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Eclipse-PlatformFilter: (osgi.ws=cocoa)
 Bundle-SymbolicName: org.eclipse.e4.ui.workbench.renderers.swt.cocoa;singleton:=true
-Bundle-Version: 0.14.400.qualifier
+Bundle-Version: 0.14.500.qualifier
 Bundle-Name: %fragmentName
 Bundle-Vendor: %providerName
 Fragment-Host: org.eclipse.e4.ui.workbench.renderers.swt;bundle-version="[0.10.0,1.0.0)"
diff --git a/bundles/org.eclipse.ui.themes/META-INF/MANIFEST.MF b/bundles/org.eclipse.ui.themes/META-INF/MANIFEST.MF
index 59abbfd238..11d118fd4c 100644
--- a/bundles/org.eclipse.ui.themes/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.ui.themes/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Plugin.name
 Bundle-SymbolicName: org.eclipse.ui.themes;singleton:=true
-Bundle-Version: 1.2.2700.qualifier
+Bundle-Version: 1.2.2800.qualifier
 Bundle-Vendor: %Plugin.providerName
 Bundle-Localization: plugin
 Require-Bundle: org.eclipse.e4.ui.css.swt.theme
diff --git a/tests/org.eclipse.e4.ui.tests.css.swt/META-INF/MANIFEST.MF b/tests/org.eclipse.e4.ui.tests.css.swt/META-INF/MANIFEST.MF
index 1cb1884bb4..6a7fd90fc5 100644
--- a/tests/org.eclipse.e4.ui.tests.css.swt/META-INF/MANIFEST.MF
+++ b/tests/org.eclipse.e4.ui.tests.css.swt/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Bundle-Name
 Bundle-SymbolicName: org.eclipse.e4.ui.tests.css.swt; singleton:=true
-Bundle-Version: 0.12.800.qualifier
+Bundle-Version: 0.12.900.qualifier
 Require-Bundle: org.eclipse.e4.ui.css.core,
  org.eclipse.e4.ui.css.swt,
  org.eclipse.e4.ui.css.swt.theme;bundle-version="0.9.1",
-- 
2.49.0

Further information are available in Common Build Issues - Missing version increments.

@BeckerWdf BeckerWdf force-pushed the isDarkTheme branch 3 times, most recently from dce1d99 to 1332d42 Compare March 5, 2025 13:09
Up to now we had some heuristics to check if a theme is a dark
theme. This now makes this explicit.

- Add attribute to extension point
- Fill this attribute for the platforms dark theme
- Remove heuristics and now check this explicit flag
- Add tests
@mickaelistria
Copy link
Contributor

I'm still torn about whether "isDark" should be further declarative. Could we instead try to get the default background color from the them, and resolve whether the background color is dark or not; using eg the usual formula to convert RGB to greyscale (0.3 * R + 0.59 * B + 0.11 * G) and evaluating whether this grey is dark (eg < 122) ?

@BeckerWdf
Copy link
Contributor Author

I'm still torn about whether "isDark" should be further declarative. Could we instead try to get the default background color from the them, and resolve whether the background color is dark or not; using eg the usual formula to convert RGB to greyscale (0.3 * R + 0.59 * B + 0.11 * G) and evaluating whether this grey is dark (eg < 122) ?

But that would replace one heuristic with another one. I think it's quite good to make this explicit. And keep in mind we don't have tons of themes in platform (and out there in the community). Adapting them should be easy.

@BeckerWdf
Copy link
Contributor Author

get the default background color from the theme

What should that be? Pls. explain based on the existing themes which color definition would be the "default background color".

@mickaelistria
Copy link
Contributor

But that would replace one heuristic with another one.

Is this really just an heuristic or is the greyscale value the actual definition of whether a theme is dark or not ?
Note that I'm not against allowing people to explicit/override whether their theme is to be treated as dark or not if they want to; but I find very strange that the default decision made by the Platform wouldn't be sufficient in most cases. We can't expect every theme to adopt the extension (we do not control all the themes from the community, many of theme are probably built-in inside some low-maintained RCP apps...); and we cannot just force burden to theme developers about forcing them to set an extra flag if the value can be reliably deduced from the payload in the vast majority of cases.

@mickaelistria
Copy link
Contributor

What should that be? Pls. explain based on the existing themes which color definition would be the "default background color".

Probably something like Composite > background-color; or even better way to infer it would be to be able to load the theme, create a dummy invisible shell, apply the theme on it and query the background color.

@BeckerWdf
Copy link
Contributor Author

What should that be? Pls. explain based on the existing themes which color definition would be the "default background color".

Probably something like Composite > background-color; or even better way to infer it would be to be able to load the theme, create a dummy invisible shell, apply the theme on it and query the background color.

To be honest I find this overcomplicated. For example we have code like this:

boolean hasDarkTheme = getThemes().stream().anyMatch(t -> t.isDark());

Do you really like to create a dummy invisible shell for each of the themes when the alternative is setting a little flag in the plugin.xml? And we don't have that many themes anyway. So I think the the cost / benefit relation is not very good for that.

@sratz
Copy link
Member

sratz commented Apr 22, 2025

I agree with @BeckerWdf. Why implement a guessing algorithm when we can make it explicit?

Other IDEs also have an explicit flag, e.g. VS Code:
https://github.com/microsoft/vscode/blob/544674f984e97b8aa9a444cb5487cfc4c27f2b7a/src/vs/platform/theme/common/theme.ts#L6-L14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants