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.

Sign up for GitHub

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

feat: support setting cursor position in text edits #2389

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c49cf3e
feat: support experimental snippet text edits
rchl Jan 4, 2024
954d911
rename to placeholder
rchl Jan 7, 2024
7a396b2
add tests
rchl Jan 12, 2024
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
View file
Open in desktop
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_placeholder = 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_placeholders: 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())
placeholder_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
placeholder_region = None # type: Optional[Tuple[Tuple[int, int], Tuple[int, int]]]
if process_placeholders 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 the 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
placeholder_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 placeholder_region is not None:
if placeholder_region_count == 0:
self.view.sel().clear()
placeholder_region_count += 1
self.view.sel().add(sublime.Region(
self.view.text_point_utf16(*placeholder_region[0], clamp_column=True),
self.view.text_point_utf16(*placeholder_region[1], clamp_column=True)
))
if placeholder_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 •
edited
Loading

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 •
edited
Loading

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_placeholder, 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
View file
Open in desktop
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_placeholders: 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_placeholders': process_placeholders})
Comment on lines +53 to +57
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit confusing that the process_placeholders is also added to this function, even though it is not used anywhere in the code. I see that you used it in the rust-analyzer PR, but apply_text_edits_to_view is not part of the public API...

I think it would be better if LspApplyDocumentEditCommand would simply take a list of TextEdit | AnnotatedTextEdit and optional version argument, instead of a list of custom TextEditTuple. With the current design there is no way to properly use LspApplyDocumentEditCommand from the outside when you have no access to parse_text_edit. I wonder why this part, which is only a single line (line 56 here), is not simply part of the command anyway to reduce code duplication. Here are the relevant usages of the command, all with the same additional code required before:

C:\Users\jwortmann\AppData\Roaming\Sublime Text\Packages\LSP\plugin\color.py:
   59              color_pres = self._filtered_response[index]
   60              text_edit = color_pres.get('textEdit') or {'range': self._range, 'newText': color_pres['label']}
   61:             self.view.run_command('lsp_apply_document_edit', {'changes': [parse_text_edit(text_edit, self._version)]})
   62  

C:\Users\jwortmann\AppData\Roaming\Sublime Text\Packages\LSP\plugin\completion.py:
  373          if additional_edits:
  374              edits = [parse_text_edit(additional_edit) for additional_edit in additional_edits]
  375:             self.view.run_command("lsp_apply_document_edit", {'changes': edits})
  376          command = item.get("command")
  377          if command:

C:\Users\jwortmann\AppData\Roaming\Sublime Text\Packages\LSP\plugin\formatting.py:
   53  def apply_text_edits_to_view(response: Optional[List[TextEdit]], view: sublime.View) -> None:
   54      edits = list(parse_text_edit(change) for change in response) if response else []
   55:     view.run_command('lsp_apply_document_edit', {'changes': edits})

Copy link
Member Author

@rchl rchl Jan 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure why is that. I imagine potential reasons could be to optimize the payload size or avoid passing unserializable data but neither of those is relevant here IMO.

As for public vs. non-public, we both know that there are many "non-public" APIs that are used from plugins. People will use whatever they need since we don't prevent them :)

But yeah, I agree with you that the command could take the original edits.



class WillSaveWaitTask(SaveTask):
Expand Down
2 changes: 1 addition & 1 deletion stubs/sublime.pyi
View file
Open in desktop
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