Impl #2402 - Migrate model processors from extension point to OSGi DS

[fipro78]

No description provided.

[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.workbench.addons.swt/META-INF/MANIFEST.MF
tests/org.eclipse.ui.tests.pluginchecks/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 039eb043333479df6aab73e5fed65b5972096304 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <platform-bot@eclipse.org>
Date: Thu, 7 Nov 2024 14:34:02 +0000
Subject: [PATCH] Version bump(s) for 4.34 stream


diff --git a/bundles/org.eclipse.e4.ui.workbench.addons.swt/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.workbench.addons.swt/META-INF/MANIFEST.MF
index 96bf781889..210347e687 100644
--- a/bundles/org.eclipse.e4.ui.workbench.addons.swt/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.ui.workbench.addons.swt/META-INF/MANIFEST.MF
@@ -1,7 +1,7 @@
 Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-SymbolicName: org.eclipse.e4.ui.workbench.addons.swt;singleton:=true
-Bundle-Version: 1.5.500.qualifier
+Bundle-Version: 1.5.600.qualifier
 Bundle-Name: %pluginName
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
diff --git a/tests/org.eclipse.ui.tests.pluginchecks/META-INF/MANIFEST.MF b/tests/org.eclipse.ui.tests.pluginchecks/META-INF/MANIFEST.MF
index 55370d8626..10c5422f8e 100644
--- a/tests/org.eclipse.ui.tests.pluginchecks/META-INF/MANIFEST.MF
+++ b/tests/org.eclipse.ui.tests.pluginchecks/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: Pluginchecks
 Bundle-SymbolicName: org.eclipse.ui.tests.pluginchecks;singleton:=true
-Bundle-Version: 1.2.300.qualifier
+Bundle-Version: 1.2.400.qualifier
 Automatic-Module-Name: org.eclipse.ui.tests.pluginchecks
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Require-Bundle: org.junit;bundle-version="4.12.0",
-- 
2.47.0

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

[github-actions]

Test Results

 1 821 files  ±0   1 821 suites  ±0   2h 5m 7s ⏱️ + 6m 38s
 7 725 tests ±0   7 496 ✅ ±0  228 💤 ±0  1 ❌ ±0 
24 336 runs  ±0  23 588 ✅ ±0  747 💤 ±0  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit e4152aa. ± Comparison against base commit 908db0f.

♻️ This comment has been updated with latest results.

[HannesWell]

Thanks for this one the change itself looks sound, but if I start an SDK from my workspace with it the start-up fails:

org.eclipse.e4.core.di.InjectionException: Unable to process "BindingService.manager": no actual value was found for the argument "BindingManager".
	at org.eclipse.e4.core.internal.di.InjectorImpl.reportUnresolvedArgument(InjectorImpl.java:462)
	at org.eclipse.e4.core.internal.di.InjectorImpl.resolveRequestorArgs(InjectorImpl.java:453)
	at org.eclipse.e4.core.internal.di.InjectorImpl.internalInject(InjectorImpl.java:128)
	at org.eclipse.e4.core.internal.di.InjectorImpl.internalMake(InjectorImpl.java:386)
	at org.eclipse.e4.core.internal.di.InjectorImpl.make(InjectorImpl.java:312)
	at org.eclipse.e4.core.contexts.ContextInjectionFactory.make(ContextInjectionFactory.java:203)
	at org.eclipse.ui.internal.Workbench$31.runWithException(Workbench.java:2337)
	at org.eclipse.ui.internal.StartupThreading$StartupRunnable.run(StartupThreading.java:35)
	at org.eclipse.swt.widgets.Synchronizer.syncExec(Synchronizer.java:183)
	at org.eclipse.ui.internal.UISynchronizer.syncExec(UISynchronizer.java:133)
	at org.eclipse.swt.widgets.Display.syncExec(Display.java:4883)
	at org.eclipse.ui.internal.StartupThreading.runWithoutExceptions(StartupThreading.java:93)
	at org.eclipse.ui.internal.Workbench.initializeDefaultServices(Workbench.java:2331)
	at org.eclipse.ui.internal.Workbench.init(Workbench.java:1714)
	at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2825)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:632)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:546)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:175)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:662)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:598)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1441)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1414)

My first guess is that ModelAssembler somehow registers contributions through OSGi services differently. I already tried to process services before extensions, but that didn't help. I can further look into this, but probably not before next week-end.

[HannesWell]

And regarding your remark in #2396 (comment)

And btw the service attribute on the @component in your snippet is not necessary, as it is derived from the implements IModelProcessorContribution. So actually the information is redundant. Just in case you wonder why I have not used it in my PR.

That's right, but using it explicitly was a suggestion from @laeubi to ensure that services don't get implemented 'accidentally' if for example later another interface is implemented that's not intended as service. And I think that's reasonable.

[laeubi]

That's right, but using it explicitly was a suggestion from @laeubi to ensure that services don't get implemented 'accidentally' if for example later another interface is implemented that's not intended as service. And I think that's reasonable.

Its actually a bit different and why I think actually specify it always is good on the long term:

1. Assume you implement no interface then your component gets immediately activated
2. Later you let it implement an interface then it is no longer getting activated unless the interface is actually fetched as a service (+ it is registered as such service)

Other case:

1. Assume you implement two interface then your component gets registered as that service
2. Later you let it derive from a base class and remove that interface then it is no longer registered as such service because only direct interfaces are considered, or you choose to implement an extension of such interface then it becomes suddenly a different service.

So actually if one relies on the default, it means one must check each time something is refactored there, because of this I simply always add it explicitly so its always clear and less prone to accidental changes. e.g. PDE also gives an error if the component does not implement the declared service (anymore).

[fipro78]

For the service attribute discussion, well from the maintenance point of view I understand this. Fine for me. And actually necessary to fix the issue.

About the issue, well we now have a startup order issue. The BindingToModelProcessor requires the ContextToModelProcessor and the CommandToModelProcessor the be executed before. Otherwise the necessary instances in the context are not yet available.

I see the following two errors in the logs:

!ENTRY org.eclipse.ui 4 4 2024-10-21 06:45:41.953
!MESSAGE Command manager was null in org.eclipse.ui.internal.BindingToModelProcessor

!ENTRY org.eclipse.ui 4 4 2024-10-21 06:45:41.987
!MESSAGE Context manager was null in org.eclipse.ui.internal.BindingToModelProcessor

I have now added a Condition service to describe the activation order dependency. With the extension points this was done by registering the extensions in the correct oder in the plugin.xml.

[HannesWell]

Thank you for the update and sorry for the late reply.
The change looks good and worked well in my testing.

But please squash your three commits into one with a commit message covering the resulting change, there is no need to effectively capture the review discussion in the git-history (you remove the version bump and let the bot add it again).
Then this is ready from my side.

[HannesWell]

Thank you for the updates and this change in general.
The code looks good, now only the build has to succeed.

[HannesWell]

Test failure it's unrelated, submitting.

Thanks again!