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

hyperlinks addon #2904

Closed
wants to merge 4 commits into from
Closed

hyperlinks addon #2904

wants to merge 4 commits into from

Conversation

jerch
Copy link
Member

@jerch jerch commented May 4, 2020

Early WIP.

* Copyright (c) 2019 The xterm.js authors. All rights reserved.
* @license MIT
*
* UnicodeVersionProvider for V11.
Copy link
Member

Choose a reason for hiding this comment

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

Remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Victim of c&p 😄

/**
* TODO:
* Need the following changes in xterm.js:
* - allow LinkProvider callback to contain multiple ranges (currently sloppy hacked into Linkifier2)
Copy link
Member

Choose a reason for hiding this comment

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

I'll be working on #2848 soon which will change the API to return all links for the current line.

Copy link
Member Author

@jerch jerch May 5, 2020

Choose a reason for hiding this comment

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

Well for the purpose of this addon all it needs are multiple buffer ranges for an URL to still recognize http://example.com as one link anchor text here:

╔═ file1 ════╗
║          ╔═ file2 ═══╗
║http://exa║Lorem ipsum║
║le.com    ║ dolor sit ║
║          ║amet, conse║
╚══════════║ctetur adip║
           ╚═══════════╝

* TODO:
* Need the following changes in xterm.js:
* - allow LinkProvider callback to contain multiple ranges (currently sloppy hacked into Linkifier2)
* - make extended attributes contributable by outer code
Copy link
Member

Choose a reason for hiding this comment

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

How about an ICellMarker API instead? Or tweaking the current one to optionally track a cell in a wrapped line?

Copy link
Member Author

Choose a reason for hiding this comment

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

This needs some more design work, yes I think this can be turned into something like cell markers (that stick to a cell, thus would move with it).

Comment on lines +140 to +145
[...this._urlMap.keys()]
.filter(key => key < this._lowestId)
.forEach(key => this._urlMap.delete(key));
[...this._idMap.entries()]
.filter(([unused, value]) => value < this._lowestId)
.forEach(([key, unused]) => this._idMap.delete(key));
Copy link
Member

Choose a reason for hiding this comment

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

You probably know but [...arr].filter() is probably a little slow to put into a parser hook?

Would LRUMap be useful here? We could pull into the addon?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, well it gets only executed once in a while (with default limits every 1000 urls). For some reason this declarative way was the only one I could find with proper type inference. Every loop attempt (which tend to be faster in general) was countered with type issues by TS lol.

LRUMap:
I think a linked list is not needed here, since the identifiers are just auto increasing integers (always to be cut from left side). Gonna rework that to a loop with early exit condition.

Comment on lines +296 to +299
activate: this._urlMap.get(urlId)!.schemeHandler.opener,
hover: (event: MouseEvent, text: string) => {
console.log('tooltip to show:', text);
}
Copy link
Member

Choose a reason for hiding this comment

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

activate, hover and leave need to be settable when creating the addon.

Copy link
Member Author

Choose a reason for hiding this comment

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

As talked about earlier, this prolly will end up with the LinkProvider being exposed on addon API level for individual customization.

@jerch jerch mentioned this pull request May 6, 2020
@jerch
Copy link
Member Author

jerch commented May 7, 2020

Playing around with this addon and a custom xtermjs: scheme:

recorded

The openBox function replaces buffer parts with the box. A closeBox function triggered on x restores the old state. This would also work for lines in the scrollbuffer. Hurray, terminal getting into browser realms now. Who said a LinkProvider should only handle http urls? 😸

@Tyriar
Copy link
Member

Tyriar commented May 8, 2020

That gif 🤯

@jerch
Copy link
Member Author

jerch commented Oct 4, 2021

@Tyriar This playground PR was mainly stalled due to the new linkprovider API back then. Do you think that API is stable enough to continue working on this PR?

(Sidenote: I'd go for #3489 first, as it is needed here as well. Oh well and the cell markers, will see about that.)

@Tyriar
Copy link
Member

Tyriar commented Oct 4, 2021

@jerch link providers have been working great in vscode, 👍 for removing experimental from it

@Tyriar
Copy link
Member

Tyriar commented Oct 4, 2021

#3493

@jerch
Copy link
Member Author

jerch commented Nov 16, 2021

This is far off, thus closing it.

@jerch jerch closed this Nov 16, 2021
@jerch jerch mentioned this pull request Jul 18, 2022
7 tasks
@Tyriar Tyriar added the reference A closed issue/pr that is expected to be useful later as a reference label Jul 25, 2022
@Tyriar Tyriar mentioned this pull request Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reference A closed issue/pr that is expected to be useful later as a reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants