IEP-1672 Replace outdated tm.terminal plugin #1378
Conversation
WalkthroughReplaces legacy org.eclipse.tm.* terminal APIs with org.eclipse.terminal.* across manifests, plugin.xml and imports; converts launcher delegates to CompletableFuture-based async execution; switches SettingsStore → InMemorySettingsStore and preference access to Platform; updates connector base classes and UI imports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Caller (Launch / UI)
participant Launcher as LauncherDelegate.execute()
participant Service as TerminalService
participant Terminal as Terminal UI
UI->>Launcher: execute(properties) → CompletableFuture<?>
Launcher->>Service: getTerminalService()
alt service available
Launcher->>Service: openConsole(properties)
Service-->>Launcher: success (console handle)
Launcher-->>UI: CompletableFuture.completedFuture(result)
Launcher->>Terminal: open/create terminal tab
else service failure / exception
Service--x Launcher: throws
Launcher-->>UI: CompletableFuture.failedFuture(exception)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bundles/com.espressif.idf.launch.serial.core/META-INF/MANIFEST.MF (1)
20-30: Inconsistent package references between Require-Bundle and Import-Package.The manifest mixes old and new API references:
Require-Bundleuses the neworg.eclipse.terminal.view.core(line 20)Import-Packageuses the oldorg.eclipse.tm.terminal.view.coreandorg.eclipse.tm.terminal.view.ui.launcher(lines 29-30)This inconsistency may cause resolution failures or runtime issues. Either update
Import-Packageto use the neworg.eclipse.terminal.*packages, or clarify if this mixed usage is intentional for backward compatibility.🔎 Suggested fix to align Import-Package with new API
Import-Package: org.eclipse.cdt.dsf.gdb, - org.eclipse.tm.terminal.view.core, - org.eclipse.tm.terminal.view.ui.launcher + org.eclipse.terminal.view.core, + org.eclipse.terminal.view.ui.launcherbundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java (1)
72-90: Null connector returned on exception may cause issues downstream.When
TerminalConnectorExtension.makeTerminalConnector()throws aCoreException, the method logs the error but returnsnull. Callers ofcreateTerminalConnector()may not expect a null result, which could lead toNullPointerExceptionlater.Consider whether to propagate the exception or document the null return contract.
🔎 Alternative: Throw a runtime exception to fail fast
try { connector = TerminalConnectorExtension.makeTerminalConnector(connectorId); } catch (CoreException e) { Logger.log(e); + throw new IllegalStateException("Failed to create terminal connector: " + connectorId, e); } - if (connector != null) - { + if (connector != null) { // Apply default settings connector.setDefaultSettings(); // And load the real settings connector.load(store); }
🧹 Nitpick comments (6)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/activator/UIPlugin.java (1)
63-68: Race condition in lazy initialization.The
volatilekeyword onscopedPreferencesdoes not make the check-then-act pattern thread-safe. Two threads could simultaneously seenulland both create new instances.While this is unlikely to cause visible issues with
ScopedPreferenceStore, consider using synchronized initialization or the initialization-on-demand holder pattern for correctness.🔎 Thread-safe lazy initialization
public static ScopedPreferenceStore getScopedPreferences() { + ScopedPreferenceStore result = scopedPreferences; + if (result == null) { + synchronized (UIPlugin.class) { + result = scopedPreferences; - if (scopedPreferences == null) { - scopedPreferences = new ScopedPreferenceStore(InstanceScope.INSTANCE, getUniqueIdentifier()); + if (result == null) { + scopedPreferences = result = new ScopedPreferenceStore(InstanceScope.INSTANCE, getUniqueIdentifier()); + } + } } - return scopedPreferences; + return result; }bundles/com.espressif.idf.terminal.connector/META-INF/MANIFEST.MF (1)
28-28: Automatic-Module-Name uses outdated naming.The
Automatic-Module-Nameis set toorg.eclipse.tm.terminal.connector.local, which references the old TM namespace. For consistency with the migration, consider updating this to reflect the new package structure or the actual bundle identity (com.espressif.idf.terminal.connector).🔎 Suggested fix
-Automatic-Module-Name: org.eclipse.tm.terminal.connector.local +Automatic-Module-Name: com.espressif.idf.terminal.connectorbundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (2)
159-163: Consider broader exception handling.The catch block only handles
RuntimeException. IfgetTerminalService()returnsnull, aNullPointerExceptionwill be caught, but other exception types fromopenConsole()may not be handled. Consider catching a broader exception type or adding null-checking.🔎 Proposed improvement
// Get the terminal service try { + var terminalService = getTerminalService(); + if (terminalService == null) { + return CompletableFuture.failedFuture(new IllegalStateException("Terminal service unavailable")); + } - return getTerminalService().openConsole(properties); + return terminalService.openConsole(properties); -} catch (RuntimeException e) { +} catch (Exception e) { return CompletableFuture.failedFuture(e); }
109-110: Extract repeated plugin ID to a constant.The string
"org.eclipse.terminal.view.ui"is hardcoded in multiple places (lines 109, 207, 242). Consider extracting it to a private constant for maintainability.🔎 Suggested refactor
private static final String TERMINAL_VIEW_UI_PLUGIN_ID = "org.eclipse.terminal.view.ui"; //$NON-NLS-1$Then use
TERMINAL_VIEW_UI_PLUGIN_IDin all three locations.bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java (1)
229-229: Consider handling the returnedCompletableFuturefor error logging.The
execute()method now returns aCompletableFuture<?>, but its result is discarded. While fire-and-forget may be intentional here, any errors from opening the serial monitor will be silently lost.🔎 Optional: Add error handling for the async result
- new SerialLauncherDelegate().execute(map); + new SerialLauncherDelegate().execute(map) + .exceptionally(ex -> { + Logger.log(ex); + return null; + });bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java (1)
109-116: Consider catchingExceptioninstead of justRuntimeException.The
getTerminalService().openConsole()chain may throw checked exceptions (depending on the implementation). Catching onlyRuntimeExceptioncould allow other exceptions to propagate unexpectedly.🔎 Proposed fix to catch broader exceptions
try { return getTerminalService().openConsole(properties); } - catch (RuntimeException e) + catch (Exception e) { + Logger.log(e); return CompletableFuture.failedFuture(e); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
bundles/com.espressif.idf.launch.serial.core/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java(2 hunks)bundles/com.espressif.idf.terminal.connector.serial/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.terminal.connector.serial/plugin.xml(1 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/activator/Activator.java(3 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialConnector.java(1 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java(1 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialSettings.java(1 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialConfigPanel.java(1 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java(2 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java(4 hunks)bundles/com.espressif.idf.terminal.connector/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.terminal.connector/plugin.xml(1 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/activator/UIPlugin.java(3 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.java(1 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java(9 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleMementoHandler.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T18:07:42.470Z
Learnt from: sigmaaa
Repo: espressif/idf-eclipse-plugin PR: 1339
File: bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java:27-34
Timestamp: 2025-10-27T18:07:42.470Z
Learning: In the file bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java, the NvsEditorHandler is tied to a project explorer right-click command, so EclipseUtil.getSelectedProjectInExplorer() will never return null due to the command's enablement conditions.
Applied to files:
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java
🧬 Code graph analysis (2)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EclipseUtil.java (1)
EclipseUtil(36-186)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFProjectNature.java (1)
IDFProjectNature(16-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_macos
- GitHub Check: build_windows
- GitHub Check: build_linux
🔇 Additional comments (16)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java (1)
12-12: Migration from deprecated TM API to public connector API looks correct; ensure the new TerminalState enum has the required values.The import change from
org.eclipse.tm.internal.terminal.provisional.api.TerminalStatetoorg.eclipse.terminal.connector.TerminalStatemigrates away from the deprecated internal/provisional API. The code usesCONNECTING,CLOSED, andCONNECTEDenum values. Please verify that the new API provides these same enum values to ensure compatibility.bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java (2)
36-38: LGTM! Import migration aligns with PR objective.The import statements have been correctly updated from the deprecated Eclipse TM internal API to the modern public Terminal API, which aligns with the PR's goal of migrating away from
tm.terminal.
104-136: Remove duplicated nature-checking logic and add exception logging.The fallback block (lines 114-129) re-implements the exact nature-checking and error-handling that
getSelectedIDFProjectInExplorer()already performs. Since that method checksproject.hasNature(IDFProjectNature.ID)and catchesCoreException, the duplicate validation is unnecessary.Additionally, the generic
Exceptioncatch at line 132-135 swallows errors silently without logging, making production issues harder to diagnose. Add logging to capture unexpected exceptions, or simplify the error handling by removing the duplicated fallback logic if backward compatibility with older Eclipse versions is no longer required.⛔ Skipped due to learnings
Learnt from: sigmaaa Repo: espressif/idf-eclipse-plugin PR: 1339 File: bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java:27-34 Timestamp: 2025-10-27T18:07:42.470Z Learning: In the file bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java, the NvsEditorHandler is tied to a project explorer right-click command, so EclipseUtil.getSelectedProjectInExplorer() will never return null due to the command's enablement conditions.bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleMementoHandler.java (1)
18-18: Import path updated correctly.The import path change from
org.eclipse.tm.terminal.view.ui.interfaces.IMementoHandlertoorg.eclipse.terminal.view.ui.IMementoHandlerreflects the new API's package structure (note the removal of theinterfacessubpackage).bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialConfigPanel.java (1)
22-24: LGTM!The import migration is consistent with the pattern used across other configuration panels in this PR, correctly updating to the new
org.eclipse.terminal.view.*package paths.bundles/com.espressif.idf.terminal.connector/META-INF/MANIFEST.MF (1)
17-21: Bundle dependencies correctly updated to new Terminal API.The manifest properly declares dependencies on the new
org.eclipse.terminal.*packages with appropriate version ranges[1.0.0,2.0.0).bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (2)
367-367: Verify InMemorySettingsStore is the intended choice.Using
InMemorySettingsStoremeans terminal connector settings will not persist across sessions. If the previous implementation used a persistent store, this is a behavioral change.Is the switch to in-memory (non-persistent) settings intentional? If terminal settings should persist, consider using a file-backed or preference-backed settings store.
391-404: Good error handling for connector creation.The try-catch properly handles
CoreExceptionduring connector creation, and the null check (line 397) ensures the connector is only configured if successfully created. Note that callers must handle anullreturn value when connector creation fails.bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.java (1)
29-31: Import migration is correct and aligned with the Eclipse 2025-09 Terminal API transition.The Eclipse Terminal migration from CDT to the Eclipse Platform project in 2025-09 introduced the new
org.eclipse.terminal.view.*package structure. The imports have been correctly updated from the legacyorg.eclipse.tm.terminal.view.*packages to theirorg.eclipse.terminal.view.*equivalents. No functional code changes are present—only the package namespace updates necessary for API compatibility with the new platform ownership.bundles/com.espressif.idf.terminal.connector.serial/plugin.xml (1)
16-30: LGTM!The extension point migrations from
org.eclipse.tm.terminal.*toorg.eclipse.terminal.*are correctly updated, and thelabelattribute addition to the launcher delegate aligns with the new API surface.bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java (1)
54-54: LGTM!Import path correctly updated to the new Terminal API package.
bundles/com.espressif.idf.terminal.connector.serial/META-INF/MANIFEST.MF (1)
18-21: LGTM!The dependency migration from
org.eclipse.tm.terminal.*bundles toorg.eclipse.terminal.*is correctly implemented with appropriate version constraints[1.0.0,2.0.0)to ensure major version compatibility.bundles/com.espressif.idf.terminal.connector/plugin.xml (1)
7-22: LGTM!Extension points and connector class references are correctly migrated to the new
org.eclipse.terminal.*packages, consistent with the serial connector plugin.xml changes.bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java (1)
16-29: LGTM!Import migrations to the new
org.eclipse.terminal.*packages are correctly implemented, including the newCompletableFutureandInMemorySettingsStoreusage.bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialConnector.java (1)
24-31: Verify the AbstractTerminalConnector API migrationThe review assumes a migration from
TerminalConnectorImpltoAbstractTerminalConnectorwith correct method signatures forconnect()anddoDisconnect(). However, the specific API details ofAbstractTerminalConnectorcould not be independently verified through available documentation. Confirm that the class hierarchy, method signatures, and inheritance pattern are correct for the Eclipse terminal connector framework version being used.bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialSettings.java (1)
18-18: API migration to public Eclipse Terminal Connector API is correct, but verify package exports.The migration from the internal/provisional TM API to the public Eclipse Terminal Connector API is a positive change. The new
ISettingsStoreinterface provides the requiredget(String, String)andput(String, String)methods used by theload()andsave()methods.However, the manifest uses
Require-Bundlefor terminal packages rather than explicitly declaringImport-Package: org.eclipse.terminal.connector. Verify that the required packages are properly exported by the bundles declared inRequire-Bundle(specificallyorg.eclipse.terminal.view.core,org.eclipse.terminal.view.ui, ororg.eclipse.terminal.control), or add an explicitImport-Package: org.eclipse.terminal.connectordeclaration with appropriate version constraints for clarity.
| import org.eclipse.jface.resource.ImageRegistry; | ||
| import org.eclipse.swt.graphics.Image; | ||
| import org.eclipse.tm.terminal.view.core.tracing.TraceHandler; | ||
| import org.eclipse.terminal.view.ui.internal.TraceHandler; |
There was a problem hiding this comment.
Use public Eclipse tracing API instead of internal packages.
The import org.eclipse.terminal.view.ui.internal.TraceHandler references an internal package. Internal packages are subject to change or removal without notice. Use the public DebugTrace API from org.eclipse.osgi.service.debug, which records debug trace statements and automatically inserts contextual information such as bundle, class, and method, or use Platform.getDebugOption() for simpler debug flag checking.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/activator/Activator.java
around line 21, the code imports the internal class
org.eclipse.terminal.view.ui.internal.TraceHandler which must be replaced with a
public tracing API; remove the internal import and switch to using the public
org.eclipse.osgi.service.debug.DebugTrace obtained from the OSGi DebugOptions
service (or, for a simpler flag check, use Platform.getDebugOption), update
calls that used TraceHandler to call DebugTrace.trace or check
Platform.getDebugOption() accordingly, and register/obtain the DebugTrace
instance via the bundle context/DebugOptions service so debug output includes
bundle/class/method context and avoids using internal packages.
| public static TraceHandler getTraceHandler() | ||
| { | ||
| if (traceHandler == null) | ||
| { | ||
| traceHandler = new TraceHandler(getUniqueIdentifier()); | ||
| } | ||
| return traceHandler; | ||
| } |
There was a problem hiding this comment.
Thread-safety issue in lazy initialization.
The getTraceHandler() method has a check-then-act race condition. Multiple threads could simultaneously see traceHandler == null and create multiple instances, despite the volatile modifier.
🔎 Proposed fix using double-checked locking
+ private static final Object TRACE_HANDLER_LOCK = new Object();
private static volatile TraceHandler traceHandler;
public static TraceHandler getTraceHandler()
{
if (traceHandler == null)
{
- traceHandler = new TraceHandler(getUniqueIdentifier());
+ synchronized (TRACE_HANDLER_LOCK)
+ {
+ if (traceHandler == null)
+ {
+ traceHandler = new TraceHandler(getUniqueIdentifier());
+ }
+ }
}
return traceHandler;
}| import org.eclipse.swt.graphics.Image; | ||
| import org.eclipse.tm.terminal.view.core.preferences.ScopedEclipsePreferences; | ||
| import org.eclipse.tm.terminal.view.core.tracing.TraceHandler; | ||
| import org.eclipse.terminal.view.ui.internal.TraceHandler; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "UIPlugin.java" -path "*/com.espressif.idf.terminal.connector/*" | head -5Repository: espressif/idf-eclipse-plugin
Length of output: 183
🏁 Script executed:
cat -n ./bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/activator/UIPlugin.java | head -80Repository: espressif/idf-eclipse-plugin
Length of output: 3173
🏁 Script executed:
rg "getTraceHandler" --type java -A 3 -B 1Repository: espressif/idf-eclipse-plugin
Length of output: 1481
🏁 Script executed:
rg "\.getTraceHandler\(\)" --type java -A 2 -B 1Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
rg "TraceHandler" --type java -B 2 -A 2Repository: espressif/idf-eclipse-plugin
Length of output: 5514
🏁 Script executed:
rg "\.trace\(|\.isSlotEnabled\(" --type javaRepository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
rg "traceHandler" --type javaRepository: espressif/idf-eclipse-plugin
Length of output: 1470
🏁 Script executed:
rg "traceHandler = null" --type java -B 3 -A 1Repository: espressif/idf-eclipse-plugin
Length of output: 772
Remove unused internal API dependency.
org.eclipse.terminal.view.ui.internal.TraceHandler is imported but never used in the codebase. Remove the unused import, the traceHandler field (line 33), the getTraceHandler() method (lines 75-79), and the cleanup in stop() (line 104).
If tracing functionality is needed in the future, use the public Eclipse tracing API via Platform.getDebugOption() or the OSGi DebugOptions service instead of internal packages.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/activator/UIPlugin.java
around lines 19 and 33,75-79,104: the code imports the internal class
org.eclipse.terminal.view.ui.internal.TraceHandler and defines an unused
traceHandler field, a getTraceHandler() method, and a stop() cleanup referencing
it; remove the unused import, delete the traceHandler field declaration, remove
the getTraceHandler() method, and remove the traceHandler-related cleanup from
stop(); if tracing is needed later, replace with Platform.getDebugOption() or
the OSGi DebugOptions service rather than internal APIs.
| public CompletableFuture<?> execute(Map<String, Object> properties) { | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "IDFConsoleLauncherDelegate.java" -type fRepository: espressif/idf-eclipse-plugin
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Find the file
find . -name "IDFConsoleLauncherDelegate.java" -type f 2>/dev/null
# Search for calls to execute method on IDFConsoleLauncherDelegate
rg -n "execute\s*\(" --type=java -C 3 | head -100Repository: espressif/idf-eclipse-plugin
Length of output: 10543
🏁 Script executed:
cat -n bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.javaRepository: espressif/idf-eclipse-plugin
Length of output: 20017
🏁 Script executed:
rg -n "IDFConsoleLauncherDelegate" --type=java -C 5Repository: espressif/idf-eclipse-plugin
Length of output: 5973
🏁 Script executed:
rg -n "\.execute\s*\(" --type=java -B 5 -A 3 | grep -E "(execute|IDFConsole|AbstractLauncherDelegate)" | head -50Repository: espressif/idf-eclipse-plugin
Length of output: 3550
🏁 Script executed:
rg -n "execute\s*\(properties\)" --type=java -C 5Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
# Search for SerialLauncherDelegate and IDFConsoleLauncherDelegate callers
rg -n "SerialLauncherDelegate" --type=java -B 3 -A 3
# Also search for the parent class to understand the interface
find . -name "AbstractLauncherDelegate.java" -type fRepository: espressif/idf-eclipse-plugin
Length of output: 4432
🏁 Script executed:
find . -name "AbstractLauncherDelegate.java" -type f -exec cat {} \;Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
# Check the SerialLauncherDelegate.execute method signature
rg -n "public.*execute" bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java -A 5Repository: espressif/idf-eclipse-plugin
Length of output: 298
🏁 Script executed:
# Search for any references to "Done" callback pattern in the codebase
rg -n "Done" --type=java bundles/com.espressif.idf.terminal.connector/ | head -20
# Check if AbstractLauncherDelegate defines the interface
find . -name "AbstractLauncherDelegate.java" -type fRepository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
# Search more broadly for "Done" in the entire codebase
rg -n "\bDone\b" --type=java | head -30
# Also check git history for the method signature change
git log -p --all -S "Done" -- "**IDFConsoleLauncherDelegate.java" | head -100Repository: espressif/idf-eclipse-plugin
Length of output: 711
🏁 Script executed:
# Check the full context around SerialFlashLaunchConfigDelegate line 229
sed -n '220,235p' bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java
# Also check if there are other callers that might not be handling the return properly
rg -n "\.execute\(map\)" --type=java -B 2 -A 2Repository: espressif/idf-eclipse-plugin
Length of output: 1711
🏁 Script executed:
# Check if there are any other similar callers that might be ignoring the return value
rg -n "new SerialLauncherDelegate\(\)\.execute" --type=java -B 3 -A 3
rg -n "new IDFConsoleLauncherDelegate\(\)\.execute" --type=java -B 3 -A 3
# Check if there are any direct usages that handle the future properly
rg -n "\.execute\(.*\)" --type=java bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/ -A 3Repository: espressif/idf-eclipse-plugin
Length of output: 1350
🏁 Script executed:
# Look at the full context of SerialFlashLaunchConfigDelegate to understand what it's doing
sed -n '215,240p' bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java
# Check if there are other implementations of AbstractLauncherDelegate
find . -name "*.java" -type f -path "*/launcher/*" | xargs grep -l "extends AbstractLauncherDelegate" 2>/dev/nullRepository: espressif/idf-eclipse-plugin
Length of output: 1399
🏁 Script executed:
sed -n '200,235p' bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.javaRepository: espressif/idf-eclipse-plugin
Length of output: 1323
🏁 Script executed:
# Final check: Look for any git history showing the "Done" parameter
git log --all --oneline -- "bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java" | head -20
# Also verify the return type more clearly
rg -n "CompletableFuture" bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.javaRepository: espressif/idf-eclipse-plugin
Length of output: 1492
🏁 Script executed:
# Check the actual commit that changed the method signature
git show 959a1252 --stat | head -30Repository: espressif/idf-eclipse-plugin
Length of output: 1751
🏁 Script executed:
# Get the actual diff of the IDFConsoleLauncherDelegate changes in that commit
git show 959a1252 -- bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java | head -300Repository: espressif/idf-eclipse-plugin
Length of output: 13199
Method signature uses CompletableFuture for asynchronous terminal launch.
The execute() method returns CompletableFuture<?> to handle asynchronous console operations. However, at least one caller (SerialFlashLaunchConfigDelegate line 229) ignores the returned future without handling its completion or potential exceptions. Callers should chain completion handlers or store the future for monitoring.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java
around lines 78-79, the execute(Map<String,Object>) method returns
CompletableFuture<?> but at least one caller (SerialFlashLaunchConfigDelegate
line 229) ignores the returned future; either ensure the future is observed or
change the API to manage errors internally. Fix options: a) keep returning
CompletableFuture but change the generic to CompletableFuture<Void> for clarity
and update all callers (e.g., SerialFlashLaunchConfigDelegate) to attach a
completion handler (whenComplete / exceptionally) that logs errors and handles
cleanup so exceptions aren’t silently dropped; or b) make execute return void
and internally handle all async errors by adding whenComplete/exceptionally to
the internal future and logging/propagating necessary state, so callers need not
observe the future. Apply one approach consistently and add unit/usage comments
showing callers must handle or that errors are already handled.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (1)
79-80: Duplicate: CompletableFuture return type requires caller handling.This issue was previously flagged in an earlier review. The
execute()method returnsCompletableFuture<?>but at least one caller (SerialFlashLaunchConfigDelegateline 229) ignores the returned future. Please address the previous review comment by either:
- Ensuring all callers attach completion handlers (
whenComplete/exceptionally) to handle errors, or- Making
executereturnvoidand handling all async errors internally
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java
🧰 Additional context used
🧬 Code graph analysis (1)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_linux
- GitHub Check: build_macos
🔇 Additional comments (5)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (5)
25-25: LGTM: Import changes support API migration.The imports have been correctly updated to use the newer
org.eclipse.terminal.*APIs instead of legacyorg.eclipse.tm.*APIs. TheCompletableFutureandLoggerimports support the async execution model and improved error handling.Also applies to: 36-48, 53-53
64-64: LGTM: Centralized bundle name constant.Using a constant for the bundle name improves maintainability and reduces the risk of typos in the multiple preference lookups throughout the class.
110-111: LGTM: Platform-based preference access.The migration from UIPlugin-based preferences to
Platform.getPreferencesService()is the correct approach for decoupled preference access. This aligns with Eclipse best practices and the broader terminal API migration.Also applies to: 208-209, 243-244
160-164: LGTM: Appropriate async error handling.The try/catch block correctly handles synchronous exceptions by returning a failed
CompletableFuture, which is the proper pattern for async execution. This ensures that errors during console initialization are propagated through the future chain.
368-368: InMemorySettingsStore is the appropriate choice for this launcher delegate.The change from
SettingsStore()toInMemorySettingsStore()is intentional and follows the established pattern in this codebase. Launcher delegates construct settings from runtime parameters (image, process, environment, etc.), save them to a transient store, and immediately apply them to the connector. Settings do not need to persist across sessions because each terminal launch receives fresh parameters that regenerate the configuration. This pattern is consistently used in bothIDFConsoleLauncherDelegateandSerialLauncherDelegate.
| ITerminalConnector connector = null; | ||
| try { | ||
| connector = TerminalConnectorExtension.makeTerminalConnector(connectorId); | ||
| } catch (CoreException e) { | ||
| Logger.log(e); | ||
| } |
There was a problem hiding this comment.
Critical: Silent failure with null connector return.
When TerminalConnectorExtension.makeTerminalConnector() throws CoreException, the exception is logged but the method continues and returns null (line 405). This can cause NullPointerException in callers that don't expect a null connector.
Consider either:
- Re-throwing the exception or wrapping it in a
RuntimeExceptionto fail fast - Returning a non-null connector that displays an error message in the terminal
- At minimum, adding a warning log message indicating that a null connector is being returned
🔎 Proposed fix to propagate the exception
// Construct the terminal connector instance
-ITerminalConnector connector = null;
-try {
- connector = TerminalConnectorExtension.makeTerminalConnector(connectorId);
-} catch (CoreException e) {
- Logger.log(e);
-}
+ITerminalConnector connector;
+try {
+ connector = TerminalConnectorExtension.makeTerminalConnector(connectorId);
+} catch (CoreException e) {
+ Logger.log(e);
+ throw new RuntimeException("Failed to create terminal connector: " + connectorId, e);
+}
if (connector != null) {
// Apply default settings
connector.setDefaultSettings();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ITerminalConnector connector = null; | |
| try { | |
| connector = TerminalConnectorExtension.makeTerminalConnector(connectorId); | |
| } catch (CoreException e) { | |
| Logger.log(e); | |
| } | |
| ITerminalConnector connector; | |
| try { | |
| connector = TerminalConnectorExtension.makeTerminalConnector(connectorId); | |
| } catch (CoreException e) { | |
| Logger.log(e); | |
| throw new RuntimeException("Failed to create terminal connector: " + connectorId, e); | |
| } | |
| if (connector != null) { | |
| // Apply default settings | |
| connector.setDefaultSettings(); | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java
around lines 392-397, the code catches CoreException, logs it and then continues
returning null which can cause NPEs; replace the silent swallow with a fail-fast
behavior by wrapping and rethrowing the exception (e.g. throw new
IllegalStateException("Failed to create terminal connector: "+connectorId, e))
so callers never receive null, or alternatively implement and return a non-null
fallback ITerminalConnector that writes a clear error to the terminal and log a
warning before returning; ensure you remove the silent log-only handling and
either throw the wrapped exception or return the fallback connector.
sigmaaa
changed the title
|
@sigmaaa hi ! Tested under: after plugin installation there is only 1 Terminal button ✅ |
alirana01
left a comment
alirana01
left a comment
There was a problem hiding this comment.
LGTM a proper testing should be done as this may affect multiple areas of the application
Description
Replace the outdated tm.terminal plugin to avoid terminal duplicates in the IDE
Fixes # (IEP-1672)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test serial monitor and esp-idf terminal in the Espressif-IDE/plugin
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.