Open Adjective-Object opened 5 months ago
After some (non expert) poking around, it looks like terminal links are evaluated by some ITerminalLinkDetector, which I think is provided by extensions.
Link detection is run on a per-line basis.
These are connected to xterm via this adaptor
Tracing into xterm, it looks like links are triggered here, in _askForLink
https://github.com/xtermjs/xterm.js/blob/2edc65baaa50f8b99c67f3df6f8d5d93c70a69e6/src/browser/Linkifier.ts#L134
Which is triggered in this onRenderedViewportChange
callback here
https://github.com/xtermjs/xterm.js/blob/2edc65baaa50f8b99c67f3df6f8d5d93c70a69e6/src/browser/Linkifier.ts#L299-L321
Internally, it looks like the start/end here are row indexes in the viewport (buffer)? so that explains the behaviour of why line 3 in the example has its links disposed, since it 's within the line start/end range. https://github.com/xtermjs/xterm.js/blob/2edc65baaa50f8b99c67f3df6f8d5d93c70a69e6/src/browser/services/RenderService.ts#L168
It seems like sparse updates that re-render only the changed rows isn't tenable; I kept digging around xterm and row re-renders being tied to start/end ranges seems like one of the core assumptions of xterm.
However, each of the built-in linkifiers essentially starts by grabbing the queried line's text: https://github.com/xtermjs/xterm.js/blob/2edc65baaa50f8b99c67f3df6f8d5d93c70a69e6/src/browser/OscLinkProvider.ts#L20
I think there's way inside xterm.js to work around this issue by caching links against the text they are underneath in Linkifier:
LinkProvider
s can define some new trait to opt-into caching, something like cacheLinksAgainstText: bool
.cacheLinksAgainstBuffer
provider, the Linkifier
computes a hash of the cell attributes + text within the BufferRange defined by the link.onRenderedViewportChange
, instead of immediately disposing all links, it could:
cacheLinksAgainstBuffer
, clear the link for that provider and re-query the link provider.Though, as expressed above there's a race condition if the buffer is updated while the provider is calculating the link.
Maybe this is better expressed by adding an optional checkCache(buffer, BufferRange)
callback on the ILink
interface so that the link provider can grab the relevant buffer contents to use for caching before it starts any asynchronous actions.
@Tyriar - didn't realize you worked at MS too!
Right now I'm just poking around the xterm repo trying to figure out what a solution would look like, as this is causing some pretty awkward UX for Outlook devs at the moment. Our devloop has a logger & a few progress bars sharing the same terminal with some clickable links, and any logging or animations we do break the links.
I'm happy to make the changes to get things fixed, but would love to run an approach by you first before blindly jumping in and proposing changes that are architecturally unsound 😅.
@Adjective-Object good investigation 🙂.
This is a known problem and so far there hasn't been that much incentive to fix it. My instinct to fix this would be more aggressive caching similar to your suggestion. In particular we can leverage the fact that if we know the link under the cursor did not change, we don't necessarily need to re-fetch links. So if we tracked the chars
of each cell in the line when linkify happens, onRenderedViewportChange
could invalidate a portion of that to determine whether we need to re-eval.
Feel free to ping me on Teams or in a draft PR if you need more guidance but that's my thinking. It gets a little hairy in the low level link code so we'll want to be careful about not introducing too much complexity with the caching.
Description
When parts of the terminal are updated, link detection breaks momentarily. For projects with an in-terminal spinner, for example, this breaks all link detection for areas of the terminal near the spinners.
Reproduction Video: https://github.com/microsoft/vscode/assets/1174858/12b53246-9922-4d67-88d6-f15188a24a1a
In the video I'm using powertoys to highlight mouse clicks. Note that both xterm links and link text are only intermittently clickable on updated lines, or lines in-between updated lines
Apologies for not submitting through the integrated help menu, it wasn't able to submit.
Reproduction Steps
Run the following bash snippet in an integrated bash terminal.
This snippet:
This breaks link detection on the text on lines 2-5. Ctrl+Clicking those links will sometimes work, but most often will fail. I believe this is because the link detection is run asynchronously, and is racing with the frame-rate of the animation. If you increase the interval of the sleep in the loop, the link detection mostly works again, unless you happen to click right after an animation frame
Curiously, links are broken on line 3, despite no text being printed on that line in the loop. So apart from not updating the text, there's not a clean way to
I've tested this against Windows Terminal (1.19.10821.0) and it does not have the same issues.
System Information
Extensions
Extensions: none
A/B Experiment Info