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

Link provider probably needs to change to provide links for entire line #2848

Closed
Tyriar opened this issue Apr 15, 2020 · 4 comments · Fixed by #2908 or #2916
Closed

Link provider probably needs to change to provide links for entire line #2848

Tyriar opened this issue Apr 15, 2020 · 4 comments · Fixed by #2908 or #2916
Assignees
Labels
area/api area/links type/enhancement Features or improvements to existing features
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Apr 15, 2020

Consider the following:

image

Currently in VS Code, depending on which cell you focus first gives a different link.

Good (validated local link provider):

image

Bad (word link provider):

image

fyi @jmbockhorst

@jmbockhorst
Copy link
Contributor

So it would look something like this?

provideLink(bufferLineNumber: number, callback: (link: ILink | undefined) => void): void;

@Tyriar
Copy link
Member Author

Tyriar commented Apr 19, 2020

Yes but multiple returned:

provideLinks(bufferLineNumber: number, callback: (links: ILink[] | undefined) => void): void;

Also when I was looking into #2849 I noticed that right now in vscode we're using leave like a dispose, which is good as xterm drops the link reference when leaving the cell range. But if we did this with the provideLinks idea then we would want to differentiate leave?(event: MouseEvent, text: string): void; and dispose?(): void so xterm owns the decision of when to get rid of it (leave = it may get reused, dispose = embedder should drop the ref).

@Tyriar
Copy link
Member Author

Tyriar commented Apr 19, 2020

I don't really see another way of fixing this problem of competing providers creating different links based on which cell is hovered first, which leads to an inconsistent experience.

This was referenced May 2, 2020
@Tyriar Tyriar added the type/enhancement Features or improvements to existing features label May 5, 2020
Tyriar added a commit to Tyriar/xterm.js that referenced this issue May 6, 2020
@Tyriar Tyriar added this to the 4.6.0 milestone May 6, 2020
@Tyriar Tyriar self-assigned this May 6, 2020
@Tyriar
Copy link
Member Author

Tyriar commented May 6, 2020

This isn't actually fixed yet, we need to delete links that are "below" higher priority links still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api area/links type/enhancement Features or improvements to existing features
Projects
None yet
2 participants