Select the current tab after displaying them

[fedejeanne]

Call handleTabSelected right after displaying all tabs in the LaunchConfigurationTabGroupViewer::displayInstanceTabs so that the "activated" method of the tab is called.

This is necessary in order to give every tab the chance to compute/show its content when it is first selected by changing between Run configurations in the Run Configurations Dialog.

Contributes to eclipse-pde/eclipse.pde#674

How to test

1. Run sdk.product
2. Open the Run configurations Dialog
3. Create 2 run configurations
4. Select one configuration and click on any Tab other than the first tab
5. Switch to the other configuration

• The same tab should be active in the newly selected run configuration
• The activated method of that tab should have been called (the method extends/overrides org.eclipse.debug.ui.ILaunchConfigurationTab.activated(ILaunchConfigurationWorkingCopy))

e.g. testing with Product Run Configurations (PDE)

1. Run sdk.product
2. Open the Run configurations Dialog
3. Create 2 Product Run configurations
4. Select one configuration and click on the Tab Java Arguments
   4.1. Add a breakpoint in org.eclipse.jdt.debug.ui.launchConfigurations.JavaArgumentsTab.activated(ILaunchConfigurationWorkingCopy)
5. Switch to the other configuration

• The tab Java Arguments should be active in the newly selected run configuration
• The execution should stop at the breakpoint

[fedejeanne]

FYI this PR is a must in order to be able to merge eclipse-pde/eclipse.pde#674

[github-actions]

Test Results

     591 files  ±0       591 suites  ±0   1h 4m 49s ⏱️ - 2m 45s
  3 798 tests ±0    3 793 ✔️ +1    5 💤 ±0  0 ❌  - 1 
12 000 runs  ±0  11 964 ✔️ +1  36 💤 ±0  0 ❌  - 1 

Results for commit 1ea5dce. ± Comparison against base commit e0f98f9.

♻️ This comment has been updated with latest results.

[fedejeanne]

Test failures already documented here: #596, #629, #768.

The new ones were documented here: #770, #771, #772.

None of the test failures seem to be related to the changes introduced in this PR though since no test failed again in the 2nd CI build.

Reviews are welcome!

[HeikoKlare]

I see the necessity and goal of the change, but to me it does not look like a proper solution for the issue. Here are my concerns:

• handleTabSelected() explicitly avoids a reactivation when the tab index did not change. Is it correct to remove that precondition? I see that if you want to (mis)use the method to reactivate a tab, this condition must be removed, but is that what the method is supposed to do (considering its other usages)? Otherwise it might make more sense to move that reactivation functionality into a separate method that is called in the scenario to be addressed without the precondition of the handleTabSelected() method. That might make sense anyway as handleTabSelected suggests that it should be called if a tab was selected, but this is not even the case here. It is rather about reloadTab or the like.
• The displayInstanceTabs() methods does already process all the tabs in its call of showInstanceTabs(). It calls setActiveTab() for all tabs once and then calls it again for the one that that is to be active afterwards. However, these setActiveTab calls finally seem to do nothing else than calling handleTabSelected, which does nothing as fInitializingTabs is set to true, so the method immediately returns without doing anything. So maybe there is something broken in the overall functionality, as it looks to me as if the last lines of code in showInstanceTabs are supposed to do exactly what this PR is trying to fix. Please check what that code does. And if it is really not related to what this PR is supposed to achieve, it may at least point to a better solution of the problem.

[fedejeanne]

@HeikoKlare I changed it so handleTabSelected() will only be called for this specific use case (switching between Run Configurations) and not in the other 2 use cases so it's safe now, the changes are contained.
It's still necessary to remove the condition fCurrentTabIndex == fTabFolder.getSelectionIndex() for these changes to work properly, but I tested all 3 use cases where the method handleTabSelected() is called and the changes don't cause any undesired behavior so it's safe to apply them.

The 3 use cases

1. Switching between Run configurations
2. Leaving the JRE tab having changed the JRE to a non-modular JRE/Execution environment (e.g. Java 1.8)
3. Selecting another launcher for the Run configuration (this is only possible in certain circumstances)

This PR should be ready to be merged now.

[fedejeanne]

The failed check is unrelated to the changes, it seems to be a problem with Tycho and the tests in Mac:

Error:  Failed to execute goal org.eclipse.tycho:tycho-surefire-plugin:4.0.4-SNAPSHOT:test (default-test) on 
project org.eclipse.core.tests.resources: Execution default-test of goal 
org.eclipse.tycho:tycho-surefire-plugin:4.0.4-SNAPSHOT:test failed: Exception creating test eclipse runtime: 
zip file is empty -> [Help 1]

[HeikoKlare]

Please see my previous comment. From my understanding, there is already code that is supposed to do what this change is trying to do. Either correct me in case I am wrong or otherwise please fix or replace that code.

[fedejeanne]

I re-read your previous comment and made some changes. I pushed the activation of the tabs all the way back to the end of displayInstanceTabs(...) (after the flag fInitializingTabs is switched-off) and I reverted my changes in handleTabSelected(). I even got rid of a small bug that activated the first tab unnecessarily.

[HeikoKlare]

I really like the latest solution 👍 In my opinion, it is very comprehensible now. I have only minor comments to the fix itself. And could we add a regression test for it?

Please also fix the Javadoc of the refreshTabs0 method when touching it anway.

[fedejeanne]

Thank you for your reviews, @HeikoKlare ! I merged this one so eclipse-pde/eclipse.pde#674 can be also merged.

And could we add a regression test for it?

Let's put a pin on that. The new functionality has been tested manually and it's looking fine.

Please also fix the Javadoc of the refreshTabs0 method when touching it anway.

I didn't touch that one.

[HannesWell]

And could we add a regression test for it?\n\nLet's put a pin on that. The new functionality has been tested manually and it's looking fine.

I want to mention that the addition of tests ist permitted all the time, so no reason to postpone that from a RelEng/freeze-time point of view. :)