Skip to content

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Jul 25, 2025

This makes the old terminal bundles delegate to the new platform API.

@jonahgraham this is not complete yet, but would it be ok for CDT to host such legacy adapter?

@jonahgraham
Copy link
Member

@jonahgraham this is not complete yet, but would it be ok for CDT to host such legacy adapter?

Yes it is ok.

@laeubi
Copy link
Contributor Author

laeubi commented Jul 28, 2025

Ok then I'll go ahead... what leads to the question if we want the same for the terminal.control as the API from that seems also be used by e.g. the Copilote plugin...

@jonahgraham
Copy link
Member

If they want to test the use case then yes that would be great. But I think that is on you to decide how much effort you want to support other projects single-sourcing their dependencies on the terminal.

@laeubi laeubi force-pushed the implement_legacy_delegate branch from e939844 to 421c679 Compare July 29, 2025 05:19
@laeubi laeubi marked this pull request as ready for review July 29, 2025 05:20
@laeubi
Copy link
Contributor Author

laeubi commented Jul 29, 2025

@jonahgraham as we have other consumers as well (mentioned here) I think it will help in the migration.

This would mean that we have to restore also code from the terminal.control here, but this should be a different topic.

So given the build succeeds I would suggest to merge this so that consumers can start testing if they find any issues.

@laeubi laeubi force-pushed the implement_legacy_delegate branch 2 times, most recently from 3f5a716 to 872ce58 Compare July 31, 2025 05:49
Currently the old Terminal View is referenced in some other codebase
and we can not update them all at once.

To ease the transition this now do the following:

- deprecate the bundles to make consumer aware of the new terminal view
- implement a delegation where the old code calls redirect / transform
to the new ones
@laeubi laeubi force-pushed the implement_legacy_delegate branch from 872ce58 to a855486 Compare July 31, 2025 05:49
@jdneo
Copy link

jdneo commented Aug 15, 2025

Will this be included in 2025-09?

Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am removing my approval. I think this ends up breaking more than it helps.

I can't see how we aren't stuck with two terminal implementations until consumers of the API move to the org.eclipse.terminal API. Or until the delegations can work more completely.

For example, with the eclipse-copilot this change would cause eclipse-copilot to fall over.

Here is the reference in eclipse-copilot that uses this code https://github.com/eclipse-copilot/eclipse-copilot/blob/ace8b6bd64b05da7a73206070c00ea4c2ce79677/org.eclipse.copilot.ui/src/org/eclipse/copilot/ui/chat/tools/RunInTerminalTool.java

@jonahgraham jonahgraham self-requested a review August 18, 2025 17:49
@jonahgraham
Copy link
Member

To be explicit about the problem.

Because this changes IDs, properties set by extenders (e.g this in eclipse-copilot) causes the types to be incompatible.

eclipse-copilot sets PROP_STDOUT_LISTENERS to array of org.eclipse.tm.terminal.view.core.interfaces.ITerminalServiceOutputStreamMonitorListener but the terminal implementation tries to cast it (here) to array of org.eclipse.terminal.view.core.ITerminalServiceOutputStreamMonitorListener leading to a stacktrace like:

Caused by: java.lang.ClassCastException: class [Lorg.eclipse.tm.terminal.view.core.interfaces.ITerminalServiceOutputStreamMonitorListener; cannot be cast to class [Lorg.eclipse.terminal.view.core.ITerminalServiceOutputStreamMonitorListener; ([Lorg.eclipse.tm.terminal.view.core.interfaces.ITerminalServiceOutputStreamMonitorListener; is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @4cdd85f1; [Lorg.eclipse.terminal.view.core.ITerminalServiceOutputStreamMonitorListener; is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @22a1785d)
	at org.eclipse.terminal.connector.local.launcher.LocalLauncherDelegate.createTerminalConnector(LocalLauncherDelegate.java:350)
	at org.eclipse.terminal.view.ui.internal.TerminalService.createTerminalConnector(TerminalService.java:250)
	at org.eclipse.terminal.view.ui.internal.TerminalService.executeServiceOperation(TerminalService.java:186)
	at org.eclipse.terminal.view.ui.internal.TerminalService.openConsole(TerminalService.java:257)
	at org.eclipse.tm.terminal.view.core.TerminalServiceFactory$ITerminalServiceImplementation.openConsole(TerminalServiceFactory.java:63)
	at org.eclipse.copilot.ui.chat.tools.RunInTerminalTool.executeCommand(RunInTerminalTool.java:203)
	at org.eclipse.copilot.ui.chat.tools.RunInTerminalTool.invoke(RunInTerminalTool.java:111)
	at org.eclipse.copilot.ui.chat.services.AgentToolService.invokeTool(AgentToolService.java:185)

@laeubi
Copy link
Contributor Author

laeubi commented Aug 19, 2025

@jdneo this is currently only a draft (and it seems @jonahgraham has already found some issues). Also as described I'm not completely convinced it is the right aproach here.

So the main question would be if you are interested in helping improving and testin this aproach to make it more bullet proof or if we more should aim to getting things using the modernized API.

@laeubi laeubi closed this Aug 27, 2025
@jonahgraham jonahgraham added this to the 12.2.0 milestone Sep 12, 2025
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.

3 participants