Bug 424256

Comment 1 Andreas Sewe 2014-08-08 07:59:43 EDT

I think there's a fundamental problem with using templates as a substitute for "normal" code completion. With a template called "${array}" the following would work:

  $arr<ctrl+space>

But with templates I don't see a way to make

  ${myField:fi<ctrl+space>

work.

Besides, I find it very confusing to have a "Template > Templates" preference page. IMHO, our editor shouldn't have the template feature enabled for itself at all. I don't think there are very many useful "meta-templates" and it may confuse users.

Comment 2 Stefan Prisca 2014-08-08 08:22:30 EDT

Hi Andreas
>But with templates I don't see a way to make
${myField:fi<ctrl+space>
work

We could have normal code proposals for this case, just like we have for simple variables.

The reason I decided to use template proposals is because it gives a nice way to complete the fields in full template variables.
i.e. after inserting a template proposal, you'll just jump from one field to the other with Tab and complete them.
And also, templates are only used for full variables (like ${id:newName(String)} or ${:import(type)}).

On the other hand, I agree that it is odd to have a preference page named Templates.

This being said, I have nothing against using only normal code proposals, and removing the template feature from our project.

Comment 3 Stefan Prisca 2014-08-08 08:30:19 EDT

Also, I forgot to mention.

There are the so-called "dynamic" templates (see [1]) that we could use to make template completion proposals in situations like 

${myField:fi<ctrl+space>

This means that we'd have a list of template proposals like newName(${type}), field(${type}), etc. that would fit in the above scenario and allow a nice field completion for template types.

For example, ${myField:fi<ctrl+space> would give the field(${type}) template proposal.

We can do this by modifying the PrefixMatcher of the context (just like in the case of normal proposals).

I just wanted to say that there is this possibility as well :-).

[1]: https://blogs.itemis.de/leipzig/archives/750

Comment 4 Andreas Sewe 2014-08-08 08:38:01 EDT

Hi Stefan. I don't disagree that you can do pretty powerful things with templates. However, I think that, at the moment, we should try to get classic code completion working (we will need it sooner or later anyway) in the simple cases:

  ${arr<ctrl+space>
  ${myField:fi<ctrl+space>
  retu<ctrl+space>

In other words, I would like to see (context-dependent: Java vs. template) keyword completion first. I think that is the simplest case and a good place to get started with classic code completion. It would also test the ability of our grammar to reliably distinguish between Java and template keyword situations.

So, can you stash the template-based completion approach for the moment and try to get keyword completion first?

Comment 5 Stefan Prisca 2014-08-08 09:20:49 EDT

Ok.

I'll put template proposals aside then.

>I would like to see (context-dependent: Java vs. template) keyword completion >first.

From what I have seen, the problem is that the editor only sees template elements, as the only rule that is not a Data Type rule in our grammar is the Template rule, and this contains template elements. This means that the editor does not know if there is an Escape or a Text element. It only knows there is a TemplateElement.

So in order to make the difference between contexts, I think we'll have to dig in the AST as we did with syntax highlighting, and see what rule is used to parse what token. I would really not want to do this.


What do you think if we would just have a list of template and java proposals combined, and only show those that match the prefix in some way. 

The shown list will then contain the proposals that start with the prefix first, and then those that just contain it.(the ones that start with the prefix have higher priority)

For example, let's take the following cases:

   (1) r<ctrl+space>


The list of proposals will start with "return", as "return" starts with 'r' and also contain "${array}" somewhere at the end as it contains 'r'.

   (2) ${a<ctrl+space>

The list of proposals will start with ${array}, ${array_element}, ${array_type} and so on.


This will kinda separate java proposals from template proposals based on the simple fact the template proposals start with '${'.

What I am trying to say is, instead of finding the context from the grammar, just do a simple string match and see what proposals would work in that scenario.

Comment 6 Stefan Prisca 2014-08-08 11:15:38 EDT

I've been playing around a bit with the grammar and found another solution (although you may not like this).

If we change the rule Escape, Variable, FullVariable and Text to no longer be data type rules but instead be parser rules, then the editor will be able to find the context itself.

What I mean is:

(1) Change the rules to:

Escape:
    {Variable}
    '$' ('{' var = Variable? '}' | '$');

Variable:
    FullVariable
    | => variable = TemplateIdentifier;

FullVariable:
    TemplateIdentifier ':' variable = FieldTemplateKeyword '(' Type (',' Type)* ')'
    |
....


Text:
    text = (JavaKeyword | JavaIdentifier | JavaLiteral ... );

(2) Override the following methods in the TemplateProposalProvider class:

-> override public void completeVariable_Variable to give a list of proposals in the case of simple variables (possible proposals : ${array}, ${collection}, etc)

-> override public void completeFullVariable_Variable to give the list of proposals for full variables (possible proposals : newName, newType, argType, etc)

-> override public void completeText_Text to give the list of proposals for java keywords (possible proposals: return, class, void, abstract, etc.)


IMHO this would be the best way to go, because we'll allow Xtext to determine the context.

Comment 7 Andreas Sewe 2014-08-15 02:55:14 EDT

(In reply to Stefan Prisca from comment #6)
> I've been playing around a bit with the grammar and found another solution
> (although you may not like this).
> 
> If we change the rule Escape, Variable, FullVariable and Text to no longer
> be data type rules but instead be parser rules, then the editor will be able
> to find the context itself.
> 
> What I mean is:
> 
> (1) Change the rules to:
> 
> Escape:
>     {Variable}
>     '$' ('{' var = Variable? '}' | '$');
> 
> Variable:
>     FullVariable
>     | => variable = TemplateIdentifier;
> 
> FullVariable:
>     TemplateIdentifier ':' variable = FieldTemplateKeyword '(' Type (','
> Type)* ')'
>     |
> ....
>
> Text:
>     text = (JavaKeyword | JavaIdentifier | JavaLiteral ... );

So the new bits are the "var = " / "variable =" parts (why var vs. variable?). If that's the case, that looks like an easy enough change.

> (2) Override the following methods in the TemplateProposalProvider class:
> 
> -> override public void completeVariable_Variable to give a list of
> proposals in the case of simple variables (possible proposals : ${array},
> ${collection}, etc)
> 
> -> override public void completeFullVariable_Variable to give the list of
> proposals for full variables (possible proposals : newName, newType,
> argType, etc)
> 
> -> override public void completeText_Text to give the list of proposals for
> java keywords (possible proposals: return, class, void, abstract, etc.)
> 
> IMHO this would be the best way to go, because we'll allow Xtext to
> determine the context.

Sounds good.

One question, though. Would the following work:

  Text:
    (keyword = JavaKeyword | JavaIdentifier | JavaLiteral ... );

  override public void completeText_Keyword

The intention is to let XText make the case distinction and only call you completeText_Keyword method if we are (or can be) in the first case?

Comment 8 Stefan Prisca 2014-08-15 03:11:02 EDT

>So the new bits are the "var = " / "variable =" parts (why var vs. variable?). >If that's the case, that looks like an easy enough change.

Yes, these are the only changes.


>One question, though. Would the following work:

It would work, yes. And it is more specific to the case. Thanks for the idea! :-)

Comment 9 Stefan Prisca 2014-08-15 12:46:07 EDT

>why var vs. variable?

I did not notice this question, sorry. The code I wrote here was just to present the idea :-). So in the grammar it will be variable, not var.


I pushed another patch-set to gerrit. I made some small changes to the grammar and removed the templates folder.

I also tried to add tests for content assist, but it seems to be more difficult than expected. 

From what I understand (I found some explanation in a forum discussion, but can't find it anymore), the first two parameters form a code completion method ( for a method completeA_B(..) ):

*EObject model: the rule inside which code completion is triggered.

  e.g.: for A: name = B; A is the model


*Assignment assignment: the targeted rule for code completion

  e.g.: for A: name = B; B is the assignment.

I don't think we can simply create them as we did with syntax highlighting. These are provided by the framework at runtime somehow.

I found a class that Xtext provides for content assist testing: AbstractContentAssistProcessorTest (See the forum discussion [1])
The problem is that the test needs to be run as JUnit plug-in test, and it creates a local testing workspace. 
I was thinking to combine this with mockito and see if there is a way to pass a virtual/temporary workspace to Xtext's testing class in order to avoid creating a local workspace for testing.

But this might take some time, so maybe it would be best to do it in some other commit, and merge the one that introduces code proposals([2]). What do you think?

[1]: http://www.eclipse.org/forums/index.php?t=msg&th=227257/
[2]: https://git.eclipse.org/r/#/c/31112/

Comment 10 Andreas Sewe 2014-08-15 13:01:12 EDT

(In reply to Stefan Prisca from comment #9)
> I found a class that Xtext provides for content assist testing:
> AbstractContentAssistProcessorTest (See the forum discussion [1])
> The problem is that the test needs to be run as JUnit plug-in test, and it
> creates a local testing workspace. 

Why is "JUnit plug-in test" a problem? That's what Tycho does at runtime anyway. Or maybe I misunderstood you.

> I was thinking to combine this with mockito and see if there is a way to
> pass a virtual/temporary workspace to Xtext's testing class in order to
> avoid creating a local workspace for testing.
> 
> But this might take some time, so maybe it would be best to do it in some
> other commit, and merge the one that introduces code proposals([2]). What do
> you think?

Sounds good. I won't have much time to do reviews right now (Recommenders 2.1.6 is due soon) and would like to concentrate my efforts on the "Make snippet editor extensible" part, as that affects the main project as well and is crucial to get right the first time.

Comment 11 Stefan Prisca 2014-08-15 13:11:39 EDT

>Why is "JUnit plug-in test" a problem?

The problem is not that it is a JUnit plug-in test, but that it create a local workspace for testing.
I know I ran into this last year when writing a test unit for the editor save/save-as actions. I had to create a temporary workspace.

But anyway, I'll have to document myself a bit more. I'm saying this from what I saw after giving a first try at testing completion proposals.

The main point of the comment was to suggest postponing the tests for a while (I don't think I managed to make it clear though). :-)

>I won't have much time to do reviews right now

No problem. I'll do my best to keep the code as clean as possible ;-)

Comment 12 Stefan Prisca 2014-08-16 08:13:37 EDT

(In reply to Andreas Sewe from comment #7)

Hi Andreas,

> One question, though. Would the following work:
> 
>   Text:
>     (keyword = JavaKeyword | JavaIdentifier | JavaLiteral ... );
> 
>   override public void completeText_Keyword
> 
> The intention is to let XText make the case distinction and only call you
> completeText_Keyword method if we are (or can be) in the first case?

I made some tests after changing the Text rule like you said above. I appears that syntax highlighting does not work correctly for comments and strings if we change it like that.

My guess is that because the Text rule is no longer a data type rule, and it will only get created in the case of a JavaKeyword (as that is the only assignment in the rule), the rule hierarchy computation from the highlighting calculator does not work anymore.

I'd say that it's best to make the modification I first proposed:

Text:
    text = (JavaKeyword | JavaIdentifier | JavaLiteral ... );

Thus, the Text rule will always be created, and not influence the syntax highlighter. We will also have enough context for java keyword proposals.

I've pushed the change to gerrit: 
< https://git.eclipse.org/r/#/c/31112/5 >

Comment 13 Stefan Prisca 2014-08-18 03:59:02 EDT

Fixed by change-set 
< https://git.eclipse.org/r/#/c/31112/ >