search

copilot-emacs / copilot.el

An unofficial Copilot plugin for Emacs.
MIT License
1.71k stars 122 forks source link

Sometimes, copilot.el's overlay is not considered an active overlay (and thus, the copilot-completion-map is not active) #251

Closed timcharper closed 4 months ago

timcharper commented 5 months ago

Sometimes, Emacs does not consider copilot.el as an activate overlay (it never, for some reason, considers copilot.el's overlay in (overlays-at (point))

I use the elisp in this gist to help debug this:

https://gist.github.com/timcharper/fbb12d28bee0a75d8d6d54e463a5e324#file-copilot-debug-env-el

The elisp sets up copilot, keycast mode and a timer which updates a buffer overlay-properties every second with the following info:

I've tried my best to show the issue in the following asciinema:

asciicast

Screens

Screenshot showing that copilot-completion-map IS active if another overlay contains the point, but copilot.el's overlay is also considered to contain the point:

image

Notice:

Screenshot showing that copilot-completion-map IS NOT active if another overlay contains the point, but copilot's overlay IS NOT considered to contain the point.

image

Notice:

Screenshot showing that copilot-completion-map IS ACTIVE if the copilot overlay is active, but NO OVERLAY is considered to contain the point (notice key binding and dimmed-out color for the copilot overlay description).

image

Notice:

timcharper commented 5 months ago

If I help Emacs to recognize the copilot-overlay as containing the point by making sure it is always at least one character after the point, it resolves the issue (but, causes other undesirable issues, like text-shifting around and replace behaving incorrectly).

diff --git a/copilot.el b/copilot.el
index 234b25e..a21eae2 100644
--- a/copilot.el
+++ b/copilot.el
@@ -544,7 +544,10 @@ To work around posn problems with after-string property.")

 (defun copilot--set-overlay-text (ov completion)
   "Set overlay OV with COMPLETION."
-  (move-overlay ov (point) (line-end-position))
+  (move-overlay ov
+                (point)
+                (min (point-max)
+                     (max (+ (point) 1) (line-end-position))))
   (let* ((tail (buffer-substring (copilot--overlay-end ov) (line-end-position)))
          (p-completion (concat (propertize completion 'face 'copilot-overlay-face)
                                tail)))
timcharper commented 5 months ago

The fundamental problem seems to be is that Emacs considers an overlay to contain the point if [overlay-start, overlay-end), where overlay-start is inclusive but overlay-end is not.

Therefore, an overlay that starts and ends at the same position can never contain the point. And if the overlay does not contain the point, then the copilot overlay keymap is ignored if another overlay does (even if the other overlay has a lower priority).

I believe the solution is either:

a) insert a phantom character for the overlay so that the cursor can always be considered in the overlay (overlay length is, at worst, (point) to (+ (point) 1)

b) activate the overlay-completion-map via some other mechanism (temporary toggling of minor mode?)

c) activate a separate (invisible) overlay that contains the entire buffer to activate the keymap

or.... IDK... find some other way to make Emacs recognize the overlay as active. Someone else is probably smarter than I am here. Based on my study of the issue, this is the best I can come up with.

timcharper commented 5 months ago

Adding a second overlay works, but it is probably a hack and there is probably a better fix.

diff --git a/copilot.el b/copilot.el
index 234b25e..3ec62c1 100644
--- a/copilot.el
+++ b/copilot.el
@@ -103,6 +103,9 @@ indentation offset."
 (defvar-local copilot--overlay nil
   "Overlay for Copilot completion.")

+(defvar-local copilot--keymap-overlay nil
+  "Overlay for Copilot completion keymap.")
+
 (defvar copilot--connection nil
   "Copilot agent jsonrpc connection instance.")

@@ -534,8 +537,11 @@ To work around posn problems with after-string property.")
   "Create or get overlay for Copilot."
   (unless (overlayp copilot--overlay)
     (setq copilot--overlay (make-overlay 1 1 nil nil t))
-    (overlay-put copilot--overlay 'keymap copilot-completion-map)
     (overlay-put copilot--overlay 'priority 100))
+  (unless (overlayp copilot--keymap-overlay)
+    (setq copilot--keymap-overlay (make-overlay (point-min) (point-max) nil nil t))
+    (overlay-put copilot--keymap-overlay 'keymap copilot-completion-map)
+    (overlay-put copilot--keymap-overlay 'priority 101))
   copilot--overlay)

 (defun copilot--overlay-end (ov)
@@ -585,6 +591,8 @@ already saving an excursion. This is also a private function."
       (copilot--async-request 'notifyRejected
                               (list :uuids `[,(overlay-get copilot--overlay 'uuid)])))
     (delete-overlay copilot--overlay)
+    (delete-overlay copilot--keymap-overlay)
+    (setq copilot--keymap-overlay nil)
     (setq copilot--real-posn nil)))

 (defun copilot-accept-completion (&optional transform-fn)
emil-vdw commented 5 months ago

@timcharper thank you for the great investigative work! I think this might be related to:

This is very unfortunate... I don't like option C. For option B we could manually active the keymap when a completion is shown and deactivate when it is cleared. This seems to me to be the least bad option but I'll have to think about it more.

On a side note, the gist you shared to observe the active overlays is not complete. Would you mind sharing the complete snippet?

timcharper commented 5 months ago

Huh! That's weird! I've updated the gist. Not sure how that truncated :confused:

Nice find on the gnu threads. Sounds like others have hit the exact same thing! :) But no resolution came about it.

I don't like option C either. Although it works reliably but it is hacky. I looked in to how corfu does it... it uses minor-mode-overriding-map-alist to activate its key bindings. Not sure if it's quite the same thing, but it could be made to work

I played around with it and got this to work.

https://gist.github.com/timcharper/e95b6de243607ffd7985923334e1b31a

Applied to copilot, use make-composed-keymap to combine copilot-completion-map and the copilot-mode-map, and then insert the override while the completion overlay is present. This would eliminate the need for an additional minor mode.

emil-vdw commented 4 months ago

I do think that the best way around this problem is manually controlling some transient keymap while the overlay is active.

I don't like option C either. Although it works reliably but it is hacky. I looked in to how corfu does it... it uses minor-mode-overriding-map-alist to activate its key bindings. Not sure if it's quite the same thing, but it could be made to work

That's an option! What do you think about using set-transient-map?

timcharper commented 4 months ago

set-transient-map looks like the way 💡

emil-vdw commented 4 months ago

I haven't looked into it too much but it seems that way to me too :thinking:. Thank you for all of your effort. Your investigation was very thorough and helped a lot.

Would you consider submitting a PR on this? Totally understandable if you don't have the time!

timcharper commented 4 months ago

Hmm, experienting with set-transient-map, I'm not 100% convinced this is the way to go. set-transient-map is global to all buffers, so if you switch buffers mid-completion, the next key in the new buffer is intercepted by the transient map.

In light of this, I think the overlay hack is better, because it is buffer local, and the mechanism for deactivating it is straight-forward, and the state of an active completion continues.

timcharper commented 4 months ago

@emil-vdw PR opened at #255

emil-vdw commented 4 months ago

@timcharper that makes total sense. Thank you for taking the time and opening a PR. Really appreciate it. :+1:

emil-vdw commented 4 months ago

Fixed by #255

timcharper commented 4 months ago

@emil-vdw you bet! Thank you for collaborating on the PR and helping it to get merged! I appreciate you maintaining the project! :)