Fix incorrect SubMonitor usage patterns #3542

vogella · 2025-11-14T00:44:07Z

This commit addresses SubMonitor anti-patterns as described in https://www.eclipse.org/articles/Article-Progress-Monitors/article.html

Changes:

Removed unnecessary done() calls on monitors after SubMonitor.convert()
- SubMonitor automatically handles done() when work is complete
- Affected files:
  - Refactoring.java
  - PerformChangeOperation.java * PerformRefactoringOperation.java * MultiStateTextFileChange.java * TextChange.java * SmartImportJob.java (2 instances) * SaveablesList.java * SaveableHelper.java * CheckConditionsContext.java
Removed redundant isCanceled() checks after split() calls
- split() already includes implicit cancellation checks
- Affected files:
  - ProcessorBasedRefactoring.java (4 instances)
  - CheckConditionsContext.java * SaveablesList.java * SaveableHelper.java

These changes improve code quality by following SubMonitor best practices and removing redundant code that could mask issues.

github-actions · 2025-11-14T01:06:43Z

Test Results

3 018 files ±0 3 018 suites ±0 2h 20m 56s ⏱️ - 1m 59s
8 242 tests ±0 7 994 ✅ ±0 248 💤 ±0 0 ❌ ±0
23 646 runs ±0 22 855 ✅ ±0 791 💤 ±0 0 ❌ ±0

Results for commit c5a9b88. ± Comparison against base commit e15e9bf.

♻️ This comment has been updated with latest results.

This commit addresses SubMonitor anti-patterns as described in https://www.eclipse.org/articles/Article-Progress-Monitors/article.html Changes: 1. Removed unnecessary done() calls on monitors after SubMonitor.convert() - SubMonitor automatically handles done() when work is complete - Affected files: * Refactoring.java * PerformChangeOperation.java * PerformRefactoringOperation.java * MultiStateTextFileChange.java * TextChange.java * SmartImportJob.java (2 instances) * SaveablesList.java * SaveableHelper.java * CheckConditionsContext.java 2. Removed redundant isCanceled() checks after split() calls - split() already includes implicit cancellation checks - Affected files: * ProcessorBasedRefactoring.java (4 instances) * CheckConditionsContext.java * SaveablesList.java * SaveableHelper.java These changes improve code quality by following SubMonitor best practices and removing redundant code that could mask issues.

eclipse-platform-bot · 2025-12-12T08:58:07Z

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.ltk.core.refactoring/META-INF/MANIFEST.MF
bundles/org.eclipse.ui.ide/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 9f347c5ec27eacf4c87fa96638fbb28067290691 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Fri, 12 Dec 2025 08:57:48 +0000
Subject: [PATCH] Version bump(s) for 4.39 stream


diff --git a/bundles/org.eclipse.ltk.core.refactoring/META-INF/MANIFEST.MF b/bundles/org.eclipse.ltk.core.refactoring/META-INF/MANIFEST.MF
index a4fdac38f2..9f0f659919 100644
--- a/bundles/org.eclipse.ltk.core.refactoring/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.ltk.core.refactoring/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@ Automatic-Module-Name: org.eclipse.ltk.core.refactoring
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.ltk.core.refactoring; singleton:=true
-Bundle-Version: 3.15.100.qualifier
+Bundle-Version: 3.15.200.qualifier
 Bundle-Activator: org.eclipse.ltk.internal.core.refactoring.RefactoringCorePlugin
 Bundle-ActivationPolicy: lazy
 Bundle-Vendor: %providerName
diff --git a/bundles/org.eclipse.ui.ide/META-INF/MANIFEST.MF b/bundles/org.eclipse.ui.ide/META-INF/MANIFEST.MF
index c115a47b5f..0d2916a41c 100644
--- a/bundles/org.eclipse.ui.ide/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.ui.ide/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Plugin.name
 Bundle-SymbolicName: org.eclipse.ui.ide; singleton:=true
-Bundle-Version: 3.22.800.qualifier
+Bundle-Version: 3.22.900.qualifier
 Bundle-Activator: org.eclipse.ui.internal.ide.IDEWorkbenchPlugin
 Bundle-ActivationPolicy: lazy
 Bundle-Vendor: %Plugin.providerName
-- 
2.52.0

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

ptziegler · 2025-12-12T09:16:54Z

bundles/org.eclipse.ltk.core.refactoring/src/org/eclipse/ltk/core/refactoring/TextChange.java

 			throw Changes.asCoreException(e);
 		} finally {
 			releaseDocument(document, subMon.newChild(1));
-			subMon.done();


Wouldn't this cause eclipse-jdt/eclipse.jdt.ui#1833 to reappear? Or is this why the call to done() in getCurrentDocument(IProgressMonitor) remains?

My understanding is that the call to done() is only irrelevant, if the progress monitor is always used correctly. But I have yet to see a project where that is actually the case.

I.e. I would expect test failures in other components, which then also need to be updated.

Good point. So should we leave it as is?

If we can already expect test failures in other components then I would definitely leave it out. For the rest, I'd have to take a closer look to see which are definitely save and which might cause side-effects.
Perhaps it makes sense to split this PR up? Have one for the removal of the isCanceled() check (which at first glance looks safe) and one for the removal of done().

This addresses the proposal by ptziegler in PR eclipse-platform#3542 to split the removal of isCanceled checks.

This addresses the proposal by ptziegler in PR #3542 to split the removal of isCanceled checks.

vogella marked this pull request as draft November 14, 2025 00:44

vogella force-pushed the claude/fix-submonitor-usage-01TksaDpbwkK6pUyfQabsKUt branch from 4a53ae5 to 4391a91 Compare November 14, 2025 00:53

vogella force-pushed the claude/fix-submonitor-usage-01TksaDpbwkK6pUyfQabsKUt branch 2 times, most recently from 326c667 to cf4e18d Compare November 14, 2025 01:24

vogella force-pushed the claude/fix-submonitor-usage-01TksaDpbwkK6pUyfQabsKUt branch from cf4e18d to 1c845b0 Compare December 12, 2025 08:50

vogella marked this pull request as ready for review December 12, 2025 08:50

Version bump(s) for 4.39 stream

c5a9b88

ptziegler reviewed Dec 12, 2025

View reviewed changes

vogella marked this pull request as draft December 12, 2025 09:29

vogella added a commit to vogella/eclipse.platform.ui that referenced this pull request Dec 12, 2025

Remove redundant isCanceled checks in SaveablesList and SaveableHelper

7d16b28

This addresses the proposal by ptziegler in PR eclipse-platform#3542 to split the removal of isCanceled checks.

vogella mentioned this pull request Dec 12, 2025

Remove redundant isCanceled checks in SaveablesList and SaveableHelper #3578

Merged

ptziegler pushed a commit that referenced this pull request Dec 14, 2025

Remove redundant isCanceled checks in SaveablesList and SaveableHelper

26ddd0d

This addresses the proposal by ptziegler in PR #3542 to split the removal of isCanceled checks.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix incorrect SubMonitor usage patterns #3542

Fix incorrect SubMonitor usage patterns #3542

Uh oh!

vogella commented Nov 14, 2025

Uh oh!

github-actions bot commented Nov 14, 2025 •

edited

Loading

Uh oh!

eclipse-platform-bot commented Dec 12, 2025

Uh oh!

ptziegler Dec 12, 2025 •

edited

Loading

Uh oh!

vogella Dec 12, 2025

Uh oh!

ptziegler Dec 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix incorrect SubMonitor usage patterns #3542

Are you sure you want to change the base?

Fix incorrect SubMonitor usage patterns #3542

Uh oh!

Conversation

vogella commented Nov 14, 2025

Uh oh!

github-actions bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

eclipse-platform-bot commented Dec 12, 2025

Uh oh!

ptziegler Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vogella Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

ptziegler Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Nov 14, 2025 •

edited

Loading

ptziegler Dec 12, 2025 •

edited

Loading