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

add go to symbol in the accessible view #189575

Merged
merged 4 commits into from
Aug 3, 2023

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Aug 3, 2023

demo.mov

fixes #188702

@mjbvz since the accessible view is not attached to an editor pane, the default language features won't work here - thus, this code.

the idea is most of the time the language will be markdown. when it's not, we defer to the provider for symbols IE quick pick items.

getSymbols(): IAccessibleViewSymbol[] | undefined {
if (!this._currentProvider) {
return;
}
const tokens = this._currentProvider.options.language && this._currentProvider.options.language !== 'markdown' ? this._currentProvider.getSymbols?.() : marked.lexer(this._currentProvider.provideContent());
if (!tokens) {

const index = provider.provideContent().split('\n').findIndex(line => line.includes(symbol.info.split('\n')[0])) ?? -1;

do you have a suggestion for a better / more performant / more precise approach?

@meganrogge meganrogge self-assigned this Aug 3, 2023
@meganrogge meganrogge marked this pull request as draft August 3, 2023 19:40
@meganrogge meganrogge added this to the August 2023 milestone Aug 3, 2023
@mjbvz mjbvz requested review from jrieken and mjbvz August 3, 2023 20:49
Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Looks like an ok approach to me. I've also copied @jrieken I believe he's worked more with go to symbol

@meganrogge meganrogge marked this pull request as ready for review August 3, 2023 21:34
@meganrogge meganrogge enabled auto-merge August 3, 2023 21:40
@meganrogge meganrogge merged commit f3a8b62 into main Aug 3, 2023
@meganrogge meganrogge deleted the merogge/go-to-symbol-accessible-view branch August 3, 2023 22:39
quickPick.items = symbols;
quickPick.show();
quickPick.onDidAccept(() => {
this._accessibleView.showSymbol(provider, quickPick.selectedItems[0]);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't going to dismiss the quick pick. Is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accepting one does because of the focus change. escaping the quick pick wasn't handled, but now is #189658

Copy link
Member

Choose a reason for hiding this comment

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

There's a setting called ignoreFocusOut that won't close the quick pick when focus is lost.

As a result this qp won't go away with that setting. I recommend explicitly closing it

@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2023
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 go to symbol in the accessible view
3 participants