Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Basic link parser for better link detection #2530
Basic link parser for better link detection #2530
Changes from 12 commits
0c5962c
3e2e4ac
1e877a0
cf082cc
ec78fc7
e57cb8b
fdfadfe
28c43ac
b3d95d2
ac6c8a5
b07a6d1
f18a6fe
700c4ac
f93939e
8924aa7
a190634
a95b1b4
e252b4b
a066bbe
ef071ac
c327116
b18ded0
f4a3125
0103c68
7f1f221
e4875f2
660cf26
54743ed
a7c2d54
0e302ba
d1d27b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently any
mousemove
will triggerprovideLink
, 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
andonScroll
:onScroll
is currently owned bysrc/Terminal.ts
but it will eventually move intocommon/
and be available on a service, in the meantime I suggest just passing it intoLinkifier2
via the constructor much like we're doing for this:xterm.js/src/common/services/CoreService.ts
Lines 26 to 27 in 16409da
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Changing the second parameter to false gives what we what, but I'm sure it breaks something elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This fires immediately after lines in the range
start
toend
have been re-rendered. We could cache the current mouse position from themousemove
event and then provide links for it in theonRender
listener?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New syntax for this as of a couple of days ago (TS 3.7):