Issue with textEdit for non-prefix completion candidates

[astoff]

If I start with the following LaTeX file, with the cursor at the first },

\ref{lor}
\section{Lorem ipsum}
\label{1}

and do company-complete, then I expect to get \ref{1} on line 1. Instead, I end up with the following:

\ref{1ection{Lorem ipsum}
\label{1}

Now, the response of the completion request from my server is

(:jsonrpc "2.0" :result
          [(:detail "\\section{Lorem ipsum} \\label{1}" :sortText "00001" :insertTextFormat 1 :filterText "lor" :textEdit
                    (:range
                     (:start
                      (:line 0 :character 5)
                      :end
                      (:line 0 :character 8))
                     :newText "1")
                    :label "1" :documentation "\\section{Lorem ipsum}\n\\label{1}")]
          :id 2)

which looks correct to me. (I'm using the Digestif TeX server, but this shouldn't matter.)

If I modify my server so it doesn't send the textEdit entry, then Eglot has the correct behaviour. That's quite clever indeed, but it seems better to provide a textEdit, since it's the only explict way to convey that "lor" is the completion prefix.

[nemethf]

This is the problematic part of eglot-completion-at-point:

             (cond (textEdit
                    ;; Undo (yes, undo) the newly inserted completion.
                    ;; If before completion the buffer was "foo.b" and
                    ;; now is "foo.bar", `proxy' will be "bar".  We
                    ;; want to delete only "ar" (`proxy' minus the
                    ;; symbol whose bounds we've calculated before)
                    ;; (github#160).
                    (delete-region (+ (- (point) (length proxy))
                                      (if bounds (- (cdr bounds) (car bounds)) 0))
                                   (point))

It seems to assume that the original item can only be increased with the proxy. But in case of this issue, "lor" is replaced with "1", so delete-region deletes more than it should.

If we want to undo, why can't we use emacs' undo functionality? I'm totally unfamiliar with the undo mechanism, but the following patch seems to work. But I admit I tested it only slightly.

diff --git a/eglot.el b/eglot.el
index a33f204..c576e91 100644
--- a/eglot.el
+++ b/eglot.el
@@ -2097,9 +2097,7 @@ is not active."
                     ;; want to delete only "ar" (`proxy' minus the
                     ;; symbol whose bounds we've calculated before)
                     ;; (github#160).
-                    (delete-region (+ (- (point) (length proxy))
-                                      (if bounds (- (cdr bounds) (car bounds)) 0))
-                                   (point))
+                    (primitive-undo 1 buffer-undo-list)
                     (eglot--dbind ((TextEdit) range newText) textEdit
                       (pcase-let ((`(,beg . ,end) (eglot--range-region range)))
                         (delete-region beg end)

For a complete patch, eglot-completion-at-point should temporary enable undo if it's disabled in the buffer, which probably complicates the patch, and I'm not even know how to that.

[joaotavora]

If we want to undo, why can't we use emacs' undo functionality?

Good question. Maybe we can. Let's page @monnier here, hopefully the issue context is enough to explain what's going on.

[joaotavora]

For a complete patch, eglot-completion-at-point should temporary enable undo if it's disabled in the buffer, which probably complicates the patch, and I'm not even know how to that.

It should be as easy has temporarily binding buffer-undo-list to some non-t value in some context. But I think we could just assume it's always on, why should it not be so?

[monnier]

> For a complete patch, eglot-completion-at-point should temporary enable > undo if it's disabled in the buffer, which probably complicates the patch, > and I'm not even know how to that. It should be as easy has temporarily binding `buffer-undo-list` to some non-`t` value in some context. But I think we could just assume it's always on, why should it not be so?

See `atomic-change-group` how to do it: (let ((handle (prepare-change-group))) (activate-change-group handle) ... (then-either (accept-change-group ,handle) (cancel-change-group ,handle))) The `activate-change-group` is the one that enables undo if needed, and the `accept/cancel-change-group` are the ones that disable it (again, when needed). Stefan

[nemethf]

Stefan, thank you.

So we should call activate-change-group early on in eglot-completion-at-point, and call cancel-change-group in the exit-function. But, if I understand correctly, we must call accept-change-group when the completion is unsuccessful. But how do we know when it is unsuccessful? We are not notified when there's no match.

(It is independent, but Eglot won't be able to complete "\ref{lorem ip", because (bounds-of-thing-at-point 'symbol) returns "ip" and not "lorem ip".)

[joaotavora]

So we should call activate-change-group early on in eglot-completion-at-point

I welcome an undo-based solution here, but if I read @monnier correctly, we have to be sure to match activate-change-group to an accept-change-group or cancel-change-group. We have to test.

But how do we know when it is unsuccessful?

We don't, I think. We just cancel-change-group, where we previously undid the expansion. I expect that to have the same effect as your naive-but-maybe-not-to-uninteresting undo patch.

[monnier]

> So we should call activate-change-group early on in eglot-completion-at-point I welcome an undo-based solution here, but if I read @monnier correctly, we have to be sure to match `activate-change-group` to an `accept-change-group` or `cancel-change-group`.

That's right.

[astoff]

(It is independent, but Eglot won't be able to complete "\ref{lorem ip", because (bounds-of-thing-at-point 'symbol) returns "ip" and not "lorem ip".)

(Maybe this should be a new issue, but let me add a (not so) quick side-remark. There are indeed a few places where one might want the completion text to include spaces, including cases unrelated to fuzzy completion. But even worse is the following situation: if you follow AUCTeX labeling scheme, you will get labels like sec:introduction for sections, fig:a-beautiful-picture for figures, etc. But then completion at \ref{sec:intr will fail, because the thing at point will be intr and not sec:intr, as it should. Of course the server returns all completions, but Eglot filters them out.)

[nemethf]

So I know where to put activate-change-group and cancel-change-group; but if the exit-function is not called, then we must call accept-change-group and I don't know where to put that call.

[nemethf]

I've just reread this old, but long discussion and I wonder whether a simple server-side modification would settle the issue.
Instead of sending

(:jsonrpc "2.0" :result
          [(:detail "\\section{Lorem ipsum} \\label{1}" :sortText "00001" :insertTextFormat 1 :filterText "lor" :textEdit
                    (:range
                     (:start
                      (:line 0 :character 5)
                      :end
                      (:line 0 :character 8))
                     :newText "1")
                    :label "1" :documentation "\\section{Lorem ipsum}\n\\label{1}")]
          :id 2)

digestif could send

(:jsonrpc "2.0" :result
          [(:detail "\\section{Lorem ipsum} \\label{1}" :sortText "00001" :insertTextFormat 1 :filterText "lor" :textEdit
                    (:range
                     (:start
                      (:line 0 :character 5)
                      :end
                      (:line 0 :character 8))
                     :newText "1")
                    :label "Lorem ipsum" :documentation "\\section{Lorem ipsum}\n\\label{1}")]
          :id 2)

That is by changing the :label field, the editor completes to the "Lorem ipsum" string instead of "1", but keeps the textEdit to "1". In this example, this approach is even increases the user-friendliness: the user stated to partially write the name of a \section, so the editor gives a list of possible section names instead of a list of \label ids.

I haven't tested this. nevertheless, what do you think, @astoff?

[astoff]

@nemethf Thanks for the suggestion! I made this change to my server, and it's much more functional indeed.

Also, I can't reproduce the situation originally described in this issue, so I'll close it.