Community
Participate
Working Groups
Since clients of the history might rely on getting certain events even if a operation causes runtime exceptions (for example listeners) the history should protect itself better agains runtime exceptions. This envloves: - listeners causing runtime exceptions in the call back - Operation execution causing runtime exceptions. In this case listeners must be informed as well.
Specific suggestion from Dirk: - IUndoableOperation execute, undo, and redo methods throw InterruptedException when an OperationCancelledException is received - IUndoableOperation execute, undo, and redo methods throw CoreException Need to determine how the operation history should handle these. Does it throw the exception? Does it handle the exception and include the exception in the returned status object? What would we expect the undo and redo action handlers to do in these cases? Will investigate.
see also bug #86243 for more specific request to throw exception from the operation history.
I agree that exceptions should be handled better, and no information should be lost so that Refactoring can still throw its CoreExceptions. However, it does not seem right for the operations framework to throw CoreException, since it is trying to be "Eclipse runtime neutral," similar to JFace. JFace catches CoreException in places but never throws it. Another thing to consider is the new 3.1 Commands/Handlers architecture. This introduces an ExecutionException which is thrown whenever a handler has an execution problem. Since many operations will be triggered from Handlers, it seems more appropriate to use a similar exception strategy. I propose the following: IUndoableOperation#execute, undo, redo - throws ExecutionException - catches OperationCanceledException and maps to an IStatus indicating cancellation. - catches other exceptions and creates ExecutionException containing the original exception. IOperationHistory#execute, undo, undoOperation, redo, redoOperation - throws ExecutionException - listeners are notified with OPERATION_NOT_OK when a cancel status is encountered (implemented already) Refactoring would catch ExecutionException and throw the original CoreException. No information is lost. I agree this is more work on the Refactoring side, but I think it's more appropriate for commands, handlers, and operations to use a consistent strategy for exceptions. cc'ing Doug Pollock for comment.
Throwing an ExecutionException or another flavour of CommandException would be okay. One thing to bear in mind: the super(String,Throwable) call use by CommandExceptions will likely need to be changed. We are trying to remove any dependencies on 1.4 API. I can provide equivalent API in CommandException itself, but it would be nice to know which 1.4 methods are actually required....
Since org.eclipse.core.commands depend on runtime anyway why can't we throw a CoreException. I really don't see the benefit of avoiding using runtime concepts if runtime is a prerequiste plug-in anyways. I can do the Exception conversion but I would like to understand why this is necessary. IUndoableOperation shouldn't catch a OperationCancelException and covert it into a status of form canceled. The current strategy in Eclipse is that clients calling a method which takes a progress monitor must deal with the case that the method can throw an OperationCanceledException. Since the framework hands in the progress monitor into IUndoableOperation it should handle the OperationCanceledException. Asking the Operation to do this will very likely end up in cases where programmers don't do it (since they are simply not aware of the fact that methods they are calling are throwing the exception) and we will end up propagating the exception up to the event loop.
In a pure JFace/SWT application, developers do not use the plug-in architecture. Plug-in dependencies don't mean the same thing. The developers manually build a class path. Typically this involves including the SWT, JFace and core runtime JAR file on the class path. It does not normally involve any other JAR files. So, the case we are trying to avoid is adding a dependency in JFace that would need to pull in support from other core or OSGi packages. (As long as the class loader doesn't load such classes from the core runtime JAR, then those other JAR files can be excluded from the class path.) Any dependency on core (e.g., progress monitor) can later force more classes on to us (e.g., OperationCanceledException). So, keeping the number of classes as small as possible is highly desirable. This also has the minor side effect that people can trim the core runtime JAR file down to only the required classes, if they have tight size constraints. There are some fundamental problems with how the OperationCanceledException works, which has been mentioned in other bugs. As you are highlighting, it circumnavigates the Java compiler for the sake of API compatibility. This makes it hard for developers to realize that there is something they need to respond to. I'm not entirely sure how getting the client to handle the OperationCanceledException is any better than getting the operation to handle it. Will the client know more about the possibility of an OperationCanceledException than the operation?
I can catch OperationCanceledException in the operation history rather than the operation itself and convert to the cancel status. This keeps the operations from having to do so and gives clients a single technique for discovering cancellation, regardless of whether the user canceled via a progress monitor or it was canceled via an IOperationApprover. Otherwise the action handler will have to catch both cases and do the same thing. This seems more reasonable than having the operation do it. As for avoiding CoreException, the class will have to be included on the classpath by JFace clients because it's already referenced in JFace. I just have a "gut feeling" that it's too specific to throw a CoreException from a general undo framework. In bug #86243, you mention that most methods that manipulate the workspace are throwing core exception. The operations framework is unaware of resources and the workspace and this is why I don't like propagating that particular exception.
Makes sense. So I will wrapper it into an ExecutionException. But this one has to support nested exceptions even if it doesn't depend on 1.4 code. Otherwise we will lose information
IOperationHistory is now propagating ExecutionException which is handled by the undo and redo action handlers. Implemented for I20050301 and beyond. Released to HEAD on 2/25/2005. This should resolve the API issues associated with exception handling. Still need to look at the listeners. OperationHistory should catch exceptions, notify listeners with a NOT_OK and then propagate the ExecutionException. Also need to look at catching exceptions while notifying listeners.
Exceptions occurring while running listeners are now caught and logged, but the operation in question (execute, redo, undo, etc.) continues. ExecutionException is propagated upward. Non-ExecutionException exceptions that occur while executing, redoing, or undoing an operation are caught. An ExecutionException with the originating exception is thrown. Will release for 0308 I build.
this has been in the M6 builds for some time. Closing bug.
Susan, I quickly looked at the code and it is catch Exceptions which is something client code should not do. We should always use Platform#run(ISafeRunnable).
the commands plug-in should not reference the Platform class. JFace deals with this using the SafeRunnable class. Our choices are to either: - move the SafeRunnable support to the commands plug-in - duplicate the SafeRunnable concept in the commands plug-in - document that we are catching Exceptions explicitly due to the avoidance of using Platform
verified on I20050330-0500 Opened separate bug #89387 to track Dirk's suggestion that Platform.run be used where possible (in action handlers this should be done)
