Move workbench running check in ColorManager to prevent SWT errors when headless

[trancexpress]

This change adjusts the check added for Eclipze bugzilla 546205 ticket. The check for running workbench is now done at the start of ColorManager.initializeDefaults(), to prevent the creation of an SWT Display object when in headless mode.

See: https://bugs.eclipse.org/bugs//show_bug.cgi?id=546205

Fixes: #1486

[trancexpress]

@iloveeclipse who would be interested in merging this?

[iloveeclipse]

@iloveeclipse who would be interested in merging this?

Me, but I can't merge alone, we are before RC2 build.

@HannesWell : the patch is pretty straightforward. Do we want it in 4.34 RC2?

[trancexpress]

We are not in a rush, we have patched our own 4.30 platform.

[iloveeclipse]

We are not in a rush, we have patched our own 4.30 platform.

We are not alone :)

[github-actions]

Test Results

   285 files     285 suites   46m 11s ⏱️
 3 586 tests  3 510 ✅  76 💤 0 ❌
10 950 runs  10 719 ✅ 231 💤 0 ❌

Results for commit 7665c9e.

[HannesWell]

@HannesWell : the patch is pretty straightforward. Do we want it in 4.34 RC2?

It is, but I wonder if use-cases exist where an SWT Display exists but the workbench is not running? Maybe a customized SWT application that does not use the Eclipse Workbench?
But I not sure what the best way would be to check if there is any display at all? Unfortunately there is no Display.anyExists() or alike. And Display.getCurrent() might return null even if Displays are available.
The only solution I could think of at the moment would be to call Display.getDefault() and catch the SWTError if any is thrown and then return and continue if none is thrown. Display.getDefault() has the known 'danger' in itself, but since the static initializer of PreferenceConverter will call it anyways, I think it should be safe. But this seems a bit hacky to me.
If we think there are more situations where we would want to know if any display exists already I think it would be nice to add a corresponding API to SWT's Display class for it.

Since the situation is not 100% clear for me and the issue exists for five years now and you are the first one to complain, I'm not confident that we should add this fix at literally the last possibility.

[laeubi]

This change adjusts the check added for Eclipze bugzilla 546205 ticket. The check for running workbench is now done at the start of ColorManager.initializeDefaults(), to prevent the creation of an SWT Display object when in headless mode.

Just some thoughts here:

1. The (original) ticket does not talk about headless mode but a Threading issue in combination with highcontrast
2. You stacktrace more originates at JFace so even if you fix it here, it might arise on other places
3. Another way would be to only initlize() on first access of a color (would also speedup Workbench loading a few milisecconds)

Regarding

But I not sure what the best way would be to check if there is any display at all?

You can collect all threads and then check for each one if there is a Display, but ...

The only solution I could think of at the moment would be to call Display.getDefault() and catch the SWTError if any is thrown and then return and continue if none is thrown. Display.getDefault() has the known 'danger' in itself, but since the static initializer of PreferenceConverter will call it anyways

I think the better way would be to get rid of static initializer as it can cause other problems as well and only is used for deprecated fields that could have been removed in these 5 years already ;-)

As an intermediate solution, it is probably much easier to use StringConverter.asRGB directly in the ColorManger to avoid the problem altogether.

[trancexpress]

Just some thoughts here:

1. The (original) ticket does not talk about headless mode but a Threading issue in combination with highcontrast

This change refers to: https://bugs.eclipse.org/bugs//show_bug.cgi?id=546205#c4

The block we are moving was introduced due to that.

2. You stacktrace more originates at JFace so even if you fix it here, it might arise on other places

So far we ran only into this issue.

As an intermediate solution, it is probably much easier to use StringConverter.asRGB directly in the ColorManger to avoid the problem altogether.

I've checked, that works for us too. Do you prefer this change:

diff --git a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/text/ColorManager.java b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/text/ColorManager.java
index 2e33ab5bc2..3f2036cc5e 100644
--- a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/text/ColorManager.java
+++ b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/text/ColorManager.java
@@ -48,19 +48,19 @@ public class ColorManager implements IColorManager, IPDEColorConstants {
 	}
 
 	public static void initializeDefaults(IPreferenceStore store) {
-		PreferenceConverter.setDefault(store, P_DEFAULT, DEFAULT);
-		PreferenceConverter.setDefault(store, P_PROC_INSTR, PROC_INSTR);
-		PreferenceConverter.setDefault(store, P_STRING, STRING);
-		PreferenceConverter.setDefault(store, P_EXTERNALIZED_STRING, EXTERNALIZED_STRING);
-		PreferenceConverter.setDefault(store, P_TAG, TAG);
-		PreferenceConverter.setDefault(store, P_XML_COMMENT, XML_COMMENT);
-		PreferenceConverter.setDefault(store, P_HEADER_KEY, HEADER_KEY);
-		PreferenceConverter.setDefault(store, P_HEADER_OSGI, HEADER_OSGI);
+		setDefault(store, P_DEFAULT, DEFAULT);
+		setDefault(store, P_PROC_INSTR, PROC_INSTR);
+		setDefault(store, P_STRING, STRING);
+		setDefault(store, P_EXTERNALIZED_STRING, EXTERNALIZED_STRING);
+		setDefault(store, P_TAG, TAG);
+		setDefault(store, P_XML_COMMENT, XML_COMMENT);
+		setDefault(store, P_HEADER_KEY, HEADER_KEY);
+		setDefault(store, P_HEADER_OSGI, HEADER_OSGI);
 		store.setDefault(P_HEADER_OSGI + IPDEColorConstants.P_BOLD_SUFFIX, true);
-		PreferenceConverter.setDefault(store, P_HEADER_VALUE, HEADER_VALUE);
-		PreferenceConverter.setDefault(store, P_HEADER_ATTRIBUTES, HEADER_ATTRIBUTES);
+		setDefault(store, P_HEADER_VALUE, HEADER_VALUE);
+		setDefault(store, P_HEADER_ATTRIBUTES, HEADER_ATTRIBUTES);
 		store.setDefault(P_HEADER_ATTRIBUTES + IPDEColorConstants.P_ITALIC_SUFFIX, true);
-		PreferenceConverter.setDefault(store, P_HEADER_ASSIGNMENT, HEADER_ASSIGNMENT);
+		setDefault(store, P_HEADER_ASSIGNMENT, HEADER_ASSIGNMENT);
 		if (!PlatformUI.isWorkbenchRunning()) {
 			return;
 		}
@@ -68,9 +68,9 @@ public class ColorManager implements IColorManager, IPDEColorConstants {
 			Display display = PlatformUI.getWorkbench().getDisplay();
 			Runnable runnable = () -> {
 				if (!display.isDisposed() && display.getHighContrast()) {
-					PreferenceConverter.setDefault(store, P_DEFAULT, DEFAULT_HIGH_CONTRAST);
-					PreferenceConverter.setDefault(store, P_HEADER_VALUE, HEADER_VALUE_HIGH_CONTRAST);
-					PreferenceConverter.setDefault(store, P_HEADER_ATTRIBUTES, HEADER_ASSIGNMENT_HIGH_CONTRAST);
+					setDefault(store, P_DEFAULT, DEFAULT_HIGH_CONTRAST);
+					setDefault(store, P_HEADER_VALUE, HEADER_VALUE_HIGH_CONTRAST);
+					setDefault(store, P_HEADER_ATTRIBUTES, HEADER_ASSIGNMENT_HIGH_CONTRAST);
 				}
 			};
 			if (display == Display.getCurrent()) {
@@ -142,4 +142,8 @@ public class ColorManager implements IColorManager, IPDEColorConstants {
 			putColor(event.getProperty(), StringConverter.asRGB(color.toString()));
 		}
 	}
+
+	private static void setDefault(IPreferenceStore store, String name, RGB value) {
+		store.setDefault(name, StringConverter.asString(value));
+	}
 }

Since the situation is not 100% clear for me and the issue exists for five years now and you are the first one to complain, I'm not confident that we should add this fix at literally the last possibility.

We can wait until after the code freeze, that is not an issue.

Still though, it would be good to know what change you prefer / will accept, so that we know how to patch on our end.