Move long running operations to the activation of the PluginsTab

[fedejeanne]

Move the long running operations in the PluginsTab from initializeFrom to activated. Only run these operations the first time the tab is activated in order to:
a. Only run them when the tab is actually selected
b. Not run them again if the tab is deselected and selected again (without leaving the current launch configuration)

How to test

• Create 2 new launch configurations from inside the Run Configurations dialog: one Eclipse Application (1) and one JUnit-Plugin Test (2)
• Select the Plugins tab in one of them and make some changes (save them)
• Move to the other configuration --> The Plugins tab should be selected and it should look fine. The activation should not be skipped i.e. it should not hit the return; in line 167
• Move back to the first configuration --> The Plugins tab should be selected and contain all changes you applied before. The activation should not be skipped
• Select another tab
• Select the Plugins tab again --> The activation should be skipped i.e. hit should hit the return; in line 167

• I tested my changes thoroughly

Contributes to #1232

[github-actions]

Test Results

  285 files   -     6    285 suites   - 6   45m 3s ⏱️ - 19m 45s
3 527 tests +    1  3 468 ✅ ±    0   58 💤 ± 0  1 ❌ +1 
8 152 runs   - 2 723  7 998 ✅  - 2 700  153 💤  - 24  1 ❌ +1 

For more details on these failures, see this check.

Results for commit bc7b0df. ± Comparison against base commit b8c244d.

♻️ This comment has been updated with latest results.

[fedejeanne]

Test failures seem to be unrelated and they fail also locally (I tested on my machine) even before applying this PR. See #1227

The license check seems unrelated too, though I don't know why it happens.

[HannesWell]

Thank you for this.

I have tested this and in general this works well, I just noticed that when I switch between the configs with the Plug-ins tab activate and don't save, the activation is never skipped. Is that intended?

[laeubi]

I just remember that there was already an attempt in the past and there where also some glitches so one should be aware of that.

The build failure effectively says that you changed the public API documentation and therefore need to touch the docs bundle.

[laeubi]

By the way if the expensive operation can be identified, it could be wrapped in a BusyIndicator.call(...) to have better UI feedback.

[fedejeanne]

Thank you for this.

I have tested this and in general this works well

👍

I just noticed that when I switch between the configs with the Plug-ins tab activate and don't save, the activation is never skipped. Is that intended?

Yes, it's intended: I can't skip the activation when first switching (or switching back) to a launch config because the tab is instantiated again and everything needs to be populated again. Skipping is only possible when switching back and forth between tabs in the same launch config because the config (and all its tab) remain in memory.

I just remember that there was already an attempt in the past and there where also some glitches so one should be aware of that.

@laeubi you mean #946 ? The glitch was that the activated method of the default tab (or better said: "the first selected tab") of a launch configuration was not being called. I checked this now and it's being called :-)

[fedejeanne]

I also inlined a call in validateTab and removed its JavaDoc since it was inconsistent.

I hope I addressed everything, @laeubi and @HannesWell ?

[fedejeanne]

I hope I addressed everything, @laeubi and @HannesWell ?

Actually I forgot to wrap the method in 'BusyIndicator.showWhile(...)'. I'll do it today.

Feel free to comment on the rest

[fedejeanne]

I found an error when creating a new Eclipse launch configuration in the dialog:

!STACK 0
java.lang.NullPointerException: Cannot invoke "org.eclipse.swt.widgets.Button.getSelection()" because "this.fIncludeOptionalButton" is null
	at org.eclipse.pde.internal.ui.launcher.AbstractPluginBlock.performApply(AbstractPluginBlock.java:951)
	at org.eclipse.pde.internal.ui.launcher.BlockAdapter.performApply(BlockAdapter.java:84)
	at org.eclipse.pde.ui.launcher.PluginsTab.performApply(PluginsTab.java:210)
	at org.eclipse.debug.ui.AbstractLaunchConfigurationTabGroup.performApply(AbstractLaunchConfigurationTabGroup.java:106)
	at org.eclipse.pde.ui.launcher.EclipseLauncherTabGroup.performApply(EclipseLauncherTabGroup.java:46)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationTabGroupWrapper.performApply(LaunchConfigurationTabGroupWrapper.java:205)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationTabGroupViewer.handleApplyPressed(LaunchConfigurationTabGroupViewer.java:1521)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.handleLaunchConfigurationSelectionChanged(LaunchConfigurationsDialog.java:1045)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationsDialog.lambda$1(LaunchConfigurationsDialog.java:607)
	at org.eclipse.jface.viewers.StructuredViewer$3.run(StructuredViewer.java:820)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
	at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:174)
	at org.eclipse.jface.viewers.StructuredViewer.firePostSelectionChanged(StructuredViewer.java:817)
	at org.eclipse.jface.viewers.ColumnViewer.firePostSelectionChanged(ColumnViewer.java:1065)
	at org.eclipse.jface.viewers.StructuredViewer.setSelection(StructuredViewer.java:1665)
	at org.eclipse.jface.viewers.TreeViewer.setSelection(TreeViewer.java:1091)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationView.handleConfigurationAdded(LaunchConfigurationView.java:330)
	at org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationView.lambda$0(LaunchConfigurationView.java:305)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
...

I'm setting this PR to be a draft so I can further look into it tomorrow. I'll let you know when it's ready to be reviewed again

[fedejeanne]

Ready for review. The NPE was because of a premature invocation of AbstractPluginBlock.performApply. Skipping that invocation until the block has been created (i.e. until after createControl has been called) fixes the issue and has no side-effects.

FTR:

• I tested my changes thoroughly :-)

[fedejeanne]

Failing test is unrelated

[HannesWell]

Thanks for the update.
While trying out the change I noticed that the activated method is already called in a 'busy-curser' context, so we don't have to start another nested one and save the extra code(complexity). Btw. in this case I would have just passed null as Display to reuse the current one.

To prevent the NPE I also think we should use a more local check.
I pushed my suggestions as another commit to your branch. Please incorporate it as you find it suitable.

If you agree with the suggestions, I'm fine with this change.

[fedejeanne]

@HannesWell thank you for the suggestion, I like the changes!

Ready to merge on my part 👍

I assume in this case 2 commits are OK? If not, let me know and I can squash them and add you as co-author :-)

[HannesWell]

Great.
Please just squash them. No need to record the discussion in the git history. :)

[fedejeanne]

Squashed 👍

[fedejeanne]

Failing test is unrelated and already documented here: #554

[HannesWell]

Thank you @fedejeanne. Nice contribution.
Activating a Eclipse launch config becomes faster and faster with your contributions. :)

[iloveeclipse]

I assume this change caused new regression, please check #1250.