[#1668] API types to simplify work with launch configuration attributes

[ruspl-afed]

• identify launch attribute
• connect it with preference metadata (to supply defaults/label/description)
• read attribute from configuration
• write attribute to configuration working copy
• demonstrate usage for ExternalTools

[eclipse-platform-bot]

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From eaa3ac17f344b48134ddc5cebf426b2d852af7f2 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Thu, 30 Jan 2025 10:48:00 +0000
Subject: [PATCH] Version bump(s) for 4.35 stream


diff --git a/debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF b/debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF
index 169587e22e..2be9d48ae2 100644
--- a/debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF
+++ b/debug/org.eclipse.core.externaltools/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@ Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-Localization: plugin
 Bundle-SymbolicName: org.eclipse.core.externaltools;singleton:=true
-Bundle-Version: 1.3.400.qualifier
+Bundle-Version: 1.3.500.qualifier
 Require-Bundle: org.eclipse.core.runtime;bundle-version="[3.29.0,4.0.0)",
  org.eclipse.debug.core;bundle-version="[3.9.0,4.0.0)",
  org.eclipse.core.variables;bundle-version="[3.2.800,4.0.0)"
-- 
2.48.1

Further information are available in Common Build Issues - Missing version increments.

[github-actions]

Test Results

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 502eedc. ± Comparison against base commit 083f7af.

♻️ This comment has been updated with latest results.

[mickaelistria]

As mentioned in the call, some example using this API proposal would be helpful. There is probably some content in org.eclipse.ui.externaltools/External Tools Base/org/eclipse/ui/externaltools/internal/launchConfigurations you can tweak to leverage this API.

[ruspl-afed]

I'm not sure why it is is failing @HannesWell

11:59:41.178 [ERROR] Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.11-SNAPSHOT:verify (verify) on project org.eclipse.debug.core: 
Execute ApiApplication failed: InvocationTargetException: EmptyStackException -> [Help 1]

[HannesWell]

I'm not sure why it is is failing @HannesWell

11:59:41.178 [ERROR] Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.11-SNAPSHOT:verify (verify) on project org.eclipse.debug.core: 
Execute ApiApplication failed: InvocationTargetException: EmptyStackException -> [Help 1]

It looks like your change is breaking API-tools^^
I have replayed the Jenkins build the -e set to get an more informative stack-trace:
https://ci.eclipse.org/platform/job/eclipse.platform/job/PR-1669/7/

[ruspl-afed]

Thank you @HannesWell for your assistance.

It looks like your change is breaking API-tools^^

WOW! But that wasn't part of my plan. It looks like this issue was already reported. Interesting, may be I can have a deeper look to it soon.

And still interested in your feedback regarding PR content.

[ruspl-afed]

After supporting record for API Tools everything is green @HannesWell

[ruspl-afed]

ok, a few more days for discussion and I'm going to merge it

[HannesWell]

After supporting record for API Tools everything is green @HannesWell

Thank you for that fix/enhancement of API-tools! It's much appreciated.

And still interested in your feedback regarding PR content.

Sorry for not responding. It has been some busy days recently. I have not forgotten this, just didn't have the time for it.
I try my best to have look at it in the next days.

[HannesWell]

I had a more detailed look at it and understand the goal and see the benefits from this: Better type-safety and more convenient read and writes.
Nevertheless I'm not sure that these benefits really carry the weight of the change, because currently the new API surface is very big.
Below I made some suggestion to make the API more compact, maybe we can then achieve a sufficiently got relation of advantages to API surface.

With these suggestions we would end up with just the LaunchAttributeIdentity and LaunchAttributeDefined types. Maybe we could even 'inline' the former into the latter and would then still get benefits mentioned above.
And eventually just having an ILaunchAttribute interface added with a few static factories to create them is something that could indeed be a in my opinion suitable addition.

[HannesWell]

Couldn't we have this if-chain in the constructor and then encapsulate the action into a lambda and just execute it here?

[ruspl-afed]

not so sure

[ruspl-afed]

Thank you for your feedback @HannesWell

PR was reimplemented according to your comments.
I'm not sure why we are fighting for reducing number of API types with adding more API methods and more implementation code the the interface compilation unit, but the result looks much more closer to the old good Eclipse 2.0 style of Debug subsystem.

[HannesWell]

I'm not sure why we are fighting for reducing number of API types with adding more API methods and more implementation code the the interface compilation unit, but the result looks much more closer to the old good Eclipse 2.0 style of Debug subsystem.

I don't consider it as a fight, more as a search for (ideally) the best solution. Of course what is best is also a matter of opinion, but at least my personal opinion is that in general I like more compact APIs better. However can you please explain to me what the advantage is of defining in types what could be methods respectively why exactly this is Eclipse-2.0 style and why it's bad (I haven't been around back then)?
With the suggestions made we could still define typed launch-attributes using LaunchAttribute(-Defined/Definition) as static constant and read/write with it from/to a LaunchConfiguration in a more convenient way, couldn't we?

But of course others can have other opinions about it and I invite everyone to share theirs.

[ruspl-afed]

You are right @HannesWell , it's definitely a matter of personal opinion. For me the old good Eclipse 2.0 style means a lot of static definitions, favoring of primitive types in API and a lot of setters to reuse one instance as long as one can. All this was actual 25 years ago for ancient JVMs. Nowadays we have modern JVM and very dynamic environment where non-replaceable static definition can cause problems and saving memory due to mutability of objects does not justify the cost of (much more complex) code support.

And even if I've stopped naming interfaces with "I" for new code and prefer simple immutable objects with very simple interfaces, I've done a lot following your comments to make this API improvement better suited to the surrounding code.

Now the question is: what else should be improved in this PR? I still can see your "change requested" veto but your latest comment doesn't content any direct change requests. Please clarify.

[ruspl-afed]

Hello @HannesWell
What else do you think should be changed in this PR?

[HannesWell]

You are right @HannesWell , it's definitely a matter of personal opinion. For me the old good Eclipse 2.0 style means a lot of static definitions, favoring of primitive types in API and a lot of setters to reuse one instance as long as one can. All this was actual 25 years ago for ancient JVMs. Nowadays we have modern JVM and very dynamic environment where non-replaceable static definition can cause problems and saving memory due to mutability of objects does not justify the cost of (much more complex) code support.

My concern is not about a few objects more or less, I look at these new types from a consumer-perspective and how difficult a new API is to understand and learn. And there my experience/opinion is that 'distributed' types are more difficult and having it compact makes the capabilities easier to discover (as an IMO bad example: try to configure a JFace resource manager properly to be disposed with a Control and a child of the global registry).
As far as I know the problematic part of the Eclipse 2.0 style you referring to is that there tend to be only one large class that provided almost everything as static methods that don't allow dynamics, e.g. the Platform class in eclipse.runtime.
But at least from my perspective we are here talking about static factories on types (classes or interfaces), which I don't consider old-style. In fact I have the impression that's often the preferred way over constructors and many are added in Eclipse. From the top of my head I remember e.g. ILog or IPath or in the JDK there is Path.of() or the brand-new Classfile-API also makes heavy use of static factory methods AFAIK.

And even if I've stopped naming interfaces with "I" for new code and prefer simple immutable objects with very simple interfaces

Personally I'm also not fully convinced that it's best to prefix all interfaces I. But in the end Eclipse is an existing code-base and when adding elements considering how new code fits into the existing code-base is IMO also an item to consider. Of course that doesn't make it simpler. Immutability is also something I think has many advantages and I try to achieve. For simple interfaces on the other hand, I would say it depends and that's IMO where the work starts and were the quality of an API is measured, if the 'right' size is found.

Now the question is: what else should be improved in this PR? I still can see your "change requested" veto but your latest comment doesn't content any direct change requests. Please clarify.

Sorry, I simply oversaw that you made changes. I was very busy of getting rid of the JXPath dependency in platform.ui.
I have now made another pass, please see the comments below.

[HannesWell]

We should think about if we want users to extend and/or implement this interface or not and should add the corresponding API-tools annotations.
If we want to have it as no-extend and no-implement, it could also be a final class instead of an interface.

[ruspl-afed]

I don't think we should "close" this type for alternative implementation, for example current default implementation considers variable substitution for strings, while for some cases it may be undesirable or more complex for some reason.
Ability to another implementation (with another constructor) will also allow to add more custom constructors where needed instead of adding more static factories to the common interface.

[HannesWell]

I'm not sure if this identity type carries it's weight.
I understand your intended to have a specific type for it, similarly modelling file-system path with Path/File vs. a String, but since the use-case I see currently (I might be missing something) is to feed the LaunchAttribute type, and I hope that we can use that as static final constants to represent launch-attributes as APIs I wonder if we really need this separation, too?

[ruspl-afed]

This is funny. The introduction of "typed" identifier like ILaunchAttributeIdentity was the main goal of this change. The idea is to reduce programming errors when we have a lot of launch-related String constants without good way to distinguish them according to its type.

If you think that the ability to check at compile time is not worth introducing a new type, I'm not sure how to argue with that.

And you are right, I need to demonstrate the usage of ILaunchAttributeIdentity better.