Provide Explicit "isDarkTheme" Attribute

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

Author: BeckerWdf

bundles/org.eclipse.e4.ui.css.swt.theme/schema/org.eclipse.e4.ui.css.swt.theme.exsd

@@ -134,6 +134,13 @@
                </documentation>
             </annotation>
          </attribute>
+         <attribute name="isDarkTheme" type="boolean">
+            <annotation>
+               <documentation>
+                  Set this to true if the theme uses dark background.
+               </documentation>
+            </annotation>
+         </attribute>
       </complexType>
    </element>
 

bundles/org.eclipse.e4.ui.css.swt.theme/src/org/eclipse/e4/ui/css/swt/internal/theme/Theme.java

@@ -19,6 +19,7 @@ public class Theme implements ITheme {
 	private String id;
 	private String label;
 	private String osVersion;
+	private String isDark;
 
 	public Theme(String id, String label) {
 		this.id = id;
@@ -43,10 +44,25 @@ public String getOsVersion() {
 		return this.osVersion;
 	}
 
+	@Override
+	public boolean isDark() {
+		if (isDark == null) {
+			// fallback for themes that don't yet set the "isDark" attribute
+			// will be removed in a later release
+			return id.contains("dark");
+		} else {
+			return Boolean.parseBoolean(isDark);
+		}
+	}
+
+	public void setIsDark(String isDark) {
+		this.isDark = isDark;
+	}
+
 	@Override
 	public String toString() {
 		return "Theme [id=" + id + ", label='" + label + "', osVersion="
-				+ osVersion + "]";
+				+ osVersion + " isDark=" + isDark + "]";
 	}
 
 

bundles/org.eclipse.e4.ui.css.swt.theme/src/org/eclipse/e4/ui/css/swt/internal/theme/ThemeEngine.java

@@ -81,6 +81,7 @@ public class ThemeEngine implements IThemeEngine {
 	private HashMap<String, List<IResourceLocator>> sourceLocators = new HashMap<>();
 
 	private static final String THEMEID_KEY = "themeid";
+	private static final String THEME_IS_DARK_KEY = "themeIsDark";
 
 	public static final String THEME_PLUGIN_ID = "org.eclipse.e4.ui.css.swt.theme";
 
@@ -121,6 +122,7 @@ public ThemeEngine(Display display) {
 						String id = ce.getAttribute("id");
 						String os = ce.getAttribute("os");
 						String version = ce.getAttribute("os_version");
+						String isDarkTheme = ce.getAttribute("isDarkTheme");
 
 						/*
 						 * Code to support e4 dark theme on Mac 10.13 and older. For e4 dark theme on
@@ -154,7 +156,8 @@ public ThemeEngine(Display display) {
 							basestylesheeturi = "platform:/plugin/" + ce.getContributor().getName() + "/"
 									+ basestylesheeturi;
 						}
-						registerTheme(themeId, label, basestylesheeturi, version);
+
+						registerTheme(themeId, label, basestylesheeturi, version, isDarkTheme);
 
 						//check for modified files
 						if (modifiedFiles != null) {
@@ -250,11 +253,11 @@ private boolean isOsVersionMatch(String osVersionList) {
 	@Override
 	public synchronized ITheme registerTheme(String id, String label, String basestylesheetURI)
 			throws IllegalArgumentException {
-		return  registerTheme(id, label, basestylesheetURI, "");
+		return registerTheme(id, label, basestylesheetURI, "", "");
 	}
 
 	public synchronized ITheme registerTheme(String id, String label,
-			String basestylesheetURI, String osVersion) throws IllegalArgumentException {
+			String basestylesheetURI, String osVersion, String isDark) throws IllegalArgumentException {
 		for (Theme t : themes) {
 			if (t.getId().equals(id)) {
 				throw new IllegalArgumentException("A theme with the id '" + id
@@ -265,6 +268,9 @@ public synchronized ITheme registerTheme(String id, String label,
 		if (osVersion != "") {
 			theme.setOsVersion(osVersion);
 		}
+		if (isDark != "") {
+			theme.setIsDark(isDark);
+		}
 		themes.add(theme);
 		registerStyle(id, basestylesheetURI, false);
 		return theme;
@@ -496,6 +502,7 @@ public void setTheme(ITheme theme, boolean restore, boolean force) {
 			EclipsePreferencesHelper.setCurrentThemeId(theme.getId());
 
 			pref.put(THEMEID_KEY, theme.getId());
+			pref.putBoolean(THEME_IS_DARK_KEY, theme.isDark());
 			try {
 				pref.flush();
 			} catch (BackingStoreException e) {
@@ -560,6 +567,10 @@ private String getPreferenceThemeId() {
 		return getPreferences().get(THEMEID_KEY, null);
 	}
 
+	private boolean getPreferenceThemeIsDark(boolean fallback) {
+		return getPreferences().getBoolean(THEME_IS_DARK_KEY, fallback);
+	}
+
 	private IEclipsePreferences getPreferences() {
 		return InstanceScope.INSTANCE.getNode(FrameworkUtil.getBundle(ThemeEngine.class).getSymbolicName());
 	}
@@ -593,12 +604,24 @@ public void restore(String alternateTheme) {
 			for (ITheme t : getThemes()) {
 				if (prefThemeId.equals(t.getId())) {
 					setTheme(t, false);
+
+					// also update "isDark" attribute in the pref-store (might be not set there)
+					// will be removed in a later release
+					IEclipsePreferences preferences = getPreferences();
+					if (preferences.get(THEME_IS_DARK_KEY, null) == null) {
+						preferences.putBoolean(THEME_IS_DARK_KEY, t.isDark());
+						try {
+							preferences.flush();
+						} catch (BackingStoreException e) {
+							ThemeEngineManager.logError(e.getMessage(), e);
+						}
+					}
 					return;
 				}
 			}
 		}
 
-		boolean hasDarkTheme = getThemes().stream().anyMatch(t -> t.getId().startsWith(E4_DARK_THEME_ID));
+		boolean hasDarkTheme = getThemes().stream().anyMatch(t -> t.isDark());
 		boolean overrideWithDarkTheme = false;
 		if (hasDarkTheme) {
 			if (prefThemeId != null) {
@@ -607,7 +630,7 @@ public void restore(String alternateTheme) {
 				 * this case want to fall back to respect whether that previous choice was dark
 				 * or not. https://github.com/eclipse-platform/eclipse.platform.ui/issues/2776
 				 */
-				overrideWithDarkTheme = prefThemeId.contains("dark");
+				overrideWithDarkTheme = getPreferenceThemeIsDark(prefThemeId.contains("dark"));
 			} else {
 				/*
 				 * No previous theme selection in preferences. In this case check if the system

bundles/org.eclipse.e4.ui.css.swt.theme/src/org/eclipse/e4/ui/css/swt/theme/ITheme.java

@@ -26,4 +26,10 @@ public interface ITheme {
 	 * @return the label
 	 */
 	String getLabel();
+
+	/**
+	 *
+	 * @return does the theme use a dark background color
+	 */
+	boolean isDark();
 }

bundles/org.eclipse.e4.ui.swt.gtk/src/org/eclipse/e4/ui/swt/internal/gtk/DarkThemeProcessor.java

@@ -40,12 +40,11 @@ public void intialize() {
 				return;
 			}
 			ITheme theme = (ITheme) event.getProperty("theme");
-			final boolean isDark = theme.getId().contains("dark"); //$NON-NLS-1$
 			Display display = (Display) event.getProperty(IThemeEngine.Events.DEVICE);
 
 			// not using UISynchronize as this is specific to SWT/GTK
 			// scenarios
-			display.asyncExec(() -> OS.setDarkThemePreferred(isDark));
+			display.asyncExec(() -> OS.setDarkThemePreferred(theme.isDark()));
 		};
 		// using the IEventBroker explicitly because the @EventTopic annotation
 		// is unpredictable with processors within the debugger

bundles/org.eclipse.e4.ui.swt.win32/src/org/eclipse/e4/ui/swt/internal/win32/DarkThemeProcessor.java

@@ -39,8 +39,7 @@ public void intialize() {
 				return;
 			}
 			ITheme theme = (ITheme) event.getProperty("theme");
-			boolean isDark = theme.getId().contains("dark"); //$NON-NLS-1$
-			OS.setTheme (isDark);
+			OS.setTheme (theme.isDark());
 		};
 		// using the IEventBroker explicitly because the @EventTopic annotation
 		// is unpredictable with processors within the debugger

bundles/org.eclipse.e4.ui.workbench.renderers.swt.cocoa/src/org/eclipse/e4/ui/swt/internal/cocoa/CocoaDarkThemeProcessor.java

@@ -39,22 +39,23 @@ public void intialize() {
 				return;
 			}
 			ITheme theme = (ITheme) event.getProperty("theme"); //$NON-NLS-1$
-			final boolean isDark = theme.getId().contains("dark"); //$NON-NLS-1$
 			Display display = (Display) event.getProperty(IThemeEngine.Events.DEVICE);
 
 			// not using UISynchronize as this is specific to SWT/Mac
 			// scenarios
-			display.asyncExec(() -> OS.setTheme(isDark));
+			display.asyncExec(() -> OS.setTheme(theme.isDark()));
 		};
 		// using the IEventBroker explicitly because the @EventTopic annotation
 		// is unpredictable with processors within the debugger
 		eventBroker.subscribe(IThemeEngine.Events.THEME_CHANGED, eventHandler);
 	}
 
 	/**
-	 * Unsubscribes the {@code eventHandler} from the {@code eventBroker} to cleanup the resources
+	 * Unsubscribes the {@code eventHandler} from the {@code eventBroker} to cleanup
+	 * the resources
 	 *
-	 * Assumption : Both {@code eventHandler} and {@code eventBroker} are initialized and non null
+	 * Assumption : Both {@code eventHandler} and {@code eventBroker} are
+	 * initialized and non null
 	 */
 	@PreDestroy
 	public void cleanUp() {

bundles/org.eclipse.ui.themes/plugin.xml

@@ -9,29 +9,33 @@
             label="%theme.classic">
       </theme>
     <theme
-        basestylesheeturi="css/e4-dark_linux.css"
-        id="org.eclipse.e4.ui.css.theme.e4_dark"
-        label="%theme.dark"
-        os="linux">
+          basestylesheeturi="css/e4-dark_linux.css"
+          id="org.eclipse.e4.ui.css.theme.e4_dark"
+          isDarkTheme="true"
+          label="%theme.dark"
+          os="linux">
     </theme>
     <theme
-        basestylesheeturi="css/e4-dark_win.css"
-        id="org.eclipse.e4.ui.css.theme.e4_dark"
-        label="%theme.dark"
-        os="win32">
+          basestylesheeturi="css/e4-dark_win.css"
+          id="org.eclipse.e4.ui.css.theme.e4_dark"
+          isDarkTheme="true"
+          label="%theme.dark"
+          os="win32">
     </theme>
     <theme
-        basestylesheeturi="css/e4-dark_mac1013.css"
-        id="org.eclipse.e4.ui.css.theme.e4_dark"
-        label="%theme.dark"
-        os="macosx"
-        os_version="10.11,10.12,10.13">
+          basestylesheeturi="css/e4-dark_mac1013.css"
+          id="org.eclipse.e4.ui.css.theme.e4_dark"
+          isDarkTheme="true"
+          label="%theme.dark"
+          os="macosx"
+          os_version="10.11,10.12,10.13">
     </theme>
     <theme
-        basestylesheeturi="css/e4-dark_mac.css"
-        id="org.eclipse.e4.ui.css.theme.e4_dark"
-        label="%theme.dark"
-        os="macosx">
+          basestylesheeturi="css/e4-dark_mac.css"
+          id="org.eclipse.e4.ui.css.theme.e4_dark"
+          isDarkTheme="true"
+          label="%theme.dark"
+          os="macosx">
     </theme>
       <theme
             basestylesheeturi="css/e4_default_gtk.css"

tests/org.eclipse.e4.ui.tests.css.swt/src/org/eclipse/e4/ui/tests/css/swt/ThemeTest.java

@@ -57,7 +57,9 @@ public void setUp() {
 	@Override
 	@AfterEach
 	public void tearDown() {
-		themeListenerRegistration.unregister();
+		if (themeListenerRegistration != null) {
+			themeListenerRegistration.unregister();
+		}
 		super.tearDown();
 	}
 
@@ -94,4 +96,32 @@ private IThemeEngine getThemeEngine(Display display) {
 		return manager.getEngineForDisplay(display);
 	}
 
+	@Test
+	void settingIsDarkToTrueShoulReportThemeIsDark() {
+		Theme theme = new Theme("IdDoesntCare", "DescriptionDoesntCare");
+		theme.setIsDark("true");
+		assertTrue(theme.isDark(), "Theme should report to be dark");
+	}
+
+	@Test
+	void settingIsDarkToFalseShoulReportThemeIsNotDark() {
+		Theme theme = new Theme("IdDoesntCare", "DescriptionDoesntCare");
+		theme.setIsDark("false");
+		assertFalse(theme.isDark(), "Theme should report to be NOT dark");
+	}
+
+	@Test
+	void settingIsDarkToNullShoulReportThemeIsNotDark() {
+		Theme theme = new Theme("IdDoesntCare", "DescriptionDoesntCare");
+		theme.setIsDark(null);
+		assertFalse(theme.isDark(), "Theme should report to be NOT dark");
+	}
+
+	@Test
+	void settingIsDarkToInvalidValueShoulReportThemeIsNotDark() {
+		Theme theme = new Theme("IdDoesntCare", "DescriptionDoesntCare");
+		theme.setIsDark("invalid");
+		assertFalse(theme.isDark(), "Theme should report to be NOT dark");
+	}
+
 }