Proposal: improved completion results #391

Sarcasm · 2018-02-01T19:24:05Z

First,
Thanks for cquery, this is a very promising project!

I'd like to propose some changes to the completion results.
You can find attached to this PR a draft implementation,
if you want to try it out.
I will be willing to clean it if there is interest in the changes I propose.

First, a list of the few things that I think could be improved:

The detail, which is the full signature, repeats the name of the candidate.
This is mostly annoying an issue when the detail is "inline" (Ctrl + Space in VS Code).
In C++, it's possible to have multiple function with the same name,
but different arguments,
it is interesting to be able to pick up the right function at a glimpse,
with the snippet completing the arguments.

My solutions:

Show the contextual parent of the candidate in the details,
this is a new information we didn't have before.
Show the function/variable signature instead simple identifier for the label.
This allow one to pick up the right signature directly.
Expand default arguments into multiple items,
so that you can pick up the right signature directly,
the same way you do with overloaded functions.

Before/After on custom/simplified struct:

Before/After with STL, heavy on the signature:

For reference, the boilerplate code I used for quick testing:

#include <string>

struct Abc {
  /// A function with 2 defaults arguments.
  void defarg(int a = 0, int b = 0);

  /// This overload takes an integer.
  void overloaded(int x);

  /// This overload takes a double.
  void overloaded(double x);

  int foobiz = 42;
};

void someFn(std::string &s) {
   
}

What do you think?
If you are positive, I will be glad to cleanup the PR for proper submission.

MaskRay · 2018-02-01T19:29:52Z

third_party/string_view.h

@@ -1,6 +1,2 @@
-#if __cplusplus >= 201703L || defined(__has_include) && __has_include(<string_view>)


#390

On Mac OS X, Xcode somehow includes <string_view> so these lines cannot be removed

I wanted to sync my repository before submitting and this issue popped up, so this was just a quickfix.
This change should be a separate issue, which I assume others will encounter soon.

jacobdufault · 2018-02-01T19:41:01Z

These changes sound great. We should keep the current name/detail split as an option though. Thanks for working on this!

MaskRay · 2018-02-01T20:19:41Z

src/clang_complete.cc


-      case CXCompletionChunk_LeftParen:


I can clean this a bit to make you easier..

MaskRay · 2018-02-01T20:27:07Z

I agree that displaying the comments of the contect (e.g. abc. in your example) will be useful.

Emacs company-lsp + company-quickhelp

Does it look as nice in Emacs with your PR?

Sarcasm · 2018-02-01T21:08:08Z

Yeah it looks reasonable in Emacs, but I'd like to make some fixes to company-lsp once I get the feature into cquery.

It's a bit difficult because VS Code seems to be tailored for LSP, but company mode has to adapt to look as cool, but I'm sure we can get something good too!

We should keep the current name/detail split as an option though.

Ok, I will look at making this customizable then.

MaskRay · 2018-02-02T00:25:05Z

ClangCompleteManager has a member Config* config_ which is the initialization options. You may add a struct Completion option in https://github.com/cquery-project/cquery/blob/master/src/config.h#L131

Sarcasm · 2018-02-04T01:39:36Z

Thanks @MaskRay for the pointer to the config, that was very helpful.
I cleaned the patch and fixed a bunch of stuff.
Now the sorting/filterting of completions is working, it wasn't right in my previous attempt.

Here are some screenshots:

VS Code

Emacs + company-lsp

Some completion UI, such as Emacs' completion-at-point and company-lsp, display completion item label and detail side by side. This does not look right, when you see things like: "foo" "int foo()" "bar" "void bar(int i = 0)" When this option is enabled, the completion item label is very detailed, it shows the full signature of the candidate. The detail just contains the completion item parent context. Also, in this mode, functions with default arguments, generates one more item per default argument so that the right function call can be selected. That is, you get something like: "int foo()" "Foo" "void bar()" "Foo" "void bar(int i = 0)" "Foo" Be wary, this is quickly quite verbose, items can end up truncated by the UIs.

MaskRay · 2018-02-04T04:41:29Z

src/messages/text_document_completion.cc

+  const auto lhsLabelLength = lhs.label.length();
+  const auto rhsFilterTextLength = rhs.filterText.length();
+  const auto rhsLabelLength = rhs.label.length();
+  return std::tie(lhs.found_, lhs.skip_, lhs.priority_, lhsFilterTextLength,


!lhs.found_ ?

Thanks, good catch, unexpected last-minute merge.

MaskRay · 2018-02-04T04:43:35Z

src/clang_complete.cc

+
+    std::string text;
+    switch (kind) {
+    case CXCompletionChunk_LeftParen:       text = '(';  break;


line 168 contains

case CXCompletionChunk_LeftParen: text = '('; break; case CXCompletionChunk_RightParen: text = ')'; break;

Can they be less duplicated

Not sure to understand, what do you mean?

There is another chunk of code below where these character branches (case CXCompletionChunk_LeftParen) and string literals text = '(' are duplicated

MaskRay · 2018-02-04T04:58:17Z

Thx!

Merged in 16dd874

MaskRay reviewed Feb 1, 2018

View reviewed changes

src/clang_complete.cc Outdated

case CXCompletionChunk_LeftParen:

Copy link

Contributor

MaskRay Feb 1, 2018

Choose a reason for hiding this comment

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

I can clean this a bit to make you easier..

MaskRay force-pushed the master branch 6 times, most recently from 5965eba to fc48e1d Compare February 2, 2018 20:33

Sarcasm force-pushed the master branch from ea1984e to 04690c8 Compare February 4, 2018 01:27

Sarcasm force-pushed the master branch from 04690c8 to f47e972 Compare February 4, 2018 01:41

Sarcasm force-pushed the master branch from f47e972 to e516f30 Compare February 4, 2018 02:03

MaskRay force-pushed the master branch from 0a356aa to 0a9b670 Compare February 4, 2018 03:51

MaskRay reviewed Feb 4, 2018

View reviewed changes

MaskRay closed this Feb 4, 2018

MaskRay mentioned this pull request Feb 4, 2018

Add +tools/lsp layer for lsp-mode & lsp-ui syl20bnr/spacemacs#10211

Merged

MaskRay mentioned this pull request Mar 12, 2019

Default value of option completion.detailedLabel makes ccls unusable in lsp compliant implementations MaskRay/ccls#318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: improved completion results #391

Proposal: improved completion results #391

Sarcasm commented Feb 1, 2018

MaskRay Feb 1, 2018

Sarcasm Feb 1, 2018

jacobdufault commented Feb 1, 2018

MaskRay Feb 1, 2018

MaskRay commented Feb 1, 2018 •

edited

Sarcasm commented Feb 1, 2018

MaskRay commented Feb 2, 2018

Sarcasm commented Feb 4, 2018

MaskRay Feb 4, 2018

Sarcasm Feb 4, 2018 •

edited

MaskRay Feb 4, 2018

Sarcasm Feb 4, 2018

MaskRay Feb 4, 2018

MaskRay commented Feb 4, 2018

		@@ -1,6 +1,2 @@
		#if __cplusplus >= 201703L \|\| defined(__has_include) && __has_include(<string_view>)

Proposal: improved completion results #391

Proposal: improved completion results #391

Conversation

Sarcasm commented Feb 1, 2018

MaskRay Feb 1, 2018

Choose a reason for hiding this comment

Sarcasm Feb 1, 2018

Choose a reason for hiding this comment

jacobdufault commented Feb 1, 2018

MaskRay Feb 1, 2018

Choose a reason for hiding this comment

MaskRay commented Feb 1, 2018 • edited

Sarcasm commented Feb 1, 2018

MaskRay commented Feb 2, 2018

Sarcasm commented Feb 4, 2018

MaskRay Feb 4, 2018

Choose a reason for hiding this comment

Sarcasm Feb 4, 2018 • edited

Choose a reason for hiding this comment

MaskRay Feb 4, 2018

Choose a reason for hiding this comment

Sarcasm Feb 4, 2018

Choose a reason for hiding this comment

MaskRay Feb 4, 2018

Choose a reason for hiding this comment

MaskRay commented Feb 4, 2018

MaskRay commented Feb 1, 2018 •

edited

Sarcasm Feb 4, 2018 •

edited