Skip to content
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

completion fix #1819

Closed
wants to merge 1 commit into from
Closed

Conversation

cossonleo
Copy link
Contributor

Completion will not work correct When We type chars between sended to completion request and recived the result. These chars will not be replaced by completion items.

@cossonleo cossonleo force-pushed the completion branch 2 times, most recently from 666b3cf to 0f6a0b4 Compare March 15, 2022 22:33
@pickfire
Copy link
Contributor

pickfire commented Mar 22, 2022

How to reproduce the issue? Oh, I just realized I have this issue.

@archseer
Copy link
Member

I don't quite understand the issue, we track the initial trigger_offset when completion is triggered:

let trigger_offset = cursor;

Then we use that trigger offset when needed:

// Some LSPs just give you an insertText with no offset ¯\_(ツ)_/¯
// in these cases we need to check for a common prefix and remove it
let prefix = Cow::from(doc.text().slice(start_offset..trigger_offset));
let text = text.trim_start_matches::<&str>(&prefix);
Transaction::change(
doc.text(),
vec![(trigger_offset, trigger_offset, Some(text.into()))].into_iter(),

We also remove any extra text:

// if more text was entered, remove it
doc.restore(view.id);

@archseer
Copy link
Member

We might need to call .savepoint sooner, inside fn completion rather than here

// initialize a savepoint
doc.savepoint();

@cossonleo
Copy link
Contributor Author

We might need to call .savepoint sooner, inside fn completion rather than here

// initialize a savepoint
doc.savepoint();

Yes
In the current implementation, there are still redundant characters.
It occurs when the completion request is sent and wait response. At this time, we type char again.

@cossonleo
Copy link
Contributor Author

cossonleo commented Mar 31, 2022

How to reproduce the issue?

Although it is a small regular event that affects normal editing

gopls 0.7.0
helix 0.6
.config/helix/config.toml

  idle-timeout = 40

helix_completion

@cossonleo cossonleo force-pushed the completion branch 2 times, most recently from a590d6c to ed44941 Compare April 3, 2022 07:21
@the-mikedavis
Copy link
Member

I've been seeing the same completion problems with ElixirLS and ErlangLS. I'm not sure if it's the slowness of the LS or my low idle-timeout config but when I type and get completion as I type, C-n/C-ping to one of the candidates does sometimes end up with a suffix like the screenshot. I hadn't really locked down what triggered it but it seems like if I deliberately pause before pressing C-n/C-p it doesn't happen (on master).

I just took this branch for a spin though and it seems to solve my completion problems.

@the-mikedavis
Copy link
Member

the-mikedavis commented Apr 21, 2022

I ran with this change yesterday and I noticed that the completion menu seems to pop up less. I imagine that what's happening is that I trigger the idle-timeout between keystrokes and the keystrokes following the auto-complete request cancel that request. (I haven't really looked at the change yet though so take that description with a grain of salt)

edit: this is my own doing - going into verbose mode and tailing the logs shows that it's the server that's being slow.

@cossonleo cossonleo force-pushed the completion branch 4 times, most recently from 334c92b to dd4dc76 Compare May 3, 2022 02:23
@cossonleo cossonleo force-pushed the completion branch 2 times, most recently from 6912c7b to 486b095 Compare May 12, 2022 03:30
@the-mikedavis the-mikedavis added the S-waiting-on-review Status: Awaiting review from a maintainer. label May 18, 2022
@archseer
Copy link
Member

archseer commented Aug 4, 2022

Hey @cossonleo I want to finally get this merged but I'm trying to understand the fix. Does it immediately set a save point, then when completion data arrives it filters that based on new input?

@cossonleo
Copy link
Contributor Author

Hey @cossonleo I want to finally get this merged but I'm trying to understand the fix. Does it immediately set a save point, then when completion data arrives it filters that based on new input?

Yes, Method Completion::new() will call recompute_filter(editor) to filter the data.

@cossonleo cossonleo force-pushed the completion branch 4 times, most recently from c57e900 to a1811c8 Compare August 25, 2022 16:56
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@pickfire pickfire added the E-testing-wanted Call for participation: Experimental features suitable for testing label Sep 9, 2022
@archseer
Copy link
Member

Looks like my comments didn't get posted last time: I think moving doc.savepoint() before the callback is the only change needed to fix the completion. Why is savepoint now storing version?

@cossonleo
Copy link
Contributor Author

Looks like my comments didn't get posted last time: I think moving doc.savepoint() before the callback is the only change needed to fix the completion. Why is savepoint now storing version?

We should discard the completion's result responsed after trigger the next completion. So we need to store the version to determine whether to discard.

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Sep 13, 2022
Comment on lines +2914 to +2916
if !config.auto_completion {
return;
}
Copy link
Contributor

@pickfire pickfire Sep 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add one more check to show completion beside this check, it's to check whether it's running from a macro (or repeat_last_motion), I notice this erratic behavior of completion popping up (and sometimes not closing and sometimes quickly switching between multiple different completions) when I type ..

This is already an issue in master but since you have this patch I think might as well just put a comment here.

@sbromberger
Copy link
Contributor

sbromberger commented Oct 6, 2022

On my system, this patch causes completion to fail completely. Logs show

2022-10-06T17:19:39.010 helix_lsp::transport [ERROR] err: <- StreamClosed
2022-10-06T17:19:39.012 helix_lsp::transport [ERROR] err: <- StreamClosed

This is gopls 0.9.5 with go 1.19.2.

EDIT: with hx -vvv, completion works, but I still get the problems described in #4131.

@rcorre
Copy link
Contributor

rcorre commented Feb 9, 2023

I don't have enough context to be confident in my conflict resolutions, but I merged this into master at rcorre@51f8b67 and it seems to fix #4851 for me.

@pascalkuthe pascalkuthe self-assigned this Feb 9, 2023
@pascalkuthe pascalkuthe removed this from the next milestone Mar 3, 2023
@pascalkuthe
Copy link
Member

Superseded by #6173

@archseer archseer closed this Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements E-testing-wanted Call for participation: Experimental features suitable for testing S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LSP completion repeats character in gdscript
8 participants