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

feat: support setting cursor position in text edits #2389

Merged
merged 3 commits into from
Jan 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions plugin/edit.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
from .core.edit import TextEditTuple
from .core.logging import debug
from .core.typing import List, Optional, Any, Generator, Iterable
from .core.typing import List, Optional, Any, Generator, Iterable, Tuple
from contextlib import contextmanager
import operator
import re
import sublime
import sublime_plugin

Expand All @@ -21,19 +22,39 @@ def temporary_setting(settings: sublime.Settings, key: str, val: Any) -> Generat


class LspApplyDocumentEditCommand(sublime_plugin.TextCommand):
re_snippet = re.compile(r'\$(0|\{0:([^}]*)\})')

def run(self, edit: sublime.Edit, changes: Optional[List[TextEditTuple]] = None) -> None:
def run(
self, edit: sublime.Edit, changes: Optional[List[TextEditTuple]] = None, process_snippets: bool = False
) -> None:
# Apply the changes in reverse, so that we don't invalidate the range
# of any change that we haven't applied yet.
if not changes:
return
with temporary_setting(self.view.settings(), "translate_tabs_to_spaces", False):
view_version = self.view.change_count()
last_row, _ = self.view.rowcol_utf16(self.view.size())
snippet_region_count = 0
for start, end, replacement, version in reversed(_sort_by_application_order(changes)):
if version is not None and version != view_version:
debug('ignoring edit due to non-matching document version')
continue
snippet_region = None # type: Optional[Tuple[Tuple[int, int], Tuple[int, int]]]
if process_snippets and replacement:
parsed = self.parse_snippet(replacement)
if parsed:
replacement, (placeholder_start, placeholder_length) = parsed
# There might be newlines before the placeholder. Find the actual line and character offset
# of the placeholder.
prefix = replacement[0:placeholder_start]
last_newline_start = prefix.rfind('\n')
start_line = start[0] + prefix.count('\n')
if last_newline_start == -1:
start_column = start[1] + placeholder_start
else:
start_column = len(prefix) - last_newline_start - 1
end_column = start_column + placeholder_length
snippet_region = ((start_line, start_column), (start_line, end_column))
region = sublime.Region(
self.view.text_point_utf16(*start, clamp_column=True),
self.view.text_point_utf16(*end, clamp_column=True)
Expand All @@ -45,6 +66,16 @@ def run(self, edit: sublime.Edit, changes: Optional[List[TextEditTuple]] = None)
last_row, _ = self.view.rowcol(self.view.size())
else:
self.apply_change(region, replacement, edit)
if snippet_region is not None:
if snippet_region_count == 0:
self.view.sel().clear()
snippet_region_count += 1
self.view.sel().add(sublime.Region(
self.view.text_point_utf16(*snippet_region[0], clamp_column=True),
self.view.text_point_utf16(*snippet_region[1], clamp_column=True)
))
if snippet_region_count == 1:
self.view.show(self.view.sel())

def apply_change(self, region: sublime.Region, replacement: str, edit: sublime.Edit) -> None:
if region.empty():
Expand All @@ -55,6 +86,15 @@ def apply_change(self, region: sublime.Region, replacement: str, edit: sublime.E
else:
self.view.erase(edit, region)

def parse_snippet(self, replacement: str) -> Optional[Tuple[str, Tuple[int, int]]]:
Copy link
Member

@jwortmann jwortmann Jan 5, 2024

Choose a reason for hiding this comment

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

I haven't taken a closer look to understand every part of this PR, but I wonder is there a particular reason for all this manual snippet handling? Also can it handle snippets with multiple tabstops correctly? If I understand the code above correcty, it manually just adds a selection for each tabstop. But this is not how snippets work, if there are multiple tabstops it should set a single cursor and then with tab you can jump to the next one.

I think instead of view.insert(...) the logic to apply text edits should better move the curser and then use the built-in commands instead (at least for snippets, to handle them correctly). Like in

LSP/plugin/completion.py

Lines 354 to 357 in 36871c2

if item.get("insertTextFormat", InsertTextFormat.PlainText) == InsertTextFormat.Snippet:
self.view.run_command("insert_snippet", {"contents": new_text})
else:
self.view.run_command("insert", {"characters": new_text})

(perhaps only in the snippets case with tabstops, otherwise the cursor position probably shouldn't change I guess)

Copy link
Member Author

@rchl rchl Jan 5, 2024

Choose a reason for hiding this comment

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

It's obviously tailored for rust-analyzer and taken from it so this implementation assumes that there is only a single placeholder (specifically $0 or ${0:...}). So no multiple tab stops. But yes, those are not so much snippets but more a functionality to set cursor(s), only using a snippet-like placeholders for that. The rust-analyzer-initiated LSP protocol feature request even calls it "Allow CodeActions to specify cursor position".

I remember pretty well that we've tried insert_snippet (or @rwols did) when implementing the original code and it didn't work at all since it contains a lot of extra magic that for example auto-figures indentation. We need to do a raw insert or replace that won't do any of that.

And we need to set the cursor after applying the edit. Otherwise the selection will shift randomly.

I can do some better naming here to reflect what it really is. Maybe "process_selection_placeholders"

Copy link
Member

Choose a reason for hiding this comment

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

I see. I guess it could be tried like this for now, but I just read through that thread and I saw that there was already an example in microsoft/language-server-protocol#724 (comment) where they consider to use it as a "regular" snippet with multiple tab stops. So dependent on how this will end up in the specs, it's probably only a matter of time until a server will use the snippet in this way. For the autocompletion the indentation problem is solved by client announcing insertTextMode capability, where this client only supports adjustIndentation. I guess something like this would be needed as well for the snippet text edits then. Or alternatively they should introduce a new "simplified snippet" structure that only supports a single tab marker (or guarantee this in some other way in the specs).


I wonder don't we need to set { "snippetTextEdit": boolean } experimental client capability for this to work, which is mentioned in the docs from rust-analyzer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder don't we need to set { "snippetTextEdit": boolean } experimental client capability for this to work, which is mentioned in the docs from rust-analyzer?

Looks like they don't explicitly check for the capability for this "move item" functionality (https://github.com/rust-lang/rust-analyzer/blob/f8eac19b3354722a6fa0177968af54a58bb5b9e1/crates/ide/src/move_item.rs#L139-L165). They do check it many other places but in that case I wouldn't go and enable it without first checking that we correctly handle it in all cases (which we probably don't).

Copy link
Member

Choose a reason for hiding this comment

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

Okay. I also realized that the proper place would probably be in the "experimental_capabilities" client config for rust-analyzer anyway.

match = re.search(self.re_snippet, replacement)
if not match:
return
placeholder = match.group(2) or ''
new_replacement = replacement.replace(match.group(0), placeholder)
placeholder_start_and_length = (match.start(0), len(placeholder))
return (new_replacement, placeholder_start_and_length)


def _sort_by_application_order(changes: Iterable[TextEditTuple]) -> List[TextEditTuple]:
# The spec reads:
Expand Down
6 changes: 4 additions & 2 deletions plugin/formatting.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,11 @@ def format_document(text_command: LspTextCommand, formatter: Optional[str] = Non
return Promise.resolve(None)


def apply_text_edits_to_view(response: Optional[List[TextEdit]], view: sublime.View) -> None:
def apply_text_edits_to_view(
response: Optional[List[TextEdit]], view: sublime.View, *, process_snippets: bool = False
) -> None:
edits = list(parse_text_edit(change) for change in response) if response else []
view.run_command('lsp_apply_document_edit', {'changes': edits})
view.run_command('lsp_apply_document_edit', {'changes': edits, 'process_snippets': process_snippets})


class WillSaveWaitTask(SaveTask):
Expand Down
2 changes: 1 addition & 1 deletion stubs/sublime.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -1758,7 +1758,7 @@ class View:
# def is_in_edit(self) -> bool: # undocumented
# ...

def insert(self, edit: Edit, pt: int, text: str) -> None:
def insert(self, edit: Edit, pt: int, text: str) -> int:
"""
Insert the given string into the buffer.

Expand Down