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

Adopt terminal link provider API #90336

Merged
merged 20 commits into from
Apr 12, 2020
Merged

Conversation

jmbockhorst
Copy link
Contributor

@jmbockhorst jmbockhorst commented Feb 10, 2020

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
Copy link
Contributor Author

jmbockhorst commented Feb 12, 2020

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
Member

Tyriar commented Feb 13, 2020

Merged in so xterm is on latest: b4c19fd

@jmbockhorst
Copy link
Contributor Author

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

@Tyriar
Copy link
Member

Tyriar commented Mar 13, 2020

@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
Member

Tyriar commented Apr 11, 2020

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
Member

Tyriar commented Apr 11, 2020

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

// 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
Member

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
Member

Tyriar commented Apr 11, 2020

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

@Tyriar
Copy link
Member

Tyriar commented Apr 11, 2020

Created xtermjs/xterm.js#2836

@jmbockhorst
Copy link
Contributor Author

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

@Tyriar
Copy link
Member

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
Copy link
Member

Tyriar commented Apr 11, 2020

Ok that seems to have fixed it 🎉

@Tyriar Tyriar added this to the April 2020 milestone Apr 12, 2020
Currently when a cell is trimmed due to a wide char wrapping isn't
covered
@Tyriar
Copy link
Member

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

Copy link
Member

@Tyriar Tyriar left a comment

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
@jmbockhorst
Copy link
Contributor Author

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

@jmbockhorst jmbockhorst deleted the linkProvider branch April 12, 2020 18:59
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt xterm.js link provider API
2 participants