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.

Sign up for GitHub

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

Jump to bottom

Suggested snippets for something already completed (c/c++ includes as an example) #21

Open
hicksy994 opened this issue Feb 2, 2018 · 5 comments
Open

Suggested snippets for something already completed (c/c++ includes as an example) #21

hicksy994 opened this issue Feb 2, 2018 · 5 comments

Comments

@hicksy994
Copy link

hicksy994 commented Feb 2, 2018 •
edited
Loading

As referenced at the bottom of this thread (there are also unrelated topics):

jacobdufault/cquery#401

If you complete an include header in c/c++, company-lsp seems to suggest a snippet for what you've just done.
@tigersoldier
Copy link
Owner

tigersoldier commented Feb 3, 2018 •
edited
Loading

This is caused by #20. The reason why it is triggering self completion incorrectly is that company-lspthinks the prefix for #includestatements does not contain#, while cquery treats #` as part of the prefix.

The results of this is:

  1. Company completes using the label, the line after company completion is ##include <...>
  2. Company-lsp recognized the textEdit value and replaced the line with #include <...>
  3. Because the lines before and after textEdit are different, self-completion is triggered
  4. cquery sends completion for the same header file.

@Sarcasm I'll use the completion suffix to detect if self-completion should be triggered. The current logic not only causes issue with cquery, but also java auto-import.

tigersoldier added a commit that referenced this issue Feb 3, 2018
@tigersoldier
Fix re-triggering condition.
47020a3 
The current logic checks whether (point) changes before and after post
completion actions to determine if re-triggering completion is desired. However,
this logic is not robust enough. It incorrectly re-triggers completion when the
post-completion action adds import statements by Javacomp, or fixes the
duplicated `#` characters by cquery (see #21).

This change proposes comparing the suffix of the completion before and after
post-completion editing. I tested that it still works for the desired case
(std::) while not re-triggering completion for either the cquery include or
Javacomp auto-import case.
@tigersoldier tigersoldier mentioned this issue Feb 3, 2018
Merged
tigersoldier added a commit that referenced this issue Feb 3, 2018
@tigersoldier
Fix re-triggering condition.
798aa0e 
The current logic checks whether (point) changes before and after post
completion actions to determine if re-triggering completion is desired. However,
this logic is not robust enough. It incorrectly re-triggers completion when the
post-completion action adds import statements by Javacomp, or fixes the
duplicated `#` characters by cquery (see #21).

This change proposes comparing the suffix of the completion before and after
post-completion editing. I tested that it still works for the desired case
(std::) while not re-triggering completion for either the cquery include or
Javacomp auto-import case.
tigersoldier added a commit that referenced this issue Feb 5, 2018
@tigersoldier
Fix re-triggering condition.
8e1df31 
The current logic checks whether (point) changes before and after post
completion actions to determine if re-triggering completion is desired. However,
this logic is not robust enough. It incorrectly re-triggers completion when the
post-completion action adds import statements by Javacomp, or fixes the
duplicated # characters by cquery (see #21).

This change proposes only re-trigger completion when the completed text before
point is any of the trigger characters. An additional custom variable is added
allowing users to turn this feature on/off.

I tested that it still works for the desired case (std::) while not
re-triggering completion for Javacomp auto-import case. However, it still
re-triggers completion for the cquery include case because `>' is one of the
trigger characters. Due to this reason, this feature is turned off by default.
tigersoldier added a commit that referenced this issue Feb 5, 2018
@tigersoldier
Fix re-triggering condition.
74242f1 
The current logic checks whether (point) changes before and after post
completion actions to determine if re-triggering completion is desired. However,
this logic is not robust enough. It incorrectly re-triggers completion when the
post-completion action adds import statements by Javacomp, or fixes the
duplicated # characters by cquery (see #21).

This change proposes only re-trigger completion when the completed text before
point is any of the trigger characters. An additional custom variable is added
allowing users to turn this feature on/off.

I tested that it still works for the desired case (std::) while not
re-triggering completion for Javacomp auto-import case. However, it still
re-triggers completion for the cquery include case because `>' is one of the
trigger characters. Due to this reason, this feature is turned off by default.
@tigersoldier tigersoldier mentioned this issue Feb 8, 2018
Merged
@MaskRay
Copy link
Contributor

MaskRay commented Feb 18, 2018

@scturtle

@scturtle
Copy link
Contributor

I do not remember being bothered with this issue. What can I do for help?

@tigersoldier
Copy link
Owner

I should have updated this issue. The issue is now mitigated by #22. I kept this issue open because the root cause is lsp-mode/company-lsp and cquery has different view of the completion prefix. If a user completes at this point:

#include|

where | denotes the cursor, or (point) in Emacs, cquery sees #include as the prefix, while lsp-mode and company-lsp sees include as the prefix.

The result is, all candidates returned by cquery are prefixed with #, which don't match the company-lsp prefix. When a candidate is chosen, it replaces the company-lsp prefix, resulting two leading # characters.

Users don't see two leading # characters because both company-lsp and cquery support textEdit. By applying textEdit, the content is fixed and both the editor and the server share the same view of content. However, for clients that don't support textEdit, the two leading #s are visible to users. This can be verified by using the plain complete-at-point command to complete right after # in Emacs.

The best solution is allowing clients to define their own functions to get the prefix bounds. This requires fundamental changes to lsp-mode. I have a PR for lsp-mode to enable this: emacs-lsp/lsp-mode#281

@sachdevaprash
Copy link

sachdevaprash commented Nov 8, 2018 •
edited
Loading

Is there an update on this? lsp-mode has that PR merged to master, and I believe I am up to date with all components of lsp and cquery, but I face the same issue(jacobdufault/cquery#401).

Edit: To add to this, all candidates are not just prefixed with "#" but "#include <" for me, all of the string being redundant since that was the triggering prefix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@tigersoldier @scturtle @MaskRay @sachdevaprash @hicksy994