[improvement] Improve active buffer context

[emil-vdw]

Implement the following suggestions from #150:

• textDocument/didFocus on focus of already opened buffer (window-selection-change-functions hook)
• textDocument/didOpen on first focus of buffer (window-selection-change-functions hook)
• textDocument/didClose event on buffer kill (kill-buffer-hook hook)
• textDocument/didChange event on buffer change (before-change-functions hook for deletions and after-change-functions for insertions) with only data relevant to the change not the whole buffer.

[CeleritasCelery]

This looks great! Thank you for implementing this. Your solution looks cleaner then what I was planning. I especially appreciate that we are no longer sending the entire buffer to copilot but only a small change set as needed.

[CeleritasCelery]

I was doing some more testing on this, and I noticed some issues.

First is that window-selection-change-functions is called both when we switch to a copilot buffer and when we switch away from one. When switching away, the current-buffer is the new buffer we switched to. So we are calling didFocus for a whole bunch of non-copilot buffers (like the minibuffer or message buffer). These are also being added to copilot--opened-buffers, which they should not be.

Second is that disabling copilot-mode in a buffer is not removing that buffer from copilot--opened-buffers, so it is still considered opened.

[emil-vdw]

@CeleritasCelery You are correct on both counts. Looks like it definitely needs a bit more work which I'm hoping to get done this week. It should not be that difficult to remedy though.

Thanks for your diligent testing and great feedback. Really appreciate it! :trophy:

[emil-vdw]

Cleanup of copilot--opened-buffers is going to be tricky for copilot-mode in the local case since we are only disabling the mode for one buffer at a time. The context for the other buffers need to remain. I suppose the correct thing to do would be to remove active buffer from copilot--opened-buffers when deactivating copilot-mode and to (setq copilot--opened-buffers '()) when disabling global-copilot-mode?

[emil-vdw]

@CeleritasCelery I have to do some more testing but it seems that the mode cleanup body is called for every open buffer when leaving the global mode so it should be fine.

Should we send didClose event when deactivating copilot-mode? Is there any other cleanup we need to do when leaving the mode?

[CeleritasCelery]

Should we send didClose event when deactivating copilot-mode?

I would say yes. We call didOpen when activitating copilot-mode, so it makes sense to be symmetrical. From copilots perspective, that buffer is gone.

One other thing I noticed in testing this change is that window-selection-change-functions is not called when we change the buffer within a window (for example, using previous-buffer, next-buffer). This means we don't get a didFocus event.

[emil-vdw]

I agree. I will implement the didClose signal on mode exit.

As for the previous-buffer, next-buffer case, it looks like hooking onto both window-selection-change-functions and window-buffer-change-functions seems to cover all bases. It does not look like they overlap but I will double check.

Also looks like window-selection-change-functions hooks are called twice. Once with the window being switched away from as the argument and again with the window switched to as argument. The active buffer is the new (switched to) buffer in either case so no bug there; however, it does mean that we sometimes send didFocus events twice. I'll see if I can ignore the first event of the two in that case.

[CeleritasCelery]

I found another issue as I was doing some testing. With the new change, the only time we send the entire buffer to copilot is on the initial call to didOpen in copilot--on-doc-focus. This uses copilot--get-source to get the :text. However copilot--get-source has default limit of 30000 characters. meaning if your buffer is over that size, copilot never receives part of the buffer.

I noticed this issue because sometimes copilot would just not be sending me any completions, but if I called the old function copilot--sync-doc it would start working again. The issue was that I was working outside of this 30000 character boundary, and so copilot had no idea what the context was, because we never gave it to it.

[emil-vdw]

However copilot--get-source has default limit of 30000 characters

Could we not significantly increase the default limit? It's not being called on every keypress anymore right.

[CeleritasCelery]

We could, and I have set it to -1 in my case so that it has no limit. But I am still observing the same behavior. copilot will stop sending me completions until I call copilot--sync-doc. I am not sure if this is something only effecting me or a problem with how I have copilot configured.

[emil-vdw]

copilot will stop sending me completions until I call copilot--sync-doc.

It's not just you. I've been having the exact same problem. Will debug this and report back as well.

[CeleritasCelery]

I wonder if this is just a case of copilots internal buffer getting screwed up when we accept completions. When we accept a completion we send notifyAccepted, but we also send the just accepted text via textDocument/didChange. If copilot adds the completion on its own to it's internal buffer when we call notifyAccepted (I don't know that it does this, I am just speculating) then it is inserting two copies of the text at the same place. Once copilots internal buffer diverges from ours all the position information we send it is now wrong, meaning it gets even more out of sync over time.

Also keep in mind that we post-process the completion text via copilot--show-completion (by both removing shared prefixes and balancing parens). If copilot is updating its internal model on notifyAccepted, then it is adding a different version then we are.

[emil-vdw]

What you're saying makes a ton of sense... I'm going to use copilot today with notifyAccepted disabled to see if that makes a difference.

At this point I'm just going to try to inspect the agent's internal state... Going to start here: https://github.com/haukot/copilot_specifications#how-to-debug.

[emil-vdw]

The node debugger route wasn't very fruitful. Still testing things out with notifyAccepted, notifyRejected disabled.

Another potential bug I've uncovered is that event uri for any buffer that is not saved to file is an empty string. This will garble the shared empty string URI buffer for copilot buffers not saved to file yet (*scratch* buffer for example)

[emil-vdw]

Another bug I've found is that in copilot--get-uri the buffer local variable buffer-file-name is used to determine the uri. However, there is a brief moment of time when this has not been set yet (I have copilot-mode hooked to prog-mode) and defaults to uri of empty string.

If a change happens in this time window the version increases with uri empty string until the var is set and the 'file://...' uri is used. This also garbles the internal state / creates a version mismatch which causes prediction requests to error.

Edit: Looks like I was wrong about this, buffer-file-name is already set when needed. My problem was an improperly formatted uri for non file buffers.

[emil-vdw]

@CeleritasCelery @MtCoin406 I think this is ready for final review and merge.

I will log a separate issue for change in URI when saving an active copilot buffer that was not saved to file before.

[zerolfx]

There might be a bug. The content of company documentation is being sent as a change to the Python file.

[zerolfx]

Will there be any issues if I enable copilot-mode while editing the current buffer? The buffer may not be opened in Copilot.

[emil-vdw]

Will there be any issues if I enable copilot-mode while editing the current buffer? The buffer may not be opened in Copilot.

No issues. When activating the mode, the on focus handler is triggered manually which will send the didOpen notification.

[emil-vdw]

There might be a bug. The content of company documentation is being sent as a change to the Python file. ![image]

I will look into this and see if I can fix it.

[emil-vdw]

@zerolfx I'm not able to reproduce this, could you give me a recipe config maybe? I'm using company and company box but even without company box I can't get anything to modify the actual buffer nor send a copilot event.

[MtCoin406]

Access

[zerolfx]

@emil-vdw I will review and make any possible fixes this weekend. Hopefully, we can get this PR merged then. I greatly appreciate your work on this.

[emil-vdw]

Thanks @zerolfx. That would be great. Let me know if there's anything else I can do. Would love to make more contributions in future.

[CeleritasCelery]

@emil-vdw what was the fix for the "copilot not sending completions until re-synced" issue?

[emil-vdw]

@CeleritasCelery there were change events that were replacements and not just an insertion or deletion. One example of this is when yanking from the kill ring, I think it inserted the text and then replaced it with text that has the correct text properties.

Just had to modify the before and after change hooks to process a deletion for the replaced text in the before hook, and an insertion for the replaced text in the after hook.

[zerolfx]

@emil-vdw I give you write access to this repo. I greatly appreciate your help with this, as I no longer use Emacs in my daily work.

[CeleritasCelery]

I have been using the master branch with these changes in it, but I am still experiencing the issue where copilot looses stare until I sync it explicitly.