Restore accurate enablement of CloseUnrelatedProjectsAction#3941
Restore accurate enablement of CloseUnrelatedProjectsAction#3941vogella wants to merge 1 commit intoeclipse-platform:masterfrom
Conversation
| for (IResourceDelta projDelta : projDeltas) { | ||
| //changing either the description or the open state can affect enablement | ||
| if ((projDelta.getFlags() & (IResourceDelta.OPEN | IResourceDelta.DESCRIPTION)) != 0) { | ||
| invalidateProjectGraph(); |
There was a problem hiding this comment.
resourceChanged() is never called (probably due to an missing subscription in initialization.
Even if it would be, graph invalidation should happen independently of selection (because project can be created and deleted when selection is unrelated).
| if (!selectionIsOfType(IResource.PROJECT)) { | ||
| return false; | ||
| } | ||
| boolean hasOpenSelectedProject = false; |
There was a problem hiding this comment.
This method is now effectively identical to super and can be removed.
| private void invalidateProjectGraph() { | ||
| projectGraph = null; | ||
| oldSelection = Collections.emptyList(); | ||
| selectionDirty = true; |
There was a problem hiding this comment.
this flag is redundant due to presence of an equivalent in superclass.
| } | ||
|
|
||
| private void invalidateProjectGraph() { | ||
| projectGraph = null; |
There was a problem hiding this comment.
Race condition with resource updating thread (would be, if these actions were actually registered as resource listeners)
vogella
force-pushed
the
fix-close-unrelated-projects-enablement
branch
from
de99193 to
13b5a44
Compare
vogella
left a comment
vogella
left a comment
There was a problem hiding this comment.
Addressed review feedback:
Fixed (issue #1 & #4): resourceChanged() was never called — the action was not registered as an IResourceChangeListener. Added addResourceChangeListener(this, POST_CHANGE) in initAction(). Also broadened the handler to invalidate the graph on project ADDED/REMOVED (not just CHANGED), and added synchronized to match the superclass pattern. Added a new test (testListenerInvalidatesGraphOnProjectClose) that validates the listener-driven invalidation.
Disagree (issue #2 – updateSelection identical to super): The override is not redundant. The super's updateSelection() calls getSelectedResources(), which in this subclass triggers computeRelated() and graph construction. The override first checks super.getSelectedResources() (the raw selection) for an open project — a cheap early-exit that avoids the expensive graph computation when the selection has no open projects.
Disagree (issue #3 – selectionDirty redundant): There is no selectionDirty field in the superclass. This flag gates re-execution of computeRelated() inside the overridden getSelectedResources() — it is specific to the subclass caching logic.
Is this comment AI generated? I ask because characters like — an em dash do not readily roll off a keyboard. |
it is redundant because we already use super.getSelectedResources() and check the result for equality. We just need to call Here is an idea, based on old revision: From 626fc5f8a96744c65159d2bb3023607cf7985fe1 Mon Sep 17 00:00:00 2001
From: Vasili Gulevich <vasili.gulevich@xored.com>
Date: Tue, 28 Apr 2026 21:53:11 +0400
Subject: [PATCH] Remove redundant state from action
---
.../actions/CloseUnrelatedProjectsAction.java | 47 ++-----------------
1 file changed, 5 insertions(+), 42 deletions(-)
diff --git a/bundles/org.eclipse.ui.ide/extensions/org/eclipse/ui/actions/CloseUnrelatedProjectsAction.java b/bundles/org.eclipse.ui.ide/extensions/org/eclipse/ui/actions/CloseUnrelatedProjectsAction.java
index 62f5fa4b4f..cee70cfeb4 100644
--- a/bundles/org.eclipse.ui.ide/extensions/org/eclipse/ui/actions/CloseUnrelatedProjectsAction.java
+++ b/bundles/org.eclipse.ui.ide/extensions/org/eclipse/ui/actions/CloseUnrelatedProjectsAction.java
@@ -32,7 +32,6 @@ import org.eclipse.core.runtime.CoreException;
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;
@@ -65,8 +64,6 @@ 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
@@ -137,30 +134,6 @@ public class CloseUnrelatedProjectsAction extends CloseResourceAction {
initAction();
}
- @Override
- protected boolean updateSelection(IStructuredSelection s) {
- selectionDirty = true;
- if (!selectionIsOfType(IResource.PROJECT)) {
- return false;
- }
- boolean hasOpenSelectedProject = false;
- for (IResource resource : super.getSelectedResources()) {
- if (resource instanceof IProject project && project.isOpen()) {
- hasOpenSelectedProject = true;
- break;
- }
- }
- if (!hasOpenSelectedProject) {
- return false;
- }
- for (IResource resource : getSelectedResources()) {
- if (resource instanceof IProject project && project.isOpen()) {
- return true;
- }
- }
- return false;
- }
-
@Override
public void run() {
if (promptForConfirmation()) {
@@ -220,13 +193,6 @@ public class CloseUnrelatedProjectsAction extends CloseResourceAction {
PlatformUI.getWorkbench().getHelpSystem().setHelp(this, IIDEHelpContextIds.CLOSE_UNRELATED_PROJECTS_ACTION);
}
- @Override
- protected void clearCache() {
- super.clearCache();
- oldSelection = Collections.emptyList();
- selectionDirty = true;
- }
-
/**
* Computes the projects unrelated to the given selection.
*/
@@ -269,18 +235,15 @@ public class CloseUnrelatedProjectsAction extends CloseResourceAction {
private void invalidateProjectGraph() {
projectGraph = null;
oldSelection = Collections.emptyList();
- selectionDirty = true;
+ 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;
}
--
2.50.1 (Apple Git-155)
Optimization of graph construction can be done in |
|
|
Yes, Lars uses Gemini, I'm trying out Copilt. It is a bit of a mess. |
Ah, nice. Sorry I did not see that PR. Will have a look tomorrow, already of my computer. |
|
I feel like a spectator at a bad theater production. Really bad theater. This cannot be headed toward a good path. Surely we cannot generate content that we expect other humans to test and review and then hit another button to generate the next round of noise. 😡 it’s like the battle of the competing AI agents with a human audience. I have Netflix for entertainment, and this is so not how I expect professional software development to function. |
I typically use Claude, the above was actually Copilot. |
Well, all parties are very annoyed, me and Lars included. And results are admittedly awful. However, there are useful insights, convenient chore management and results improve with better prompting. I just treat this as inevitable evil, that we will have to live with. I want to be ready when that time comes, and keep reviewing slop hoping to get necessary experience. |
|
I hope and expect that much of this can and will be moved to personal forks where the results are carefully reviewed and properly tested before they are foisted into the public broadcasting system. We will all become deaf from the roaring volume of the current public AI theatre approach. It must improve or we will need rules to force it to improve. One hates to be seen as a barrier to progress, but I see way too much change that seems poorly justified and is just begging for regressions we can only hope will be noticed sooner rather than later. This being a case in point. |
…ed projects After PR eclipse-platform#3871, updateSelection() only checked whether the selection contained an open project, so the action stayed enabled even when no unrelated projects were left to close. PR eclipse-platform#3871 avoided calling buildConnectedComponents() on every selection change, since IProject#getReferencedProjects() can resolve classpath containers and block the IDE. Cache the workspace project graph and invalidate it from resourceChanged() on project open/description changes. The graph is now built at most once per project-state change rather than once per selection event, while updateSelection() can again check whether any open unrelated project exists. Add CloseUnrelatedProjectsActionEnablementTest covering the regression.
vogella
force-pushed
the
fix-close-unrelated-projects-enablement
branch
from
13b5a44 to
8f71ec5
Compare
|
Thanks @basilevs. Your test helped me to find the problem, I integrated it into the updated PR. Locally your and the other new tests are passing. Please re-test your scenario with the patch. |
| super.clearCache(); | ||
| oldSelection = Collections.emptyList(); | ||
| selectionDirty = true; | ||
| ResourcesPlugin.getWorkspace().addResourceChangeListener(this, IResourceChangeEvent.POST_CHANGE); |
There was a problem hiding this comment.
Why CloseResourceAction (supreclass) does not have this? Should not this fix be applied to it instead?
When is this listener removed?
| selectionChanged(getStructuredSelection()); | ||
| return; | ||
| } | ||
| public synchronized void resourceChanged(IResourceChangeEvent event) { |
There was a problem hiding this comment.
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).
| } | ||
|
|
||
| private DisjointSet<IProject> getProjectGraph() { | ||
| DisjointSet<IProject> graph = projectGraph; |
There was a problem hiding this comment.
Race with resource thread. Both threads should use same mutex to synchronze and avoid races.
|
I could not quickly find a way to add dispose listener to JFace action. It would be interesting to research why were resource listeners removed and how cleanup had been done when they were still working. I've considered removing resource listener whenever project graph is cleared and adding it back before graph is recreated, but this approach still leaks listeners when action is forgotten (because we can't guarantee user would change projects at all). |
3 participants
PR #3871 made
CloseUnrelatedProjectsAction.updateSelection()cheap by checking only whether the user selection contains an open project. As a result the action now stays enabled after the last unrelated project is closed (reported by @basilevs on #3871, also visible as an RCPTT regression on platform staging).This change keeps the performance goal of #3871 while restoring correct enablement: the workspace project graph is built lazily and cached in the action, and is invalidated from
resourceChanged()only when projects open, close, or change description. Selection changes therefore no longer triggerIProject#getReferencedProjects()across the workspace, so classpath container resolution is not pulled onto the UI thread per selection event, butupdateSelection()can again use the graph to ask whether any open unrelated project actually exists. A new test inorg.eclipse.ui.tests.navigatorreproduces the regression scenario.