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 contribute links to the terminal (aka link providers) #91290

Closed
Tyriar opened this issue Feb 24, 2020 · 30 comments · Fixed by #100496
Closed

Allow extensions to contribute links to the terminal (aka link providers) #91290

Tyriar opened this issue Feb 24, 2020 · 30 comments · Fixed by #100496
Assignees
Labels
api api-finalization feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders terminal Integrated terminal issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Feb 24, 2020

Currently the way terminal links work is by waiting until the viewport has stopped updating and then parsing the entire viewport. This has a few problems, most notable the performance cost of validating this every time (particularly bad on some environments, terminal.integrated.enableFileLinks was added to allow people to disable it) and this awfulness:

const lineAndColumnClause = [
'((\\S*)", line ((\\d+)( column (\\d+))?))', // "(file path)", line 45 [see #40468]
'((\\S*)",((\\d+)(:(\\d+))?))', // "(file path)",45 [see #78205]
'((\\S*) on line ((\\d+)(, column (\\d+))?))', // (file path) on line 8, column 13
'((\\S*):line ((\\d+)(, column (\\d+))?))', // (file path):line 8, column 13
'(([^\\s\\(\\)]*)(\\s?[\\(\\[](\\d+)(,\\s?(\\d+))?)[\\)\\]])', // (file path)(45), (file path) (45), (file path)(45,18), (file path) (45,18), (file path)(45, 18), (file path) (45, 18), also with []
'(([^:\\s\\(\\)<>\'\"\\[\\]]*)(:(\\d+))?(:(\\d+))?)' // (file path):336, (file path):336:9
].join('|').replace(/ /g, `[${'\u00A0'} ]`);

xterm.js just recently merged a new link provider API that allows moving away from this regex approach and instead resolving links when a hover happens. The PR to adopt this API is here #90336.

@connor4312 and I just wrote up this proposal as a starting point for allowing extensions to leverage this API. This has some really cool possibilities:

  • Language-specific handling, for example Node.js stack traces allowing to ctrl+click directly to node sources (eg. at Script.runInThisContext (vm.js:96:20))
  • Compiler-specific line/col formatting instead of core having to add each one explicitly
  • Auto-correcting CLIs based on suggestions
  • Handling of file links not relative to the initial cwd (this will probably be in core eventually)
  • Live share forwarding ports (instead of parsing all terminal output that it does now)
  interface LinkHoverContext {
    line: string;
    index: number;
    terminal: Terminal;
  }

  export interface TerminalLinkProvider {
    provideTerminalLink(context: LinkHoverContext): ProviderResult<TerminalLink>

    /**
     * Optionally provides custom handling logic for links returned from this
     * provider. This method can mutate the link `target`, or handle the
     * link internally. If a link is returned from this method, then VS
     * Code will try to open it using its default logic.
     */
    handleTerminalLink?(link: ResolvedTerminalLink): ProviderResult<ResolvedTerminalLink>;
  }

  interface TerminalLink {
    startIndex: number;
    length: number;

    /**
     * The uri this link points to. If set, and {@link TerminalLinkProvider.handlerTerminalLink}
     * is not implemented or returns false, then VS Code will try to open the Uri.
     */
    target?: Uri;

    /**
     * The tooltip text when you hover over this link.
     *
     * If a tooltip is provided, is will be displayed in a string that includes instructions on how to
     * trigger the link, such as `{0} (ctrl + click)`. The specific instructions vary depending on OS,
     * user settings, and localization.
     */
    tooltip?: string;
  }

  interface ResolvedTerminalLink {
    /**
     * The link text from the terminal, derived from the startIndex and length
     * returned in {@link TerminalLink}.
     */
    text: string;

    /**
     * The uri this link points to. If set in the link returned from {@link TerminalLinkProvider.handlerTerminalLink},
     * then VS Code will try to open it.
     */
    target?: Uri;

    /**
     * The tooltip text when you hover over this link.
     *
     * If a tooltip is provided, is will be displayed in a string that includes instructions on how to
     * trigger the link, such as `{0} (ctrl + click)`. The specific instructions vary depending on OS,
     * user settings, and localization.
     */
    tooltip?: string;
  }

Some notes/questions/unknowns:

  • How to handle conflicting links?
    • It looks like DocumentLinkProvider just overwrites links based on the most recently registered provider
    • Advise that implementers try match links as precisely/narrowly as possible
  • How would it work if Live Share used this API and js-debug did too?
    • Don't have an answer, the order being potentially inconsistent (based on activation time) is one problem. If it were consistent, js-debug could call into Live Share to get it to do the forwarding ahead of time, not ideal though.
  • Should this be generic to documents as well?
    • Since DocumentLinkProvider gets the links for the entire file this is slow for really large files and it does not allow validating links. An example of this is the GitHub issue links where #999999 would get a link that fails to open.
    • Files deal with ranges, the terminal doesn't
    • The downside of doing this is that we cannot show underlines until after hovering. The terminal never did this anyway as it would conflict with terminal underline attributes.
@Tyriar Tyriar added this to the March 2020 milestone Feb 24, 2020
@Tyriar Tyriar modified the milestones: March 2020, Backlog Feb 26, 2020
@Tyriar Tyriar added feature-request Request for new features or functionality terminal Integrated terminal issues labels Feb 26, 2020
@Tyriar Tyriar changed the title Allow extensions to contribute links to the terminal Allow extensions to contribute links to the terminal (aka link providers) Mar 18, 2020
@Tyriar
Copy link
Member Author

Tyriar commented Apr 14, 2020

@alexr00 this could be useful for GH:

image

I just clicked it instinctively even though I knew it was going to fail 😄

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

image

The new link system now lets you set hover labels, this will very likely come to the link provider API. eg. "Open on GitHub", "Open commit with in GitLens", "Launch debug browser".

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

Unknown: How do link providers conflicts resolve? If you have GitLens and GitHub PR installed and click on a commit, who wins? Should we show both in the hover in this case and ctrl+click lets you select the default/priority?

@jmbockhorst
Copy link
Contributor

Yeah, I think it should be left up to the user. Showing both providers on the hover popup could work well. Ctrl+click without a default set would force the user to use the popup and choose one. Maybe also a dialog when they click a default for the first time, something like: "Do you want to make this the default link handler for this type of link?". And also there should be a way to change/reset the defaults. This might be able to be directly on the hover popup (For Example: Highlight the default provider with an "x" to reset it), as long it isn't too cluttered.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

This might be able to be directly on the hover popup

I like this idea:

Open on GitHub (make default)
Open commit (ctrl + click)

That way you can action it both ways, change the click behavior right in the hover. I guess behind the scenes we'd just track the decision that extension A beats out extension B.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 30, 2020

If link providers has some additional context of the buffer they could support links like this.

Git diff:

image

eslint error

image

The tricky thing is how much context is it, and how does the extension access it?

@Tyriar
Copy link
Member Author

Tyriar commented May 5, 2020

Current xterm API:

https://github.com/xtermjs/xterm.js/blob/1168c6c52f49c7155094fe173ac037ea0e9ce8ac/typings/xterm.d.ts#L752-L758

This will change to this soon to avoid unconsistent links based on what is hovered first:

  interface ILinkProvider {
    provideLinks(position: IBufferCellPosition, callback: (links: ILink[] | undefined) => void): void;
  }

https://github.com/xtermjs/xterm.js/blob/1168c6c52f49c7155094fe173ac037ea0e9ce8ac/typings/xterm.d.ts#L1097-L1168

@Tyriar
Copy link
Member Author

Tyriar commented May 5, 2020

Idea: a contextLine which is a regex that will be looked at above the line to provide context. Example for the git and eslint examples about are /^+++/ and /^(?! )/ (might be wrong syntax, first line that doesn't start with 2 spaces).

Tyriar added a commit that referenced this issue Jun 24, 2020
Aligns more with other APIs

Part of #91290
@Tyriar Tyriar modified the milestones: June 2020, July 2020 Jun 29, 2020
@Tyriar
Copy link
Member Author

Tyriar commented Jul 1, 2020

Things to discuss in API call:

@DanTup
Copy link
Contributor

DanTup commented Jul 6, 2020

Consider making handleTerminalLink return a Promise #101391

Isn't this already handled? In the definition above, it's returning a ProviderResult<boolean> and ProviderResult includes Thenable?

The API looks like it should support what I want (Dart-Code/Dart-Code#2581 (comment)), though if there may be further changes (eg. returning a command - which if I understand correctly sounds like a good idea - allowing us to do things other than just open a file from clicks) I'll hold off until they're done before trying to implement against it in Insiders.

@connor4312
Copy link
Member

connor4312 commented Jul 6, 2020

  • The new API could provide the ability to cleanly have multiple actions for a single link. For example, being able to debug a URL via ctrl+click is a nice default, but I could provide a second link for the same text range to open a URL without debugging.
  • 👍 on returning a promise from handleTerminalLink
  • 👎 on making endIndex inclusive. String and array slicing in JS and (most?) other languages is non-inclusive at the end, I think it will be surprising to most implementors if the end index is inclusive.

Because of the possible changes to endIndex/length being inclusive (breaking) I won't push this to js-debug yet, but you can see my implementation here: https://github.com/microsoft/vscode-js-debug/compare/update-terminal-link-api

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2020

@DanTup the old proposal comment isn't up to date:

/**
* Handle an activated terminal link.
*/
handleTerminalLink(link: T): void;

@Tyriar
Copy link
Member Author

Tyriar commented Jul 14, 2020

Feedback from call:

  • Keep as is, don't return command
  • Use ProviderResult<void> as return, we may want to do something while waiting for resolve in the future (like show UI, block multiple calls, etc.)
  • Go back to length. While there is an existing [number, number] in the API for start/end(exclusive) in the API, it's a different form and considering the fact we would need to call out that it's exclusive anyway to differentiate it from the document link provider, let's just use length here.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 21, 2020

Looks like this will stay proposed this iteration and finalize next, please test it out and give feedback now before it's too late 😃

@testforstephen
Copy link

@Tyriar I tried the proposed API on the terminal output from Java Debugger, it works well.

Here is a demo to detect the Java exception stacktrace in the terminal.
output

Only one tiny feedback is to allow the provider show inline message when no result after clicking the link.

User Scenario:

In Java Debugger, it launches the Java application in the terminal by default. And many users expect the exception stacktrace printed to the terminal to be clickable and able to jump to the source file. This proposed API satisfies the feature pretty well. Thanks for this awesome API.

Regarding the implementation, we just use regular expression in the provideTerminalLinks API to check whether the current line looks like a stacktrace. This will keep the provideTerminalLinks fast and lightweight. And only when the user clicks the link, then searching the source mapping at the backend and jump to the file if there is result. Meanwhile, if no source found on clicking, we want to show some inline message to feedback user.

Expected:

Expect to display inline message inside the terminal, just like what the "go to definition" action did, where it displays an inline message inside the editor if no definition found.
image

@Tyriar
Copy link
Member Author

Tyriar commented Aug 10, 2020

@testforstephen great feedback thanks!

Only one tiny feedback is to allow the provider show inline message when no result after clicking the link.

The intent for the API is that if you provide a link, you must do something when it's clicked. I suggest reverting to the default behavior and trigger a quick open search for something relevant, in your case either a symbol search for doCreateBean, or a file search for AbstractAutowireCapableBeanFactory. This is even better than a no impl found message imo as it provides more information and potentially finds what the user was looking for anyway.

I'm not sure how you do this via the API exactly I'm sure it's possible though. Maybe running the command workbench.action.quickOpen or workbench.action.showAllSymbols with args if there is not an API in vscode.d.ts?

@Tyriar Tyriar closed this as completed in 6ad6379 Aug 10, 2020
@Tyriar
Copy link
Member Author

Tyriar commented Aug 10, 2020

Moved the API to stable and added a sample: microsoft/vscode-extension-samples@83261e3

@Tyriar
Copy link
Member Author

Tyriar commented Aug 31, 2020

This was tested last iteration, to verify run the new sample and make sure it works as expected.

@connor4312 connor4312 added the verified Verification succeeded label Sep 2, 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-finalization feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders terminal Integrated terminal issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@DanTup @jrieken @Tyriar @connor4312 @testforstephen @jmbockhorst and others