search

TerminalFi / LSP-copilot

GitHub Copilot support for Sublime Text LSP plugin provided through Copilot.vim.
658 stars 24 forks source link

feat: implement getCompletionCycling #29

Closed TerminalFi closed 2 years ago

TerminalFi commented 2 years ago

What

Fully implement getCompletionCycling

jfcherng commented 2 years ago

I noticed that I get two completions which are exactly the same.

image image

jfcherng commented 2 years ago

Somehow look at the speed of sending payloads during typing, I feel the debouncing is not working...

TerminalFi commented 2 years ago

I noticed that I get two completions which are exactly the same.

image

image

Looking at this

jfcherng commented 2 years ago

Looking at this

unique util may help. https://github.com/TheSecEng/LSP-copilot/blob/1703e6a1351874e23b216a0a664a85182d10476d/plugin/utils.py#L182

jfcherng commented 2 years ago

I think the functionality of this PR is working as expect. Let's merge this and move other minor issues to other PRs.

Potential Issues

timfjord commented 2 years ago

I also notice the flickering issue.
It could be potentially fixed if we won't do another request if there is already a visible completion and the region of the completion is the same as the region of the potential future request. I was going to send a PR later next week

But in general the debouncer requires some research, we might need to also increase the timeout a bit

jfcherng commented 2 years ago

But in general the debouncer requires some research, we might need to also increase the timeout a bit

Yeah. I don't like how that debouncer works at this moment. It just delays requests and stops only VERY few of them. You can't cancel a closure which has been sent to sublime.set_timeout regardless async or not.

If you increase the timeout to like 5 seconds and you type a lot of chars during 5 seconds, you will see a lot of requests are sent suddenly right at the 5th second. Ideally, only the last request is meaningful, all others before it should be cancelled.

I have a similar goal (throttle, debounce) in my OpenUri plugin and I eventually create a background long-running thread to repeated do the job every n milliseconds so that the job is guaranteed to be executed at most once per n milliseconds.

timfjord commented 2 years ago

Currently we have 300ms delay, my idea was to increase it to 600ms or so. This won't solve the other problems but might at least help stoping some unwanted requests

jfcherng commented 2 years ago

Currently we have 300ms delay, my idea was to increase it to 600ms or so. This won't solve the other problems but might at least help stoping some unwanted requests

Even if you set it to 10s, it won't stop unwanted requests. It's just deferred and burst at once.

jfcherng commented 2 years ago

Try to resolve debounce (and flickering) issue in https://github.com/TheSecEng/LSP-copilot/pull/40