Skip the method PluginsTab::performApply until after activation

[fedejeanne]

What it does

Fixes https://github.com/eclipse-pde/eclipse.pde/issues/1250 (a regression introduced by #1233) by skipping the whole method PluginsTab::performApply before the Plugins tab was activated. The reason for this is that this method changes some values in the configuration it receives as a parameter and it does it too early. If the method is called after having completely activated the tab then the configuration already has the proper values and they will not be overwritten.

Bug description

When a Launch configuration that has a tab called Plugins (e.g. an Eclipse Application or a JUnit Plug-In Test) is opened and the Plugins tab is selected, the default values are wrong (see screenshots in https://github.com/eclipse-pde/eclipse.pde/issues/1250#issuecomment-2074775823)

How to reproduce

• Launch a JUnit Plugin Test with Right click > Run As... > JUnit Plugin Test
• Open the Run configurations dialog and select that run configuration that was just created
• Switch to the Plugins tab --> You'll see that the usual default values for Default Start Level (4) and Default Auto-Start (false) will be replaced by 1 and true, like this:

After applying this PR

• The default values should be 4 and false again

DISCLAIMER: the screenshots are from this https://github.com/eclipse-pde/eclipse.pde/issues/1250#issuecomment-2074775823

[fedejeanne]

@iloveeclipse this is my proposed fix, could you please also test it?

[github-actions[bot]]

Test Results

   291 files  ±0     291 suites  ±0   53m 28s :stopwatch: +32s  3 526 tests ±0   3 468 :white_check_mark: ±0   58 :zzz: ±0  0 :x: ±0  10 875 runs  ±0  10 698 :white_check_mark: ±0  177 :zzz: ±0  0 :x: ±0 

Results for commit dc7ca510. ± Comparison against base commit 80462868.

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

[iloveeclipse]

What you should test is that opening Launch dialog should NOT change launch config and not add anything to the launch configuration without user actually changing any value in the dialog, so just opening and closing dialog should NOT ask that "something is changed, do you want to save"?

[HannesWell]

I haven't looked into the details (and will probably not have the time to do it this evening), but maybe instead of skipping the call to performApply() entirely, maybe it is possible to just make the code that caused the NPE in #1233 null save?

[fedejeanne]

@iloveeclipse which wizard did you use? I can't reproduce the error by creating a new JUnit Plugin Test configuration in the Run configurations dialog. I am moving through all tabs in the launch configuration (coming to the Plugins tab at the end) and the Apply button is never enabled, there are no changes :-\

@HannesWell I can look into that once I'm able to reproduce the error that @iloveeclipse mentioned in https://github.com/eclipse-pde/eclipse.pde/pull/1252#pullrequestreview-2020273264

[iloveeclipse]

No wizards, no dialogs, simply run a test from package explorer - this will create launch config. Opening same config in dialog changes it as decribed.

[HannesWell]

Alternatively or at the same time some of the config reads should probably be moved back to the initialize method. Especially those that correspond to the problematic attributes (they are cheap to compute).

[fedejeanne]

@iloveeclipse I was able to reproduce the error and I found the culprit: some attributes like show_selected_only are being written to the configuration and they hold the default value (false). This enables the Apply button because the button compares the original configuration with the "new" one and they only differ in the fact that the "new" one has 1 extra entry: show_selected_only: false (default value). If I delete this entry then I fix the problem for the JUnit Plugin Test configurations but I break it for the Eclipse Application configuration (this one works in the exact opposite way: the "original" configuration has the entry with the default value so I shouldn't remove it from the "new" one).

@HannesWell making the code null-safe doesn't work (I tried it) because writing into the configuration is based on the values of the fields that were null so when I skip those write-operations, the "original" and the "new" configuration end up: a) Being different --> The Apply button is enabled b) Being wrong --> The "new" one (the one that reflects what the UI says) is wrong and sets the Auto-Start and the Default Start Level to true and 1 respectively.

I'm going to keep looking for a solution for a while longer but I think that, regardless of whether or not I find one today (and I don't think I will), we should apply #1251 and give ourselves some time to test this properly. WDYT?

[iloveeclipse]

we should apply #1251 and give ourselves some time to test this properly. WDYT

Yes. It would be also better to have one commit that fixes original issue as multiple. I will merge the revert now.

[merks]

FYI. That launch configuration resources change, i.e., reformat or add new attributes, merely by visiting the configuration is something that’s been happening for a very long time.

[iloveeclipse]

FYI. That launch configuration resources change, i.e., reformat or add new attributes, merely by visiting the configuration is something that’s been happening for a very long time.

May be, but in concrete case the commit in question was yet another case that was problematic, reproducible and fixed with reverting the change.

[fedejeanne]

I was finally able to track down all the attributes that were causing the Apply button to be enabled by simply selecting a tab in a run configuration.

I did it with the help of these changes in the class org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationTabGroupViewer

/**
 * updates the button states
 */
private void updateButtons() { // modifiy this method
    boolean isDirty = isDirty();
    printDebugInfo(isDirty); // add this call
    fApplyButton.setEnabled(isDirty && canSave());
    fRevertButton.setEnabled(isDirty);
    fShowCommandLineButton.setEnabled(canLaunch());
}

@SuppressWarnings("nls")
private void printDebugInfo(boolean dirty) { // add this method
    if (!dirty)
        return;

    System.out.println("The launch configuration has changed");

    printMissingKeys(fOriginal, getWorkingCopy());
    printMissingKeys(getWorkingCopy(), fOriginal);
}

/**
 * Print the keys of <code>a</code> that are not present in <code>b</code>
 */
@SuppressWarnings("nls")
private void printMissingKeys(ILaunchConfiguration a, ILaunchConfiguration b) { // add this method
    try {
        Map<String, Object> aAttr = a.getAttributes();
        Map<String, Object> temp = new HashMap<>(b.getAttributes());
        for (String k : aAttr.keySet())
            temp.remove(k);

        if (!temp.isEmpty())
            System.out.println(
                    "Keys missing in '" + a.getName() + "' (" + a.getType().getName() + "): " + temp.keySet());
    } catch (CoreException e) {
        DebugUIPlugin.log(e);
    }
}

I also removed all those attributes instead of setting them to their original values (see the 2nd commit in this PR). This fixes that problem.

How I tested

• I modified org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationTabGroupViewer as described above (it helps spotting the offending attribute)
• I created new run configurations directly from the Run Configurations dialog
• I selected each and every tab in the newly created configurations --> The Apply button should not be enabled
• I deleted all the created configurations and closed the Run Configurations dialog
• I created run configurations again by doing (in the package explorer) (Right click) Run As... > XYZ
• I opened the Run Configurations dialog again
• I selected each and every tab in the newly created configurations --> The Apply button should not be enabled

The reason I did this is because creating the run configurations (instances of LaunchConfiguration and LaunchConfigurationWorkingCopy) is done by different methods when creating them from the wizard vs doing it via (Right click) Run As... > XYZ

I also double checked that the Default Start Level and Default Auto-Start still have their appropriate default values of 4 and true, both for configurations of type Eclipse Application and JUnit Plu-in Test.

Would you guys please take a final look into the new changes and report back in case of new regressions? It would be helpful for me if you could modify org.eclipse.debug.internal.ui.launchConfigurations.LaunchConfigurationTabGroupViewer as described above and paste the output in the console after you selected each and every tab in the run configuration. This would show the problematic attributes like:

Keys missing in 'Eclipse Application' (Eclipse Application): [includeOptional, show_selected_only, useCustomFeatures, automaticAdd]

If this PR is then OK for you then I can squash the commits.

[fedejeanne]

The failing test seems unrelated:

Failures: 
  Java7MethodUsageTests>TestCase.runTest:972->testStringSwitchF:76->x1:113->Java7UsageTest.deployUsageTest:106->ApiBuilderTest.expectingNoJDTProblemsFor:215 Should not be a JDT error: Access restriction: The method 'MethodUsageClass.m3()' is not API (restriction on required project 'refprojectjava7')

[fedejeanne]

@iloveeclipse are the changes introduced in 1f17fbd72bef3f9d81de7b2353b43671d5048ca6 ok?

[iloveeclipse]

are the changes introduced in 1f17fbd ok?

I have no time for this, sorry. As before: 1) please make sure the launch configs that are created from Package Explorer don't get altered by opening them in the dialog 2) the launch config defaults aren't changed compared to the pre-PR state 3) Try to actually run something with the launch config, both application and tests and check they can be started as before.

[fedejeanne]

@iloveeclipse I re-checked points 1., 2. and 3. and I see no regressions ✔️

Would you retract your request for changes so this PR can be merged as soon as @HannesWell gives the green light? Thank you.

[iloveeclipse]

Note: I just quickly scanned through the PR without looking into the actual fix.

[HannesWell]

Would you retract your request for changes so this PR can be merged as soon as @HannesWell gives the green light? Thank you.

I'll have a look at this tomorrow, I was too busy today with preparing Equinox for Windows on ARM.