-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Implement previously stubbed window#registerURIhandler (#13169) #13306
Conversation
7ea3fc1
to
98d8f0c
Compare
@tsmaeder, would you have time to review this one? |
@tsmaeder This would be good to have for the release |
} | ||
|
||
@injectable() | ||
export class ExtensionOpenHandler implements OpenHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmh...this is an extension mechanism to the OpenerService
, which itself already has an extension mechanism. IMO, it would make more sense to make OpenerService properly extensible.
|
||
readonly id = 'extensionsURIHandlers'; | ||
|
||
private providers = new Map<string, UriHandler>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it stores handlers, it should be called handlers
ideally handlersByAuthority
.
if (provider) { | ||
return provider.handleUri(uri); | ||
} | ||
return Promise.reject(`Impossible to handle ${uri}.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should reject with an error. This should not happen if canHandle
returns true.
const authority = uri.authority; | ||
const provider = this.providers.get(authority); | ||
if (provider) { | ||
return provider.handleUri(uri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation of OpenHandler
requires an opened widget or undefined
as the return value, not a boolean.
} | ||
|
||
public registerHandler(extensionId: string, handler: UriHandler): void { | ||
this.providers.set(extensionId, handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't talk about "extensionId" in the core package: plugin support might not be included in any particular Theia application. This handling needs to go into the UriMain
service.
|
||
} | ||
|
||
class ExtensionUriHandler implements UriHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must not talk about "Extensions" when we mean plugins. Extensions are NPM modules that extend Theia at compile time. This is confusing enough without mixing the terms in our source code.
const handler = this.handlers.get(handle); | ||
|
||
if (!handler) { | ||
return Promise.resolve(undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the undefined
.
try { | ||
handler.handleUri(URI.revive(uri)); | ||
} catch (err) { | ||
console.log(`error while handling external uri: ${uri}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not let the error propagate?
@rschnekenbu what I don't understand is where we hook up theia to be the system-wide URI handler for some uri's. Is this already hooked up somewhere? |
} | ||
|
||
@injectable() | ||
export class DefaultPluginUriHandlerService implements PluginUriHandlerService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why we need this class. Can't this code just live in the UriMainImpl
class?
I would like to build the release soon, what is the status here? |
The PR is under review again, not sure when it will be accepted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still a bunch of "extensions" left.
this.handlers.clear(); | ||
} | ||
|
||
async $registerUriHandler(handle: number, extensionId: string, extensionDisplayName: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"plugin"! It's probably just easiest to do a "find" on this PR page to find all occurrences of "extension".
} | ||
|
||
async open(uri: URI, options?: OpenerOptions | undefined): Promise<undefined> { | ||
if (this.canHandle(uri) === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this check is necessary: if we do not return an O.K.-value from canHandle()
, this method should never be called.
if (this.openerService.addHandler?.(extensionUriHandler)) { | ||
this.handlers.set(handle, extensionUriHandler); | ||
} | ||
return Promise.resolve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
if (this.canHandle(uri) === 0) { | ||
throw new Error(`Extension ${this.pluginName} is not supposed to handle this URI: ${uri}`); | ||
} | ||
await Promise.resolve(this.proxy.$handleExternalUri(this.handle, uri.toComponents())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just write await this.proxy.handle....
as the last line. No need to return a promise from an async method.
export class UriExtImpl implements UriExt { | ||
|
||
private static handle = 0; | ||
private handles = new Set<string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This set does not store handles, it stores the ids of the extensions that have already registered a handler. registeredExtensions
maybe?
|
||
$handleExternalUri(handle: number, uri: UriComponents): Promise<void> { | ||
const handler = this.handlers.get(handle); | ||
if (!handler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this throw an error? It sounds like this should never happen. In general, it's not a good idea to fail silently.
return Promise.resolve(); | ||
} | ||
handler.handleUri(URI.revive(uri)); | ||
return Promise.resolve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not necessary.
@rschnekenbu when I open the "run" dialog from the Windows start menu and type "theia://ryanleonhardt.open-external" Windows tells me there is not handler in the system for the "theia" scheme. Isn't this what the "sytem-wide" url handler (https://code.visualstudio.com/api/references/vscode-api#window) is supposed to do? When I open a "vscode://foo.bar" uri, VS Code handles it. |
b5bad35
to
3406d23
Compare
@msujew since @rschnekenbu and @tsmaeder both worked on this, we're look for a third party to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have time to test the changes yet, but the code looks mostly good. Just one issue see below.
I'll perform a final review tomorrow once I have tested everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, did some testing, things look good to me. Please fix the issue I've commented on above and feel free to merge afterwards.
fixes eclipse-theia#13169 contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <[email protected]>
What it does
This PR implements the stubbed API window#registerUriHandler(). This was not marked properly as '@stubbed', but the implementation returned nothing.
A detailed explanation on how this is supposed to be used in extensions is located here: https://github.com/microsoft/vscode/blob/1.85.1/src/vscode-dts/vscode.d.ts#L10146.
Fixes #13169
Contributed on behalf of STMicroelectronics
How to test
trigger the "Open External: Open external URI", then select "uri handler" menu. Nothing will happened
Update to this PR
The same sequence of actions will display a notification, as implemented by the uri handler provided by the test extension.
Follow-ups
theia://
links. theia-blueprint#378Review checklist
Reminder for reviewers