Bug 166972

Comment 1 Thomas Watson 2006-12-06 12:20:27 EST

Created attachment 55146 [details]
patch

This patch adds the new stop method to IDEApplication.  The main advantage to moving to the new IApplication interface is to give an external way to shutdown the application.  Currently there is no way to force an application to shutdown.  This causes problems when the framework is shutdown because it leaves the application running and does not give it a chance to shutdown.

I'm not sure if the code I put in the stop() method is the best way to force the workbench to shutdown.  Can someone on the UI team review this and let me know what the best practice would be here.  We have updated the RCP templates to use this approach.  Please let me know if there is a better way.  Thanks.

Comment 2 Tod Creasey 2006-12-07 13:41:50 EST

See WorkbenchWindow line 597 for how we close the Workbench. I would be very reluctant to make any changes to this without a strong reason.

I'll commit the rest of the patch however.

Comment 3 Tod Creasey 2006-12-07 14:14:31 EST

The test suites will not run if I apply this patch. You can replicate this by loading org.eclipse.platform.ui.tests from HEAD and running the UiTestSuite configuration

Here is the exception.


!ENTRY org.eclipse.equinox.app 4 0 2006-12-07 18:59:22.184
!MESSAGE Error in invoking method.
!STACK 0
junit.framework.AssertionFailedError
	at junit.framework.Assert.fail(Assert.java:47)
	at junit.framework.Assert.assertTrue(Assert.java:20)
	at junit.framework.Assert.assertNotNull(Assert.java:217)
	at junit.framework.Assert.assertNotNull(Assert.java:210)
	at org.eclipse.pde.internal.junit.runtime.UITestApplication.run(UITestApplication.java:41)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:615)
	at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethod(EclipseAppContainer.java:522)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:147)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:74)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:354)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:170)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:615)
	at org.eclipse.core.launcher.Main.invokeFramework(Main.java:339)
	at org.eclipse.core.launcher.Main.basicRun(Main.java:283)
	at org.eclipse.core.launcher.Main.run(Main.java:984)
	at org.eclipse.core.launcher.Main.main(Main.java:959)

Comment 4 Thomas Watson 2006-12-07 15:33:28 EST

This is because org.eclipse.pde.internal.junit.runtime.UITestApplication needs to be migrated to an IApplication also.  I will attach a patch to the org.eclipse.pde.junit.runtime project.

Tod I don't really understand comment 2.  Do you have a recommended approach we should use in the IApplication.stop() method or is the patch good enough here?  (it does call IWorkbench.close())

Comment 5 Wassim Melhem 2006-12-07 15:36:41 EST

tom, I am not sure pde junit applications can migrate.

sometimes people target an RCP drop and want to run plugin junit tests.  In this case, pde launches the junit apps that are in the host.

If the rcp target is 3.2 based or earlier, the pde junit apps will not run if they are upgraded to implement IApplication.

Comment 6 Thomas Watson 2006-12-07 15:40:25 EST

Created attachment 55273 [details]
pde.junit.runtime patch

Here is a patch to UITestApplication to allow it to handle both the old IPlatformRunnable objects and IApplication objects as application types.

Wassim, can you please release this fix?

Comment 7 Tod Creasey 2006-12-07 15:51:55 EST

I pointed you to how we do it. You should not close the workbench yourself - the workbench plug-in stop() will handle it for you.

Could you log a bug to PDE about the PDE changes? I would like to make this dependant on it and I want to wait until they are ready before I release it.

Comment 8 Thomas Watson 2006-12-07 16:08:01 EST

Opened PDE bug 167163

Comment 9 Thomas Watson 2006-12-07 17:09:59 EST

Tod and I discussed this a bit more.  It looks like the javadoc for IApplication needs to be improved.  It was unclear when the stop() method would be called.  This method is *only* called to force an application to exit.  It is not called under normal conditions when an application exits normally.

I updated the IApplication javadoc to clear this up (hopefully).

Given that I think the patch is a reasonable way to force the ide application to exit.

Comment 10 Tod Creasey 2006-12-08 08:28:09 EST

I will apply Toms patch when Bug 167163 is fixed.

Comment 11 Wassim Melhem 2006-12-12 01:21:26 EST

will this patch go into M4?  Also, what, if anything, will happen if the PDE fix is in and the workbench fix is not?

Comment 12 Tod Creasey 2006-12-12 08:07:26 EST

No - as I am dependant on Bug 167163 I had to wait until it was in and it is too late now for M4.

Tom is there any issue with this?

Comment 13 Thomas Watson 2006-12-12 08:48:48 EST

It should not pose any functional issues for M4.  The motivation behind migrating the IDE was to get more testing on the M4 build on the new app model (and get rid of your deprecation errors).  The fix in PDE is able to handle both old IPlatformRunnables and IApplicaitons.

Comment 14 Tod Creasey 2007-01-09 10:02:58 EST

Patch released for build >20060109

Comment 15 Tod Creasey 2007-02-06 13:52:12 EST

Verified in 20070206-0010