Show progress indicator of "Run configurations..." after delay #591

fedejeanne · 2023-07-25T15:38:34Z

Wait 1 second before showing the progress indicator in the dialog "Run configurations...". This avoids the flickering that happens when the task is completed in under 1 second.

Contributes to eclipse-pde/eclipse.pde#679

github-actions · 2023-07-25T16:06:14Z

Test Results

      42 files ±0       42 suites ±0 57m 2s ⏱️ ±0s
  3 771 tests ±0   3 767 ✔️ - 1   3 💤 ±0 1 ❌ +1
11 316 runs ±0 11 288 ✔️ - 1 27 💤 ±0 1 ❌ +1

For more details on these failures, see this check.

Results for commit c0b72e9. ± Comparison against base commit 7ed493c.

♻️ This comment has been updated with latest results.

...bug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LaunchConfigurationsDialog.java

HannesWell

Looks reasonable.

If nobody else does in the meantime, I'll try to review this in detail and try it out at the weekend.

HannesWell · 2023-07-27T22:41:27Z

...bug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LaunchConfigurationsDialog.java

 			fActiveRunningOperations++;

+			UIJob showProgressMonitorPart = createJobToShowProgressMonitorPart();
+			// only show the progress bar if necessary after 1 second
+			showProgressMonitorPart.schedule(1_000);


Since potentially not everybody wants a delay, maybe the delay should be configurable in the context?

Do you mean like DebugUIPlugin.getDefault().getPreferenceStore().getLong("somelong"); ? That would also mean that there needs to be a new preference in the Preferences dialog under Run/Debug > Launching > Launch configurations, right?

Or did you mean something else?

Since potentially not everybody wants a delay, maybe the delay should be configurable in the context?

I was thinking again about making it configurable and it still doesn't feel right.

Don't get me wrong, I'm all for customizing the user experience but in this particular case, a low value would mean that the progress bar might flicker and a high value would mean the UI might freeze. Letting the user set this value feels like giving him/her the choice between bad and worse 😅

Maybe you and I can settle for a value and let it hard-coded. Say 500 ms?

There is definitely a tradeoff with configuration preferences that users are unlikely to find and as there are more and more, the user is even less likely to find and understand such things...

Sorry for the late reply.
I meant not configurable for the user but for developers calling this code.

If a dev knows that an operation will definitvly take some time, it would be good to show i immediatly.

Furthermore If we could somehow estimate the remaining runtime(e.g. from a ProgressMonitor, it would be good to consider if it is worth to show it.
E.g. for a 501ms running Task showing Progress after 500ms is pointless.

I meant not configurable for the user but for developers calling this code.

Thank you for clarifying it :-) . Do you have any suggestion (code) on how to achieve that?

Not really, I would guess that this should somehow be stored in the context? But this is just a simple guess, there might be better places.

Furthermore If we could somehow estimate the remaining runtime(e.g. from a ProgressMonitor, it would be good to consider if it is worth to show it.

Hm, the problem is that the progress monitor does not have any method to see how much work is still to be done so there is no basis to estimate if it's worth showing it.

E.g. for a 501ms running Task showing Progress after 500ms is pointless.

I agree and I'm convinced pretty much any value someone selects will end up provoking either a flickering or a UI freeze. The timeout is just a simple way of catching all jobs that run in under X ms. I have no way of knowing how long a job is going to take, but In my experience if the job takes > 1s then it will probably take >2 s (i.e. the progress bar will appear for at least a second). Again, this is a very simplistic approach based on my experience. I wanted to provide a solution without burdening the user (or programmer) with a new setting/parameter that needs to be understood and set correctly.

Makes sense. So my suggestion is for now to just a threshold a developer can add and maybe later enhance the progress-manager to get an (probably quickly outdated) estimation about the done work later and with that enhance this further.

Not really, I would guess that this should somehow be stored in the context?

Sorry to insist but I don't understand. What context do you mean?

Surely not the org.eclipse.e4.core.contexts.IEclipseContext, right?

Ah, you probably meant the BundleContext right? I'll look into it on Monday.

Ah, you probably meant the BundleContext right? I'll look into it on Monday.

I think it should be a org.eclipse.jface.operation.IRunnableContext, as you are using that in eclipse-pde/eclipse.pde#674 to show the pop-up that shows the progress indication.
Unfortunately the progress-indicator is not created directly so have to pass trough the threshold value.

Btw. Monday is the last day of development for the upcomming 2023-09 release and I probably won't have the time to review/try this Monday evening (if another Committer does it is of course welcome).

I'll try to review your PR in PDE over the weekend, but cannot promise it.

Bananeweizen · 2023-08-12T12:49:04Z

The topic of "deferred progress reporting" is also handled within org.eclipse.ui.internal.operations.TimeTriggeredProgressMonitorDialog (which is probably a copy of org.eclipse.debug.internal.ui.viewers.model.TimeTriggeredProgressMonitorDialog). Not sure if you could reuse the first one in your implementation maybe.

fedejeanne · 2023-08-14T08:01:06Z

The topic of "deferred progress reporting" is also handled within org.eclipse.ui.internal.operations.TimeTriggeredProgressMonitorDialog (which is probably a copy of org.eclipse.debug.internal.ui.viewers.model.TimeTriggeredProgressMonitorDialog). Not sure if you could reuse the first one in your implementation maybe.

Good hint @Bananeweizen, I didn't know about that one, thank you! This could be a good compromise since the concept of delayed progress reporting has already been tackled and it seems a delay of 800 ms is the accepted norm. I changed the code to simply use this threshold in a21c1d7.

What do you say if we just take this implementation out for a spin in the upcoming release and see how this works, @HannesWell? Maybe this simple solution is better than letting the user/programmer set a value that will probably be quickly outdated anyway?

fedejeanne · 2023-09-08T09:21:12Z

Hello all,
there has been little activity on this one in the past few weeks. Any chance we can resume this PR for 4.30?

mickaelistria · 2023-09-11T08:13:31Z

As build log mentions, a version bump seems necessary:

[ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.2:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.debug.ui: Only qualifier changed for (org.eclipse.debug.ui/3.18.100.v20230911-0736). Expected to have bigger x.y.z than what is available in baseline (3.18.100.v20230802-1257) -> [Help 1]

fedejeanne · 2023-09-11T13:14:47Z

As build log mentions, a version bump seems necessary:

[ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.2:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.debug.ui: Only qualifier changed for (org.eclipse.debug.ui/3.18.100.v20230911-0736). Expected to have bigger x.y.z than what is available in baseline (3.18.100.v20230802-1257) -> [Help 1]

Thank you for the hint. It's odd, I just bumped the version and I get this error in Eclipse:

Any hint on how to get rid of it?

iloveeclipse · 2023-09-11T13:23:15Z

Any hint on how to get rid of it?

You should set API baseline to 4.29 RC2a. I assume you are still using 4.28 in the IDE.

merks · 2023-09-11T13:28:02Z

FYI, the Oomph setups won't be correct (correct baseline and TP) until Wednesday morning...

fedejeanne · 2023-09-11T13:30:18Z

FYI, the Oomph setups won't be correct (correct baseline and TP) until Wednesday morning...

i.e. Perform setup tasks won't help until (late) Wednesday, right?

merks · 2023-09-11T13:41:20Z

As a workaround, you can do this. Use Navigate -> Open Setup -> Workspace and copy (select text below) and paste (onto the root Workspace object) these two redirection tasks

<?xml version="1.0" encoding="UTF-8"?>
<xmi:XMI xmi:version="2.0"
    xmlns:xmi="http://www.omg.org/XMI"
    xmlns:setup="http://www.eclipse.org/oomph/setup/1.0">
  <setup:RedirectionTask
      sourceURL="https://download.eclipse.org/eclipse/updates/4.28/R-4.28-202306050440"
      targetURL="https://download.eclipse.org/eclipse/updates/4.29/R-4.29-202309031000"/>
  <setup:RedirectionTask
      sourceURL="https://download.eclipse.org/eclipse/updates/4.29-I-builds"
      targetURL="https://download.eclipse.org/eclipse/updates/4.30-I-builds"/>
</xmi:XMI>

Then the baseline task will use the 4.29 release and the target platform will use 4.30 I builds...

HannesWell · 2023-09-11T22:04:33Z

This could be a good compromise since the concept of delayed progress reporting has already been tackled and it seems a delay of 800 ms is the accepted norm. I changed the code to simply use this threshold in a21c1d7.

Sounds good.
While in some cases callers might want to adjust this value, most callers are probably happy with a reasonable default. So I think it is sufficient to add that posibility when there is a demand for it.

I'm still thinking about if it would be good to add means to IProgressMonitor to get the percentage of work done (or at least an Approximation since the value can be outdated quickly)? This would allow to estimate if it is worth to open the progress indicator.
On the other hand it highly depends on the called code if, for example the last 20% really take only 20% of the total time or more or less. I have seen it often that the indicator got stuck at 80/90% for a long time. And there is even a recommendation to just half the work remaining all the time if the total work is unknown.
So overall I'm not sure if that is worth it.

laeubi · 2023-09-12T03:29:56Z

I'm still thinking about if it would be good to add means to IProgressMonitor to get the percentage of work done (or at least an Approximation since the value can be outdated quickly)? This would allow to estimate if it is worth to open the progress indicator.

the implementation of the monitor should/must handle this so there is no need for an public API at all, so for example ProgressMonitorDialog could have something similar to what Swing uses here:

https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/javax/swing/ProgressMonitor.html#setMillisToDecideToPopup(int)

HannesWell · 2023-09-12T20:32:08Z

...bug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LaunchConfigurationsDialog.java

@@ -1409,6 +1416,20 @@ public void run(boolean fork, boolean cancelable, IRunnableWithProgress runnable
 		}
 	}

+	private UIJob createJobToShowProgressMonitorPart() {
+		return new UIJob(IInternalDebugCoreConstants.EMPTY_STRING) {


Besides UIJob.create() I think we can just use an empty string literal here.
IMHO it does not make sense to get that from a constant.

Done in 6c443cc

HannesWell · 2023-09-12T20:35:13Z

I'm still thinking about if it would be good to add means to IProgressMonitor to get the percentage of work done (or at least an Approximation since the value can be outdated quickly)? This would allow to estimate if it is worth to open the progress indicator.

the implementation of the monitor should/must handle this so there is no need for an public API at all, so for example ProgressMonitorDialog could have something similar to what Swing uses here:

https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/javax/swing/ProgressMonitor.html#setMillisToDecideToPopup(int)

But the ProgressMonitor is designed differently and does not handle the context, so there must be at least and internal method to get that value.

Any way, since everything additionally seems to be more complex, I think this is fine as a first step, with the two minor points (using the UIJob-factory and the empty string) fixed I would be fine to submit this.

laeubi · 2023-09-13T05:30:40Z

But the ProgressMonitor is designed differently and does not handle the context, so there must be at least and internal method to get that value.

The value should be passed to the dialog and then it could handle it internally, but users of that Interface must not care nor do we need to store such values in general...

HannesWell

Looks good to me.

Does anybody else plan to review this?

@fedejeanne you need to bump the Micro version segment in in the Manifest order to fix the build failure:

[ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.2:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.debug.ui: Only qualifier changed for (org.eclipse.debug.ui/3.18.100.v20230913-0532). Expected to have bigger x.y.z than what is available in baseline (3.18.100.v20230802-1257) -> [Help 1]

HannesWell · 2023-09-13T07:15:34Z

But the ProgressMonitor is designed differently and does not handle the context, so there must be at least and internal method to get that value.

The value should be passed to the dialog and then it could handle it internally, but users of that Interface must not care nor do we need to store such values in general...

I think we have a missunderstanding here. The amount of work done is interesting do help deside if after the Initial delay it is worth for the remaining time to show the Pop-up.

Making the initial delay configurable is an orthogonal issue.

HeikoKlare

Changes look good to me. I only have a minor remark w.r.t. to a code comment.

...bug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LaunchConfigurationsDialog.java

Wait a while before showing the progress indicator in the dialog "Run configurations...". This avoids the flickering that happens when the task is completed in under 800 ms (which is the current norm in PlatformUI.getWorkbench().getProgressService().getLongOperationTime()) Contributes to eclipse-pde/eclipse.pde#679

* org.eclipse.debug.ui --> 3.18.200

HannesWell

Thank you @fedejeanne for this PR and your patience.

On the last pass I thought about if fActiveRunningOperations should be turned into a AtomicLong, but since the code is running in the UI thread in both cases (just later) that's fine.

Submitting.

I'll have a look at the PDE pendant in the next days.

fedejeanne · 2023-09-14T04:01:02Z

Thank you @fedejeanne for this PR and your patience.

Thank you for reviewing!

I'll have a look at the PDE pendant in the next days.

Looking forward to it :-)

laeubi · 2023-09-18T10:50:42Z

FYI, the Oomph setups won't be correct (correct baseline and TP) until Wednesday morning...

Just to make sure was this meant to be 13.09.2023? The setup still fails for me and I can't use the baseline...

merks · 2023-09-18T11:53:13Z

It doesn't look like this?

laeubi · 2023-09-18T11:56:16Z

It seems that Oomph was confused by the pre-req-target project imported as a project in my workspace somehow :-
At laest now I removed that project and it seems to came across the point now...

merks · 2023-09-18T12:01:05Z

Yes, there was a problem caused by the two occurrences and the "wrong" one winning. That should no longer happen because of the filter in the .project file.

laeubi · 2023-09-18T12:04:35Z

Alright it seems I was now able to get the API error (hurray!):

Add stream support to IStructuredSelection eclipse.platform.ui#1099

fedejeanne mentioned this pull request Jul 25, 2023

Show a progress indicator when opening a PDE run configuration. eclipse-pde/eclipse.pde#674

Merged

fedejeanne mentioned this pull request Jul 26, 2023

Random failing LaunchConfigurationTests.testTerminateTimeStamp #582

Closed

fedejeanne force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch 2 times, most recently from 3a9124c to f830a3f Compare July 26, 2023 13:36

fedejeanne marked this pull request as ready for review July 26, 2023 13:37

fedejeanne force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch from f830a3f to 843f79f Compare July 26, 2023 13:38

fedejeanne mentioned this pull request Jul 26, 2023

Random failing ProcessConsoleManagerTests.testBug546710_ConsoleCreationRaceCondition #596

Open

HannesWell reviewed Jul 27, 2023

View reviewed changes

...bug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LaunchConfigurationsDialog.java Show resolved Hide resolved

HannesWell reviewed Jul 27, 2023

View reviewed changes

fedejeanne force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch from 843f79f to a21c1d7 Compare August 14, 2023 08:01

mickaelistria force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch from a21c1d7 to 52070f1 Compare September 11, 2023 07:36

mickaelistria added this to the 4.30 M1 milestone Sep 11, 2023

fedejeanne force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch 2 times, most recently from 77899f2 to 3229184 Compare September 11, 2023 13:12

merks mentioned this pull request Sep 12, 2023

Update SDK fails (with Oomph) eclipse-platform/.github#150

Closed

fedejeanne force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch from 3229184 to 1112f1e Compare September 12, 2023 07:37

HannesWell reviewed Sep 12, 2023

View reviewed changes

fedejeanne force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch 2 times, most recently from 694a2e4 to f50e586 Compare September 13, 2023 05:28

fedejeanne force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch from f50e586 to 6c443cc Compare September 13, 2023 05:32

HannesWell approved these changes Sep 13, 2023

View reviewed changes

fedejeanne force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch from 6c443cc to d2a2ce2 Compare September 13, 2023 08:30

HeikoKlare reviewed Sep 13, 2023

View reviewed changes

...bug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LaunchConfigurationsDialog.java Outdated Show resolved Hide resolved

fedejeanne added 2 commits September 13, 2023 11:00

Bump version in Manifest

c0b72e9

* org.eclipse.debug.ui --> 3.18.200

fedejeanne force-pushed the enhancements/show-progress-in-run-configurations-dialog-after-delay branch from d2a2ce2 to c0b72e9 Compare September 13, 2023 09:01

HeikoKlare approved these changes Sep 13, 2023

View reviewed changes

HannesWell approved these changes Sep 13, 2023

View reviewed changes

HannesWell merged commit f999b9d into eclipse-platform:master Sep 13, 2023

fedejeanne deleted the enhancements/show-progress-in-run-configurations-dialog-after-delay branch September 14, 2023 04:00

Show progress indicator of "Run configurations..." after delay #591

Show progress indicator of "Run configurations..." after delay #591

Conversation

fedejeanne commented Jul 25, 2023 • edited Loading

github-actions bot commented Jul 25, 2023 • edited Loading

Test Results

HannesWell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bananeweizen commented Aug 12, 2023

fedejeanne commented Aug 14, 2023 • edited Loading

fedejeanne commented Sep 8, 2023

mickaelistria commented Sep 11, 2023

fedejeanne commented Sep 11, 2023

iloveeclipse commented Sep 11, 2023

merks commented Sep 11, 2023

fedejeanne commented Sep 11, 2023

merks commented Sep 11, 2023

HannesWell commented Sep 11, 2023

laeubi commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesWell commented Sep 12, 2023

laeubi commented Sep 13, 2023

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell commented Sep 13, 2023

HeikoKlare left a comment

Choose a reason for hiding this comment

HannesWell left a comment • edited Loading

Choose a reason for hiding this comment

fedejeanne commented Sep 14, 2023

laeubi commented Sep 18, 2023

merks commented Sep 18, 2023

laeubi commented Sep 18, 2023

merks commented Sep 18, 2023

laeubi commented Sep 18, 2023

fedejeanne commented Jul 25, 2023 •

edited

Loading

github-actions bot commented Jul 25, 2023 •

edited

Loading

fedejeanne commented Aug 14, 2023 •

edited

Loading

HannesWell left a comment •

edited

Loading