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

feat(plugin-ext): Fix instanceof check to handle VSCode URIs #13961

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

t1m0thyj
Copy link

What it does

Fixes #13949

How to test

Follow the steps to reproduce in the issue and verify that it's fixed.

Follow-ups

There are more places in the plugin-ext package that have an instanceof check for Theia URIs. I have restricted my changes to the scope of the file plugin-context.ts.

Review checklist

Reminder for reviewers

@msujew msujew added the vscode issues related to VSCode compatibility label Jul 25, 2024
@JonasHelming JonasHelming requested a review from msujew July 26, 2024 06:47
@JonasHelming JonasHelming requested review from tsmaeder and removed request for msujew August 7, 2024 06:38
@tsmaeder
Copy link
Contributor

tsmaeder commented Aug 9, 2024

Hi @t1m0thyj, thanks for the PR. I'm afraid I'll have to disagree with my esteemed colleague @msujew: the instanceof check is for the API-class that functions as the declared (theia|vscode).URI class and I think it's correct. Plugins must pass in an instance of that class when invoking showTextDocument(). That class is currently based on the VS Code-internal URI class, but that is purely accidental. We already have a mechanism for converting command arguments to their proper types: CommandRegistryImpl.registerArgumentProcessor(). We cold implement an argument processor that converts VS Code-internal URI's to proper API URIs and register it in the constructor. VS Code extensions can never check for the VS Code-internal class, so we're safe there. The only way we could have breakage is if a registered command would invoke a built-in command with one of it's arguments, but with the current implementation based on the VS Code URI, it would still work.

@tsmaeder
Copy link
Contributor

tsmaeder commented Aug 9, 2024

@t1m0thyj btw: in order to contribute to this project, you'll have to sign the committer agreement with the Eclipse foundation for the email address used to sign the commits. There's a link to the signature page on the right column here: https://api.eclipse.org/git/eca/status/gh/eclipse-theia/theia/13961

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

showTextDocument doesn't work with URI argument from editor/context menu command
3 participants