rolandschulz / PTP

Eclipse Parallel Tools Platform
http://www.eclipse.org/ptp/
6 stars 2 forks source link

Better Progress Monitoring #10

Closed eblen closed 13 years ago

eblen commented 13 years ago

The progress monitor for sync'ing is never updated. Also, progress monitoring for other operations, such as git operations or build operations, may also need to be improved.

rolandschulz commented 13 years ago

Partly done. Committed even though it is only partly done to reduce the number of possible merge conflicts. Status: Done:

Missing:
 - Monitor needs to be used correctly in CommandRunner (depends on Issue #13)
 - Tasks have to be created and updated to have moving progress bar
eblen commented 13 years ago

I just uploaded my latest changes. There were several merge conflicts, so I had to revert a few things. (I had already done translatable strings, and I had refactored synchronization.) I kept all of your new progress monitoring code except for a few places inside the sync call. I copied it, though, for later reference.

rolandschulz commented 13 years ago

you copied it where? And why did you not keep all in the sync call?

eblen commented 13 years ago

I simply copied it to a text file so I can have it if I need it later while working on the monitor.

It looks like the only monitor code I didn't include is:

    while (!syncLock.tryLock(50, TimeUnit.MILLISECONDS)) {
                                    if (progress.isCanceled()) {
                                            throw new CoreException(new Status(IStatus.CANCEL,Activator.PLUGIN_ID,Messages.GitServiceProvider_1));
                                    }
                            }

I wasn't sure where to place this code (our implementations were quite different), and I didn't know what its purpose was.

rolandschulz commented 13 years ago

you made another mistake. You removed the finally blocked. I committed a fix.

The tryLock loop replaces lock(). It aquires the lock but allows the progress-monitor to cancel the operation.

Please don't just leave things out in a merge. If you are not sure than ask before you commit a merge. Finding mistakes in a merge takes a long time (if I woudn't have reviewed everything I wouldn't have caught the missing finally block). Thus making sure merges are correct is really important.

On Wed, May 11, 2011 at 1:50 PM, eblen < reply@reply.github.com>wrote:

I simply copied it to a text file so I can have it if I need it later while working on the monitor.

It looks like the only monitor code I didn't include is:

   while (!syncLock.tryLock(50, TimeUnit.MILLISECONDS)) {
                                   if (progress.isCanceled()) {
                                           throw new CoreException(new

Status(IStatus.CANCEL,Activator.PLUGIN_ID,Messages.GitServiceProvider_1)); } }

I wasn't sure where to place this code (our implementations were quite different), and I didn't know what its purpose was.

Reply to this email directly or view it on GitHub: https://github.com/rolandschulz/PTP/issues/10#comment_1141900

ORNL/UT Center for Molecular Biophysics cmb.ornl.gov 865-241-1537, ORNL PO BOX 2008 MS6309

eblen commented 13 years ago

Which finally block? The finally block in sync that unlocks the lock was still there.

The best solution is to avoid complex merges altogether, which means we should commit and upload frequently. I wasn't able to upload until this morning (more network problems - I can explain later.) So I had two days of work in one commit. Had I committed earlier, it would have all been much simpler. Oh well, live and learn...

rolandschulz commented 13 years ago

On Wed, May 11, 2011 at 3:43 PM, eblen < reply@reply.github.com>wrote:

Which finally block? The finally block in sync that unlocks the lock was still there.

the finally block with the monitor.done()

The best solution is to avoid complex merges altogether, which means we should commit and upload frequently. I wasn't able to upload until this morning (more network problems - I can explain later.) So I had two days of work in one commit. Had I committed earlier, it would have all been much simpler. Oh well, live and learn...

true

Reply to this email directly or view it on GitHub: https://github.com/rolandschulz/PTP/issues/10#comment_1142587

ORNL/UT Center for Molecular Biophysics cmb.ornl.gov 865-241-1537, ORNL PO BOX 2008 MS6309

rolandschulz commented 13 years ago

f197af9ef1865757ace31c51209c1d38c2076796 fixes it mostly. Remaining depends on issue #13