Implement previously stubbed window#registerURIhandler (#13169) #13306
Conversation
fixes eclipse-theia#13169 contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
rschnekenbu
force-pushed
the
issues/13169
branch
from
7ea3fc1
to
98d8f0c
Compare
|
@tsmaeder, would you have time to review this one? |
|
@tsmaeder This would be good to have for the release |
rschnekenbu
changed the title
| } | ||
|
|
||
| @injectable() | ||
| export class ExtensionOpenHandler implements OpenHandler { |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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. |
| this.handlers.clear(); | ||
| } | ||
|
|
||
| async $registerUriHandler(handle: number, extensionId: string, extensionDisplayName: string): Promise<void> { |
There was a problem hiding this comment.
"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.
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(); |
| 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.
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.
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.
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.
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. |
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
no follow-up
Review checklist
Reminder for reviewers