Skip to content

Restore accurate enablement of CloseUnrelatedProjectsAction #3941

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vogella wants to merge 1 commit into from
+281 −55
Closed

Restore accurate enablement of CloseUnrelatedProjectsAction #3941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 56 additions & 55 deletions ...es/org.eclipse.ui.ide/extensions/org/eclipse/ui/actions/CloseUnrelatedProjectsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IResource;
Expand All @@ -30,7 +32,6 @@
import org.eclipse.jface.dialogs.IDialogConstants;
import org.eclipse.jface.dialogs.MessageDialogWithToggle;
import org.eclipse.jface.preference.IPreferenceStore;
import org.eclipse.jface.viewers.IStructuredSelection;
import org.eclipse.jface.window.IShellProvider;
import org.eclipse.osgi.util.NLS;
import org.eclipse.swt.widgets.Shell;
Expand Down Expand Up @@ -63,10 +64,12 @@ public class CloseUnrelatedProjectsAction extends CloseResourceAction {

private List<IResource> projectsToClose = new ArrayList<>();

private boolean selectionDirty = true;

private List<? extends IResource> oldSelection = Collections.emptyList();

// Cached workspace project graph. Built lazily, reused across selection
// changes, invalidated when project open state or description changes.
private DisjointSet<IProject> projectGraph;


/**
* Builds the connected component set for the input projects.
Expand Down Expand Up @@ -131,25 +134,6 @@ public CloseUnrelatedProjectsAction(IShellProvider provider){
initAction();
}

/**
* Overrides to avoid calling the expensive
* {@code computeRelated(List)} during selection changes. Uses only
* the raw selection to determine enablement.
*/
@Override
protected boolean updateSelection(IStructuredSelection s) {
selectionDirty = true;
if (!selectionIsOfType(IResource.PROJECT)) {
return false;
}
for (IResource resource : super.getSelectedResources()) {
if (resource instanceof IProject project && project.isOpen()) {
return true;
}
}
return false;
}

@Override
public void run() {
if (promptForConfirmation()) {
Expand Down Expand Up @@ -207,46 +191,60 @@ private void initAction(){
setId(ID);
setToolTipText(IDEWorkbenchMessages.CloseUnrelatedProjectsAction_toolTip);
PlatformUI.getWorkbench().getHelpSystem().setHelp(this, IIDEHelpContextIds.CLOSE_UNRELATED_PROJECTS_ACTION);
}

@Override
protected void clearCache() {
super.clearCache();
oldSelection = Collections.emptyList();
selectionDirty = true;
ResourcesPlugin.getWorkspace().addResourceChangeListener(this, IResourceChangeEvent.POST_CHANGE);
Copy link
Copy Markdown
Contributor

@basilevs basilevs Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why CloseResourceAction (supreclass) does not have this? Should not this fix be applied to it instead?

When is this listener removed?

}

/**
* Computes the related projects of the selection.
* Computes the projects unrelated to the given selection.
*/
private List<IResource> computeRelated(List<? extends IResource> selection) {
if (selection.contains(ResourcesPlugin.getWorkspace().getRoot())) {
return new ArrayList<>();
}
//build the connected component set for all projects in the workspace
DisjointSet<IProject> set = buildConnectedComponents(ResourcesPlugin.getWorkspace().getRoot().getProjects());
//remove the connected components that the selected projects are in
DisjointSet<IProject> set = getProjectGraph();
Set<IProject> excludedRoots = new HashSet<>();
for (IResource resource : selection) {
IProject project = resource.getProject();
if (project != null) {
set.removeSet(project);
IProject root = set.findSet(project);
if (root != null) {
excludedRoots.add(root);
}
}
}
//the remainder of the projects in the disjoint set are unrelated to the selection
List<IProject> all = new ArrayList<>();
set.toList(all);
List<IResource> projects = new ArrayList<>();
set.toList(projects);
for (IProject project : all) {
IProject root = set.findSet(project);
if (root != null && !excludedRoots.contains(root)) {
projects.add(project);
}
}
return projects;
}

private DisjointSet<IProject> getProjectGraph() {
DisjointSet<IProject> graph = projectGraph;
Copy link
Copy Markdown
Contributor

@basilevs basilevs Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race with resource thread. Both threads should use same mutex to synchronze and avoid races.

if (graph == null) {
graph = buildConnectedComponents(ResourcesPlugin.getWorkspace().getRoot().getProjects());
projectGraph = graph;
}
return graph;
}

private void invalidateProjectGraph() {
projectGraph = null;
Copy link
Copy Markdown
Contributor

@basilevs basilevs Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition with resource updating thread (would be, if these actions were actually registered as resource listeners)

oldSelection = Collections.emptyList();
clearCache();
}

@Override
protected List<? extends IResource> getSelectedResources() {
if (selectionDirty) {
List<? extends IResource> newSelection = super.getSelectedResources();
if (!oldSelection.equals(newSelection)) {
oldSelection = newSelection;
projectsToClose = computeRelated(newSelection);
}
selectionDirty = false;
List<? extends IResource> newSelection = super.getSelectedResources();
if (!oldSelection.equals(newSelection)) {
oldSelection = newSelection;
projectsToClose = computeRelated(newSelection);
}
return projectsToClose;
}
Expand All @@ -259,19 +257,22 @@ protected List<? extends IResource> getSelectedResources() {
* the selection when the open state or description of any project changes.
*/
@Override
public void resourceChanged(IResourceChangeEvent event) {
// don't bother looking at delta if selection not applicable
if (selectionIsOfType(IResource.PROJECT)) {
IResourceDelta delta = event.getDelta();
if (delta != null) {
IResourceDelta[] projDeltas = delta.getAffectedChildren(IResourceDelta.CHANGED);
for (IResourceDelta projDelta : projDeltas) {
//changing either the description or the open state can affect enablement
if ((projDelta.getFlags() & (IResourceDelta.OPEN | IResourceDelta.DESCRIPTION)) != 0) {
selectionChanged(getStructuredSelection());
return;
}
public synchronized void resourceChanged(IResourceChangeEvent event) {
Copy link
Copy Markdown
Contributor

@basilevs basilevs Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lock might be held for too long. Most of the time, there are no projects changed and invalidateProjectGraph() is not called. Holding a lock even if we are not going to use it is wasteful (and would block UI thread if synchronized access would actually be properly implemented).

IResourceDelta delta = event.getDelta();
if (delta == null) {
return;
}
IResourceDelta[] projDeltas = delta
.getAffectedChildren(IResourceDelta.ADDED | IResourceDelta.REMOVED | IResourceDelta.CHANGED);
for (IResourceDelta projDelta : projDeltas) {
int kind = projDelta.getKind();
if (kind == IResourceDelta.ADDED || kind == IResourceDelta.REMOVED
|| (projDelta.getFlags() & (IResourceDelta.OPEN | IResourceDelta.DESCRIPTION)) != 0) {
invalidateProjectGraph();
if (selectionIsOfType(IResource.PROJECT)) {
selectionChanged(getStructuredSelection());
}
return;
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions ...org.eclipse.ui.tests.navigator/src/org/eclipse/ui/tests/navigator/NavigatorTestSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import org.eclipse.ui.tests.navigator.cdt.CdtTest;
import org.eclipse.ui.tests.navigator.jst.JstPipelineTest;
import org.eclipse.ui.tests.navigator.resources.CloseUnrelatedProjectsActionEnablementTest;
import org.eclipse.ui.tests.navigator.resources.FoldersAsProjectsContributionTest;
import org.eclipse.ui.tests.navigator.resources.NestedResourcesTests;
import org.eclipse.ui.tests.navigator.resources.PathComparatorTest;
Expand All @@ -36,6 +37,7 @@
LabelProviderTest.class, SorterTest.class, ViewerTest.class, CdtTest.class, M12Tests.class,
FirstClassM1Tests.class, LinkHelperTest.class, ShowInTest.class, ResourceTransferTest.class,
EvaluationCacheTest.class, ResourceMgmtActionProviderTests.class,
CloseUnrelatedProjectsActionEnablementTest.class,
NestedResourcesTests.class, PathComparatorTest.class, FoldersAsProjectsContributionTest.class,
GoBackForwardsTest.class, CopyPasteActionTest.class
// DnDTest.class, // DnDTest.testSetDragOperation() fails
Expand Down
Loading
Loading