Bug 65504 - No progress feedback when changing VM installs
Summary: No progress feedback when changing VM installs
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Debug (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.0 RC2   Edit
Assignee: Darin Wright CLA
QA Contact:
URL:
Whiteboard:
Keywords: polish
Depends on: 64937 66429
Blocks:
  Show dependency tree
 
Reported: 2004-06-03 05:15 EDT by Erich Gamma CLA
Modified: 2004-06-10 17:30 EDT (History)
3 users (show)

See Also:


Attachments
patch for org.eclipse.jdt.debug.ui (5.12 KB, patch)
2004-06-08 16:47 EDT, Kevin Barnes CLA
no flags Details | Diff
Version of JREs updater with a user job (10.61 KB, text/plain)
2004-06-10 09:08 EDT, Tod Creasey CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Erich Gamma CLA 2004-06-03 05:15:14 EDT
from bug# 64397

- lock workspace in background operation
- open installed JREs
- add a JRE
- press OK

observe: nothing happens. 

The real issue is that installing JREs is using null progress monitors and not 
properly passing on progress monitors. Therefore no real progress feedback can 
be given in the blocking case. 

The recommended solution is to use IProgressService.busyCursorWhile()
Comment 1 Kevin Barnes CLA 2004-06-08 16:47:11 EDT
Created attachment 11766 [details]
patch for org.eclipse.jdt.debug.ui
Comment 2 Kevin Barnes CLA 2004-06-08 16:48:14 EDT
Not seeing the desired behavior from busyCursorWhile(). The code currently uses BusyIndicator in 
JREsPreferencePage and JREsUpdater. The code in the preference page is not necessary because the only 
long running operation is the call to JREsUpdate. Patch removes the use of BusyIndicator in 
JREsPreferencePage, and replaces the BusyIndicator in JREsUpdater with 
IProgressService.busyCursorWhile(). Even with this patch the progress dialog is not shown, just a busy 
cursor (even when the workspace has been locked for close to a minute).
Tod am I doing something incorrectly?
Comment 3 Tod Creasey CLA 2004-06-08 16:58:35 EDT
Which build are you looking at Kevin? I have made some improvements in 20040607
Comment 4 Kevin Barnes CLA 2004-06-08 17:00:20 EDT
I200406081200
Comment 5 Kevin Barnes CLA 2004-06-08 17:27:23 EDT
A simple test case is to lock the workspace and then change your default VM. You'll be prompted to 
rebuild. After you respond to the prompt the IRunnableWithProgress will start, and when it's done, the 
build will start.
Comment 6 Kevin Barnes CLA 2004-06-09 11:55:17 EDT
more info.
I've been using the Job Factory view to lock the workspace while testing for this fix. I notice that 
ProgressMonitorJobsDialog used a wrappered monitor to check ticks and switch the busy cursor to a 
progress dialog with a syncExec().  The syncExec() doesn't execute until the Test jobs created by the Job 
Factory are completed. 
Comment 7 Tod Creasey CLA 2004-06-09 12:07:35 EDT
Is your test job in the UI Thread?
Comment 8 Kevin Barnes CLA 2004-06-09 15:09:36 EDT
no. In Job Factory I checked "Lock the Workspace" only. If the test job runs in the UI thread, I can't open 
the preferences dialog to change VMs.
Comment 9 Tod Creasey CLA 2004-06-09 17:47:24 EDT
Kevin is right - we are blocking on a syncExec in the UIThread. Bug 66429 has 
a patch that fixes this - I would appreciate it if you tested against your 
case before I release it.

This will go into the 20040610-2000 build if all is good.
Comment 10 Tod Creasey CLA 2004-06-10 09:07:42 EDT
I dug through your code and it isn't anything to do with the syncExecs.

In JRESUpdater you have a call to 

JavaRuntime.getPreferences().setValue(JavaRuntime.PREF_VM_XML, vmDefXML);

which is where the process is blocked.

The problem is that you do not pass in a progress monitor so there is nothing 
for us to inform of the blockage.

I wrote a version of this class that uses a user job instead - we still can't 
report the blockage because you don't pass on the monitor but at least you get 
the dialog.

You also use a user job and busyCursor while at different times in the same 
operation - if the user chooses to build first you can have two dialogs 
contending on opening.

I would suggest you move the both operations to a user job and use that.
Comment 11 Tod Creasey CLA 2004-06-10 09:08:29 EDT
Created attachment 11863 [details]
Version of JREs updater with a user job
Comment 12 Kevin Barnes CLA 2004-06-10 12:33:19 EDT
thanks for the help Tod. I understand the problem with using busyCursorWhile() now. I'm concerned 
about using a User Job to set preferences and notify listeners because the side effects of a user 
canceling the job may be bad. I'm investigating using a System Job to set the preferences, and a User 
Job to do the build with a scheduling rule so to ensure that the preferences are set before the build is 
run.
Comment 13 Tod Creasey CLA 2004-06-10 13:52:09 EDT
Generally speaking if you can use a job do so - it is much easier to manage 
the UI that way.
Comment 14 Tod Creasey CLA 2004-06-10 13:52:50 EDT
P.S. you plan sounds fine but watch about system jobs - if they can be blocked 
you should avoid making it a system job as the user will get no feedback
Comment 15 Kevin Barnes CLA 2004-06-10 17:11:49 EDT
*The code that was blocking is in LaunchingPlugin.VMChanges.rebind(). That method recomputes 
classpath entries (which can trigger a build). rebind() is now called from within a job to prevent the 
blocking.
*Code in JREsUpdater has been changed to use IProgressService.busyCursorWhile(), but it no longer 
blocks anyway because of the changes in LaunchingPlugin. 
*JREsUpdater no longer triggers builds itself. The LaunchingPlugin may trigger a build right away, but 
it's also possible that no build is triggered. This is fine because the next time a build occurs (by saving 
a file, launching, cleaning...) the correct settings will be used
Comment 16 Kevin Barnes CLA 2004-06-10 17:12:42 EDT
Darin, please verify. 

(Tod, thanks again for your help!)
Comment 17 Darin Wright CLA 2004-06-10 17:30:52 EDT
Verified.