Basic link parser for better link detection

[jmbockhorst]

Here is my first attempt at a basic link parser for #583. This is a very rough draft and I realize that it may be in the wrong direction, but I wanted feedback on how this compares to what you guys had in mind.

This adds a new Linkifier2 class with that handles registering a link parser. I also updated the Web Links Addon to use the new parser system. I mainly used the LinkDetector from VSCode and modified it as needed for this application. It seems to work well and it fixes most of the cases that were broken with the old system.

I didn't do anything with markers because I'm not sure that how that fits in, since a marker only holds the line number, and we need to store the link range.

Still things to do:

• Support for multiline links
• Handle when the buffer changes above a cached link

EDIT: Sorry, I meant to make this a draft pull request

[Tyriar]

This is awesome, great job! 🤩

I didn't check it too thoroughly but it looks like the right basic direction, it's also no problem that it's not a draft.

[jmbockhorst]

@Tyriar I've implemented a new caching system that only uses the cache for the lines before the current one. (Is there any issues doing it like this) When there is any data input, if the mouse is currently over a link, it hides the tooltip and deletes it from the cache. This also happens on scroll. This seems to work well from my testing, but I'm sure there are cases that I am missing.

Still to do:

• Handle when the scrollback is full (Invalidate the entire cache. Using onTrim?)
• Multi line link detection in the WebLinksAddon

[Tyriar]

Handle when the scrollback is full (Invalidate the entire cache. Using onTrim?)

There's some discussion on caching strategy in #583 (comment), I still recommend for the MVP just going with invalidating the cache when the cursor leaves the link's range and improving upon it after this PR is merged.

[jmbockhorst]

This PR might fix #2115 and #2458, but it'll need to be tested thoroughly.

[Tyriar]

Yeah we should be getting #2458 for free, multi-line maybe not as I think it was to do with line wrapped status breaking after a resize.

[Tyriar]

It seems to work really nicely so far based on my testing 👍

[Tyriar]

Currently any mousemove will trigger provideLink, it should only fire as a minimum when the mouse is moved off of the current cell:

Ideally we would cache just the current link and only fire provideLink when the cursor moved off of the current link's range. We could clear out the current link by listening to onData and onScroll:

// Register listeners and dispose them immediately after either of them are fired
this._linkCacheDisposables.push(this._coreService.onData(() => this._clearCurrentLink()));
this._linkCacheDisposables.push(this._onScroll(() => this._clearCurrentLink()));

...

private _clearCurrentLink() {
  this._currentLink = undefined;
  this._linkCacheDisposables.forEach(l => l.dispose());
  this._linkCacheDisposables = [];
}));

onScroll is currently owned by src/Terminal.ts but it will eventually move into common/ and be available on a service, in the meantime I suggest just passing it into Linkifier2 via the constructor much like we're doing for this:

xterm.js/src/common/services/CoreService.ts

Lines 26 to 27 in 16409da

	// TODO: Move this into a service
	private readonly _scrollToBottom: () => void,

[jmbockhorst]

Looks like scroll events don't get fired when the viewport is scrolled, only when a new line is added. Ref:

xterm.js/src/browser/Viewport.ts

Line 159 in 16409da

this._scrollLines(diff, true);

Changing the second parameter to false gives what we what, but I'm sure it breaks something elsewhere.

[Tyriar]

Changing that to false will break a bunch of other stuff... Hmm I thought there was an event for when scroll happens but I guess not. I think what we're actually after is this one:

xterm.js/src/browser/services/Services.ts

Line 46 in b68a4d2

onRender: IEvent<{ start: number, end: number }>;

This fires immediately after lines in the range start to end have been re-rendered. We could cache the current mouse position from the mousemove event and then provide links for it in the onRender listener?

[Tyriar]

I think that providers are registered is important here, it looks like currently it's non-deterministic if 2 link providers that call callback async are used? How about we use a strategy something like asking all link providers to provide links and wait for them to all callback before actually showing the link?

Improving upon this a little, consider what VS Code will probably do which is register a web link provider first (top priority), then a FS link provider second. We could fire off both link providers and if the web link provider comes back first action that immediately as it's the highest priority, if the FS link provider comes back first then wait for the web link provider to return no results first before calling that. This would mean that we get deterministic results based on the order that link providers are registered and also that the web link provider would not need to wait on the FS call before creating the link (the FS call can be expensive for example when using Remote-SSH in vscode).

[Tyriar]

I see an issue where the cell immediately following a link will count as part of the link's range as well, something related to end.x will probably need to change in here to fix that.

[Tyriar]

New syntax for this as of a couple of days ago (TS 3.7):

this._linkProvider?.dispose();

[jerch]

@jmbockhorst
I dont want to disrupt the review process with @Tyriar thus I dont go into coding details. Just a few general remarks from my side:

• Terminal.onScroll This is mainly used for ringbuffer rotations (terminal buffer). Imho in earlier releases viewport scrolls were also coupled to it to some degree if it happened together with line inserts/removals. We might need here a better separation of concerns and better wording.

• about linkprovider and caching later on: Currently you call providers with IBufferCellPosition as argument. Imho thats an unlucky choice, since the position can and will mutate by many buffer actions. This makes caching almost undoable or invalidates entries often (not even sure how you gonna spot all those invalidations). Furthermore all individual providers will have to pull the buffer content with translateToString since their workdata is not the position but the line's content.

  Suggestion: Do the translateToString on Linkifier2 level once for the current line under the mouse (with wrapping in mind) and call the individual providers with this string data. Now they can directly do their link pattern parsing and return string offsets. The string offsets can be turned back to buffer positions on Linkifier2 level.
  Caching can be achieved quite simple with this - use the line content as cache keys and save the provider results as entries. This has the big advantage that any buffer content change will not match anymore and trigger a fresh eval. The cache itself can be made as a short FIFO ringbuffer (maybe 10 entries only).

• debouncing the mouse events to grid changes: I had the same problem in the CoreMouseService and went with a simple row/col check. This has a disadvantage - if a user steals the focus, the event will not re-trigger when the focus comes back and the mouse didnt move out of the cell. @Tyriar Maybe we should move mouse-to-grid normalization into some "MouseService" later on to avoid doing the same over and over (well there is still my early stub to get some unification for mouse stuff ;) ).

[jmbockhorst]

I made all of the requested changes and switched everything to using a single cached link. Everything should work the same as before. (Except for scrolling because of the onScroll issue)

[Tyriar]

Fixed conflicts caused by #2547

[Tyriar]

Changing that to false will break a bunch of other stuff... Hmm I thought there was an event for when scroll happens but I guess not. I think what we're actually after is this one:

xterm.js/src/browser/services/Services.ts

Line 46 in b68a4d2

onRender: IEvent<{ start: number, end: number }>;

This fires immediately after lines in the range start to end have been re-rendered. We could cache the current mouse position from the mousemove event and then provide links for it in the onRender listener?

[Tyriar]

Terminal.onScroll This is mainly used for ringbuffer rotations (terminal buffer).

This is right, we should be using IRenderService.onRender instead of onData/onScroll as I suggested.

about linkprovider and caching later on

Again we should avoid caching outside of just the current line for the first iteration of this which I think is what's happening now. The reason I push for this so much as because cache invalidation is complicated and the sooner this gets in (after we've sorted out the fundamentals), the easier it is to iterate on.

This does bring up the point that links with unicode probably won't work at the moment. I guess the question is how much information should be exposed to provideLink, resolving buffer vs string indexes gets awfully complicated and one of the elegant things about how we expose it right now is that it's up to the addon to fetch the content of each cell in which case they already know what the buffer range is. Personally I'd say we can go ahead as it is and see if my idea of having the link provider addon fetch the buffer cells manually works out. We can always change it later since it's experimental (and should do if it makes sense).

debouncing the mouse events to grid changes

Some debouncing would definitely be useful, consider when moving the mouse quickly from one side of the terminal to the other for example. This can easily get deferred to a future PR though imo.

[Tyriar]

Found a couple of bugs writing some tests: 0103c68

[Tyriar]

I made a change to use link matcher for WebLinksAddon unless an argument is provided, that was my main concern with merging this as link matcher and link providers will probably cause problems when used together. This lets people slowly adopt it with less risk.

[Tyriar]

Thanks a lot for all your work @jmbockhorst, this is a fantastic contribution and am looking forward to seeing it in action 🙂

[jmbockhorst]

Thank you for all of the guidance along the way and for finishing it up. I’ll be looking forward to contributing more in the future.

[Tyriar]

@jmbockhorst if you're interested in pulling this into VS Code and hooking it up to LinkComputer as well you can give microsoft/vscode#90298 a shot. Basically we'll want an opt-in setting to use the new system, can defer supporting local links initially as getting that right is hard.

I'll probably be updating xterm in vscode in the coming days.