Skip to content

fix(suggestion): Trigger suggestion on InsertEnter #318

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

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

jsongerber
Copy link
Contributor

This pull request make that the suggestion get triggered when entering insert mode.
It was not working because both InsertEnter and CursorMovedI would fire at the same time and the timer variable would be messed up in the debounce function like this:

  • InsertEnter is called, copilot.timer is set to timer id 1
  • CursorMovedI is called, copilot.timer is set to timer id 2
  • Debounce function 1 is called, timer is 1, copilot.timer is 2, early return, copilot.timer is set to nil
  • Debounce function 2 is called, timer is 2, copilot.timer is nil, early return

This pull requests fixes this, I'm testing this since yesterday and it's working fine, but I don't know the codebase very well so I'm hoping I'm not breaking something.

Also the debounce function doesn't seem to be working (with or without this pull request), it works only as a timeout, does this need to be fixed or is this the intended behavior?

@folke
Copy link

folke commented Jan 31, 2025

@zbirenbaum any chance we can have this merged?
This also fixes suggestions not working when doing o or O.

@folke
Copy link

folke commented Jan 31, 2025

Nevermind, this PR seems to break in a lot of other ways

@jsongerber
Copy link
Contributor Author

Nevermind, this PR seems to break in a lot of other ways

Can you give me some examples of bugs this introduce? I'm using this PR with no issue, at least for suggestions (I'm not using panels and other features).
Maybe if I can fix thoses bugs this PR could be merged. Or I can close otherwise.
Thank you!

@folke
Copy link

folke commented Jan 31, 2025

Haven't looked into it, but I started getting errors where suggestions tried to show when it was not allowed to change the buffer

@jsongerber
Copy link
Contributor Author

Thank you for your response. I rebased my fork and will use this PR with the latest changes for the next few days to see if I can reproduce.
Feel free to tag me if you or anyone using this PR has more information to help fix the issue.

@folke
Copy link

folke commented Jan 31, 2025

Great, I'm also using the update branch. Will let you know when I experience issues but seems to work fine now.
ty!

@bulletmark
Copy link

I also tried that updated commit and it doesn't work the first time I open a new line in a newly opened file. If I hit escape, and then open the line again it works. Doesn't matter if I wait for a while. I.e. only works 2nd and subsequent times I open a new line. I am doing the test I described here in both a Python file and a shell script.

@jsongerber
Copy link
Contributor Author

Thank you @bulletmark for your feedback, I will check this weekend and see if I can reproduce and fix

@jsongerber
Copy link
Contributor Author

@bulletmark I could not reproduce the error, here is a screen recording showing Copilot working using O directly after opening the buffer with the Fibonacci example (it's slow to appear as I'm on the train with slow connection, but it eventually works):

Screen.Recording.2025-01-31.at.18.14.45.mov

I could reproduce the buggy behavior using lazy loading, and removing event = 'InsertEnter' fixes the issue (which LazyVim seems to be using).
I don't know if the problem should be fixed in this plugin or if it's normal behavior.

I tried to use lazy loading in my config and manually trigger a suggestion when entering insert mode the first time, but the server is not ready yet RPC[Error] code_name = ServerNotInitialized, message = "Agent service not initialized.". I could not find a reliable way to get the status of the server. Using a timer should work but is hacky.

@folke
Copy link

folke commented Jan 31, 2025

Great. Just changed it in LAzyVim and works as expected indeed.
And that annoyoing error is gone. I've been seeing it since a while, but never took the effort to further investigate :)

@apaydev
Copy link

apaydev commented Jan 31, 2025

So, is there an estimated date when this will be implemented in LazyVim? I just opened today nvim to work on some stuff and just came across this exact issue hahaha, and modifying my config files does not seem to change the behavior.

@folke
Copy link

folke commented Jan 31, 2025

@ADPenrose what do you mean? LazyVim != copilot.lua

@folke
Copy link

folke commented Jan 31, 2025

Or you mean that LSP notification? That's already solved on main

@folke
Copy link

folke commented Feb 7, 2025

Have been using this PR for a while now without issues, so would be great if this could get merged.

Thank you!

@bulletmark
Copy link

@folke, does LazyVim have a feature where I can merge this patch to my local installation? I see this but can't find any docs on how to use it?

@zbirenbaum
Copy link
Owner

This looks good to me, merging. Thanks for the fix @jsongerber

@zbirenbaum zbirenbaum merged commit 30321e3 into zbirenbaum:master Feb 10, 2025
@bulletmark
Copy link

Yay, updated LazyVim which picked up this change. Then tested against my two fibanacci examples (python and shell) and it worked perfectly! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants