Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vscode: Make TerminalLink compatible to VS Code 1.64.2 #11522

Closed

Conversation

planger
Copy link
Contributor

@planger planger commented Aug 3, 2022

What it does

Replaces the out-dated definition of the TerminalLink interface with the current class that also provides a constructor.

In the course of fixing this, I discovered #11521. So it looks like Theia doesn't support registering TerminalLinkProvider at the moment. Anyway, making TerminalLink compatible to VS Code will help anyway, but only really gets effective once #11521 is fixed.

Fixes #11507

Contributed on behalf of STMicroelectronics.

How to test

There is nothing really to test as TerminalLink isn't used in the Theia code base anyway and now its definition is at least equivalent to how it is defined in VS Code 1.64.2.
Also I tested that this change makes the check in the vscode-theia-comparator happy.

Review checklist

Reminder for reviewers

Replaces the out-dated definition of the TerminalLink interface with the current class that also provides a constructor.

In the course of fixing this, I discovered eclipse-theia#11521.
So it looks like Theia doesn't support registering TerminalLinkProvider
at the moment. Anyway, making TerminalLink compatible to VS Code
will help anyway, but only really gets effective once eclipse-theia#11521 is fixed.

Fixes eclipse-theia#11507
Contributed on behalf of STMicroelectronics.

Change-Id: If183c73b8152770c4c9062355f92143f7e201e53
Signed-off-by: Philip Langer <[email protected]>
@planger planger force-pushed the planger/issues/11507 branch from 0f56515 to 7beb5ea Compare August 3, 2022 16:07
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@planger should we not update TerminalLink once #11521 is fixed? I don't see a strong reason to update them separately and would rather have full support.

@vince-fugnitto vince-fugnitto added terminal issues related to the terminal vscode issues related to VSCode compatibility labels Aug 3, 2022
@planger
Copy link
Contributor Author

planger commented Aug 3, 2022

@vince-fugnitto Sure, we can wait, if you prefer. Since terminal link providers of plugins that register a provider will never be called by Theia, I guess it doesn't make a difference whether TerminalLink that they would create is already 100 % compatible or not. Just thought, it doesn't take effort to already adjust this right away. But as you prefer... Should I close this PR and link #11507 in #11521?

@vince-fugnitto
Copy link
Member

@planger I'd prefer if we link the two yes, it gives a better representation that it is actually supported and they are tied together.

@planger
Copy link
Contributor Author

planger commented Aug 3, 2022

Closing this to wait for an actual implementation of the TerminalLinkProvider integration and registration, tracked by #11521.

@planger planger closed this Aug 3, 2022
@planger planger deleted the planger/issues/11507 branch August 3, 2022 17:23
@planger
Copy link
Contributor Author

planger commented Aug 10, 2022

This change is now part of #11552

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
terminal issues related to the terminal vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vscode] TerminalLink turned into a class with constructor
2 participants