Community
Participate
Working Groups
Created attachment 117624 [details] Sample ViewPart to reproduce the problem. IWorkbenchSiteProgressService's schedule(Job, long, boolean) method spec's that the third boolean argument is for specifying whether a "half busy" mouse cursor should be used or not while the job is being run. However, this boolean parameter is actually only honoured on the first call (or the first call to schedule(Job) or schedule(Job, long), which uses 'false' as the parameter). Subsequent calls will ignore this parameter completely. The implementation method in WorkbenchSiteProgressService is like so... public void schedule(Job job, long delay, boolean useHalfBusyCursor) { job.addJobChangeListener(getJobChangeListener(job, useHalfBusyCursor)); job.schedule(delay); } ...and getJobChangeListener(Job, boolean) looks like so... public IJobChangeListener getJobChangeListener(final Job job, boolean useHalfBusyCursor) { if (listener == null) { updateJob.useWaitCursor = useHalfBusyCursor; listener = new JobChangeAdapter() { /* code commented out */ }; } return listener; } ...so as you can see, after the job listener has been created by the first invocation of the schedule method, the parameter is no longer read and the service's updateJob is stuck with the boolean flag that was set by the first invocation. Please see the attached ViewPart subclass of some sample code that demonstrates the problem. Once you click one of the two buttons, you will notice that clicking the other one will not alter the cursor state as you might expect.
Created attachment 120539 [details] Patch v01 I don't find any reason why the wait cursor can't be set in schedule() method itself. Attached patch does that and works fine
(In reply to comment #1) > Created an attachment (id=120539) [details] > Patch v01 > > I don't find any reason why the wait cursor can't be set in schedule() method > itself. Attached patch does that and works fine This solution does not take the scheduling of multiple jobs into account as it's just arbitrarily setting the boolean parameter based on the most recent call. Consider the code below... service.schedule(job, 10000, true); service.schedule(job2, 1000, false); One second passes and 'job2' starts running without the cursor change, let's say it ends after four seconds. Five seconds later, 'job' will start running but the cursor will not change even though the boolean parameter was set to 'true' because the most recent invocation has overrode the flag with 'false'.
Created attachment 120792 [details] Patch v02 The busy cursor will be shown only if there is at least one job which has requested for it is in busy state. This also fixes Bug# 255005. Remy, Whats wrong in removing a listener in side a callback method?
(In reply to comment #3) > Created an attachment (id=120792) [details] > Patch v02 This looks pretty good. Please write a test that has overlapping jobs that demonstrates the code works (and that the removal during done() has no ill effects). PW
(In reply to comment #3) > This also fixes Bug# 255005. > > Remy, > Whats wrong in removing a listener in side a callback method? It "feels" like an unorthodox method of doing things to me but if that's what we have to do then that's how it is. On Paul's note about tests, if you're going to fix 255005 too in one go then please merge my leak test from attachment 117862 [details] into your patch while you're at it (or write your own variant). Thanks for the help, Prakash.
(In reply to comment #4) > (In reply to comment #3) > > Created an attachment (id=120792) [details] [details] > > Patch v02 > > This looks pretty good. Please write a test that has overlapping jobs that > demonstrates the code works (and that the removal during done() has no ill > effects). > I just need a test for this and Remy's leak test from bug 255005 attachment 117862 [details] and it can go in as well. PW
Created attachment 124128 [details] Patch v03 + Tests Patch with tests added. Remy's leak test (Bug# 255005) is also added in this patch
*** Bug 255005 has been marked as a duplicate of this bug. ***
Released to HEAD >20090210 PW
(In reply to comment #9) > Released to HEAD >20090210 Wait, don't we need a CQ for this?
(In reply to comment #10) > > Wait, don't we need a CQ for this? no, our good friend Prakash works for a member company and is contributing with the full knowledge of the PMC :-) PW
Verified in I20090310-0100
