feat: support setting cursor position in text edits

[rchl]

Add opt-in handling for placeholders in text edits which define the position of the cursor after text edits are applied. This is required (well, not required but it improves experience) for custom rust-analyzer moveItem command that I'm adding support for in sublimelsp/LSP-rust-analyzer#111 (review)

Based on rust-analyzer extension code: https://github.com/rust-lang/rust-analyzer/blob/f7823f31069c6ec5be24d0497847bf1bb8a4c683/editors/code/src/snippets.ts#L41-L87

Also check rust-analyzer description of their custom snippet text edit: https://github.com/rust-lang/rust-analyzer/blob/master/docs/dev/lsp-extensions.md#snippet-textedit

[jwortmann]

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)

[rchl]

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"

[jwortmann]

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?

[rchl]

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).

[jwortmann]

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

[jwortmann]

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})

[rchl]

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.

[jwortmann]

Should be safe to add, since the process_placeholders argument is disabled by default. But we should keep an eye on how it will end up in the LSP specs, for the case if any server will start to use multiple tab stops.

[predragnikolic]

Could you add a test for this?

[rchl]

I will do some further refactoring separately but for now I think this is fine. If there are any remaining comments (about added tests) then I can address them in the next PR.