Skip to content

Adopt terminal link provider API#90336

Merged
Tyriar merged 20 commits intomicrosoft:masterfrom
jmbockhorst:linkProvider
Apr 12, 2020
Merged

Adopt terminal link provider API#90336
Tyriar merged 20 commits intomicrosoft:masterfrom
jmbockhorst:linkProvider

Conversation

@jmbockhorst
Copy link
Copy Markdown
Contributor

@jmbockhorst jmbockhorst commented Feb 10, 2020 •
edited
Loading

This adopts the new xterm.js link provider API and uses VS Code's shared link detection system. Added a new setting terminal.integrated.experimentalLinkProvider to control this behavior. Closes #90298.

Also, it was very complicated figuring out what was 0-based or 1-based and what to use where, so hopefully that can be cleared up with xtermjs/xterm.js#2706.

@jmbockhorst jmbockhorst requested a review from Tyriar February 10, 2020 01:38
Tyriar
Tyriar reviewed Feb 12, 2020
View reviewed changes
@jmbockhorst
Copy link
Copy Markdown
Contributor Author

jmbockhorst commented Feb 12, 2020 •
edited
Loading

I reverted the old changes and pushed a new solution that doesn't modify LinkComputer and still works as before. I used part of your idea so that the adapter handles the wrapped lines.

@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Feb 13, 2020

Merged in so xterm is on latest: b4c19fd

@Tyriar Tyriar mentioned this pull request Feb 24, 2020
Closed
@jmbockhorst
Copy link
Copy Markdown
Contributor Author

jmbockhorst commented Mar 13, 2020

I updated the branch and fixed the issues with widget hovering so that it works reliably.

@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Mar 13, 2020 •
edited
Loading

@jmbockhorst thanks and sorry about the delay, I was busy in the week I would try to merge (debt week) due to launching settings sync. Hopefully I'll finish this iteration's work early or get to it early April. I haven't forgotten 😃

@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Apr 11, 2020 •
edited
Loading

I merged from master and did my best to fix the conflicts but I'm seeing the activation not happen when cmd+clicking (tested on mac). See the activate logs in the gif below, the first one worked but the other 3 didn't:

ca72bf00-b39e-4561-b334-c1281a61cf7e

The reason was because _currentLink was undefined here:

https://github.com/xtermjs/xterm.js/blob/e2ac4b45d0747ae3d6ab3eb1e3ba9be7cbdec351/src/browser/Linkifier2.ts#L129

I also notice the link seems to get disposed when you click it, or just the rendering part of it anyway. We definitely want to keep the underline when clicked.

@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Apr 11, 2020

It could be related to this new feature if this wasn't happening before:

vscode/src/vs/workbench/contrib/terminal/browser/terminalLinkHandler.ts

Lines 254 to 275 in 433380c

// Allow the link to be intercepted if there are listeners
if (this._hasBeforeHandleLinkListeners) {
const wasHandled = await new Promise<boolean>(r => {
const timeoutId = setTimeout(() => {
canceled = true;
this._logService.error(`An extension intecepted a terminal link but it timed out after ${TerminalLinkHandler.LINK_INTERCEPT_THRESHOLD / 1000} seconds`);
r(false);
}, TerminalLinkHandler.LINK_INTERCEPT_THRESHOLD);
let canceled = false;
const resolve = (handled: boolean) => {
if (!canceled) {
clearTimeout(timeoutId);
r(handled);
}
};
this._onBeforeHandleLink.fire({ link, resolve });
});
if (!wasHandled) {
handler(link);
}
return;
}

@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Apr 11, 2020

Also can't seem to repro this in xterm.js' demo so not sure what's going on.

@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Apr 11, 2020

Ah I figured it out, "terminal.integrated.rendererType": "experimentalWebgl" does another renderer which clears the current link.

@Tyriar Tyriar mentioned this pull request Apr 11, 2020
Closed
@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Apr 11, 2020

Created xtermjs/xterm.js#2836

@jmbockhorst
Copy link
Copy Markdown
Contributor Author

jmbockhorst commented Apr 11, 2020

Are there still problems with link activation in the other renderers?

@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Apr 11, 2020

@jmbockhorst just webgl, will update vscode's xterm with the fix to xtermjs/xterm.js#2836 soon

Tyriar added a commit that referenced this pull request Apr 11, 2020
@Tyriar
[email protected]
4f4cf76 
Diff: xtermjs/xterm.js@e05bf87...aecfb57

Contains webgl fix for #90336
@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Apr 11, 2020

Ok that seems to have fixed it 🎉

@Tyriar Tyriar added this to the April 2020 milestone Apr 12, 2020
Tyriar added 2 commits April 12, 2020 08:19
@Tyriar
Consider wide characters in link positions
1db2966 
Currently when a cell is trimmed due to a wide char wrapping isn't
covered
@Tyriar
Cover case where a wide character lands on last cell in row
c12076a
@Tyriar
Copy link
Copy Markdown
Contributor

Tyriar commented Apr 12, 2020

Made some changes to cover wide characters including edge cases like this:

Screen Shot 2020-04-12 at 8 37 08 AM

Screen Shot 2020-04-12 at 8 37 20 AM

It beats out old link matchers in this respect now:

Screen Shot 2020-04-12 at 8 38 16 AM

Screen Shot 2020-04-12 at 8 38 20 AM

Tyriar
Tyriar approved these changes Apr 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Tyriar Tyriar left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI passed, merging. Thanks a lot for all the work here @jmbockhorst, this is much better than it was for web links, just need to close the functionality gap and we should be good to switch this over.

@Tyriar Tyriar merged commit 304fc63 into microsoft:master Apr 12, 2020
@Tyriar Tyriar mentioned this pull request Apr 12, 2020
Closed
@jmbockhorst
Copy link
Copy Markdown
Contributor Author

jmbockhorst commented Apr 12, 2020

Thank you for all of the help and guidance along the way!

@jmbockhorst jmbockhorst deleted the linkProvider branch April 12, 2020 18:59
@Tyriar Tyriar mentioned this pull request Apr 13, 2020
Closed
14 tasks
This was referenced Apr 13, 2020
Open
Open
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Tyriar Tyriar Tyriar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

April 2020

Development

Successfully merging this pull request may close these issues.

Adopt xterm.js link provider API

2 participants

@jmbockhorst @Tyriar