Skip to content

Conversation

@vogella
Copy link
Contributor

@vogella vogella commented Nov 14, 2025

This commit addresses several SubMonitor anti-patterns identified throughout the codebase, based on best practices from the Eclipse article on Progress Monitors.

Changes made:

  1. Remove unnecessary null checks before SubMonitor.convert()

    • SubMonitor.convert() can handle null monitors
    • Fixed in team Policy.java files (core and ui)
  2. Remove unnecessary SubMonitor.done() calls

    • SubMonitor.done() is a no-op and should not be called
    • Removed 27 instances across resources and team bundles:
      • org.eclipse.core.filesystem: LocalFile.java (2 calls)
      • org.eclipse.core.resources: File.java (6), Folder.java (1), Project.java (3), SaveManager.java (4), Workspace.java (7) * org.eclipse.compare.core: RangeComparatorLCS.java (1) * org.eclipse.compare: AddFromHistoryAction.java (1) * org.eclipse.team.core: BundleImporterDelegate.java (1), Subscriber.java (1)
  3. Remove redundant cancellation checks before split()

    • SubMonitor.split() already checks for cancellation
    • Fixed in AutoBuildJob.java

Reference: https://www.eclipse.org/articles/Article-Progress-Monitors/article.html

@vogella vogella marked this pull request as draft November 14, 2025 01:15
@vogella vogella force-pushed the claude/fix-submonitor-usage-01TY1bqpmu4uYDXhMeewaN12 branch 2 times, most recently from c06f361 to 0da857c Compare November 14, 2025 01:28
@eclipse-platform-bot
Copy link
Contributor

eclipse-platform-bot commented Nov 14, 2025

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

team/bundles/org.eclipse.jsch.core/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 0b4558ece14037bf6a653283c7ae3d9ef1279f4c Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <platform-bot@eclipse.org>
Date: Fri, 14 Nov 2025 08:10:08 +0000
Subject: [PATCH] Version bump(s) for 4.38 stream


diff --git a/team/bundles/org.eclipse.jsch.core/META-INF/MANIFEST.MF b/team/bundles/org.eclipse.jsch.core/META-INF/MANIFEST.MF
index 82861fbee2..83de036ad6 100644
--- a/team/bundles/org.eclipse.jsch.core/META-INF/MANIFEST.MF
+++ b/team/bundles/org.eclipse.jsch.core/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jsch.core;singleton:=true
-Bundle-Version: 1.5.700.qualifier
+Bundle-Version: 1.5.800.qualifier
 Bundle-Activator: org.eclipse.jsch.internal.core.JSchCorePlugin
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.51.2

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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 14, 2025

Test Results

 1 872 files   -  81   1 872 suites   - 81   1h 22m 18s ⏱️ - 1m 22s
 4 591 tests  - 153   3 871 ✅  -   849   24 💤 ± 0    694 ❌ +  694  2 🔥 +2 
13 773 runs   - 459  11 826 ✅  - 2 224  165 💤  - 17  1 778 ❌ +1 778  4 🔥 +4 

For more details on these failures and errors, see this check.

Results for commit e39b98b. ± Comparison against base commit 3e8e40e.

This pull request removes 153 tests.
AllCompareTests AsyncExecTests ‑ testCancelOnRequeue
AllCompareTests AsyncExecTests ‑ testQueueAdd
AllCompareTests AsyncExecTests ‑ testWorker
AllCompareTests CompareFileRevisionEditorInputTest ‑ testPrepareCompareInputWithNonLocalResourceTypedElements
AllCompareTests CompareUIPluginTest ‑ testFindContentViewerDescriptorForTextType_StreamAccessor
AllCompareTests CompareUIPluginTest ‑ testFindContentViewerDescriptor_TextType_NotStreamAccessor
AllCompareTests CompareUIPluginTest ‑ testFindContentViewerDescriptor_UnknownType
AllCompareTests ContentMergeViewerTest ‑ testFFFX
AllCompareTests ContentMergeViewerTest ‑ testFFTX
AllCompareTests ContentMergeViewerTest ‑ testFFXF
…

♻️ This comment has been updated with latest results.

This commit addresses several SubMonitor anti-patterns identified
throughout the codebase, based on best practices from the Eclipse
article on Progress Monitors.

Changes made:

1. Remove unnecessary null checks before SubMonitor.convert()
   - SubMonitor.convert() can handle null monitors
   - Fixed in team Policy.java files (core and ui)

2. Remove unnecessary SubMonitor.done() calls
   - SubMonitor.done() is a no-op and should not be called
   - Removed 27 instances across resources and team bundles:
     * org.eclipse.core.filesystem: LocalFile.java (2 calls)
     * org.eclipse.core.resources: File.java (6), Folder.java (1),
       Project.java (3), SaveManager.java (4), Workspace.java (7)
     * org.eclipse.compare.core: RangeComparatorLCS.java (1)
     * org.eclipse.compare: AddFromHistoryAction.java (1)
     * org.eclipse.team.core: BundleImporterDelegate.java (1),
       Subscriber.java (1)

3. Remove redundant cancellation checks before split()
   - SubMonitor.split() already checks for cancellation
   - Fixed in AutoBuildJob.java

Reference: https://www.eclipse.org/articles/Article-Progress-Monitors/article.html
@vogella vogella force-pushed the claude/fix-submonitor-usage-01TY1bqpmu4uYDXhMeewaN12 branch from f1091b3 to b39c458 Compare November 14, 2025 07:59
eclipse-platform-bot and others added 3 commits November 14, 2025 08:04
The previous implementation used SubMonitor.convert(monitor, ticks) which
internally calls beginTask(), causing "beginTask may only be called once"
errors when the returned monitor is passed to code that also calls beginTask().

The correct pattern is SubMonitor.convert(monitor).split(ticks), which creates
a child monitor without calling beginTask on it, allowing the caller to call
beginTask as needed.

This fixes test failures where FussyProgressMonitor detected multiple beginTask
calls (e.g., "beginTask may only be called once (old name=Opening 'MyProject'.)").

Files changed:
- resources/bundles/org.eclipse.core.resources/.../Policy.java
- team/bundles/org.eclipse.team.core/.../Policy.java
- team/bundles/org.eclipse.team.ui/.../Policy.java
- team/bundles/org.eclipse.jsch.core/.../Policy.java

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants