From 9b4df2143f5b9a93bc1ab098d307484ddfa6305b Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Thu, 6 Aug 2026 15:50:57 +0200 Subject: [PATCH] Refresh dependency error decorations on target platform changes The error decorations on the Dependencies tab of the manifest editor are computed on the fly from isResolved(), so they only stay correct if the result is recomputed and if the sections learn that the world changed. Two things prevented that. PluginReference memoized the plug-in it looked up and never invalidated it, so a required bundle that disappeared from the target kept reporting itself as resolved and no error decoration ever appeared. Only an explicitly assigned plug-in is cached now; the id is looked up on every call, which is a hash lookup on the model entry table. RequiresSection listened to plug-in model deltas only, and ImportPackageSection listened to nothing beyond its own bundle model. A target reload fires neither, so both kept painting the previous resolution result until the editor was reopened. Both now also listen to IStateDeltaListener.stateChanged, which is what a reload does fire, and ImportPackageSection gained the plug-in model listener it was missing for ordinary workspace changes. --- .../internal/core/plugin/PluginReference.java | 11 +- .../core/tests/internal/AllPDECoreTests.java | 1 + .../StaleDependencyResolutionTest.java | 174 ++++++++++++++++++ .../editor/plugin/ImportPackageSection.java | 50 ++++- .../ui/editor/plugin/RequiresSection.java | 23 ++- 5 files changed, 252 insertions(+), 7 deletions(-) create mode 100644 ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/StaleDependencyResolutionTest.java diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginReference.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginReference.java index 34d440d469b..525eb3bf143 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginReference.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginReference.java @@ -41,11 +41,14 @@ public String getId() { } public IPlugin getPlugin() { - if (fPlugin == null && fId != null) { - IPluginModelBase model = findModel(); - fPlugin = model instanceof IPluginModel i ? i.getPlugin() : null; + // Only an explicitly assigned plug-in is cached. Looking the id up on + // every call keeps isResolved() in sync with the current target + // platform, which the error decorations in the editor rely on. + if (fPlugin != null || fId == null) { + return fPlugin; } - return fPlugin; + IPluginModelBase model = findModel(); + return model instanceof IPluginModel i ? i.getPlugin() : null; } protected IPluginModelBase findModel() { diff --git a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java index 17129fd3cd4..5b0ddf26898 100644 --- a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java +++ b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java @@ -8,6 +8,7 @@ @SelectClasses({ // DependencyManagerTest.class, // DependencyLoopFinderTest.class, // + StaleDependencyResolutionTest.class, // WorkspaceModelManagerTest.class, // WorkspaceProductModelManagerTest.class, // }) diff --git a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/StaleDependencyResolutionTest.java b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/StaleDependencyResolutionTest.java new file mode 100644 index 00000000000..ebcee7f4513 --- /dev/null +++ b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/StaleDependencyResolutionTest.java @@ -0,0 +1,174 @@ +/******************************************************************************* + * Copyright (c) 2026 Lars Vogel and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Lars Vogel - initial API and implementation + *******************************************************************************/ +package org.eclipse.pde.core.tests.internal; + +import static java.util.Map.entry; +import static org.eclipse.pde.ui.tests.util.TargetPlatformUtil.bundle; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.osgi.framework.Constants.EXPORT_PACKAGE; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CopyOnWriteArrayList; + +import org.eclipse.core.runtime.CoreException; +import org.eclipse.jface.text.Document; +import org.eclipse.osgi.service.resolver.State; +import org.eclipse.osgi.service.resolver.StateDelta; +import org.eclipse.pde.core.plugin.PluginRegistry; +import org.eclipse.pde.core.target.NameVersionDescriptor; +import org.eclipse.pde.internal.core.IStateDeltaListener; +import org.eclipse.pde.internal.core.PDECore; +import org.eclipse.pde.internal.core.PluginModelDelta; +import org.eclipse.pde.internal.core.PluginModelManager; +import org.eclipse.pde.internal.core.plugin.PluginReference; +import org.eclipse.pde.internal.core.text.bundle.BundleModel; +import org.eclipse.pde.internal.core.text.bundle.ImportPackageHeader; +import org.eclipse.pde.internal.core.text.bundle.ImportPackageObject; +import org.eclipse.pde.ui.tests.util.ProjectUtils; +import org.eclipse.pde.ui.tests.util.TargetPlatformUtil; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.rules.TestRule; +import org.osgi.framework.Constants; + +/** + * Guards the Dependencies tab of the manifest editor against stale error + * decorations. The decorations are computed by {@code PDELabelProvider} from + * {@code isResolved()}, so they are only correct if the resolution result is + * recomputed and if the editor is notified that the target platform changed. + */ +public class StaleDependencyResolutionTest { + + @ClassRule + public static final TestRule RESTORE_TARGET_DEFINITION = TargetPlatformUtil.RESTORE_CURRENT_TARGET_DEFINITION_AFTER; + @ClassRule + public static final TestRule CLEAR_WORKSPACE = ProjectUtils.DELETE_ALL_WORKSPACE_PROJECTS_BEFORE_AND_AFTER; + + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + + private Path targetWithBundleA; + private Path targetWithoutBundleA; + + @Before + public void setUp() throws IOException { + targetWithBundleA = folder.newFolder("targetWithBundleA").toPath(); + targetWithoutBundleA = folder.newFolder("targetWithoutBundleA").toPath(); + // ensure the PluginModelManager is initialized before listening to it + PluginModelManager.getInstance().getState(); + } + + /** + * A target reload never fires a {@link PluginModelDelta}, so the dependency + * sections cannot rely on plug-in model listeners alone. They also listen to + * {@link IStateDeltaListener#stateChanged}, which is the only notification a + * reload produces. + */ + @Test + public void testTargetReloadNotifiesStateListeners() throws Exception { + setTargetPlatform(targetWithBundleA, bundle("bundle.a", "1.0.0")); + + List states = new CopyOnWriteArrayList<>(); + IStateDeltaListener listener = new IStateDeltaListener() { + @Override + public void stateResolved(StateDelta delta) { + // not the notification the sections depend on for a reload + } + + @Override + public void stateChanged(State newState) { + states.add(newState); + } + }; + PluginModelManager manager = PDECore.getDefault().getModelManager(); + manager.addStateDeltaListener(listener); + try { + setTargetPlatform(targetWithoutBundleA, bundle("bundle.b", "1.0.0")); + + assertNull("precondition: bundle.a must be gone from the target", + PluginRegistry.findModel("bundle.a")); + assertFalse("target reload did not notify the state listeners", states.isEmpty()); + } finally { + manager.removeStateDeltaListener(listener); + } + } + + /** + * {@link PluginReference} must not memoize a looked-up plug-in. A required + * bundle that disappears from the target has to report itself as unresolved + * so the error decoration appears. + */ + @Test + public void testResolutionIsRecomputedWhenBundleLeavesTarget() throws Exception { + setTargetPlatform(targetWithBundleA, bundle("bundle.a", "1.0.0")); + + PluginReference reference = new PluginReference("bundle.a"); + assertTrue("precondition: bundle.a must resolve while it is in the target", reference.isResolved()); + + setTargetPlatform(targetWithoutBundleA, bundle("bundle.b", "1.0.0")); + + assertNull("precondition: bundle.a must be gone from the target", PluginRegistry.findModel("bundle.a")); + assertFalse("resolution result was not recomputed after bundle.a left the target", reference.isResolved()); + } + + /** + * The counterpart for imported packages. {@code ImportPackageObject} queries + * the live {@code PDEState} on every call, so its result follows a target + * change without any caching to invalidate. + */ + @Test + public void testImportedPackageResolutionFollowsTarget() throws Exception { + setTargetPlatform(targetWithBundleA, bundle("bundle.a", "1.0.0", entry(EXPORT_PACKAGE, "bundle.a.pack"))); + + ImportPackageObject importedPackage = importedPackage("bundle.a.pack"); + assertNotNull("precondition: bundle.a must be in the target", PluginRegistry.findModel("bundle.a")); + assertTrue("bundle.a.pack must resolve while its exporter is in the target", importedPackage.isResolved()); + + setTargetPlatform(targetWithoutBundleA, bundle("bundle.b", "1.0.0")); + + assertFalse("bundle.a.pack must not resolve after its exporter left the target", + importedPackage.isResolved()); + } + + private static ImportPackageObject importedPackage(String packageName) throws CoreException { + Document document = new Document(); + document.set(""" + Manifest-Version: 1.0 + Bundle-ManifestVersion: 2 + Bundle-SymbolicName: bundle.importer + Bundle-Version: 1.0.0 + Import-Package: %s + """.formatted(packageName)); + BundleModel model = new BundleModel(document, false); + model.load(); + ImportPackageHeader header = (ImportPackageHeader) model.getBundle() + .getManifestHeader(Constants.IMPORT_PACKAGE); + return header.getPackage(packageName); + } + + @SafeVarargs + private static void setTargetPlatform(Path jarDirectory, + Map.Entry>... bundles) throws Exception { + TargetPlatformUtil.setDummyBundlesAsTarget(Map.ofEntries(bundles), List.of(), jarDirectory); + } +} diff --git a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/ImportPackageSection.java b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/ImportPackageSection.java index 64019a4be10..4b0e6e1a98d 100644 --- a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/ImportPackageSection.java +++ b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/ImportPackageSection.java @@ -49,6 +49,7 @@ import org.eclipse.osgi.service.resolver.ExportPackageDescription; import org.eclipse.osgi.service.resolver.HostSpecification; import org.eclipse.osgi.service.resolver.State; +import org.eclipse.osgi.service.resolver.StateDelta; import org.eclipse.pde.core.IBaseModel; import org.eclipse.pde.core.IModel; import org.eclipse.pde.core.IModelChangedEvent; @@ -58,6 +59,11 @@ import org.eclipse.pde.core.target.NameVersionDescriptor; import org.eclipse.pde.internal.core.ClasspathUtilCore; import org.eclipse.pde.internal.core.ICoreConstants; +import org.eclipse.pde.internal.core.IPluginModelListener; +import org.eclipse.pde.internal.core.IStateDeltaListener; +import org.eclipse.pde.internal.core.PDECore; +import org.eclipse.pde.internal.core.PluginModelDelta; +import org.eclipse.pde.internal.core.PluginModelManager; import org.eclipse.pde.internal.core.TargetPlatformHelper; import org.eclipse.pde.internal.core.WorkspaceModelManager; import org.eclipse.pde.internal.core.bundle.BundlePluginBase; @@ -92,6 +98,7 @@ import org.eclipse.swt.graphics.Image; import org.eclipse.swt.layout.GridData; import org.eclipse.swt.widgets.Composite; +import org.eclipse.swt.widgets.Control; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Table; import org.eclipse.ui.IWorkingSet; @@ -105,7 +112,7 @@ import org.osgi.framework.Version; import org.osgi.resource.Resource; -public class ImportPackageSection extends TableSection { +public class ImportPackageSection extends TableSection implements IPluginModelListener, IStateDeltaListener { private static final int ADD_INDEX = 0; private static final int REMOVE_INDEX = 1; @@ -265,10 +272,49 @@ public int compare(Viewer viewer, Object e1, Object e2) { IBundleModel model = getBundleModel(); fPackageViewer.setInput(model); model.addModelChangedListener(this); - section.addDisposeListener(e -> model.removeModelChangedListener(ImportPackageSection.this)); + PluginModelManager modelManager = PDECore.getDefault().getModelManager(); + modelManager.addPluginModelListener(this); + modelManager.addStateDeltaListener(this); + section.addDisposeListener(e -> { + model.removeModelChangedListener(ImportPackageSection.this); + modelManager.removePluginModelListener(ImportPackageSection.this); + modelManager.removeStateDeltaListener(ImportPackageSection.this); + }); updateButtons(); } + @Override + public void modelsChanged(PluginModelDelta delta) { + refreshPackages(); + } + + @Override + public void stateResolved(StateDelta delta) { + // already covered by modelsChanged, which is fired for the same batch + } + + @Override + public void stateChanged(State newState) { + // a target reload replaces the state without firing a PluginModelDelta + refreshPackages(); + } + + /** + * Repaints the table so the resolution decorations match the current target + * platform. The imported packages themselves are unchanged, only their + * resolution status is recomputed. + */ + private void refreshPackages() { + Control control = fPackageViewer.getControl(); + if (!control.isDisposed()) { + control.getDisplay().asyncExec(() -> { + if (!control.isDisposed()) { + fPackageViewer.refresh(); + } + }); + } + } + @Override public boolean doGlobalAction(String actionId) { diff --git a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/RequiresSection.java b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/RequiresSection.java index 42f948db0f5..17f5320528f 100644 --- a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/RequiresSection.java +++ b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/plugin/RequiresSection.java @@ -41,6 +41,8 @@ import org.eclipse.jface.viewers.TableViewer; import org.eclipse.jface.viewers.ViewerDropAdapter; import org.eclipse.jface.window.Window; +import org.eclipse.osgi.service.resolver.State; +import org.eclipse.osgi.service.resolver.StateDelta; import org.eclipse.pde.core.IModel; import org.eclipse.pde.core.IModelChangedEvent; import org.eclipse.pde.core.plugin.IPlugin; @@ -51,6 +53,7 @@ import org.eclipse.pde.core.plugin.IPluginModelFactory; import org.eclipse.pde.core.plugin.PluginRegistry; import org.eclipse.pde.internal.core.IPluginModelListener; +import org.eclipse.pde.internal.core.IStateDeltaListener; import org.eclipse.pde.internal.core.PDECore; import org.eclipse.pde.internal.core.PluginModelDelta; import org.eclipse.pde.internal.core.bundle.BundlePluginBase; @@ -91,7 +94,8 @@ import org.eclipse.ui.forms.widgets.Section; import org.eclipse.ui.progress.UIJob; -public class RequiresSection extends TableSection implements IPluginModelListener, IPropertyChangeListener { +public class RequiresSection extends TableSection + implements IPluginModelListener, IStateDeltaListener, IPropertyChangeListener { private static final int ADD_INDEX = 0; private static final int REMOVE_INDEX = 1; @@ -262,6 +266,7 @@ public void dispose() { model.removeModelChangedListener(this); } PDECore.getDefault().getModelManager().removePluginModelListener(this); + PDECore.getDefault().getModelManager().removeStateDeltaListener(this); super.dispose(); } @@ -546,6 +551,7 @@ private void addSystemBundle(java.util.List list) { public void initialize() { PDECore.getDefault().getModelManager().addPluginModelListener(this); + PDECore.getDefault().getModelManager().addStateDeltaListener(this); if (getPage().getModel() instanceof IPluginModelBase model) { fImportViewer.setInput(model.getPluginBase()); updateButtons(); @@ -672,6 +678,21 @@ public void modelChanged(final IModelChangedEvent event) { @Override public void modelsChanged(PluginModelDelta delta) { + refreshImports(); + } + + @Override + public void stateResolved(StateDelta delta) { + // already covered by modelsChanged, which is fired for the same batch + } + + @Override + public void stateChanged(State newState) { + // a target reload replaces the state without firing a PluginModelDelta + refreshImports(); + } + + private void refreshImports() { fImports = null; final Control control = fImportViewer.getControl(); if (!control.isDisposed()) {