Bug 135344

Comment 1 Steven R. Shaw 2006-04-06 15:07:16 EDT

Or rather then face another flame war :), GMF could force load of this specific class on initialization of it's Editor.

Comment 2 Randy Hudson 2006-04-06 15:33:26 EDT

I think this is valid, and maybe a duplicate of another open bug. (Unlike the update manager, which does its work on the UI thread by design). But, if you are just constructing some figures and then printing them or capture them to an image, background threads are valid. SWT lets you create Colors, Images, and Fonts on background threads. You can also paint onto an Image in a background thread. So, there is a use case for building an entire diagram in a background thread, in which case ColorConstants might get referenced. An IBM product generates file thumbnails this way.

Comment 3 Steven R. Shaw 2006-04-10 16:52:11 EDT

Committed change.  Modified Display.getCurrent() to Display.getDefault in ColorConstants

Comment 4 Steven R. Shaw 2006-04-20 11:05:05 EDT

This fix is unlikely to work since getSystemColor calls checkDevice which checks for invalid thread access.  Randy was going to consult with SWT team to see if the checkDevice call was necessary in this case.

Comment 5 Randy Hudson 2006-04-20 13:27:02 EDT

Needs target milestone.

This is still broken:
Caused by: org.eclipse.swt.SWTException: Invalid thread access
	at org.eclipse.swt.SWT.error(SWT.java:3374)
	at org.eclipse.swt.SWT.error(SWT.java:3297)
	at org.eclipse.swt.SWT.error(SWT.java:3268)
	at org.eclipse.swt.widgets.Display.error(Display.java:978)
	at org.eclipse.swt.widgets.Display.checkDevice(Display.java:638)
	at org.eclipse.swt.widgets.Display.getSystemColor(Display.java:1931)
	at org.eclipse.draw2d.ColorConstants.<clinit>(ColorConstants.java:26)
	... 3 more

Comment 6 Randy Hudson 2006-04-20 13:27:37 EDT

Test case:

public static void main(String[] args) {
	final Display display = new Display();
	Shell shell = new Shell(display);
	shell.open();
	
	new Thread(){
		public void run() {
			try {
				Class.forName("org.eclipse.draw2d.ColorConstants");
			} catch (Exception e) {
				e.printStackTrace();
			}
		}
	}.start();
	
	while (!shell.isDisposed()) {
		if (!display.readAndDispatch())
			display.sleep();
	}
	display.dispose();
}

Comment 7 Randy Hudson 2006-04-20 13:29:49 EDT

Created attachment 39070 [details]
patch

Here is a patch to fix the problem.

+1 for RC1

Comment 8 Steven R. Shaw 2006-04-20 13:53:30 EDT

Thanks Randy.

Only comments:
- rename Internal to something more relevant. --> SystemColorFactory (?)
- could we add your test case as a JUnit?

+1

Comment 9 Steven R. Shaw 2006-04-20 14:20:03 EDT

I seem to run into deadlock when I run a JUnit I created:

	public void test_Thumbnail() {
		final Boolean result[] = new Boolean[1];
		Thread testThread = new Thread() {
			public void run() {
				try {
					Class.forName("org.eclipse.draw2d.ColorConstants");
					result[0] = Boolean.TRUE;
				} catch (Exception e) {
					result[0] = Boolean.FALSE;
				}
				
				interrupt();
			}
		};
		
		testThread.start();
		try {
			testThread.join();
		} catch (InterruptedException e) {
			assertTrue(result[0].booleanValue());
		}

	}

Comment 10 Steven R. Shaw 2006-04-20 14:21:14 EDT

the name of the JUnit is obviously incorrect... should read test_ColorConstantsInit

Comment 11 Randy Hudson 2006-04-20 14:30:30 EDT

The junit setup is blocking the UI thread since it runs from the UI. You might be able to avoid the problem by running the event loop from inside the test case while waiting for the background thread to complete.

Comment 12 Steven R. Shaw 2006-04-20 15:03:12 EDT

Created attachment 39089 [details]
updated patch

- Updated patch with minor name change of Internal class + added passing JUnit.
- Verified that JUnit doesn't pass without changes to ColorConstants

Comment 13 Steven R. Shaw 2006-04-20 16:19:26 EDT

Committed patch + new JUnit

Comment 14 Steven R. Shaw 2006-04-21 08:11:37 EDT

Updated JUnit test to make event loop thread safe.  Apparently, when a JUnit Suite is run, it's on a separate thread.--> caused NPE when using Display.getCurrent().  When running the JUnit individually, it's on the main thread, so it passed in my dev environment.

Comment 15 Randy Hudson 2006-04-21 08:26:43 EDT

When you ran it individually, did you invoke it as a plug-in junit, or a normal junit?