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

Implement LSP window/showDocument request #6079

Closed
wants to merge 2 commits into from

Conversation

MDeiml
Copy link
Contributor

@MDeiml MDeiml commented Feb 22, 2023

Implements the LSP request for window/showDocument.

This does not yet implement requests with the external parameter set to true, but it would probably best to do this in the same PR (as LSP servers would probably assume that to work). I believe there is no similar functionality in helix yet, and I wanted to check if my suggested approach would be welcome.

I suggest adding a External variant to the Action enum which is passed to Editor::open. The actual implementation could either be done using the open crate or by reimplementing the functionality of this crate.

@the-mikedavis the-mikedavis added A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 22, 2023
helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
@MDeiml
Copy link
Contributor Author

MDeiml commented Feb 22, 2023

Thanks for the quick feedback :)

Copy link
Contributor

@poliorcetics poliorcetics left a comment

Choose a reason for hiding this comment

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

You can avoid adding a new method by simply storing the offset_encoding earlier and keeping it for the duration of the method, since that type is Copy it won't carry the borrow of self.editor

helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-lsp/src/lib.rs Outdated Show resolved Hide resolved
@MDeiml
Copy link
Contributor Author

MDeiml commented Feb 23, 2023

You can avoid adding a new method by simply storing the offset_encoding earlier and keeping it for the duration of the method, since that type is Copy it won't carry the borrow of self.editor

Hm, that would work, but I would need to call get_by_id again after the match block. I feel like it's a bit neater with the Arc. Also future contributions might also need the Arc in this method or other places so it probably makes sense for Registry to have this method.

@poliorcetics
Copy link
Contributor

Ah I didn't check if the language server itself was used later

helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Show resolved Hide resolved
@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 26, 2023
@MDeiml MDeiml force-pushed the lsp-show-document branch 2 times, most recently from aa56442 to 8f45f15 Compare February 27, 2023 12:24
@MDeiml MDeiml force-pushed the lsp-show-document branch 2 times, most recently from 6148f41 to 478a5b3 Compare March 11, 2023 14:44
@MDeiml
Copy link
Contributor Author

MDeiml commented Mar 11, 2023

Should be ready to merge now. It would also make sense to wait on Byron/open-rs#64.

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2023
@matoous
Copy link
Contributor

matoous commented Mar 11, 2023

There’s open PR for opening files externally: #5820

@MDeiml
Copy link
Contributor Author

MDeiml commented Mar 12, 2023

Oh, the open PR was merged in the meantime. So I guess it's worth waiting for #5820 and rebasing the changes on that to support opening externally.

@pascalkuthe pascalkuthe added S-waiting-on-pr Status: This is waiting on another PR to be merged first and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Mar 12, 2023
@matoous
Copy link
Contributor

matoous commented Nov 20, 2023

@MDeiml I am sorry, I completely forgot about existence of this PR 🙈 here's second implementation of this based on the PR for handling opening documents in external programs: #8865

@MDeiml
Copy link
Contributor Author

MDeiml commented Nov 20, 2023

@matoous haha I already saw it, no worries. Let's just use yours then.

@pascalkuthe
Copy link
Member

Closing in favor of #8865 as discussed above. Thanks for working on this @MDeiml

@pascalkuthe pascalkuthe closed this Jan 8, 2024
@MDeiml MDeiml deleted the lsp-show-document branch January 8, 2024 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client S-waiting-on-pr Status: This is waiting on another PR to be merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants