Do not enable Link Prototypes if there are no prototypes

[SougandhS]

This PR disables the Link Prototypes context menu in launch configurations when no prototypes are defined for the selected launch type.

Previously, selecting this option displayed an empty list when no prototypes were available.

With this change, the context menu option will only be enabled if at least one prototype is defined for the selected launch type, preventing users from seeing an empty list.

[github-actions]

Test Results

1 320 files   -   657  1 320 suites   - 657   1h 4m 10s ⏱️ - 28m 29s
4 743 tests ±    0  4 718 ✅ +    1   25 💤 + 1  0 ❌  - 2 
9 487 runs   - 4 742  9 379 ✅  - 4 666  108 💤  - 74  0 ❌  - 2 

Results for commit f94a02f. ± Comparison against base commit b7ae22a.

This pull request skips 1 test.

AutomatedResourceTests AllRegressionTests Bug_329836 ‑ testBug(Path)

♻️ This comment has been updated with latest results.

[SougandhS]

Hi @vogella, could you please check this PR when you get some time ?

[SougandhS]

Hi @merks, could you please merge this small change when you get a moment ?

[merks]

I think it's not possible for type == null if launchConfigurationTypes.size()== 1 because you only add non-null types this list.

[SougandhS]

Yes, but compiler was throwing warnings there, so only added the null check

[SougandhS]

To avoid unnecessary iterations for getting the type, I went with this approach

[merks]

Let's get another opinion. @akurtakov @laeubi

[laeubi]

I think the compiler is confused by the hard to understand logic here.

From the description I think you actually not need the set here at all and I would not premature optimize this, we can expect this is not containing millions of elements so splitting up the logic into different steps would help e.g. you can early exit if the selection is empty.

Then the if/else is unnecessarily nested (as you immediately return from the method in the case).

Next your check for null can become

if (type == null || type == launchConfig.getType()) {
  type = launchConfig.getType();
} else {
 return false;
}

(and a core exception should probably be rethrown or also return false..)

[iloveeclipse]

What about this patch (applied on master):

diff --git a/debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LinkPrototypeAction.java b/debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LinkPrototypeAction.java
index e8d2a66..7f1a9d2 100644
--- a/debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LinkPrototypeAction.java
+++ b/debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LinkPrototypeAction.java
@@ -15,5 +15,2 @@
 
-import java.util.Collection;
-import java.util.HashSet;
-
 import org.eclipse.core.runtime.CoreException;
@@ -108,11 +105,12 @@
 		// is(are) selected and the launch configuration type allows prototypes
-		Collection<ILaunchConfigurationType> launchConfigurationTypes = new HashSet<>();
-		for (Object object : selection.toList()) {
-			if (object instanceof ILaunchConfiguration) {
-				if (((ILaunchConfiguration) object).isPrototype()) {
+		for (Object object : selection) {
+			if (object instanceof ILaunchConfiguration config) {
+				if (config.isPrototype()) {
 					return false;
 				} else {
-					ILaunchConfigurationType type = null;
 					try {
-						type = ((ILaunchConfiguration) object).getType();
+						ILaunchConfigurationType type = config.getType();
+						if (type != null) {
+							return type.supportsPrototypes() && type.getPrototypes().length > 0;
+						}
 					} catch (CoreException e) {
@@ -120,7 +118,2 @@
 					}
-					if (type != null) {
-						launchConfigurationTypes.add(type);
-					} else {
-						return false;
-					}
 				}
@@ -130,5 +123,2 @@
 		}
-		if (launchConfigurationTypes.size() == 1) {
-			return launchConfigurationTypes.iterator().next().supportsPrototypes();
-		}
 		return false;

[merks]

There is a comment about all the selection being of the same type so I think this maybe misses that point by returning early. Or?

[SougandhS]

@iloveeclipse with your patch, one of the launch config will be removed if different launch configs are selected accidently by the user - and do a prototype selection

bug.mp4

[SougandhS]

Option should show only if its a same launch config type

[SougandhS]

I have optimised the code bit more

[merks]

I hate to pick things to death, but if this test were done after line 116, you could return early at that point, there would be one less try/catch block, and if you then get to the end of the loop, you can return true (because line 125 will return false when all the types are not the same). Or?

[SougandhS]

I hate to pick things to death, but if this test were done after line 116, you could return early at that point, there would be one less try/catch block

currentType = launchConfig.getType();
return currentType != null && currentType.getPrototypes().length > 0;

I tried this but got the same issue as #2216 (comment) :(

[merks]

The point is to test if currentType.getPrototypes().length == 0 and return false early in that case, not to return true early for any other reason. We of course must get through the whole loop before we can know to return true ever.

[SougandhS]

Thanks, Updated 👍

[merks]

Yes this looks more comprehensible. (I believe this would even perform better than previous "optimized" versions without sacrificing clarity.)