Page MenuHomePhabricator

[CodeComplete] Don't replace the rest of line in #include completion.
ClosedPublic

Authored by hokein on Mar 25 2020, 6:23 AM.

Details

Summary

The previous behavior was aggressive,

#include "abc/f^/abc.h"
              foo/  -> candidate

"f/abc.h" is replaced with "foo/", this patch will preserve the "abc.h".

Diff Detail

Event Timeline

hokein created this revision.Mar 25 2020, 6:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2020, 6:23 AM

Does this regress the case where you complete a file rather than a directory?
e.g. #include "foo/^one.h"
If you select "two.h", then we definitely want to replace "one.h", right?

The root here is that we have to decide what to replace independently of the completion item.
One compromise might be to replace up to the quote, but if you're completing "foo/^one.h" and bar is a directory, check if foo/bar/one.h exists and offer it as a completion. Seems complicated though.

Or we can take the tradeoff here if it's better but it'd be good to understand why.

Does this regress the case where you complete a file rather than a directory?
e.g. #include "foo/^one.h"
If you select "two.h", then we definitely want to replace "one.h", right?

yes, it has a regression, but we don't change the behavior for the above case. A regression case is like

#include "foo/^dir1/one.h"
// with two candidates: dir2 and foo.h

if we choose 2), after completion, "foo/foo.h/one.h" (now) vs foo/foo.h (before)

The root here is that we have to decide what to replace independently of the completion item.
One compromise might be to replace up to the quote, but if you're completing "foo/^one.h" and bar is a directory, check if foo/bar/one.h exists and offer it as a completion. Seems complicated though.

agree, it doesn't seem to be worth the effort.

Or we can take the tradeoff here if it's better but it'd be good to understand why.

it was originally reported by one internal user.

I think the new behavior (with this patch)

  • is more aligned with mental model of code completion (most people think code completion just inserts the code text at the cursor position )
  • keeps the consistent behavior with other code completions, e.g. foo.^member() if you select the member in code completion, another member text will be inserted
sammccall accepted this revision.Mar 26 2020, 1:43 AM

yes, it has a regression, but we don't change the behavior for the above case. A regression case is like

Oops, right :-)

Or we can take the tradeoff here if it's better but it'd be good to understand why.

it was originally reported by one internal user.

I think the new behavior (with this patch)

  • is more aligned with mental model of code completion (most people think code completion just inserts the code text at the cursor position )
  • keeps the consistent behavior with other code completions, e.g. foo.^member() if you select the member in code completion, another member text will be inserted

OK, let's try it and see if people complain.
(There's also an argument for only replacing the text on the left of the cursor, that's more consistent with completion in general though I think less helpful too)

This revision is now accepted and ready to land.Mar 26 2020, 1:43 AM
This revision was automatically updated to reflect the committed changes.