Move long running operations to the activation of the PluginsTab

fedejeanne commented 2 months ago

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

[x] I tested my changes thoroughly

Contributes to https://github.com/eclipse-pde/eclipse.pde/issues/1232

github-actions[bot] commented 2 months ago

Test Results

285 files - 6 285 suites - 6 45m 3s :stopwatch: - 19m 45s 3 527 tests + 1 3 468 :white_check_mark: ± 0 58 :zzz: ± 0 1 :x: +1 8 152 runs - 2 723 7 998 :white_check_mark: - 2 700 153 :zzz: - 24 1 :x: +1

For more details on these failures, see this check.

Results for commit bc7b0df9. ± Comparison against base commit b8c244de.

:recycle: This comment has been updated with latest results.

fedejeanne commented 2 months ago

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.

laeubi commented 2 months ago

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 commented 2 months ago

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

fedejeanne commented 2 months ago

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 https://github.com/eclipse-pde/eclipse.pde/pull/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 commented 2 months ago

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

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

fedejeanne commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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:

[x] I tested my changes thoroughly :-)

fedejeanne commented 2 months ago

Failing test is unrelated

fedejeanne commented 2 months ago

@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 commented 2 months ago

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

fedejeanne commented 2 months ago

Squashed 👍

fedejeanne commented 2 months ago

Failing test is unrelated and already documented here: https://github.com/eclipse-pde/eclipse.pde/issues/554

iloveeclipse commented 2 months ago

I assume this change caused new regression, please check https://github.com/eclipse-pde/eclipse.pde/issues/1250.

eclipse-pde / eclipse.pde

Move long running operations to the activation of the PluginsTab #1233

How to test

Test Results