Community
Participate
Working Groups
Created attachment 55048 [details] The Handling in Jobs and JFace
Questions to the patch 1. The methods openWarning and openError in the MessageDialog class from jface don't show dialogs now. They create statuses and pass them to the logger from the policy. Is it ok? Alternative solution is that these methods can open dialogs if default logger is set, or I can have another logger used only in these two methods? What do you think? 2. In ProgressManager method crateChangeListener(). There is JobChangeAdapter and done() method. Is FinishedJobs.getInstance().remove(info) ok? Or maybe I have to remove JobInfo from FinishedJobs using commented code with iterator?
Created attachment 55116 [details] The Handling in Jobs, JFace and parts opening
1) No - the Policy class will need to open dialogs in the JFace case 2)IDEWorkbenchErrorHandler is using a UI Job. Either make this a system job or externalize the name as it will show up in the progress view. I would make it a system job. 3)You have several new methods with no comments. All methods need to be commented (see ErrorEditorPart for instance). 4) You might want to use a utility class to do handleException and handleStatus rather than the plug-in. Maybe in StatusUtil or something you have in your new code. 5)We need to remove the commented out code in the ProgressManager of course but thanks for the explanation. We should not play with FinishedJobs with anything more than remove(info). If there is some tidying up we can do it. 6) Why did jobs.remove(event.getJob()); // Only refresh if we are showing it removeJobInfo(info); get moved up? I want to be sure there is not anything subtle about moving this - I wrote the code so long ago that I am always concerned about changes for no reason.
4) You might want to use a utility class to do handleException and handleStatus > rather than the plug-in. Maybe in StatusUtil or something you have in your new > code. If we put it to the WorkbenchPlugin we can use it like log() methods and we don't have to care about pluginId. > 5)We need to remove the commented out code in the ProgressManager of course but > thanks for the explanation. > We should not play with FinishedJobs with anything more than remove(info). If > there is some tidying up we can do it. ok > 6) Why did > > jobs.remove(event.getJob()); > // Only refresh if we are showing it > removeJobInfo(info); > > get moved up? I want to be sure there is not anything subtle about moving this > - I wrote the code so long ago that I am always concerned about changes for no > reason. > It is because this adds jobs to the FinishedJobs. If want to remove jobs from FinishedJobs, we have to have jobs.remove(event.getJob()); // Only refresh if we are showing it removeJobInfo(info); first.
>If we put it to the WorkbenchPlugin we can use it like log() methods >and we don't have to care about pluginId. log() is API defined on Plugin. The only code that accesses the WorkbenchPlugin is workbench code so there is no need for any symettry as it is not API. The other comments are good for me. Looks like you found a bug in the ProgressManager.
Created attachment 55142 [details] The Handling in Jobs, JFace and parts opening
Created attachment 55143 [details] The Handling in Jobs, JFace and parts opening
Point 2 is an issue but otherwise this looks OK for me. Fix that up and we'll have Paul take a look.
I took the liberty of externalizing the status string in the IDEExceptionHandler as it wasn't clear to me that the user would never see it but I have otherwise released the patch. Szymon please find something Paul can look at as these time critical fixes always end up committed by me.
This fix caused a major regression. See bug 168720 for the details. A temporary fix has been released to HEAD.
