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

Enable Java terminal link provider on all terminals #1168

Merged
merged 1 commit into from
May 24, 2022

Conversation

testforstephen
Copy link
Contributor

This is a follow-up of PR microsoft/java-debug#414 and redhat-developer/vscode-java#2463.

Enabling Java terminal link providers on all terminals can also benefit the scenarios of running Maven command in VS Code terminal.

@CsCherrYY
Copy link
Contributor

Is there any potential issue if we remove this restriction? e.g. conflicts in terminals created by another extension.

If so, we can also keep this restriction for the known usages via a whitelist for example.

Copy link
Member

@Eskibear Eskibear left a comment

Choose a reason for hiding this comment

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

LGTM

@testforstephen
Copy link
Contributor Author

Yes, this is a good concern. VS Code supports returning a list of terminal links for each line. From our experience with existing providers, VS Code should merge results from different extensions.

See its API definition.
image

@Eskibear
Copy link
Member

Is there any potential issue if we remove this restriction? e.g. conflicts in terminals created by another extension.

If so, we can also keep this restriction for the known usages via a whitelist for example.

I think the restriction was designed so. But I prefer to remove the restriction as it doesn't seem to do any harm. We can revisit it if there's any case that links are overlap with popular extensions.

@testforstephen
Copy link
Contributor Author

Agree, we can roll out first. If there are any conflicts with other popular extensions, then we can limit its scope later.

@CsCherrYY
Copy link
Contributor

Agree. Let's remove this for now.

@testforstephen testforstephen added this to the 0.41.0 milestone May 24, 2022
@testforstephen testforstephen merged commit e1326ce into microsoft:main May 24, 2022
@testforstephen testforstephen deleted the jinbo_terminalLink branch May 24, 2022 08:53
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.

4 participants