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

Support links in the terminal #455

Closed
Tyriar opened this issue Jan 6, 2017 · 3 comments · Fixed by #538
Closed

Support links in the terminal #455

Tyriar opened this issue Jan 6, 2017 · 3 comments · Fixed by #538
Assignees
Labels
type/enhancement Features or improvements to existing features
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jan 6, 2017

Context

The linkify addon has been broken for some time with little attention to it, yet it remains quite a popular request to have be have active links for VS Code (microsoft/vscode#7321). I'm proposing we pull the linkify logic into the core and expose it as an option.

Why?

One of the reasons linkify is currently broken with no way forward as it is implemented as an addon outside of xterm.js, dealing with things that are very much internal. In addition, with all the performance changes on their way in (particularly this one #450), it is not safe to modify .xterm-rows. This is also very performance critical so it should not be messed with at all my addons.

How it will work

When linkify gets triggered

In order to prevent this from causing a big performance hit I propose we only check for links a certain period of time (100ms, 200ms?) after there has been no changes to the viewport's buffer. This is in contrast to the current implementation which I believe requires an external call.

Web vs local

The current implementation only linkifies hyperlinks as it was designed just for the browser. One of the most compelling use cases however is to linkify local files so that they can be opened in the editor.

Obviously regular <a> tags won't cut it for non-hyperlinks as they won't know how to handle them, we will need something like an attachLinkifyHandler function (even better deprecate and merge attachCustomKeydownHandler into some generic attachHandler/registerHandler function).

It's also important to note that in Electron it's probably not desirable to jump use <a> tags but handle them custom as well. We do however want to keep <a> tags for web files to get the nice middle click/ctrl+click behavior. So the links will be:

  • web: <a> with href set
  • local: <a> with no href provided a linkify handler has been attached, otherwise don't check for local links

Better viewport row reuse

How Terminal.refresh works currently, all links would disappear when a single line is added to bottom of the buffer. To better support linkify and also save some CPU cycles while we're at it, we should look into reusing rows by informing Terminal.rowContainers that we simply shifted some elements whenever possible and only run a targeted refresh on a specific row.

Make the span object pool aware of the links

The DomElementObjectPool logic within Terminal.refresh which keeps <span>s around so not as many DOM nodes are constructed will need to be made aware of the potential for <a> tags wrapping <span> and even splitting <span>s in some cases.

@Tyriar Tyriar added the type/proposal A proposal that needs some discussion before proceeding label Jan 6, 2017
@vincentwoo
Copy link
Contributor

This is probably a good idea. However, it raises the question of just what addons are good for in their current state. If it's not possible to make an addon that turns links into, well, links performantly, what can they do?

@Tyriar
Copy link
Member Author

Tyriar commented Jan 8, 2017

Well the other ones are to do with fitting the content and attaching xterm.js under various environments. Another idea I had was to make one that makes the selection invert like gnome-terminal. Basically anything that can be done without touching private apis, unless there are tests I'm place being the exception to that.

@Tyriar
Copy link
Member Author

Tyriar commented Feb 8, 2017

I've started working on this, web links were relatively easy to implement. Still thinking about it but I'm leaning towards exposing generic register link regex/handler support and then creating an addon to easily expose decent local link support.

@Tyriar Tyriar added this to the 2.4.0 milestone Feb 8, 2017
@Tyriar Tyriar added feature and removed type/proposal A proposal that needs some discussion before proceeding labels Feb 8, 2017
@Tyriar Tyriar changed the title Proposal: Move linkify into core Support links in the terminal Feb 8, 2017
@Tyriar Tyriar added the type/enhancement Features or improvements to existing features label Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Features or improvements to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants