Show a progress indicator when opening a PDE run configuration. #674

fedejeanne · 2023-07-21T06:55:51Z

The progress indicator uses IProgressMonitor.UNKNOWN as the total amount of work because the (sometimes) long running operation is deep in the call stack: it goes all the way down to the TracingOptionsManager in org.eclipse.pde.core (dependency issues).

The operation can not be canceled.

Fixes #679

github-actions · 2023-07-21T20:53:36Z

Test Results

  264 files ±0   264 suites ±0 51m 2s ⏱️ - 1m 56s
3 328 tests ±0 3 295 ✔️ - 2 30 💤 ±0 3 ❌ +2
7 557 runs ±0 7 488 ✔️ - 2 66 💤 ±0 3 ❌ +2

For more details on these failures, see this check.

Results for commit 8d12915. ± Comparison against base commit d551a60.

♻️ This comment has been updated with latest results.

HannesWell · 2023-07-23T12:30:04Z

With this change now always the progress-indicator pops when opening the launch-configuration Editor, even if it is just for a very short time.
A pop-up that vanishes within the fraction of a second so that the user cannot read its content is IMHO disturbing.
Wouldn't it be possible to show the pop-up only after a certain amount of time or estimate better if the operation is long-runnning?

fedejeanne · 2023-07-25T15:40:25Z

Wouldn't it be possible to show the pop-up only after a certain amount of time or estimate better if the operation is long-runnning?

I made some changes and shifted all the heavy lifting to the activation of the Tracing tab, so I managed to get rid of the dialog at the beginning.

The problem with the flickering still remains, but it now happens while changing some of the configurations in the Tracing tab and the flickering happens in the lower part of the Run configurations... dialog (in the progress bar). The good thing is that fixing that is easier than fixing the dialog, here's a draft PR to do that --> eclipse-platform/eclipse.platform#591

HannesWell

I looked into this and as written below an additional cache seems not to be necessary.

But shifting the first call to the TracingManager seems reasonable.
I suggest we add a method to TracingOptionsManager like ensureInitialized() that basically calls getTracingTemplate() and passes a ProgressMonitor to it so that we can at least see progress. It shouldn't be canclable because otherwise the state would become inconsistent or more complex to manage.

I'm currently working on a greater clean up of many of the launcher classes, which would also make the TracingOptionsManager a bit simpler. Maybe you want to await that before adding the suggested ensureInitialized() method, in order to avoid conflicts.

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingBlock.java

fedejeanne · 2023-08-15T14:15:04Z

I'm currently working on a greater clean up of many of the launcher classes, which would also make the TracingOptionsManager a bit simpler. Maybe you want to await that before adding the suggested ensureInitialized() method, in order to avoid conflicts.

👌
Please let me know when your PR is ready/merged and I can change this one.

HannesWell · 2023-10-21T18:26:16Z

Please let me know when your PR is ready/merged and I can change this one.

Since #755 is now submitted, we can continue on this one. :)

HannesWell · 2023-10-22T14:34:14Z

I suggest we add a method to TracingOptionsManager like ensureInitialized() that basically calls getTracingTemplate() and passes a ProgressMonitor to it so that we can at least see progress. It shouldn't be canclable because otherwise the state would become inconsistent or more complex to manage.

Thinking about it again, a TracingOptionsManager.ensureInitialized(IProgressMonitor) method could be canceled but then all results computed until then should be discarded.

To achieve this it is probably sufficient to replace TracingOptionsManager.getTracingTemplate() by

	public void ensureInitialied(IProgressMonitor monitor) {
		getTracingTemplate(monitor);
	}

	private synchronized Map<String, String> getTracingTemplate(IProgressMonitor monitor) {
		if (template == null) {
			Map<String, String> temp = new HashMap<>();
			IPluginModelBase[] models = PluginRegistry.getAllModels();
			SubMonitor subMonitor = SubMonitor.convert(monitor, models.length);
			Arrays.stream(models).map(TracingOptionsManager::getOptions).filter(Objects::nonNull).forEach(p -> {
				subMonitor.split(1);
				@SuppressWarnings({ "rawtypes", "unchecked" })
				Map<String, String> entries = (Map) p;
				temp.putAll(entries); // All entries are of String/String
			});
			template = temp;
		}
		return template;
	}

and pass just null as monitor in all other callers of getTracingTemplate().

Call handleTabSelected right after displaying all tabs in the LaunchConfigurationTabGroupViewer::displayInstanceTabs so that the "activated" method of the tab is called. Contributes to eclipse-pde/eclipse.pde#674

fedejeanne · 2023-10-26T09:16:23Z

@HannesWell I just pushed some changes and took your advice about using the ProgressMonitor inside the method TracingOptionsManager::getTracingTemplate, thank you for that! I made some adjustments though:

I didn't add any method to make sure the template is initialized ( e.g. ensureInitialized) because I had no need for it
I converted the stream into a for-loop to properly report the progress since one needs to call monitor.split(1) for every model and not for every filtered (non-null) model.
I inverted the if-logic in the method to reduce the cognitive complexity (nesting level).

Edit: additional changes

I did not make the tasks/monitors cancelable for now because that requires to rethink how the UI behaves once the task has been canceled (probably some elements need to be disabled) and also where to handle the proper exceptions in the code. I'd like to leave that for a future PR since these changes already provide a good enough performance and usability improvement even without the task being cancelable

Important: do NOT merge yet

This PR is blocked by eclipse-platform/eclipse.platform#766

fedejeanne · 2023-10-26T11:52:17Z

Test failure already documented here: #820

HannesWell

I made some adjustments though:

* I didn't add any method to make sure the template is initialized ( _e.g._ `ensureInitialized`) because I had no need for it


* I inverted the `if`-logic in the method to reduce the [cognitive complexity (nesting level)](https://docs.sonarsource.com/sonarqube/latest/user-guide/metric-definitions/).

That's fine.

* I converted the stream into a `for`-loop to properly report the progress since one needs to call `monitor.split(1)` for every model and not for every _filtered_ (non-null) model.

Good catch!

I did not make the tasks/monitors cancelable for now because that requires to rethink how the UI behaves once the task has been canceled (probably some elements need to be disabled) and also where to handle the proper exceptions in the code. I'd like to leave that for a future PR since these changes already provide a good enough performance and usability improvement even without the task being cancelable

Sounds reasonable. If you one day make it cancalable, the Tracing-tab could look similar to the disabled tracing state, but the Enable tracing check-box state should not be affected by that. The editor could then contain a button instead to load the tracing options again.

With the mentioned changes this would be perfectly ready from PDE side.

This PR is blocked by eclipse-platform/eclipse.platform#766

I'll try to participate there over the weekend.

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/TracingOptionsManager.java

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingBlock.java

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/PDEUIMessages.java

HannesWell · 2023-10-27T22:22:56Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingBlock.java

+				fRunnableContext.run(true, false, monitor -> {
+					SubMonitor subMonitor = SubMonitor.convert(monitor, taskName, 1);
+					result.putAll(task.apply(subMonitor.split(1)));
+				});


It would be nice if there would be some kind of ICallableWithProgress.

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingBlock.java

Call handleTabSelected when switching to a new run configuration so that the "activated" method of the tab is called. Contributes to eclipse-pde/eclipse.pde#674

fedejeanne · 2023-10-30T12:53:11Z

Sounds reasonable. If you one day make it cancalable, the Tracing-tab could look similar to the disabled tracing state, but the Enable tracing check-box state should not be affected by that. The editor could then contain a button instead to load the tracing options again.

Thank you! I'll keep that in my for the next PR :-)

I'll try to participate there over the weekend.

Thank you!

fedejeanne · 2023-10-30T14:21:56Z

Test failures documented in #554 and #844

HannesWell

Thanks for the adjustments.

I just have two non-critical remarks and then this is ready from the PDE site.

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingBlock.java

Don't start unnecessary operations that only concern the "Tracing" tab upon initialization of the PDE-Run configuration (in the dialog "Run configurations..."). Instead, only do them when the tab is first selected. This speeds up opening the dialog "Run configurations..." (and also switching to a PDE-Run configuration when the dialog is already opened). Show a progress indicator when doing some (possibly) long-running operations in the "Tracing" tab. Fixes to eclipse-pde#679

HannesWell

Great. So this looks ready from a PDE point of view.

Should we wait until your related Platform PR ist merged, before merging this?

fedejeanne · 2023-11-02T10:46:38Z

Yes please, let's wait until eclipse-platform/eclipse.platform#766 is merged otherwise this PR will introduce a bug.

Make sure that the selected tab is activated (i.e. that its "activate" method is called) when switching between existing run configurations in the "Run configurations" dialog. Contributes to eclipse-pde/eclipse.pde#674

fedejeanne · 2023-11-06T06:30:52Z

@HannesWell eclipse-platform/eclipse.platform#766 is merged --> this PR is good to go :)

HannesWell · 2023-11-06T07:53:32Z

Great!
Thanks for this and the platform fix.
Nice work :)

…acing" tab." This reverts commit 7c1f0ed from eclipse-pde#674. The change introduced a regression causing Tracing tabs not to be initialized properly in specific situation. It requires an update to the Eclipse platform, which will not be integrated into the upcoming release anymore. Therefore, the change is reverted for the release to be reapplied after the next release. The effect of reverting the change will be a loss of the performance improvement in the launch configurations dialog that was gained by the change.

…acing" tab." This reverts commit 7c1f0ed from #674. The change introduced a regression causing Tracing tabs not to be initialized properly in specific situation. It requires an update to the Eclipse platform, which will not be integrated into the upcoming release anymore. Therefore, the change is reverted for the release to be reapplied after the next release. The effect of reverting the change will be a loss of the performance improvement in the launch configurations dialog that was gained by the change.

Make sure that the selected tab is activated (i.e. that its "activate" method is called) when switching between existing run configurations in the "Run configurations" dialog. Contributes to eclipse-pde/eclipse.pde#674

fedejeanne force-pushed the enhancements/vi-pde-1_show-progress-in-run-configuration-dialog branch 2 times, most recently from 447c930 to afc0dca Compare July 25, 2023 15:16

fedejeanne force-pushed the enhancements/vi-pde-1_show-progress-in-run-configuration-dialog branch 2 times, most recently from 3159f64 to d0bfdb8 Compare July 26, 2023 09:49

This was referenced Jul 26, 2023

[Mac] testImport no-configuration (org.eclipse.ui.tests.smartimport.PlainJavaProjectTest) fails #555

Closed

org.eclipse.pde.api.tools.builder.tests.compatibility.BundleMergeSplitTests fails #261

Open

HannesWell mentioned this pull request Aug 12, 2023

Show progress indicator of "Run configurations..." after delay eclipse-platform/eclipse.platform#591

Merged

HannesWell reviewed Aug 14, 2023

View reviewed changes

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingBlock.java Outdated Show resolved Hide resolved

HannesWell mentioned this pull request Sep 21, 2023

Clean-up and simplify Tracing-Options processing #755

Merged

This was referenced Oct 25, 2023

Select the current tab after displaying them eclipse-platform/eclipse.platform#766

Merged

The progress indicator of the LaunchConfigurationsDialog is not shown when calling "run" with "fork == false" eclipse-platform/eclipse.platform#769

Open

fedejeanne force-pushed the enhancements/vi-pde-1_show-progress-in-run-configuration-dialog branch 3 times, most recently from 68ce965 to a11dd95 Compare October 26, 2023 09:05

HannesWell reviewed Oct 27, 2023

View reviewed changes

fedejeanne force-pushed the enhancements/vi-pde-1_show-progress-in-run-configuration-dialog branch from a11dd95 to 9fe6dc6 Compare October 30, 2023 13:43

fedejeanne mentioned this pull request Oct 30, 2023

[Win] Unknown test (org.eclipse.pde.ui.tests.launcher.FeatureBasedLaunchTest) failed #554

Open

fedejeanne force-pushed the enhancements/vi-pde-1_show-progress-in-run-configuration-dialog branch from 9fe6dc6 to 970130a Compare October 30, 2023 19:21

fedejeanne force-pushed the enhancements/vi-pde-1_show-progress-in-run-configuration-dialog branch 2 times, most recently from 8f47de8 to 6a39932 Compare October 30, 2023 19:46

HannesWell reviewed Oct 30, 2023

View reviewed changes

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingBlock.java Outdated Show resolved Hide resolved

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/TracingBlock.java Show resolved Hide resolved

HannesWell force-pushed the enhancements/vi-pde-1_show-progress-in-run-configuration-dialog branch from 6a39932 to f8d18e2 Compare October 30, 2023 23:51

fedejeanne force-pushed the enhancements/vi-pde-1_show-progress-in-run-configuration-dialog branch from f8d18e2 to 8d12915 Compare October 31, 2023 10:46

HannesWell approved these changes Oct 31, 2023

View reviewed changes

vogella merged commit 7c1f0ed into eclipse-pde:master Nov 6, 2023
12 of 14 checks passed

fedejeanne deleted the enhancements/vi-pde-1_show-progress-in-run-configuration-dialog branch November 6, 2023 07:08

fedejeanne mentioned this pull request Nov 6, 2023

Run Configurations Dialog: Make the Loading of Tracing Options Cancelable vi-eclipse/Eclipse-PDE#3

Open

This was referenced Nov 16, 2023

Call the "activated" method of the default tab in a Launch Config Dialog eclipse-platform/eclipse.platform#860

Merged

The first tab is not properly activated when opening the "Run configurations" dialog eclipse-platform/eclipse.platform#859

Closed

HeikoKlare mentioned this pull request Nov 16, 2023

Revert "Postpone long-running operations to the activation of the "Tracing" tab." #925

Merged

fedejeanne mentioned this pull request Nov 29, 2023

Recreate #674: Postpone long-running operations to the activation of the "Tracing" tab. #946

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a progress indicator when opening a PDE run configuration. #674

Show a progress indicator when opening a PDE run configuration. #674

fedejeanne commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 21, 2023 •

edited

Loading

HannesWell commented Jul 23, 2023

fedejeanne commented Jul 25, 2023

HannesWell left a comment

fedejeanne commented Aug 15, 2023

HannesWell commented Oct 21, 2023

HannesWell commented Oct 22, 2023

fedejeanne commented Oct 26, 2023 •

edited

Loading

fedejeanne commented Oct 26, 2023

HannesWell left a comment

HannesWell Oct 27, 2023

fedejeanne commented Oct 30, 2023

fedejeanne commented Oct 30, 2023

HannesWell left a comment

HannesWell left a comment

fedejeanne commented Nov 2, 2023

fedejeanne commented Nov 6, 2023

HannesWell commented Nov 6, 2023

Show a progress indicator when opening a PDE run configuration. #674

Show a progress indicator when opening a PDE run configuration. #674

Conversation

fedejeanne commented Jul 21, 2023 • edited Loading

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

Test Results

HannesWell commented Jul 23, 2023

fedejeanne commented Jul 25, 2023

HannesWell left a comment

Choose a reason for hiding this comment

fedejeanne commented Aug 15, 2023

HannesWell commented Oct 21, 2023

HannesWell commented Oct 22, 2023

fedejeanne commented Oct 26, 2023 • edited Loading

Edit: additional changes

Important: do NOT merge yet

fedejeanne commented Oct 26, 2023

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Oct 27, 2023

Choose a reason for hiding this comment

fedejeanne commented Oct 30, 2023

fedejeanne commented Oct 30, 2023

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell left a comment

Choose a reason for hiding this comment

fedejeanne commented Nov 2, 2023

fedejeanne commented Nov 6, 2023

HannesWell commented Nov 6, 2023

fedejeanne commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 21, 2023 •

edited

Loading

fedejeanne commented Oct 26, 2023 •

edited

Loading