Community
Participate
Working Groups
We need a SubSystem.connect(IProgressMonitor) method that can be called from a background job and does all necessary tasks. During the org.eclipsecon.tmtutorial, we had to write some nasty workaround for this. I think that in SubSystem there should be two alternatives for connect: - connect(IRSECallback cb) to initiate a connect Job which is created by RSE, put into backgorund and notifies the caller when complete. - connect(IProgressMonitor mon) which must be called from a background job, runs synchronously and uses the given progress monitor for reporting.
For the callback, APIs I've got the following in mind: /** * Connect to the remote system on a background job. The callback is * called when the connect is complete. * * @param callback to call after connect is complete * @throws Exception */ public void connect(IRSECallback callback) throws Exception; /** * Connect to the remote system on a background job. The callback is * called when the connect is complete. * * @param forcePrompt forces the prompt dialog even if the password is in mem * @param callback to call after connect is complete * @throws Exception */ public void connect(boolean forcePrompt, IRSECallback callback) throws Exception; Calls to connect(forcePrompt) would end up calling connect(forcePrompt, callback) using null for the callback. For IRSECallback, I've got the following in mind: public interface IRSECallback { public void operationComplete(String operationName, ISubSystem subsystem, IProgressMonitor monitor); } What do you think? Also do you think we should call operationComplete on the main thread or leave it on the background thread?
(In reply to comment #1) > Calls to connect(forcePrompt) would end up calling connect(forcePrompt, > callback) using null for the callback. I would like to avoid unnecessary utility methods, if possible. Thus get rid of connect() connect(boolean) in favor of a single connect(boolean, IRSECallback) > public interface IRSECallback { > public void operationComplete(String operationName, ISubSystem > subsystem, IProgressMonitor monitor); > } Why do you want all these parameters for the callback? The caller would know what operation and subsystem it was. The monitor would be useless if threads were changed. I'd prefer having a parameter carrying error information instead, as Uwe and I have proposed on bug 166338 comment #8. > Also do you think we should call operationComplete on the > main thread or leave it on the background thread? Good question. Perhaps Uwe has an opinion? - In our commercial product, we always do callbacks on the main thread through Display.asyncExec(). On the other hand, clients who think they need this, can do it themselves. The drawback of asyncExec() is that it cannot be easily done in non-UI code where a Display is not available. Therefore, I would vote for keeping it on the background thread for now (it's simpler) with an option to change the API in the future to always do it on the display thread. I think it's easier for clients to change this way rather than the other way round; and we save the problem of finding an event loop in non-UI code.
I wouldn't mind getting rid of those connect() and connect(boolean) methods. It will effect a lot of stuff but I guess now is the best time to change it. I didn't write the original code so I'm hoping there are no nasty side effects to worry about. I wasn't sure how much we should assume about the callbacks. Whether a callback could be reused for different situations or whether it would always be one to one with the operation it is performed on. If we did allow reuse, there could be cases where a single callback instance is passed in for use in multiple operations on a subsystem and/or operations that are called on more than one subsystem. I should have posted these comments in bug 166338 (I knew there was another defect regarding this). And now I see the originally proposed interface: public interface IRSECallback { public void done(IStatus status, Object result); } IStatus makes sense. The result would be open to interpretation but that's been pretty standard in RSE. I could take or leaving having a progress monitor. I'd rather leave the thread as is too and leave it to the implementor to deal with asynchExec().
Getting rid of the original methods forces revisiting the code, and in many occasions we will see that using a callback is a better option that what we have now (often waiting for the synchronous call to return, which involves a nested dispatch loop which is bad). If you are worried about this being too much work for M6, you can set the old methods deprecated with a clear indication that they will be removed for 2.0. Searching for the deprecated usage will also make it easier to find the places that need to be changed, since it works more reliable than just searching for references (which often finds potential matches).
Re-using a single callback for multiple things is a very bad idea, we should not do that. The boilerplate code with the anonymous inner class proved to be the best method for callbacks, because it is type-safe, clear and easy to understand. In some occasions, one can also do something like public void myJob extends Job implements IRSECallback public void doWork() { connect(false, this) } public void done(IStatus status, Object result) { //callback } } although I do not like this format at all since for the casual reader, the flow of control is not visible. It is much better to do the nested class: public void doWork() { connect(false, new IRSECallback() { public void done(IStatus s, Object result) { doneWork(s,result); } }); } public void doneWork(IStatus status, Object result) { //callback } This is slightly more to type, but makes the flow of control really obvious.
a) Use PlatformUI.getWorkbench().getDisplay(), never Display.getDefault() (as it may try to create a new display if already shutting down) or Display.getCurrent() (as it may return null if called from a non display thread). PlatformUI.getWorkbench().getDisplay() is not bound to a display thread as far as I know. b) Please use the IRSECallback with 'done(IStatus status, Object result)'. As long as not using Java 5, this form is very efficient. The status can tell the three important states warning, ok and error plus having additional information or even being a multi status. And for the result, the caller always needs to know and interpret what might come back. For API methods using callbacks with specific result types, the javadoc should clearly state it :).
(In reply to comment #6) Uwe are you OK with doing the callbacks on the original (Background) Job's worker thread? Clients can do PlatformUI.getWorkbench().getDisplay().asyncExec() themselves if they want. API would guarantee that the callback is _never_ done on the display thread, and that the callback is the LAST operation that the worker thread performs, but before marking the Job done.
I'm totally ok to invoke the callback in the original thread. As long it is clear, well documented and consistent, I have absolutly no preference which thread it is :).
We'll leave Bug 166338 to deal with the callbacks. I'm marking this one as done as it deals with the progress monitor api.
[target cleanup] 2.0 M6 was the original target milestone for this bug
