Skip to content

feat: highlight / select symbol under cursor using LSP textDocument/documentHighlight - #2738

Merged
archseer merged 3 commits into
helix-editor:masterfrom
lazytanuki:lsp_document_highlight
Jun 27, 2022
Merged

feat: highlight / select symbol under cursor using LSP textDocument/documentHighlight#2738
archseer merged 3 commits into
helix-editor:masterfrom
lazytanuki:lsp_document_highlight

Conversation

@lazytanuki

Copy link
Copy Markdown
Contributor

Hi !

This PR implements the LSP textDocument/documentHighlight request. The command asks the LSP server for the ranges of references to current symbol, and select them (using multiple cursors).

I initially just wanted to have these symbols highlighted (like in nvim or vscode), but using multi-cursors is more convenient because the user can navigate between the references using ( and ), and also because it reuses already existing components :

Peek 2022-06-10 14-15

I have set the Space + h (for highlight) mapping to use this.

Have a good day !

Comment thread helix-term/src/commands.rs Outdated
Comment thread helix-term/src/commands/lsp.rs Outdated
Comment thread helix-term/src/commands/lsp.rs Outdated
@lazytanuki

Copy link
Copy Markdown
Contributor Author

Changes made, thanks for the review !

@sudormrfbin sudormrfbin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@joegm

joegm commented Jun 12, 2022

Copy link
Copy Markdown
Contributor

As for the keybind, wouldn't m-h be better than space-h? This feels like a "match" command.

@lazytanuki

Copy link
Copy Markdown
Contributor Author

Now that you mention it, indeed it does look like a match command. I chose 'h' for highlight because the call to LSP is textDocument/documentHighlight, although maybe something else than 'h' would make more sense as well ?
What do others think ?

@joegm

joegm commented Jun 12, 2022

Copy link
Copy Markdown
Contributor

Maybe m-S for 'Symbol' ? Lowercase s is already used (although we could change that to w for 'wrap'..?).

@lazytanuki

Copy link
Copy Markdown
Contributor Author

I don't think changing m-s would be such a good idea, it might already be muscle memory for current users. m-S could do it.

@archseer

Copy link
Copy Markdown
Member

@the-mikedavis any opinions on keymaps here?

I initially just wanted to have these symbols highlighted (like in nvim or vscode), but using multi-cursors is more convenient because the user can navigate between the references using ( and )

I think this is a smart way of doing it, especially since it makes it easy to rename all occurrences (if the lsp doesn't support the rename code action)

@the-mikedavis

Copy link
Copy Markdown
Member

I didn't mind space-h but I don't have any problems with mS either. My only hold-up with m is that it's usually used for matches and textobjects and I don't think this fits in perfectly with that. Given that hover and rename are already in space I think adding more is ok

@lazytanuki

Copy link
Copy Markdown
Contributor Author

Okay, should we go back to space-h then ?

@the-mikedavis

Copy link
Copy Markdown
Member

Yeah let's go for space-h for now, we can always revisit the binding later

@lazytanuki
lazytanuki force-pushed the lsp_document_highlight branch from c615788 to 2c205cc Compare June 22, 2022 13:29
@lazytanuki

Copy link
Copy Markdown
Contributor Author

Alright, done !

@lazytanuki
lazytanuki force-pushed the lsp_document_highlight branch from 2c205cc to 33461ba Compare June 22, 2022 13:47

@the-mikedavis the-mikedavis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is awesome, thanks for working on it!

@archseer archseer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants