Community
Participate
Working Groups
Currently, when you try to use a SubMonitor.convert to convert the incoming monitor into a SubMonitor, but the incoming parameter is already a SubMonitor, the convert method just returns itself. This isn't very useful. I think it would be more useful to have convert() behave similarly for SubMonitor arguments as it does for IProgressMonitor arguments. That is, If you convert a SubMonitor into a new SubMonitor, it should create a new SubMonitor (or as an optimization, it could call setRemainingWork() on the passed in SubMonitor) For example, this doesn't work as expected SubMonitor monitor = SubMonitor.convert( iprogressmonitor ); monitor.beginTask("",2); SubMonitor step1 = monitor.newChild(1); ... step1.done(); SubMonitor step2 = monitor.newChild(2); ... // Here we find out that we had really 5 additional steps to accomplish SubMonitor subStep2 = SubMonitor.convert(step2, 5); subStep2.worked(1); subStep2.worked(1); subStep2.worked(1); subStep2.worked(1); subStep2.worked(1); Here, since subStep2 is really the same instance as step2, calling all the worked(1) methods on it will cause the progress monitor to automatically go to completetion earlier than what was expected.
> If you convert a SubMonitor into a new SubMonitor, it should create a new > SubMonitor (or as an optimization, it could call setRemainingWork() on the > passed in SubMonitor) Isn't this what it currently does? SubMonitor.convert calls SubMonitor.beginTask, which calls SubMonitor.setWorkRemaining.
Min, the code already works the way you're requesting it to. > Here, since subStep2 is really the same instance as step2, calling all > the worked(1) methods on it will cause the progress monitor to automatically > go to completetion earlier than what was expected. If what you say is true, then it means that there is a bug in setWorkRemaining(...), but it is not caused by the fact that the monitor shares the same instance. > SubMonitor subStep2 = SubMonitor.convert(step2, 5); If you replace this with: step2.setWorkRemaining(5) ...your example will do exactly the same thing.
Just to be sure that everything works, I wrote the following JUnit test. It passes. SubMonitor is working correctly, and does not reach completion until the very end of the method. public void test21039() { TestProgressMonitor testMonitor = new TestProgressMonitor(); SubMonitor monitor = SubMonitor.convert( testMonitor ); monitor.beginTask("",2); SubMonitor step1 = monitor.newChild(1); step1.done(); assertEquals(500.0, testMonitor.getTotalWork(), 1.0); SubMonitor step2 = monitor.newChild(2); // Here we find out that we had really 5 additional steps to accomplish SubMonitor subStep2 = SubMonitor.convert(step2, 5); subStep2.worked(1); assertEquals(600.0, testMonitor.getTotalWork(), 1.0); subStep2.worked(1); assertEquals(700.0, testMonitor.getTotalWork(), 1.0); subStep2.worked(1); assertEquals(800.0, testMonitor.getTotalWork(), 1.0); subStep2.worked(1); assertEquals(900.0, testMonitor.getTotalWork(), 1.0); subStep2.worked(1); assertEquals(1000.0, testMonitor.getTotalWork(), 1.0); }
CC'ing obesedin in case he wants to include this regression test as part of SubMonitorTest.
Thanks Stefan. I have committed the JUnit test. Extra tests are always good.
I have updates. I think you've proved me wrong, but I still have some issues. Issue 1) Seems like the documentation for SubMonitors encourages you not to call done() on the arguments that are returned to you. If you don't do this, the percentage complete isn't successfully computed. (The next usage of the parent monitor doesn't automatically "work" the remaining results) (see enclosed test case to reproduce the problem) Issue 2) I think this "gotcha" was got me starting thinking that this was a bug in SubMonitor. (Which of course it really wasn't, but it was an easy mistake to make.) Seems like it's unwise to call SubMonitor.convert() when the passed in argument is a SubMonitor that's already had beginTask() called on it, and had already had some work performed on it. This is because the SubMonitor.convert() will basically reset the total worked on the monitor to a new value. Seems like what the programmer wanted to do was call do a newChild() first, and then other subroutines could safely issue a SubMonitor.convert() on the result. Seems like this would be a common programmer error, and a big gotcha. Some code to illustrate the gotcha SubMonitor sub1 = SubMonitor.convert(monitor); sub1.beginTask("", 4); sub1.worked(1); // first sub task (25% of total) SubMonitor converted = SubMonitor.convert(sub1, 5); // OOPS converted.worked(5); sub1.worked(1); // third sub task (25% of total) ----------------- The OOPS line really should have been this: SubMonitor nested = sub1.newChild(2); // second subtask (50% of total) SubMonitor converted = SubMonitor.convert(nested, 5); // OOPS Is there a way to detect this condition and throw an IllegalArgumentException to help the programmer with this gotcha?
Created attachment 84809 [details] Test Code to illustrate bug
Reopen bug
Issue 1: > The next usage of the parent monitor doesn't automatically "work" the > remaining results I suppose this could be changed without breaking anything. I didn't do it this way originally because I was concerned that a future code change might cause the child to delegate to the parent's worked(...) method in some situation, which would cause the child to be prematurely consumed. However, I can see how this would produce more accurate progress in the situation where you're mixing a lot of newChild(...) calls with worked(...) and never calling done(). John: What do you think? > Is there a way to detect this condition and throw an IllegalArgumentException That's a good idea and wouldn't have been too hard to do... but there's no way we could make this change at this point, since it would break API. It might be possible to add a new convertUnused(...) method that would make the check... but having too many convert(...) methods with subtly different semantics may make the API confusing. A better approach might be to add getters to SubMonitor that let you determine if the monitor has ever reported any work. That would let clients write such a helper method themselves.
Created attachment 84830 [details] Addresses the behavior described as "issue 1" Should we decide that this is correct behavior, this patch makes SubMonitor clean up unconsumed work from its children when you call worked() or internalWorked() on the parent.
Created attachment 84831 [details] Updates the test suites to check for issue 1 Updates the test suites to check for issue 1.
I've attached patches for issue 1. Regarding issue 2: We might want to consider adding a getWorkRemaining() method to SubMonitor - that would let clients write a version of convert(...) that asserts that no ticks have been allocated on the converted monitor.
Applied patch to CVS Head, thanks Stefan! As for addition of getWorkRemaining(), that seems to over complicate things as if developer realizes that he needs to check getWorkRemaining(), he probably knows the difference between #newChild() and #convert(SubMonitor) and used the proper method to beging with. So while it is possible to imagine a situation where getWorkRemaining() would be nice to have, it is probably not the one described in issue 2.
