Include required parameter names when completing a method call.

Include required parameter names when completing a method call.

When a method such as "void m(int i, int j)" is completed, instead of
simply inserting "m()", we insert "m(i, j)", we highlight "i", and we
set up linked position groups for "i" and "j" so that the user can
press tab to move from one parameter to the next.

Optional parameters (both named and positional) are not handled by
this logic--the user must still enter them manually.

R=danrubel@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42667

[Paul Berry, 2015-01-06 22:48:59]

paulberry@google.com changed reviewers:
+ danrubel@google.com

[danrubel, 2015-01-06 23:49:37]

Overall LGTM. Thanks!

https://codereview.chromium.org/837793003/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dart/DartServerProposal.java
(right):

https://codereview.chromium.org/837793003/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dart/DartServerProposal.java:189:
* Otherwise null. Computed by computeReplacement().
I recommend dropping the "Computed by ..." from all the above comments. It is
not needed to understand the field and will not be noticed by the refactoring
tools. If it is important to document this, then perhaps use @see

More specifically... there is no 'computeReplacement()' method.

https://codereview.chromium.org/837793003/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dart/DartServerProposal.java:384:
return getCompletion().substring(0, length);
Replace getCompletion() with suggestion.getCompletion() because the
prefixCompletionText should only be on the selector not the entire method
signature.

https://codereview.chromium.org/837793003/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dart/DartServerProposal.java:469:
* Compute {@link replacementString} and {@link selectionOffset} if they haven't
been computed
If you are listing specifically what it computes, do you want to list them all?
selectionLength, argumentOffsets, argumentLengths?

https://codereview.chromium.org/837793003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/suggestion_builder.dart
(right):

https://codereview.chromium.org/837793003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/suggestion_builder.dart:391:
(ParameterElement parameter) => parameter.type.displayName).toList();
Can parameter.type ever be null?

https://codereview.chromium.org/837793003/diff/1/pkg/analysis_server/test/ser...
File pkg/analysis_server/test/services/completion/invocation_computer_test.dart
(right):

https://codereview.chromium.org/837793003/diff/1/pkg/analysis_server/test/ser...
pkg/analysis_server/test/services/completion/invocation_computer_test.dart:37:
class C {
I find this left justified target source very hard to read.
Would you indent the source to match the other completion tests ?

[Paul Berry, 2015-01-07 15:39:14]

https://codereview.chromium.org/837793003/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dart/DartServerProposal.java
(right):

https://codereview.chromium.org/837793003/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dart/DartServerProposal.java:189:
* Otherwise null. Computed by computeReplacement().
On 2015/01/06 23:49:36, danrubel wrote:
> I recommend dropping the "Computed by ..." from all the above comments. It is
> not needed to understand the field and will not be noticed by the refactoring
> tools. If it is important to document this, then perhaps use @see
> 
> More specifically... there is no 'computeReplacement()' method.

Whoops, that was supposed to be computeCompletion().  IMHO, if a field isn't
valid for the full lifetime of the object, the documentation for the field
should say so.  I've fixed the comment and used a @see link.

https://codereview.chromium.org/837793003/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dart/DartServerProposal.java:384:
return getCompletion().substring(0, length);
On 2015/01/06 23:49:36, danrubel wrote:
> Replace getCompletion() with suggestion.getCompletion() because the
> prefixCompletionText should only be on the selector not the entire method
> signature.

Done.

https://codereview.chromium.org/837793003/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/dart/DartServerProposal.java:469:
* Compute {@link replacementString} and {@link selectionOffset} if they haven't
been computed
On 2015/01/06 23:49:36, danrubel wrote:
> If you are listing specifically what it computes, do you want to list them
all?
> selectionLength, argumentOffsets, argumentLengths?

Oops, yes, I do.  Fixed.

https://codereview.chromium.org/837793003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/suggestion_builder.dart
(right):

https://codereview.chromium.org/837793003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/suggestion_builder.dart:391:
(ParameterElement parameter) => parameter.type.displayName).toList();
On 2015/01/06 23:49:36, danrubel wrote:
> Can parameter.type ever be null?

My belief is that it should never be null, but I will double check with Brian
this morning.

https://codereview.chromium.org/837793003/diff/1/pkg/analysis_server/test/ser...
File pkg/analysis_server/test/services/completion/invocation_computer_test.dart
(right):

https://codereview.chromium.org/837793003/diff/1/pkg/analysis_server/test/ser...
pkg/analysis_server/test/services/completion/invocation_computer_test.dart:37:
class C {
On 2015/01/06 23:49:36, danrubel wrote:
> I find this left justified target source very hard to read.
> Would you indent the source to match the other completion tests ?

Let's talk about this in the stand-up meeting.  I have a slight personal
preference for the left justified style, but as with most stylistic questions,
what's most important to me is that we decide on a style and use it
consistently.  Currently the vast majority of the tests in analyzer and
analysis_server use the left justified style, so if you want to lobby for a
different style, we should probably discuss it as a group.

[danrubel, 2015-01-07 15:54:57]

lgtm

[Paul Berry, 2015-01-07 16:57:49]

Committed patchset #2 (id:20001) manually as 42667 (presubmit successful).