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

improve terminal link keyboard navigation experience #140121

Merged
merged 7 commits into from
Jan 5, 2022

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Jan 4, 2022

Recording 2022-01-04 at 15 11 30

This PR fixes #95570

@meganrogge meganrogge changed the title Merogge/links cleanup improve terminal link keyboard navigation experience Jan 4, 2022
Comment on lines 101 to 103
const wordLinks = (await new Promise<ILink[] | undefined>(r => this._standardLinkProviders.get(TerminalWordLinkProvider.id)?.provideLinks(y, r)));
let webLinks = (await new Promise<ILink[] | undefined>(r => this._standardLinkProviders.get(TerminalProtocolLinkProvider.id)?.provideLinks(y, r)));
let fileLinks = (await new Promise<ILink[] | undefined>(r => this._standardLinkProviders.get(TerminalValidatedLocalLinkProvider.id)?.provideLinks(y, r)));
Copy link
Member

Choose a reason for hiding this comment

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

Using Promise.all here would be best so we can do these in parallel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't do that since we want to keep them separate

Copy link
Member

Choose a reason for hiding this comment

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

Could do it on the side of whatever consumed the getLinks call though: Promise.all([getLinks(...), getLinks(...)]), this is mainly an issue when extension link providers come in (more than 1 slow provider).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

referenced in the extension issue.

@isidorn
Copy link
Contributor

isidorn commented Jan 5, 2022

Once we merge this in I can give it a quick test run.
@meganrogge thanks a lot for working on this 👏

@Tyriar Tyriar added this to the January 2022 milestone Jan 5, 2022
@meganrogge meganrogge merged commit 95a0e2b into main Jan 5, 2022
@meganrogge meganrogge deleted the merogge/links-cleanup branch January 5, 2022 23:27
@meganrogge
Copy link
Contributor Author

@isidorn went with option 1, give it a test in Insider's tomorrow pls and let us know what you think #95570 (comment)

@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support terminal link keyboard navigation
3 participants