Bugzilla – Full Text Bug Listing
| Summary: | [RCP] JFace dependency on org.eclipse.core.runtime enlarges standalone JFace applications | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Ed Burnette <ed.burnette> | ||||||||
| Component: | UI | Assignee: | Nick Edgar <n.a.edgar> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | andre_weinand, bogofilter+eclipse.org, bokowski, bpasero, c.hauser, daniel_megert, david, dev, douglas.pollock, gmendel, ian.leslie, jean-michel_lemieux, jeem, jeffmcaffer, john.arthorne, kehn, luigiwalser, mlists, mn, overholt, ovidr, pascal, richkulp, robert.simmons, schtoo, support, susan, Tod_Creasey, vulcannis, walkerp, yymadrian | ||||||||
| Version: | 3.0 | Keywords: | helpwanted | ||||||||
| Target Milestone: | 3.2 M4 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | 50499, 88577, 115372, 117545 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Ed Burnette
While the eclipse runtime is needed for running RCP apps, JFace is not "eclipse runtime aware" aside from its dependencies on utility classes like IProgressMonitor, and IStatus. One exception is a dependency on Platform.run (ISafeRunnable), which requires the runtime to actually be running if the runnable fails, in order to log the exception. It would be good to clean this up and minimize dependencies on the runtime. Jeff, can you comment on the minimum requirements for the runtime? Did we ever come to a concrete decision as to whether or not we were going to support JFace running independently of Eclipse? If we are then this should be fixed. If not, then there is no need. Note that fixing this either requires JFace to have copies of all the required classes or some hacking with jars etc to put all the utils that JFace needs somewhere it can get it when the runtime is not around and yet get it from the runtime if the runtime is there. Not sure what exactly you are after wrt minimum runtime requirements. For the record, JFace has the following dependencies on org.eclipse.core.runtime, classified by API refs and non-API refs: refs to Platform.getDebugOption: non-API: ActionContributionItem.handleWidgetSelection refs to Platform.run / ISafeRunnable: API: SafeRunnable implements ISafeRunnable non-API: AbstractTreeViewer.doUpdateItem, fireTreeCollapsed, fireTreeExpanded CellEditor.fireApplyEditorValue, fireCancelEditor, fireEditorValueChanged, fireEnablementChanged CheckboxTableViewer.fireCheckStateChanged DelegatingDragAdapter.dragFinished, dragSetData, dragStart, dragOperationChanged, dragOver, drop, dropAccept LabelProvider.fireLabelProviderChanged PreferenceDialog.cancelPressed, showPage StructuredViewer.fireDoubleClick, fireOpen, firePostSelectionChanged, refreshItem, updateItem Viewer.fireSelectionChanged refs to IStatus: API: ErrorDialog constructor and other API methods ProgressMonitorDialog.setBlocked(IStatus) // from IProgressMonitorWithBlocking AccumulatingProgressMonitor.setBlocked(IStatus) // from IProgressMonitorWithBlocking refs to IProgressMonitor: API: IElementCollector.add(...) IRunnableWithProgress.run(IProgressMonitor) IStatusLineManager.getProgressMonitor() ModalContext.canProgressMonitorBeUsed(...) ModalContext.checkCanceled(IProgressMonitor) ModalContext.run(...) ProgressMonitorDialog.getProgressMonitor() ProgressMonitorPart implements IProgressMonitor StatusLineManager.getProgressMonitor() SubStatusLineManager.getProgressMonitor() WizardDialog.getProgressMonitor() non-API: StatusLine (internal) implements IProgressMonitor refs to IProgressMonitorWithBlocking: non-API: ProgressMonitorDialog.ProgressMonitor implements IProgressMonitorWithBlocking refs to NullProgressMonitor: non-API: ApplicationWindow.run(...) refs to ProgressMonitorWrapper: non-API: AccumulatingProgressMonitor extends ProgressMonitorWrapper refs to OperationCanceledException: non-API: ModalContext.runInCurrentThread Of these referenced types, most are self-contained. That is, they don't lead to refs to other runtime objects. Refs to the Platform class are problematic because it in turn refers to many other runtime types: IAdapterManager, IExtensionRegistry, IJobManager, ILog, ILogListener, IPath, IPluginRegistry, Plugin. And these are just API refs -- there are more dependencies in its implementation. The refs to Platform could be fixed up as follows. Refs to Platform.getDebugOption could get replaced with refs to new JFace static methods (e.g. ActionContributionItem.get/setDebugTracing(boolean)), which the Workbench would initialize with values from the runtime. Refs to Platform.run(ISafeRunnable) could get replaced using a pluggable strategy for running an ISafeRunnable. The default strategy would just log errors to the console. The Workbench would replace this with one that calls Platform.run. Alternatively, the internal refs could be replaced with a JFace-specific exception handling mechanism, but we'd still be left with the API ref from SafeRunnable. With these changes, JFace would be left with refs to the following types from runtime: IProgressMonitor IProgressMonitorWithBlocking ISafeRunnable IStatus NullProgressMonitor OperationCanceledException ProgressMonitorWrapper These could be packaged into a small, separate, utility jar in runtime that would be ~6K in size (assuming our build process could handle such fine granularity of packaging). Then, to use JFace standalone, it would only require this utility jar from runtime, not the 238K runtime.jar. Changed title to reflect that the size impact is really only an issue for standalone JFace apps, not RCP apps based on the Eclipse runtime. Just to be clear, what we are talking about here is splitting up runtime.jar into runtime.jar and utility.jar. Both of these would be part of the runtime plugin but someone wanting to do a JFace stand-alone app could copy the jar out and setup their classpath accordingly. This can be done by the runtime team moving the relevant classes to different source folders and configuring the build to create two jars instead of one. (create the utility.jar first and then runtime.jar). "Should be easy"(tm) Note that in the future we are looking to ship Eclipse plugins as jars themselves. These jars would contain directories of code not subjars. In this case we would likely end up with two dirs (e.g., bin-runtime and bin-utility) in the runtime plugin jar. Standalone JFace folks would have to extract and jar up the contents of bin-utility. In this scenario it may not be feasible/easy to have the build process produce the utility.jar as well. Antoher related possibility is for JFace to have copies of the utility code and produce/include utility.jar in their deliverable but not put it on their classpath. If you want to go forward on this, suggest you enter a bug in Equinox to split up the runtime as described. Having JFace copy the classes isn't so tasty since they start with "org.eclipse.core.runtime". Of course, this won't be so tasty for standalone JFace clients either way. Two other options I can think of: 1. Emasculate org.eclipse.core.runtime so that it contains only the most basic public classes and interfaces like IStatus, moving most of the guts to something like org.eclipse.core.platform . JFace would depend on the old plug- in but code requiring platform services (for example the Platform class) would depend on the new plug-in. I think this is similar to what was done to the help plug-in. 2. Make org.eclipse.core.runtime depend on a new plug-in, for example org.eclipse.core.common. The new plug-in contains only the common public classes and interfaces like IStatus that even non-platform JFace applications need. JFace would depend on the new plug-in. #1 would probably require the fewest changes in existing code unless you do the trick you did with IPlatformRunnable - defining it in two places (boot and runtime) with one referring to the other. I just wanted to add my voice to the crowd that would liketo see JFace be a legit standalone jar. I think the advantages to the advancement of SWT would be significant. As for the decoupling of the JFace API from the core, I would suggest that the core depend on JFace and not vice versa. Simply move dependent interfaces to JFace and repackage them. We could never make the runtime depedant on JFace as then you could no longer run it headless. Breaking up the runtime to reduce the dependancies would make more sense. I added my vote to this issue, I am really looking forward in using JFace without the need of the runtime. Ben I would like to see this happen for 3.1 as well. At a minimum, we should partition the runtime jar as Jeff suggested above, and eliminate the direct dependency on class Platform from JFace. If anyone would like to help work on this, or help out with testing, please speak up. Sure, I would immediately make use of the seperated JFace and test it to report problems in this report. Ben I could do it. I think I'm familiar enough with JFace (at least from a user's perspective) to work my way through this. I've certainly created my fair share of standalone SWT/JFace apps to know the whats intended. I should be able to start on this fairly soon. The main uses of class Platform are Platform.run(ISafeRunnable) and for checking debug options. The latter should have already been refactored into org.eclipse.jface.util.Policy but if there are some stragglers, that's where they should go. When JFace is used non-standalone, these settings are obtained from Platform by the workbench. For run(ISafeRunnable), the best approach would be to provide a pluggable strategy for running an ISafeRunnable, e.g. SafeRunnable.setRunner(ISafeRunnableRunner) where ISafeRunnableRunner has a run method whose spec is the same as Platform.run. Ok. I'm going to create an ISafeRunnableRunner with two methods:
run(ISafeRunnable)
handleException(ISafeRunnable,Throwable)
The second method will allow the Platform to get a first look at the error and
log it before the ISafeRunnable.handleException() is called (to maintain
current behavior). This interface will go in the runtime
(org.eclipse.core.runtime).
I will also add a static instance of ISafeRunnableRunner with getters/setters
to SafeRunnable. I will add a default implementation of ISafeRunnableRunner.
I will also create a new method run() on SafeRunnable which will delegate to
the current ISafeRunnableRunner. Therefore the new code throughout JFace will
be:
MySafeRunnableClass runnable = new MySafeRunnableClass();
runnable.run();
or
SafeRunnable.getRunner().run(new ISafeRunnable(){...});
Also, I looked at the JFace Policy class and I see how I should use it, but I
also noticed that it currently references the Platform. Shouldn't this be
refactored as well?
> The second method will allow the Platform to get a first look at the error
I don't think the second method is needed to maintain current behaviour.
I think the run method will suffice.
Althought Platform.run() does call a handleException method, this is internal
and does not need to be exposed to the client of the ISafeRunnableRunner.
Ok, I understand. I've completed those changes and did another search to find
any other runtime references. I've found a couple that weren't mentioned in
the previous comments:
refs to Status:
Dialog (constructor)
PreferenceDialog (okPressed)
refs to CoreException:
ErrorDialog (throughout)
refs to Platform:
PreferenceDialog (okPressed)
Policy (getDebugOptions)
refs to ILog,ILogListener,Bundle:
Policy (throughout)
Let me propose how I would handle them and you can review. For Status, I think
its appropriate to include Status in the new utility.jar. This would also mean
that the runtime's Assert and AssertionFailedException would have to come along
too. The only other issue with bringing Status into utility.jar is that it
calls the runtime's Policy. This would have to be removed/refactored. It
calls Policy.bind("ok") to get the standard message for the Status.OK_STATUS.
I don't see how useful this is. Maybe it could just be removed and a blank
message used (a blank message is used for Status.CANCEL_STATUS).
For CoreException, this could also be included in utility.jar. The only
reference within CoreException is IStatus so it would be easy to include.
For Platform, on PreferenceDialog the okPressed() just uses Platform to log an
error. This could be refactored to use the Policy.getLog().log(). The
Policy.getDebugOptions() checks if the Platform.isRunning() and if so,
delegates the call to the Platform. We'd have to use a strategy similar to the
SafeRunnable stuff. That is, we'd have to have the Platform call Policy
instead of the Policy calling Platform (basic IOC/DI). There would have to be
some sort of IDebugOptionManager (or whatever).
Finally there's the refs coming from Policy. ILog, ILogListener and Bundle all
could be put in utility.jar. Bundle would have to bring along BundleException
but thats all.
So our new additions to utility.jar would be:
Status
Assert (runtime's)
AssertionFailedException (runtime's)
CoreException
ILog
ILogListener
Bundle
BundleException
also ISafeRunnableRunner
and also anything we create to fix the debug options issue in Policy.
The utility jar should not include the OSGi classes Bundle and BundleException. I also don't think it should include ILog. The log references should be refactored to a JFace level log interface, with a simple default implementation which the workbench can override to call the runtime's log mechanism. Others might want to talk to Log4J or whatever. Success. I've refactored everything, created my own utility.jar for testing and everything seems to be working. In order to wire things together I created a JFacePlugin. In startup(), it references the Platform and ties it together with JFace. This will be the only remaining class in jface.jar that references the runtime (well the runtime classes that aren't being put in utility.jar), but since this should never be called in a standalone app its ok. I also needed to hardcode the runtime's plug-in id in Status to remove the reference to Platform.PI_RUNTIME so Status could be put in utility.jar. I wouldn't like to do that, but it seemed the easiest and cleanest solution (and didn't change any API). For lack of a better name, I called the new log interface in JFace, ILogger. I also needed to alter the workbench plugin. The WorkBenchPlugin referenced the Policy.setLog(). In fact, this is the only significant area of concern for me (Policy.setLog() and Policy.getLog()) because I've introduced an API change. That is, replacing ILog with ILogger. I tested with some small standalone apps and within the workbench itself. Happy New Year! Created attachment 16877 [details]
Runtime patch
Created attachment 16878 [details]
JFace patch
Created attachment 16879 [details]
Workbench patch
Oh almost forgot, here is the final list of classes for utility.jar: from org.eclipse.core.internal.runtime: Assert AssertionFailedException from org.eclipse.core.runtime: CoreException IProgressMonitor IProgressMonitorWithBlocking ISafeRunnable ISafeRunnableRunner IStatus NullProgressMonitor OperationCanceledException ProgressMonitorWrapper Status I'm not sure how we are going to create the utility.jar. the runtime plugin will be shipping as a single JAR which needs to include all the required code. We can get the build to produce the utility.jar but it will be a challenge to do something with it? For example, where would we put it? It can't go in the runtime plugin. Similarly it can't go in the JFace plugin. Its like it needs to be made available somewhere on a web page that details how to use JFace in stand-alone mode. Luckily we don't change that code very often so perhaps the utility.jar can just be built/updated by hand every so often? If that is not going to work then Releng should be queried to see how to handle this random jar that no one wants to own. I thought it would go in the runtime plug-in as per comment #6. Stand-alone apps that don't use plug-ins would just suck in the jars from wherever they are now and put them in the deployment directory. It seems like an awful lot of classes in a utility jar though. Do you have an updated size for the jars after this change? What affect did this turn out to have on the size of the standalone Jface programs? The resulting utility.jar was around 9k. The runtime.jar and osgi.jar were around 400k or so. comment 6 was made before we were really doing the plugins as JARs approach. The current plan is to go down that path. As a result, there really is no place to put the utility.jar in the runtime plugin. The runtime plugin itself will be one jar and nested jars are not supported in the tooling. So, utility.jar is somewhat of an orphan. This approach is still useful/interesting. What was an implementation problem has now been reduced to a packaging/delivery problem. That is good progress. My suggestion is that the UI team decide how they want to support standalone JFace. That will dictate how utility.jar should be handled. Jeff, can you point to a reference about nested jars not being supported? I thought I had tried that in one of the 3.1M builds and it worked ok; it looked like somebody was simply unpacking the top level jar and creating a plugins directory in a temporary location and from that point everything worked normally. Not the most efficient implementation but it would support (in fact requried) nested jars. nested jars work at runtime (OSGi extracts the inner jar and caches it) but not at development time. JDT (actually any Java compiler) is unable to handle nested jars on the classpath. So JFace in your workspace will not compile against the deployed (binary) org.eclipse.core.runtime.jar because the utility.jar classes will not be found (they are nested inside). Of course, JDT could be updated to work for this usecase but I didn't have any luck convincing them plus other Java compilers in the world will not do it and some people like to compile their world with javac or whatever so we are stuck. Officially the only supported plugin configurations are "traditional" (in directories as normal) and "JAR'd" with code at the root, not nested, not in a subdir. How about reconsidering putting copies of the classes in question into jface.jar. Its not the prettiest solution, but it makes standalone development a little simpler by removing a jar (one less thing to remember/know). I don't think they belong in jface.jar. How about a new plugin, say org.eclipse.core.base, that contains only the utility classes common to both RCP and Jface apps. Look at org.eclipse.help.base for a parallel. org.eclipse.core.runtime would require it. Would that address the packaging/building problem? Chris, sorry for the delay in responding. I haven't forgotten you, and will be reviewing your patches for M6. No problem. I'm patient :) Per bug #87669, there is a new dependency on IAdaptable. Please annotate that bug if there is any reason IAdaptable can't be included in the JAR. I would say that making a new plugin would be the best way to go. The question would be are there any downsides to that solution? I have reviewed and released Chris's patches, with some changes. Here are my notes. - I haven't applied the patch to org.eclipse.core.runtime itself. Instead, I've copied the utility classes, plus slight modifications to reduce dependencies, to a separate source folder in org.eclipse.jface/runtime-util. This was included on the classpath instead of the org.eclipse.core.runtime when doing this refactoring (the classpath change has not been released). Still need to sort out the best way to ship the utility.jar with the runtime team. - replaced Assert.isLegal(c) with: if (!c) throw IllegalArgumentException - saves having to include Assert and AssertionFailedException from org.eclipse.core.internal.runtime - added IAdaptable, which is needed by org.eclipse.core.commands (a new prerequisite of JFace) - see bug 87669 - changed default ISafeRunnableRunner to only handle Exceptions and LinkageErrors, not all Throwables, and to not printStackTrace() for OperationCanceledExceptions. - this is equivalent to what Platform.run was doing when the runtime was not initialized - moved JFace-in-eclipse setup code to the Workbench layer, in class JFaceUtil - this avoids dependencies on the runtime from JFace, but does mean that apps using JFace within the Eclipse runtime, but not using the Workbench, will need to do similarly - this logs to org.eclipse.ui.workbench's log, rather than org.eclipse.jface's log, which is the same as before (one of our commands tests relies on this behaviour) - added utility classes MultiStatus and SubProgressMonitor - although not used by JFace itself, these are useful for JFace clients - changed SafeRunnable.setRunner to allow null, to reset to default runner, and fixed up getRunner() accordingly - changed PreferenceStore.firePropertyChangeEvent to wrap each notification in a SafeRunnable, rather than wrapping all notifications in a single SafeRunnable - cleaned up some of the Javadoc in: Policy, SafeRunnable, ILogger - fixed up copyrights on new compilation units, with proper attribution to Chris on new CUs and those with major changes Notes: - Need to document breaking API changes. Filed Bug 88608 [JFace] Document breaking API change to Policy.get/setLog Problems: - JFace's deferred viewers package (new in 3.1) has a dependency on the jobs mechanism in runtime - filed Bug 88577 [Viewers] Deferred viewers in JFace have a dependency on Job Questions: - Who should provide the utility jar? If JFace, should it always be included? If yes, then it can't exist as a separate jar due to jar'ed bundles. How to set up the plugin.xml/manifest.mf such that o.e.core.runtime is seen, not the utility classes? BTW, thanks Chris, nice work. No problem. I'm just excited to see more standalone SWT/JFace apps! The patch went in for M6. Still need to sort out how best to deploy the utility jar. Will address this in M7. Currently the proposal is: - move the runtime-util source folder down from org.eclipse.jface to org.eclipse.core.commands, since it requires these too, with the same restrictions - include the corresponding class files in the plugin jar produced for org.eclipse.core.commands (a small duplication of what's in org.eclipse.core.runtime) - use imports in an explicit manifest.mf for org.eclipse.core.commands to ensure that, when running within Eclipse, it gets these classes from org.eclipse.core.runtime instead of its own jars *** Bug 67050 has been marked as a duplicate of this bug. *** Note that this is currently breaking API. See Bug 92819 Bug 88608 was already filed for the breaking API change. The current proposal is to create a separate project containing the various prerequisites that various plug-ins require for different RCP configurations, including things like: - the runtime utility classes (currently under org.eclipse.jface/runtime-util) - the JCL/Foundation classes (currently in org.eclipse.osgi/osgi/ee.foundation.jar) - the XML parser APIs used when compiling against JCL/Foundation (currently in org.eclipse.osgi/osgi/xmlParserAPIs.jar) - the other exception cases when compiling against JCL/Foundation (currently in org.eclipse.osgi/osgi/exceptions.jar) Different dependent projects may use some or all of these. One downside with this approach is that navigating to the base classes using F3 will not show the appropriate source. To fix this would require extra support in JDT for referring to source jars indirectly via classpath variables. Open Type (Ctrl+Shift+T) is a workaround, but is inconvenient. There aren't really any great answers here for compile-time for 3.1, since we weren't able to eliminate -all- the dependencies on org.eclipse.core.runtime other than the utility classes. There's a reference to Job from org.eclipse.jface.viewers.deferred.BackgroundContentProvider (see bug 88577). I think the best we can do for 3.1 is to build the utility classes as a separate jar, and make it available off of the RCP web page. Jeff do you agree with fixing this for RC3, see comment #45. The fix in comment 45 seems independent of the 3.1 code. That is, I don't see any impact on 3.1 so sure. Is that the right answer? It's ok with me if you all want to delay this to 3.2. At this point it's just an issue of how best to deliver the jar containing the runtime utility classes. For 3.1, we'll probably just move the .java files to a separate source folder either in org.eclipse.core.runtime or org.eclipse.core.commands, provide a separate Ant build script for building the jar, and post the built jar off of the RCP web page. to be reviewed June 21, this is a build/packaging issue for 3.1 see comment #45 and comment #49 to be fixed post 3.1 Note that we are about to release a refactored runtime that includes a separate org.eclipse.equinox.common bundle full of common bits and pieces out of runtime. We've not done the analysis yet but the hope is that this will satisfy the needs of JFace. Changed the dependency in MANIFEST.MF from org.eclipse.core.runtime to org.eclipse.equinox.common. Fixed for builds >20051122. I have also removed the runtime-util folder. Verified in I20051213-0010 with the following steps: - exported the JFace tests from org.eclipse.ui.tests into a regular jar (test.jar) - copied the jars for the following plug-ins from the SDK build: org.eclipse.core.commands org.eclipse.jface org.eclipse.swt.win32.win32.x86 org.eclipse.equinox.common - extracted the swt-win32 DLL from the SWT jar - deleted this and the other DLLs from the SWT jar - ran the TestTree test using the following command line: java -cp test.jar;org.eclipse.core.commands_3.2.0.I20051212-2000.jar;org.eclipse.jface_3.2.0.I20051212-001 0.jar;org.eclipse.swt.win32.win32.x86_3.2.0.v3218.jar;org.eclipse.equinox.common_1.0.0.v20051205.jar org.eclipse.jface.tests.viewers.interactive.TestTree The disk footprint excluding test.jar was ~2.5M. org.eclipse.equinox.common is only 64K (compared to 444K for org.eclipse.core.runtime in Eclipse 3.1.1). *** Bug 127285 has been marked as a duplicate of this bug. *** |