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

Allow extensions to intercept and handle terminal links #91606

Closed
Tyriar opened this issue Feb 26, 2020 · 15 comments · Fixed by #92613
Closed

Allow extensions to intercept and handle terminal links #91606

Tyriar opened this issue Feb 26, 2020 · 15 comments · Fixed by #92613
Assignees
Labels
api api-proposal debt Code quality issues insiders-released Patch has been released in VS Code Insiders terminal General terminal issues that don't fall under another label
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Feb 26, 2020

Splitting out the handle part of this: #91290

This proposal will let js-debug for example handle and cancel bubbling of the handler, live share could handle it and not cancel.

@Tyriar Tyriar added feature-request Request for new features or functionality api terminal General terminal issues that don't fall under another label api-proposal labels Feb 26, 2020
@Tyriar Tyriar added this to the March 2020 milestone Feb 26, 2020
@Tyriar
Copy link
Member Author

Tyriar commented Mar 2, 2020

Proposal:

// Modeled after UriHandler

export namespace window {
	export function registerTerminalLinkHandler(handler: TerminalLinkHandler): Disposable;
}

export interface TerminalLinkHandler {
	/**
	 * @return true when the link was handled (and should not be considered by
	 * other providers including the default), false when the link was not handled.
	 */
	handleLink(terminal: Terminal, link: string): ProviderResult<boolean>;
}

@connor4312
Copy link
Member

It might be useful to allow the provider to return string | undefined so it can modify the link without needing to know how to open it. For instance maybe some company builds a "safe link" plugin like Outlook does where a link https://example.com would be converted to https://linkchecker.contoso.com?uri=https://example.com. Returning undefined would cancel the built-in link handling.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 2, 2020

@connor4312 that could just as easily be accomplished with the current proposal too with openExternal?

@connor4312
Copy link
Member

Oh, good point. Works for me then.

@jrieken
Copy link
Member

jrieken commented Mar 5, 2020

What kind of links are those and how are they generated? If you have control over the latter you could just generate a command-link (for a command that you own). Can we have a end to end sample?

@connor4312
Copy link
Member

The use case I want it for is allowing js-debug to automatically debug links printed in the terminal. For instance, say you have a brand new create-react-app application. You run npm start in the terminal, it says "listening on http://localhost:3000". You ctrl+click that link, and Chrome opens, automatically attaching a debugger and debugging your new React application with no further config.

In js-debug I might implement it something like this:

const handledProtocols = new Set(['http', 'https', 'file']);
vscode.window.registerTerminalLinkHandler({
  handleUri(terminal, link) {
    // Only handle links from debug terminals
    if (!isDebugTerminal(terminal)) {
      return false;
    }

    // Only handle http/https/file links
    const url = new URL(link);
    if (!handledProtocols.has(url.protocol)) {
      return false;
    }

    // For those links, launch them in a debuggable Chrome browser.
    const userDefaults = vscode.workspace.getConfiguration('debug.javascript.defaultLinkConfig');
    vscode.debug.startDebugging(vscode.workspace.workspaceFolders?.[0], {
      ...chromeLaunchConfigDefaults,
      ...userDefaults,
      url: link,
    });

    return true;
  }
});

@Tyriar
Copy link
Member Author

Tyriar commented Mar 5, 2020

@jrieken the how they are generated part was going to be handled by this API #91290, this proposal is splitting out the handling part such that extensions do not need to bother with link parsing and instead just handle/act upon the links if they want.

Also I notice I used handleUri as the callback, I've updated this as it could be any arbitrary string (eg. "filename", "./folder/file", or something else contributed by the eventual #91290 API.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 9, 2020

Notes from api sync:

  • Should we integrate the link protection dialog? eg. "Do you want js-debug to handle this link?". If we did this it would need to ask even if the link wasn't being used. Possible fixes:
    • Split handleLink into "handleLink" and "willHandleLink"?
    • Return a ProviderResult<function | undefined>
    • This may be overkill though?
  • Multiple extensions handling brings up the question of order. Not sure if we need to worry about this or if first come best dressed is fine.

@jrieken
Copy link
Member

jrieken commented Mar 10, 2020

Not sure if we need to worry about this or if first come best dressed is fine.

For language features we actually have LIFO because you can force yourself coming after a particular extension by defining a dependency.

Alternatively, and in combination with the protection dialog you could show a picker-like UX that allows me to select the extension. This is what we do for formatting.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 13, 2020

For the initial proposed API I am leaving off the link protection mechanism as it seems pretty awkward to do, for example if 2 extensions registered link handlers, when you click a link a dialog would show for each asking whether to allow that extension to consider this link, only after which it would trigger. I've also added a 3 second window to allow extensions to handle it before logging an error and triggering the default handler.

@Tyriar Tyriar linked a pull request Mar 13, 2020 that will close this issue
@jrieken jrieken reopened this Mar 13, 2020
@jrieken
Copy link
Member

jrieken commented Mar 13, 2020

Let's leave this open and drag it along the API discussion

@Tyriar
Copy link
Member Author

Tyriar commented Mar 27, 2020

Created test plan item for March, moving this to April for finalization.

@Tyriar Tyriar modified the milestones: March 2020, April 2020 Mar 27, 2020
@Tyriar Tyriar changed the title Allow extensions to intercept handle terminal links Allow extensions to intercept and handle terminal links Apr 6, 2020
@Tyriar Tyriar closed this as completed in 74ea30b Apr 9, 2020
Tyriar added a commit that referenced this issue Apr 20, 2020
@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

I've moved this back to proposed as I feel we may regret this after link providers #91290 come out.

Link providers definitely handle a link, which lets us give a nice label to the link (internal and from extensions):

image

However, link handlers may handle a particular link, so we cannot label it and ctrl+clicking may not accurately do what the hover says anymore. The one downside of going link providers only would be that extensions need to create their own link matchers probably via regex, and as such could miss cases. This isn't much of an issue for localhost links, but detecting file names for example is very complex and a link provider couldn't possible say they will definitely be able to handle something like that.

@Tyriar Tyriar reopened this Apr 20, 2020
@Tyriar
Copy link
Member Author

Tyriar commented Apr 21, 2020

Perhaps the next incarnation of this will just allow to override the default handling of folders/files/web links by providing a uri handler? We could then also have multiple actions in the hover as suggested in #91290 (comment)

@Tyriar Tyriar modified the milestones: April 2020, May 2020 Apr 27, 2020
@Tyriar Tyriar modified the milestones: May 2020, June 2020 Jun 1, 2020
@Tyriar Tyriar modified the milestones: June 2020, July 2020 Jun 29, 2020
@Tyriar Tyriar modified the milestones: July 2020, August 2020 Jul 30, 2020
@Tyriar
Copy link
Member Author

Tyriar commented Aug 10, 2020

I plan on removing this in this iteration. I believe js-debug was the only consumer which has since moved onto link providers.

@Tyriar Tyriar added debt Code quality issues and removed feature-request Request for new features or functionality labels Aug 10, 2020
@Tyriar Tyriar closed this as completed in 2b353aa Aug 10, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-proposal debt Code quality issues insiders-released Patch has been released in VS Code Insiders terminal General terminal issues that don't fall under another label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@jrieken @Tyriar @connor4312 and others