[E4 XPath] Add new API in EModelService to find elements via Xpath and deprecate E4-XPath bundle for removal

[HannesWell]

As found in #2290, these methods seem not to provide any real value and just complicate the implementation.

I'm contemplating to even remove XPathContext.getValue() as it also complicates the implementation and has no real usage in the SimRel:

eclipse.platform.ui/bundles/org.eclipse.e4.emf.xpath/src/org/eclipse/e4/emf/xpath/XPathContext.java

Lines 39 to 51 in c9f129d

	Object getValue(String xpath);
	/**
	* Evaluates the xpath, converts the result to the specified class and
	* returns the resulting object.
	*
	* @param xpath
	* to evaluate
	* @param requiredType
	* required type
	* @return Object found
	*/
	<T> T getValue(String xpath, Class<T> requiredType);

[github-actions]

Test Results

 1 818 files  ±0   1 818 suites  ±0   1h 28m 47s ⏱️ - 8m 32s
 7 721 tests ±0   7 493 ✅ +1  228 💤 ±0  0 ❌  - 1 
24 324 runs  ±0  23 575 ✅ +1  749 💤 ±0  0 ❌  - 1 

Results for commit b20f60c. ± Comparison against base commit 4e127ef.

♻️ This comment has been updated with latest results.

[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:

bundles/org.eclipse.e4.ui.model.workbench/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 fade84ed63fba949c9d34c6cf4f0581cf233c381 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Sat, 15 Feb 2025 18:46:02 +0000
Subject: [PATCH] Version bump(s) for 4.35 stream


diff --git a/bundles/org.eclipse.e4.ui.model.workbench/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.model.workbench/META-INF/MANIFEST.MF
index c3a661b45c..5a27e839f9 100644
--- a/bundles/org.eclipse.e4.ui.model.workbench/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.ui.model.workbench/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.e4.ui.model.workbench;singleton:=true
-Bundle-Version: 2.4.400.qualifier
+Bundle-Version: 2.4.500.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Bundle-RequiredExecutionEnvironment: JavaSE-17
-- 
2.48.1

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

[HannesWell]

@ptziegler as you contributed test-cases and worked on the parent-context handling, I wonder if you are actually using it and therefore have objections against deprecating XPathContextFactory.newContext(XPathContext parentContext, T contextBean).

The same question applies to XPathContext.getValue() to you ore anybody else?

[laeubi]

I know it is an "API break" but should we not mark XPathContextFactory with @NoExtend?

[HannesWell]

I know it is an "API break" but should we not mark XPathContextFactory with @NoExtend?

I ask this question to myself as well. On one hand the Eclipse SDK only provide one specific implementation of that class that can only handle a specific type of model, while the interface is designed (maybe over-engineered) generic. So there could be a demand to have other implementations that can handle other type of models.
On the other hand, there is no benefit from doing that as there is nothing that would work with such other specialization in the SDK and third-parties that would want to do that, could just use their own classes and abstraction.

The same question also applies to XPathContext. In the end I'm not sure about it as this entire API seems to be over-engineered to me for the use-cases we have in the SDK. So a lot could be reduced just from POV of the Eclipse SDK.

[laeubi]

I think the only interesting use-case would be to query the Model programmatically with an xpath expression, maybe it was though of a way to be used with other EMF model, but tin this case it would have ben better be implemented/provided by emf itself.

For this purpose, I would rather want to have a method in org.eclipse.e4.ui.workbench.modeling.EModelService similar to the findElements or an XPathSelector that implements org.eclipse.e4.ui.workbench.Selector.

As all of this basically boils down to org.eclipse.e4.ui.model.workbench one could even thing about deprecate the whole bundle (PDE now supports a warning for users), and with the next release copy the now much simpler implementation into org.eclipse.e4.ui.model.workbench... then we won't need any "public" API anymore and if there is really users they can still use the "old" impl and platform can just not include the emf.xpath anymore.

[HannesWell]

I think the only interesting use-case would be to query the Model programmatically with an xpath expression, maybe it was though of a way to be used with other EMF model, but tin this case it would have ben better be implemented/provided by emf itself.

There is also one other user in PDE, see e.g.

• Clean-up and simplify usage of E4's XPathContext in E4-tools eclipse-pde/eclipse.pde#1612

For this purpose, I would rather want to have a method in org.eclipse.e4.ui.workbench.modeling.EModelService similar to the findElements or an XPathSelector that implements org.eclipse.e4.ui.workbench.Selector.

Adding something like EModelService.findMatchingElements(MApplicationElement contextBean, String xpath, Class<T> clazz) sounds like a good idea. It would boil down the actually used API to the minimum and would make it more accessible.
I added this in a very first prototype.
However using the new method in the StringModelFragmentImpl class to replace the reference to the existing XPathContext, is not possible as o.e.e4.ui.model.workbench doesn't depend on o.e.e4.ui.workbench but the other way round.

As all of this basically boils down to org.eclipse.e4.ui.model.workbench one could even thing about deprecate the whole bundle (PDE now supports a warning for users), and with the next release copy the now much simpler implementation into org.eclipse.e4.ui.model.workbench... then we won't need any "public" API anymore and if there is really users they can still use the "old" impl and platform can just not include the emf.xpath anymore.

If we make the approach from above work, then we could deprecate the entire bundle and inline it's content to wherever the new API lands when the bundle is finally removed.

[HannesWell]

However using the new method in the StringModelFragmentImpl class to replace the reference to the existing XPathContext, is not possible as o.e.e4.ui.model.workbench doesn't depend on o.e.e4.ui.workbench but the other way round.

We could also add it as something internal in o.e.e4.ui.model.workbench use it there and make it public only through EModelService.

[HannesWell]

Adding something like EModelService.findMatchingElements(MApplicationElement contextBean, String xpath, Class<T> clazz) sounds like a good idea. It would boil down the actually used API to the minimum and would make it more accessible.

This also allows us to avoid the clean-up of the parameteriziation problem of XPathContextFactory.newInstance().

[HannesWell]

I have looked at the usage in PDE of the new API and in general it looks good, the only thing that makes using this cumbersome is the fact that the new API is an instance method and not a class/static method and one needs an EModelService instance. This makes it cumbersome to use in static methods (which is the case in PDE).

[laeubi]

I'm not sure I found the right place but the only one I can found in PDE is

org.eclipse.e4.tools.emf.ui.internal.common.component.StringModelFragment.getTargetClassFromXPath(MApplication, String)

As it has the MApplication you can do:

EModelService modelService = application.getContext().get(EModelService.class);

if you want avoid an additional parameter.

[HannesWell]

I have now added the missing documentation and think this is ready for submission.
I would like to land this for the upcoming 2025-03 release and think it's save to do this as this just deprecate methods and a bundle and adds one alternative API.

@laeubi are you fine with this now? @merks do you also want to have a look?

As it has the MApplication you can do:

EModelService modelService = application.getContext().get(EModelService.class);

if you want avoid an additional parameter.

I tried this but it looks like it doesn't work in all cases, because (at least in some cases) that actually an MApplication loaded from a resource/file of an Editor and not the 'active' application model of the running application. And in that case the context is not set to these model-elements and getContext() therefore returns null.

So for PDE I either need to get the context purely static (not sure if that's possible) or pass an EModelService instance as parameter.

[laeubi]

So for PDE I either need to get the context purely static

Du to the nature of E4 this is not reliable possible but I think the case of an editor is rather special.

But I think we have some cases:

1. PDE can simply "fake" it by setting its own EclipseContext to the loaded model
2. PDE can call an (internal) API on e4 for this purpose and become a friend of the package.

[laeubi]

Can the method that is currently at XPathHelper maybe betterl ife in ModelServiceImpl or StringModelFragmentImpl?

[laeubi]

I don't think we need any deprecation here if we deprecate the bundle

[HannesWell]

I removed the additional deprecation of some methods, but the explicit deprecation of the classes should be kept as one otherwise doesn't get a corresponding warning in the code. And from my experience warnings in Manifests are overlooked more easily than in code.
Furthermore the deprecation javadoc contains additional information.

[laeubi]

but the explicit deprecation of the classes should be kept as one otherwise doesn't get a corresponding warning in the code.

You should already get a warning in the Manifest about the deprecation.

[HannesWell]

Can the method that is currently at XPathHelper maybe betterl ife in ModelServiceImpl or StringModelFragmentImpl?

Moving it to ModelServiceImpl is not feasible due to dependency structure and StringModelFragmentImpl would be a class were I would expect such functionality.
And as the comment in XPathHelper suggests, this class is intended to contain all the (then used) code that's currently in JavaXPathContextFactoryImpl when the org.eclipse.e4.emf.xpath bundle is finally removed. So it won't be that empty as it's now on the long run. With that in mind, having a separate class for it seems suitable to me (and as it's all internal it can be changed easily in the future if necessary).

[HannesWell]

@laeubi I would like to submit this PR this evening so that I can apply the new API/methods in PDE tomorrow after the next I-build.
I think the most critical part is the new API method in EModelService:
<T> Stream<T> findMatchingElements(MApplicationElement searchRoot, String xPath, Class<T> clazz);

But I think that's fine. Everything else is internal and can be changed at any time later, if necessary. Do you have any critical points that should definitively be addressed?

[laeubi]

Then I won't call it XPathHelper .. there are way to much "helper", "util" and a like and they tend to grow over time with no real scope. What about name it ModelXPathSupport or similar then? Beside that I think it is fine and goes into the right direction.

e.g. we already have three P2Utils (and two of them in PDE):

[HannesWell]

Then I won't call it XPathHelper .. there are way to much "helper", "util" and a like and they tend to grow over time with no real scope. What about name it ModelXPathSupport or similar then? Beside that I think it is fine and goes into the right direction.

Ok, as discussed in a private conversion I have renamed it to ModelXPathEvaluator.

[laeubi]

Thanks for working on this!
New EModelService method should maybe mention in N&N

[HannesWell]

You are welcome. I'm happy if that whole XPath topic is finally reduced to the actually used minimum.

New EModelService method should maybe mention in N&N

Yes. Together with the replacement of JXPath. That might be interesting for others as well.

[HannesWell]

New EModelService method should maybe mention in N&N

Yes. Together with the replacement of JXPath. That might be interesting for others as well.

Done via eclipse-platform/www.eclipse.org-eclipse#276.