Skip to content

Commit 4391a91

Browse files
vogellaclaude
authored andcommitted
Fix incorrect SubMonitor usage patterns
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.
1 parent 9c171a0 commit 4391a91

File tree

10 files changed

+18
-55
lines changed

10 files changed

+18
-55
lines changed

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -435,7 +435,6 @@ public final IDocument getCurrentDocument(IProgressMonitor monitor) throws CoreE
435435
releaseDocument(result, subMon.newChild(1));
436436
}
437437
}
438-
subMon.done();
439438
if (result == null) {
440439
result= new Document();
441440
}

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

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -199,22 +199,18 @@ public void setSchedulingRule(ISchedulingRule rule) {
199199
@Override
200200
public void run(IProgressMonitor pm) throws CoreException {
201201
SubMonitor subMon= SubMonitor.convert(pm, 4);
202-
try {
203-
fChangeExecuted= false;
204-
if (createChange()) {
205-
// Check for cancellation before executing the change, since canceling
206-
// during change execution is not supported
207-
// (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=187265 ):
208-
fCreateChangeOperation.run(subMon.split(3));
209-
fChange= fCreateChangeOperation.getChange();
210-
if (fChange != null) {
211-
executeChange(subMon.newChild(1));
212-
}
213-
} else {
214-
executeChange(subMon.newChild(4));
202+
fChangeExecuted= false;
203+
if (createChange()) {
204+
// Check for cancellation before executing the change, since canceling
205+
// during change execution is not supported
206+
// (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=187265 ):
207+
fCreateChangeOperation.run(subMon.split(3));
208+
fChange= fCreateChangeOperation.getChange();
209+
if (fChange != null) {
210+
executeChange(subMon.newChild(1));
215211
}
216-
} finally {
217-
subMon.done();
212+
} else {
213+
executeChange(subMon.newChild(4));
218214
}
219215
}
220216

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,6 @@ public void run(IProgressMonitor monitor) throws CoreException {
135135
fUndo= perform.getUndoChange();
136136
}
137137
} finally {
138-
subMon.done();
139138
if (fRefactoringContext != null) {
140139
fRefactoringContext.dispose();
141140
}

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,6 @@ public RefactoringStatus checkAllConditions(IProgressMonitor pm) throws CoreExce
162162
if (!result.hasFatalError()) {
163163
result.merge(checkFinalConditions(subMon.split(refactoringTickProvider.getCheckFinalConditionsTicks())));
164164
}
165-
subMon.done();
166165
return result;
167166
}
168167

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,6 @@ public Change perform(IProgressMonitor pm) throws CoreException {
247247
throw Changes.asCoreException(e);
248248
} finally {
249249
releaseDocument(document, subMon.newChild(1));
250-
subMon.done();
251250
}
252251
}
253252

bundles/org.eclipse.ltk.core.refactoring/src/org/eclipse/ltk/core/refactoring/participants/CheckConditionsContext.java

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,11 +123,7 @@ public RefactoringStatus check(IProgressMonitor pm) throws CoreException {
123123
SubMonitor sm= SubMonitor.convert(pm, "", values.size()); //$NON-NLS-1$
124124
for (IConditionChecker checker : values) {
125125
result.merge(checker.check(sm.split(1)));
126-
if (pm.isCanceled()) {
127-
throw new OperationCanceledException();
128-
}
129126
}
130-
pm.done();
131127
return result;
132128
}
133129

bundles/org.eclipse.ltk.core.refactoring/src/org/eclipse/ltk/core/refactoring/participants/ProcessorBasedRefactoring.java

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -232,9 +232,6 @@ public RefactoringStatus checkFinalConditions(IProgressMonitor pm) throws CoreEx
232232
if (result.hasFatalError()) {
233233
return result;
234234
}
235-
if (sm.isCanceled()) {
236-
throw new OperationCanceledException();
237-
}
238235

239236
SharableParticipants sharableParticipants= new SharableParticipants(); // must not be shared when checkFinalConditions is called again
240237
RefactoringParticipant[] loadedParticipants= getProcessor().loadParticipants(result, sharableParticipants);
@@ -270,9 +267,6 @@ public RefactoringStatus checkFinalConditions(IProgressMonitor pm) throws CoreEx
270267

271268
stats.endRun();
272269

273-
if (sm2.isCanceled()) {
274-
throw new OperationCanceledException();
275-
}
276270
}
277271
if (result.hasFatalError()) {
278272
return result;
@@ -292,9 +286,6 @@ public Change createChange(IProgressMonitor pm) throws CoreException {
292286
try {
293287
SubMonitor sm= SubMonitor.convert(pm, RefactoringCoreMessages.ProcessorBasedRefactoring_create_change, fParticipants.size() * 2 + 1);
294288
Change processorChange= getProcessor().createChange(sm.split(1));
295-
if (sm.isCanceled()) {
296-
throw new OperationCanceledException();
297-
}
298289

299290
fTextChangeMap= new HashMap<>();
300291
addToTextChangeMap(processorChange);
@@ -334,9 +325,6 @@ public Change createChange(IProgressMonitor pm) throws CoreException {
334325
disableParticipant(participant, e);
335326
throw e;
336327
}
337-
if (sm.isCanceled()) {
338-
throw new OperationCanceledException();
339-
}
340328
}
341329

342330
fTextChangeMap= null;

bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/wizards/datatransfer/SmartImportJob.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,6 @@ private Set<IProject> searchAndImportChildrenProjectsRecursively(final IContaine
383383
for (CrawlFolderJob job : jobs) {
384384
job.join(0, subMonitor.split(1));
385385
}
386-
subMonitor.done();
387386
return res;
388387
}
389388

@@ -512,7 +511,6 @@ private Set<IProject> importProjectAndChildrenRecursively(final IContainer conta
512511
// make sure this folder isn't going to be processed again
513512
excludedPaths.add(project.getLocation());
514513
}
515-
subMonitor.done();
516514
return projectFromCurrentContainer;
517515
}
518516

bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/SaveableHelper.java

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -184,21 +184,14 @@ private static boolean saveModels(ISaveablesSource modelSource, final IWorkbench
184184
IRunnableWithProgress progressOp = monitor -> {
185185
IProgressMonitor monitorWrap = new EventLoopProgressMonitor(monitor);
186186
SubMonitor subMonitor = SubMonitor.convert(monitorWrap, WorkbenchMessages.Save, dirtyModels.size());
187-
try {
188-
for (Saveable model : dirtyModels) {
189-
// handle case where this model got saved as a result of
190-
// saving another
191-
if (!model.isDirty()) {
192-
subMonitor.worked(1);
193-
continue;
194-
}
195-
doSaveModel(model, subMonitor.split(1), window, confirm);
196-
if (subMonitor.isCanceled()) {
197-
break;
198-
}
187+
for (Saveable model : dirtyModels) {
188+
// handle case where this model got saved as a result of
189+
// saving another
190+
if (!model.isDirty()) {
191+
subMonitor.worked(1);
192+
continue;
199193
}
200-
} finally {
201-
monitorWrap.done();
194+
doSaveModel(model, subMonitor.split(1), window, confirm);
202195
}
203196
};
204197

bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/SaveablesList.java

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -796,11 +796,7 @@ public boolean saveModels(final List<Saveable> finalModels, final IShellProvider
796796
continue;
797797
}
798798
SaveableHelper.doSaveModel(model, subMonitor.split(1), shellProvider, blockUntilSaved);
799-
if (subMonitor.isCanceled()) {
800-
break;
801-
}
802799
}
803-
monitorWrap.done();
804800
};
805801

806802
// Do the save.

0 commit comments

Comments
 (0)