-
Notifications
You must be signed in to change notification settings - Fork 126
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
[Improvement] Defer sending doc changes to the agent to an idle timer. #176
base: main
Are you sure you want to change the base?
Conversation
b738349
to
a046810
Compare
I am not an Emacs expert, but my hypothesis is that jsonrpc-notify causes buffer navigation to occur, which causes the (org-todo) function to break when called twice in a row. This fixes issue: copilot-emacs#172
also refactor out some variables so lines are not as long, and add a commented out debug message for future use.
This is used in case copilot stops responding due to buffer state being desynchronized with the copilot process. It is unsure whether this is still necessary after the previous commits.
a046810
to
a0a685f
Compare
The original pull request would never have worked properly, since I thought There is also some other extra stuff here, like the But assuming this is the wrong way to fix #172, there isn't anything too important here to merge. |
@raymond-w-ko I am confused. does this PR fix the linked issue or not? |
I will push changes and get this merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. :)
Some nits that are good to resolve:
In toplevel form:
copilot.el:39:35: Warning: Alias for ‘copilot-completion-idle-delay’ should be declared before its referent
In copilot-diagnose:
copilot.el:311:7: Warning: reference to free variable ‘copilot-mode’
In copilot--handle-notification:
copilot.el:542:14: Warning: ‘mark-whole-buffer’ is for interactive use only.
In copilot--on-doc-change:
copilot.el:665:32: Warning: reference to free variable ‘copilot-mode’
In copilot--on-doc-focus:
copilot.el:675:14: Warning: reference to free variable ‘copilot-mode’
In copilot-async-start-process:
copilot.el:[105](https://github.com/copilot-emacs/copilot.el/actions/runs/8222298553/job/22483626619?pr=176#step:5:106)0:10: Warning: Unused lexical variable ‘name’
In end of data:
copilot.el:804:14: Warning: the function ‘vterm-insert’ is not known to be defined.
copilot.el:803:14: Warning: the function ‘vterm-delete-region’ is not known to be defined.
copilot.el:543:14: Warning: the function ‘org-sort-entries’ is not known to be defined.
copilot.el:532:54: Warning: the function ‘org-entry-get’ is not known to be defined.
copilot.el:532:26: Warning: the function ‘org-map-entries’ is not known to be defined.
But we can fix these warnings later.
One last thing to check is that |
@@ -26,7 +26,7 @@ | |||
:group 'completion | |||
:prefix "copilot-") | |||
|
|||
(defcustom copilot-idle-delay 0 | |||
(defcustom copilot-completion-idle-delay 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the motivation for this change?
Personally, I think copilot-idle-delay
is fine and probably doesn't warrant the complexity of adding a deprecated variable.
;; | ||
;; Doc changes | ||
;; | ||
(cl-defmacro copilot--widening (&rest body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like this change doens't quite fit the PR description? Separate PR? Not clear to me why it's necessary since the point should be within the narrowed region, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change appears to not create any regressions, however, I'm unable to reproduce the original issue on master, which causes me to wonder if we really understand what is broken?
I have concerns about the widen change. Why is this introduced with this PR? There's no explanation as to what motivated it, and it appears unrelated and also I cannot understand why it is needed (copilot operates right after the point, and the point should always be in a narrowed region? In my experiments, I have no trouble using copilot within a narrowed region. A separate issue should be filed.
In my recordings, I'm using Emacs 29.2, with the built-in version of org-mode, and a minimal environment (package-initialize, keycast-mode enabled, copilot-mode enabled (from source at commit 0d48e75)
Given that I can't reproduce the original issue on master, I'm wondering if this change is needed
@timcharper Although I did manage to reproduce the problem initially I am also not seeing it anymore on Emacs I do, however, feel like this is a good improvement nonetheless. That being deferring jsonrpc communication with the server to some idle time or until the user requests a completion (as implemented in and shamelessly stolen from In any event, the PR title and description needs some work. I will update. |
Currently, doc changes (like
textDocument/didChange
) are communicated on every change (essentially every key press). It would make more sense and improve responsiveness to defer communicating with the server until some idle time or until it is actually needed for a completion.To implement this we don't have to reinvent the wheel and can just import most of it from the
eglot
implementation.Implementation
This fixes issue:
#172 (although this is also seems to not be an issue in newer
org
versions).