Skip the method PluginsTab::performApply until after activation #1252
+86
−29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@iloveeclipse this is my proposed fix, could you please also test it? |
fedejeanne
force-pushed
the
bugs/1250_default_config_in_PluginsTab_is_wrong
branch
from
097e35b to
d9768bf
Compare
Test Results 285 files ±0 285 suites ±0 48m 58s ⏱️ - 6m 10s Results for commit 1cdde48. ± Comparison against base commit 33efe8c. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
iloveeclipse
requested changes
Apr 24, 2024
There was a problem hiding this comment.
Created new test in a plugin with wizard
Executed plugin test for first time from package explorer (without any dialog)
Opened launch config after that
Clicked on every tab except plugins - "Apply" is disabled
Clicked on Plugins tab - "Apply" is enabled.
I see that this attribute is added:
<booleanAttribute key="show_selected_only" value="false"/>
It doesn't happen with #1251
|
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"? |
|
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 |
|
@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 #1252 (review) |
|
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. |
|
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). |
|
@iloveeclipse I was able to reproduce the error and I found the culprit: some attributes like @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 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? |
Yes. It would be also better to have one commit that fixes original issue as multiple. I will merge the revert now. |
|
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
force-pushed
the
bugs/1250_default_config_in_PluginsTab_is_wrong
branch
from
d9768bf to
1a7c452
Compare
|
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 /**
* 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
The reason I did this is because creating the run configurations (instances of I also double checked that the Default Start Level and Default Auto-Start still have their appropriate default values of 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 If this PR is then OK for you then I can squash the commits. |
|
The failing test seems unrelated: |
fedejeanne
force-pushed
the
bugs/1250_default_config_in_PluginsTab_is_wrong
branch
from
1a7c452 to
1f17fbd
Compare
|
@iloveeclipse are the changes introduced in 1f17fbd ok? |
I have no time for this, sorry.
|
fedejeanne
force-pushed
the
bugs/1250_default_config_in_PluginsTab_is_wrong
branch
from
1f17fbd to
791518f
Compare
|
@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
previously requested changes
May 6, 2024
ui/org.eclipse.pde.ui/src/org/eclipse/pde/ui/launcher/PluginsTab.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.ui/src/org/eclipse/pde/ui/launcher/PluginsTab.java
Outdated
Show resolved
Hide resolved
|
Note: I just quickly scanned through the PR without looking into the actual fix. |
fedejeanne
force-pushed
the
bugs/1250_default_config_in_PluginsTab_is_wrong
branch
from
791518f to
dc7ca51
Compare
I'll have a look at this tomorrow, I was too busy today with preparing Equinox for Windows on ARM. |
fedejeanne
force-pushed
the
bugs/1250_default_config_in_PluginsTab_is_wrong
branch
from
3a6e1c2 to
9b4712f
Compare
|
The build failures seem unrelated but I'm not sure what they mean. LinuxMac (first one)Mac (second one)@HannesWell any ideas? |
|
Linux is #1360, but I think I found a fix. But I have no idea for Mac, maybe a hick-up? |
HannesWell
force-pushed
the
bugs/1250_default_config_in_PluginsTab_is_wrong
branch
from
9b4712f to
82d234f
Compare
fedejeanne
force-pushed
the
bugs/1250_default_config_in_PluginsTab_is_wrong
branch
from
82d234f to
69cc890
Compare
If this method (or part of it) is run before the tab has been activated then the default configuration passed as parameter will be modified and incorrect values will be introduced. Remove attributes in PDE launch configurations instead of explicitly setting them to their default values The attributes in question are: - DESELECTED_WORKSPACE_BUNDLES --> [] - USE_CUSTOM_FEATURES --> false - SHOW_SELECTED_ONLY --> false - INCLUDE_OPTIONAL --> true - AUTOMATIC_ADD --> true The reason to do this is that 2 launch configurations are considered different if one of them explicitly sets an attribute to its default value (because it then has 1 more attribute) This commit fixes a regression introduced in 98a5865 Fixes eclipse-pde#1250
fedejeanne
force-pushed
the
bugs/1250_default_config_in_PluginsTab_is_wrong
branch
from
69cc890 to
1cdde48
Compare
|
@HannesWell any chance of getting this one merged for M1? :-) |
I'll try to have a look at it but I'm currently focusing on the topics I want to present at EclipseCon/OCX, so I can't make any promises. But it's not forgotten. :) |
|
@HannesWell FTR the conflict in this PR is probably the file |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What it does
Reintroduces the improvements of #1233 but without the regressions that it generated (see #1250) by skipping the whole method
PluginsTab::performApplybefore the Plugins tab was activated. The reason for this is that this method changes some values in theconfigurationit 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.Reintroduces #1233
While also fixing #1250
Bug description
(Regression introduced by #1233 and reported in #1250)
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 #1250 (comment))
How to reproduce bug #1250
4) and Default Auto-Start (false) will be replaced by1andtrue, like this:After applying this PR
4andfalseagainDISCLAIMER: the screenshots are from this #1250 (comment)
How to test