Provide TARGET_SAVED and TARGET_DELETED events #1434

estepper · 2024-10-10T08:12:26Z

When building tools on top of PDE's target platform framework it's often necessary to be informed about state changes in targets. PDE already uses an IEventBroker to send "workspaceTargetChanged" events. This change is about sending two new events:

TARGET_SAVED and TARGET_DELETED

Please note that I also added the method TargetEditor.getAdapter(), which provides callers with the ITargetHandle of the editor.

merks · 2024-10-10T08:22:54Z

/request-license-review

github-actions · 2024-10-10T08:25:35Z

/request-license-review

License review requests:

p2/orbit/p2.eclipse.plugin/org.apache.lucene.analysis-common/9.12.0.v20240929-0900

After all reviews have concluded, re-run the license-vetting check from the Github Actions web-interface to update its status.

Workflow run (with attached summary files):
https://github.com/eclipse-pde/eclipse.pde/actions/runs/11270215782

laeubi · 2024-10-10T08:29:01Z

Is this really required?

Why can't you just listen for the resource changes and see if it is a target file or not?

github-actions · 2024-10-10T08:45:16Z

Test Results

285 files ±0 285 suites ±0 53m 30s ⏱️ + 4m 35s
3 586 tests ±0 3 510 ✅ ±0 76 💤 ±0 0 ❌ ±0
10 950 runs ±0 10 719 ✅ ±0 231 💤 ±0 0 ❌ ±0

Results for commit 50244a3. ± Comparison against base commit 5cac9e2.

♻️ This comment has been updated with latest results.

estepper · 2024-10-10T09:19:50Z

Is this really required?

Yes.

Why can't you just listen for the resource changes and see if it is a target file or not?

Not all target definitions are contained in IFiles. By default, actually, they're contained in normal files hidden somewhere under .metadata/.plugins/o.e.pde.core/

laeubi · 2024-10-10T09:29:18Z

By default, actually, they're contained in normal files hidden somewhere under .metadata/.plugins/o.e.pde.core/

The I'm a bit confused about the use-case, if you want to capture general save (not sure about delete) I would expect only a change in the target editor or handled it in the target handle / target service (so we capture any save) but not on different places.

laeubi · 2024-10-10T09:32:12Z

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/targetdefinition/TargetEditor.java

@@ -184,6 +186,9 @@ public void doSave(IProgressMonitor monitor) {
 		fDirty = false;
 		editorDirtyStateChanged();
 		fInputHandler.setSaving(false);
+
+		TargetPlatformService service = (TargetPlatformService) TargetPlatformService.getDefault();


Why is this needed? Should not saving the target file already trigger an event by default?

Only if it's the active target.

laeubi · 2024-10-10T09:33:16Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java

 			}
 		}
 	}

-	static void notifyTargetChanged(ITargetDefinition target) {
-		IEclipseContext context = EclipseContextFactory.getServiceContext(PDECore.getDefault().getBundleContext());
+	public final void scheduleEvent(String topic, Object data) {


Why not having

notifyTargetChanged

notifyTargetSaved

notifyTargetDeleted

...

Counter question: Why have them?

To hide implementation details on how the actual notifications are performed, what data is included and so on so we later can change it. Beside that it won't require an change of the internal API but just adding new API what is often less disruptive.

In general I understand your point @laeubi, but considering the way the event firing is currently implemented, based on the E4-EventBroker respectively the OSGi EventHandler service, the current state of this PR seems fitting to me.
To implement your suggestion the notification mechanism for target-events would have to be re-implemented entirely and would render the existing TargetEvents API obsolete.

So although type-safety regarding the event-data is not ensured, I think the proposed solution, to extend the list of TargetEvents, is a suitable choice here.

estepper · 2024-10-10T09:49:05Z

The I'm a bit confused about the use-case, if you want to capture general save (not sure about delete) I would expect only a change in the target editor or handled it in the target handle / target service (so we capture any save) but not on different places.

That changes are needed in many different places is caused by PDE's core design. For instance, have a look at how many places are required to maintain the weird TargetPlatformHelper.fgCachedTargetDefinitionMap!

And there are at least three (!) different ways to actually save a target:

estepper · 2024-10-14T07:48:08Z

I know that EclipseCon is coming soon and is likely causing a lot of work, too. But is there a chance to get this in for M2? I have large downstream changes waiting for it in Oomph 😄

HannesWell · 2024-10-14T23:09:46Z

I'll try to have a look at it too, but I first have to finish my preparation for EclipseCon.
So I believe the chances to get this ready before the weekend are not very high. Sorry for that.

I have large downstream changes waiting for it in Oomph 😄

Can't you use temporarily a fixed I-build repository in Oomph once this is done?
E.g. https://download.eclipse.org/eclipse/updates/4.34-I-builds/I20241014-0530/ won't change it's content, it will just vanish after the release.

laeubi · 2024-10-15T07:23:11Z

But is there a chance to get this in for M2? I have large downstream changes waiting for it in Oomph 😄

If its related to Oomph maybe @merks can have a look.

By the way sharing a PR that demonstrate the usage of a new feature might also help to convince maintainers its a good addition :-)

At the moment at least for me it is not completely clear if/how this would be actually used. In the initial text you wrote

When building tools on top of PDE's target platform framework it's often necessary to be informed about state changes in targets.

In general there is only one target platform state and that's why there is an event if it changes the target definition to be used for that, so why is it important to know if a target file (?) was edited that is not active at all?

As mentioned above if it is about when editing a file I would then see this as a matter of the Target editor or whoever changes that target file as such a file can be e.g. edited/deleted in the file system as well.

Also this

Please note that I also added the method TargetEditor.getAdapter(), which provides callers with the ITargetHandle of the editor.

Makes me feel you actually want some specific behavior from the target editor itself but also with this change its a bit unclear, e.g. why one can't use some other means (e.g. using the EditorInput) and how/when this has to be used especially as PDE itself can work without such feature.

estepper · 2024-10-15T11:58:46Z

In the branch "target-monitor" I implemented a new ViewPart that monitors targets and creates content snapshots which can be compared to each other in order to get a clear picture about the impact of target definition changes. You can check it out from https://github.com/eclipse-oomph/oomph/tree/target-monitor . Here's a screenshot:

The new code in TargetEditor.getAdapter() is needed by the "Link With Editor" functionality of the new view.

laeubi · 2024-10-15T12:03:06Z

@estepper is this really Omph specific or do you think it could be useful to implement such view in PDE directly e.g. does it depend on any oomph specific classes/function?

estepper · 2024-10-15T13:22:49Z

It uses a few Oomph-specific classes and a Nebula TableCombo widget. Apart from that I can see that at some point it could be moved to PDE. But especially at this early point I expect that additional changes will be needed to address initial problems and add smaller enhancements. And for those I'm much faster when I have it under my own control.

HannesWell

Hi @estepper, sorry for the delayed reply.

I finally had a look into this PR and think that it's a suitable solution for your problem, although it's not ideal, but that's due to event notification mechanism that's already in use.
Please see my detailed remarks below.

I have a few minor clean-ups that should be applied to this PR, but since I'm currently short in time I'll add the details tomorrow.

HannesWell · 2024-11-08T14:52:06Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java

 			}
 		}
 	}

-	static void notifyTargetChanged(ITargetDefinition target) {
-		IEclipseContext context = EclipseContextFactory.getServiceContext(PDECore.getDefault().getBundleContext());
+	public final void scheduleEvent(String topic, Object data) {


In general I understand your point @laeubi, but considering the way the event firing is currently implemented, based on the E4-EventBroker respectively the OSGi EventHandler service, the current state of this PR seems fitting to me.
To implement your suggestion the notification mechanism for target-events would have to be re-implemented entirely and would render the existing TargetEvents API obsolete.

So although type-safety regarding the event-data is not ensured, I think the proposed solution, to extend the list of TargetEvents, is a suitable choice here.

ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/targetdefinition/TargetEditor.java

HannesWell

With #1466 I have made a few simplifications in the TargetPlatformService class that also simplify this PR. I have rebased this and added a second commit with suggestions for improvements, a few clean-ups and unification of the doc so that there is less duplication.
@estepper if there's no objection from your side I'll squash the suggestions into one commit and submit it this evening.

HannesWell · 2024-11-09T14:39:43Z

In the branch "target-monitor" I implemented a new ViewPart that monitors targets and creates content snapshots which can be compared to each other in order to get a clear picture about the impact of target definition changes. You can check it out from https://github.com/eclipse-oomph/oomph/tree/target-monitor . Here's a screenshot:

That's very interesting.

It uses a few Oomph-specific classes and a Nebula TableCombo widget. Apart from that I can see that at some point it could be moved to PDE. But especially at this early point I expect that additional changes will be needed to address initial problems and add smaller enhancements. And for those I'm much faster when I have it under my own control.

Oomph has multiple quite convenient tools that would (in same cases maybe a bit generalized) also fit very well into PDE. So if you are interested in moving them into the Eclipse-SDK I would be glad to discuss that.

When building tools on top of PDE's target platform framework it's often necessary to be informed about state changes in targets. PDE already uses an IEventBroker to send "workspaceTargetChanged" events. This change is about sending two new events: TARGET_SAVED and TARGET_DELETED

HannesWell · 2024-11-10T14:56:34Z

The comparator differences are unrelated (see #1467), submitting.

Thanks again Eike for this contribution.

laeubi reviewed Oct 10, 2024

View reviewed changes

HannesWell reviewed Nov 8, 2024

View reviewed changes

HannesWell force-pushed the master branch 2 times, most recently from 5856baf to 71139e3 Compare November 9, 2024 14:30

HannesWell approved these changes Nov 9, 2024

View reviewed changes

HannesWell force-pushed the master branch 2 times, most recently from c5b22c3 to a3ea2b4 Compare November 10, 2024 12:22

HannesWell force-pushed the master branch from a3ea2b4 to 50244a3 Compare November 10, 2024 14:15

HannesWell merged commit 428cf43 into eclipse-pde:master Nov 10, 2024
14 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide TARGET_SAVED and TARGET_DELETED events #1434

Provide TARGET_SAVED and TARGET_DELETED events #1434

estepper commented Oct 10, 2024

merks commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

laeubi commented Oct 10, 2024

github-actions bot commented Oct 10, 2024 •

edited

Loading

estepper commented Oct 10, 2024

laeubi commented Oct 10, 2024

laeubi Oct 10, 2024

estepper Oct 10, 2024

laeubi Oct 10, 2024

estepper Oct 10, 2024

laeubi Oct 10, 2024

HannesWell Nov 8, 2024

estepper commented Oct 10, 2024 •

edited

Loading

estepper commented Oct 14, 2024

HannesWell commented Oct 14, 2024

laeubi commented Oct 15, 2024

estepper commented Oct 15, 2024

laeubi commented Oct 15, 2024

estepper commented Oct 15, 2024

HannesWell left a comment

HannesWell Nov 8, 2024

HannesWell left a comment

HannesWell commented Nov 9, 2024

HannesWell commented Nov 10, 2024

Provide TARGET_SAVED and TARGET_DELETED events #1434

Provide TARGET_SAVED and TARGET_DELETED events #1434

Conversation

estepper commented Oct 10, 2024

merks commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

laeubi commented Oct 10, 2024

github-actions bot commented Oct 10, 2024 • edited Loading

Test Results

estepper commented Oct 10, 2024

laeubi commented Oct 10, 2024

laeubi Oct 10, 2024

Choose a reason for hiding this comment

estepper Oct 10, 2024

Choose a reason for hiding this comment

laeubi Oct 10, 2024

Choose a reason for hiding this comment

estepper Oct 10, 2024

Choose a reason for hiding this comment

laeubi Oct 10, 2024

Choose a reason for hiding this comment

HannesWell Nov 8, 2024

Choose a reason for hiding this comment

estepper commented Oct 10, 2024 • edited Loading

estepper commented Oct 14, 2024

HannesWell commented Oct 14, 2024

laeubi commented Oct 15, 2024

estepper commented Oct 15, 2024

laeubi commented Oct 15, 2024

estepper commented Oct 15, 2024

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Nov 8, 2024

Choose a reason for hiding this comment

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell commented Nov 9, 2024

HannesWell commented Nov 10, 2024

github-actions bot commented Oct 10, 2024 •

edited

Loading

estepper commented Oct 10, 2024 •

edited

Loading