Ensure DownloadJob.run(IProgressMonitor) provides monitor task details

[merks]

#576

[merks]

@ptziegler

An unfortunate consequence of using masterMonitor.slice(1), which creates a org.eclipse.core.runtime.SlicedProgressMonitor, is that a SlicedProgressMonitor does not report task details to the monitor being sliced. I'm not sure what improvement exactly that #576 brought but one consequence is that the master monitor has no details about the tasks and subtasks being monitored. This is particularly glaring in the installer where those details are funneled to the task log, e.g,

Downloading org.apache.felix.gogo.shell
Fetching org.apache.felix.gogo.shell_1.1.4.jar from https://eclipse.mirror.liteserver.nl/releases/2024-12/202412041000/plugins/ (57.69kB)

With the master monitor task details missing, the installer logs that its about to collect several hundred artifacts from some specific repository. e.g.,

Collecting 556 artifacts from https://download.eclipse.org/releases/2024-12/202412041000

But then sits their quietly until they are all done. I imagine that if one looked at the progress view there too the details about what is happening during the progress is also missing.

So rather than revert the change, I've ensure that the slice propagates the task details...

[github-actions]

Test Results

  375 files  ±0    375 suites  ±0   46m 59s ⏱️ +31s
1 904 tests ±0  1 901 ✅ ±0  3 💤 ±0  0 ❌ ±0 
6 712 runs  ±0  6 703 ✅ ±0  9 💤 ±0  0 ❌ ±0 

Results for commit 3a42275. ± Comparison against base commit 1cfa5de.

[ptziegler]

Whoops... That is indeed an oversight that I missed when I changed it. 😦

I'm not sure what improvement exactly that #576 brought but one consequence is that the master monitor has no details about the tasks and subtasks being monitored.

The problem is that getArtifact() internally calls beginTask() on the given IProgressMonitor. Because this is always the same "master" progress monitor, beginTask() is called multiple times on the same monitor, which is illegal.

So every time this method is called, a new IProgressMonitor instance needs to be used, so using a custom SlidedProgressMonitor should definitely work. But I wonder if it's cleaner to change the masterMonitor to a SubMonitor and then simply call newChild(), rather than creating an anonymous subclass...

See #586

[merks]

I think your other solution is cleaner and therefore better. Thanks. 😄