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

Completing deletes too much text (with snippets or TextEdits) #160

Closed
mkcms opened this issue Nov 20, 2018 · 4 comments
Closed

Completing deletes too much text (with snippets or TextEdits) #160

mkcms opened this issue Nov 20, 2018 · 4 comments

Comments

@mkcms
Copy link
Collaborator

mkcms commented Nov 20, 2018

Given this file

int foo(int);
int bar(int);
int baz(int);

int main () {
    foo(bar(b));
}

When I set point to 71 and complete to 'baz', I end up with

int foo(int);
int bar(int);
int baz(int);

int main () {
    foo(bar(baz(int));
}

(notice that Emacs deleted one parenthesis at the end of completed line)

This happens if either TextEdits or snippets are present in the completion.
The reason is that the completion provided to :exit-function is not the inserted text, but the whole completed symbol, from car of completion bounds to point.
In this case Emacs deletes one to many character because the completed symbol is b. If the completed symbol is ba, Emacs will delete 2 characters, etc.

Events:


client-request (id:2) Tue Nov 20 13:50:16 2018:
(:jsonrpc "2.0" :id 2 :method "textDocument/completion" :params
	  (:textDocument
	   (:uri "file:///tmp/prog.cpp")
	   :position
	   (:line 5 :character 13)))

server-reply (id:2) Tue Nov 20 13:50:16 2018:
(:id 2 :jsonrpc "2.0" :result
     (:isIncomplete :json-false :items
		    [(:detail "int" :filterText "bar" :insertText "bar(${1:int})" :insertTextFormat 2 :kind 3 :label " bar(int)" :sortText "3f2cccccbar" :textEdit
			      (:newText "bar(${1:int})" :range
					(:end
					 (:character 13 :line 5)
					 :start
					 (:character 12 :line 5))))
		     (:detail "int" :filterText "baz" :insertText "baz(${1:int})" :insertTextFormat 2 :kind 3 :label " baz(int)" :sortText "3f2cccccbaz" :textEdit
			      (:newText "baz(${1:int})" :range
					(:end
					 (:character 13 :line 5)
					 :start
					 (:character 12 :line 5))))
		     (:filterText "bool" :insertText "bool" :insertTextFormat 2 :kind 14 :label " bool" :sortText "3f800000bool" :textEdit
				  (:newText "bool" :range
					    (:end
					     (:character 13 :line 5)
					     :start
					     (:character 12 :line 5))))]))
@joaotavora
Copy link
Owner

Can we try just undo ing everything? Perhaps that works as is safe for both company and vanilla completion-at-point. Otherwise we have to save some info lexically and recover it when we want the snippet or textEdit approach.

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 20, 2018

Can we try just undo ing everything?

Hmm, I tried it a bit and with emacs -Q I get

user-error: No further undo information

in new buffers. When undo-list is not empty, it doesn't appear to work reliably either.

I also tried using change groups which I cancel in :exit-function. This worked pretty well without company. With company-mode it didn't undo the changes at all.
Another option is basically saving the line at point and then in :exit-function deleting the current line and using that. Although that is very ugly...

However I got the best results with just using the bounds which we already calculate from (bounds-of-thing-at-point 'symbol). E.g. just something like this

diff --git a/eglot.el b/eglot.el
index d9c1c3a..fe68f87 100644
--- a/eglot.el
+++ b/eglot.el
@@ -1610,7 +1610,9 @@ is not active."
                             (eglot--snippet-expansion-fn))))
                (when (or fn textEdit)
                  ;; Undo the completion
-                 (delete-region (- (point) (length comp)) (point)))
+                 (delete-region (+ (- (point) (length comp))
+                                   (if bounds (- (cdr bounds) (car bounds)) 0))
+                                (point)))
                (cond (textEdit
                       (cl-destructuring-bind (&key range newText) textEdit
                         (pcase-let ((`(,beg . ,end) (eglot--range-region range)))

@joaotavora
Copy link
Owner

However I got the best results with just using the bounds which we already calculate from (bounds-of-thing-at-point 'symbol). E.g. just something like this

Good. Let's go with this then. Just add another line of comment or so after "Undo the completion" sumarrily explaining the problem and the strategy.

And if possible add a test for one of these newer servers.

Thanks!

@mkcms
Copy link
Collaborator Author

mkcms commented Nov 20, 2018

And if possible add a test for one of these newer servers.

Alright, I will also add a general test for TextEdits in completion. This will require downloading or building a server which supports them on travis though. Although maybe one of the servers we use already supports them, I will check.

@mkcms mkcms closed this as completed in fc03d7c Nov 21, 2018
joaotavora added a commit that referenced this issue Nov 22, 2018
Fixes a slight regression from #160.

* eglot.el (eglot-completion-at-point): When there is plain
`insertText' snippet, delete the full completion text.
bhankas pushed a commit to bhankas/emacs that referenced this issue Sep 18, 2022
* eglot.el (eglot-completion-at-point): In :exit-function, delete only
  the region of buffer that was inserted by completion.
bhankas pushed a commit to bhankas/emacs that referenced this issue Sep 18, 2022
Fixes a slight regression from joaotavora/eglot#160.

* eglot.el (eglot-completion-at-point): When there is plain
`insertText' snippet, delete the full completion text.
bhankas pushed a commit to bhankas/emacs that referenced this issue Sep 19, 2022
* eglot.el (eglot-completion-at-point): In :exit-function, delete only
  the region of buffer that was inserted by completion.
bhankas pushed a commit to bhankas/emacs that referenced this issue Sep 19, 2022
Fixes a slight regression from joaotavora/eglot#160.

* eglot.el (eglot-completion-at-point): When there is plain
`insertText' snippet, delete the full completion text.
bhankas pushed a commit to bhankas/emacs that referenced this issue Sep 19, 2022
* eglot.el (eglot-completion-at-point): In :exit-function, delete only
  the region of buffer that was inserted by completion.

#160: joaotavora/eglot#160
bhankas pushed a commit to bhankas/emacs that referenced this issue Sep 19, 2022
Fixes a slight regression from #160.

* eglot.el (eglot-completion-at-point): When there is plain
`insertText' snippet, delete the full completion text.

#167: joaotavora/eglot#167
#160: joaotavora/eglot#160
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this issue Oct 12, 2022
* eglot.el (eglot-completion-at-point): In :exit-function, delete only
  the region of buffer that was inserted by completion.

GitHub-reference: fix joaotavora/eglot#160
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this issue Oct 12, 2022
Fixes a slight regression from joaotavora/eglot#160.

* eglot.el (eglot-completion-at-point): When there is plain
`insertText' snippet, delete the full completion text.

GitHub-reference: fix joaotavora/eglot#167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants