Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better Progress Monitoring #10

Closed
eblen opened this issue May 3, 2011 · 8 comments
Closed

Better Progress Monitoring #10

eblen opened this issue May 3, 2011 · 8 comments

Comments

@eblen
Copy link
Collaborator

eblen commented May 3, 2011

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.

@ghost ghost assigned rolandschulz May 11, 2011
@rolandschulz
Copy link
Owner

Partly done. Committed even though it is only partly done to reduce the number of possible merge conflicts. Status:
Done:
- Monitor is passed everywhere it is needed
- Converted to jgit monitor for jgit transport
- Should allow cancel in most places (see Missing)

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
Copy link
Collaborator Author

eblen commented May 11, 2011

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
Copy link
Owner

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

@eblen
Copy link
Collaborator Author

eblen commented May 11, 2011

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
Copy link
Owner

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:
#10 (comment)

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

@eblen
Copy link
Collaborator Author

eblen commented May 11, 2011

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
Copy link
Owner

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:
#10 (comment)

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

@rolandschulz
Copy link
Owner

f197af9 fixes it mostly. Remaining depends on issue #13

rolandschulz added a commit that referenced this issue May 13, 2011
finishes #10 and finished #13

additional minor changes:
- removed if(rfm==null) check - better to have NPE than wrong result
- syle-nit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants