Skip to content

support prefilling prompt - #2459

Merged
archseer merged 5 commits into
helix-editor:masterfrom
QiBaobin:prompt-prefill
Jul 18, 2022
Merged

support prefilling prompt#2459
archseer merged 5 commits into
helix-editor:masterfrom
QiBaobin:prompt-prefill

Conversation

@QiBaobin

@QiBaobin QiBaobin commented May 11, 2022

Copy link
Copy Markdown
Contributor

No description provided.

@QiBaobin

QiBaobin commented May 11, 2022

Copy link
Copy Markdown
Contributor Author

Add capabilities like reusing current symbol when renaming etc.

pickfire
pickfire approved these changes May 11, 2022
View reviewed changes

@pickfire pickfire left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Not tested but I like this behavior, miss this for rename symbol.

pickfire
pickfire reviewed May 11, 2022
View reviewed changes
Comment thread helix-term/src/commands/lsp.rs Outdated
archseer
archseer reviewed May 11, 2022
View reviewed changes
Comment thread helix-term/src/ui/prompt.rs Outdated
@sudormrfbin

sudormrfbin commented May 11, 2022

Copy link
Copy Markdown
Member

For rename symbol I think it would be more useful to prefill with the symbol to be renamed rather than the selection which feels more intuitive.

@QiBaobin

QiBaobin commented May 12, 2022

Copy link
Copy Markdown
Contributor Author

For rename symbol I think it would be more useful to prefill with the symbol to be renamed rather than the selection which feels more intuitive.

It's not easy to determine current symbol without talking with lsp server as it depends on the language.

pickfire
pickfire reviewed May 16, 2022
View reviewed changes
Comment thread helix-term/src/ui/mod.rs Outdated
pickfire
pickfire approved these changes May 16, 2022
View reviewed changes

@pickfire pickfire left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Thanks. But I think maybe @archseer should have another look, since I am not sure if there is a better way around this.

@the-mikedavis the-mikedavis added the S-waiting-on-review Status: Awaiting review from a maintainer. label May 18, 2022
@archseer

archseer commented May 20, 2022

Copy link
Copy Markdown
Member

There's a textDocument/prepareRename if the server supports it. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_prepareRename

It will return the offsets of the string we're renaming: https://github.com/emacs-lsp/lsp-mode/blob/6157b3dde2c56f734a0789225240c521acdf2c7c/lsp-mode.el#L5849-L5871=

@the-mikedavis the-mikedavis removed the S-waiting-on-review Status: Awaiting review from a maintainer. label May 20, 2022
@QiBaobin

QiBaobin commented Jun 24, 2022

Copy link
Copy Markdown
Contributor Author

There's a textDocument/prepareRename if the server supports it. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_prepareRename

It will return the offsets of the string we're renaming: https://github.com/emacs-lsp/lsp-mode/blob/6157b3dde2c56f734a0789225240c521acdf2c7c/lsp-mode.el#L5849-L5871=

Do we need below option too? This would make the case complex and I am not sure if I can handle this right now.

(defcustom lsp-rename-use-prepare t
Whether `lsp-rename' should do a prepareRename first.
For some language servers, textDocument/prepareRename might be
too slow, in which case this variable may be set to nil.
`lsp-rename' will then use `thing-at-point' `symbol' to determine
the symbol to rename at point."

@sudormrfbin

sudormrfbin commented Jun 24, 2022

Copy link
Copy Markdown
Member

I think it's okay to skip this for now and add a per LSP config option in the future if issues arise.

sudormrfbin
sudormrfbin reviewed Jul 16, 2022
View reviewed changes
Comment thread helix-term/src/commands/lsp.rs Outdated
@sudormrfbin sudormrfbin added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 16, 2022
@archseer
archseer merged commit e5c7aae into helix-editor:master Jul 18, 2022
thomasskk pushed a commit to thomasskk/helix that referenced this pull request Sep 9, 2022
@QiBaobin @thomasskk
support prefilling prompt (helix-editor#2459)
3536462 
* support prefilling prompt

* introduce with_line builder method in Prompt

* extract show_prompt

* use textobject_word as fallback input
@the-mikedavis the-mikedavis mentioned this pull request Feb 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@archseer archseer archseer left review comments
+2 more reviewers
@sudormrfbin sudormrfbin sudormrfbin left review comments
@pickfire pickfire pickfire approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@QiBaobin @sudormrfbin @archseer @pickfire @the-mikedavis