Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 346652

Summary:	[xtext][grammarEditor] Autoedit quirks
Product:	[Modeling] TMF	Reporter:	Sebastian Zarnekow <sebastian.zarnekow>
Component:	Xtext	Assignee:	Jan Koehnlein <jan>
Status:	CLOSED FIXED	QA Contact:
Severity:	major
Priority:	P3	CC:	jan, knut.wannheden, sven.efftinge, tmf.xtext-inbox
Version:	2.0.0	Flags:	sebastian.zarnekow: juno+
Target Milestone:	M6
Hardware:	PC
OS:	Mac OS X - Carbon (unsup.)
See Also:	https://bugs.eclipse.org/bugs/show_bug.cgi?id=346032
Whiteboard:

Description Sebastian Zarnekow

2011-05-20 07:54:15 EDT

(| denotes cursor position)

grammar org.xtext.example.mydsl1.MyDsl with org.eclipse.xtext.common.Terminals

generate myDsl "http://www.xtext.org/example/mydsl1/MyDsl"

Model:
	greetings+=Greeting*;

Greeting:
	('Hello' name=ID '!'|

Hit enter and be surprised:

Greeting:
	('Hello' name=ID '!'
		
	)|
		
	);

=======================
Second example for unwanted auto edit:

Greeting:
	|'Hello' name=ID '!';

Type '(' and you'll get

Greeting:
	(|)'Hello' name=ID '!';

where the Java editor only adds the opening brace which works better imo.

Comment 1 Sven Efftinge

2011-09-26 09:07:28 EDT

maybe we should simplify the second issue such that it never adds the closing bracket if there's a non WS-char right to the cursor and make it the default for all languages.

Comment 2 Sebastian Zarnekow

2011-11-09 14:45:10 EST

Not 2.1

Comment 3 Sebastian Zarnekow

2012-01-31 15:35:08 EST

grammar org.xtext.example.mydsl.MyDsl with org.eclipse.xtext.xbase.Xbase

generate myDsl "http://www.xtext.org/example/mydsl/MyDsl"

Model:
	'package' name=QualifiedName '{'|
;

Hit enter at cursor position and you'll get

grammar org.xtext.example.mydsl.MyDsl with org.eclipse.xtext.xbase.Xbase

generate myDsl "http://www.xtext.org/example/mydsl/MyDsl"

DomainModel:
	'package' name=QualifiedName '{'
		|
	}
;

which is _really_ annoying. Set severity to major and scheduled for M6.

Comment 4 Jan Koehnlein

2012-03-02 07:53:32 EST

(In reply to comment #1)
> maybe we should simplify the second issue such that it never adds the closing
> bracket if there's a non WS-char right to the cursor and make it the default
> for all languages.

I have tried that and it feels strange, especially adding an opening parenthesis before a ';' or a closing parenthesis. 

The current rule is not to insert the closing temrinal when the following character can be part of a Java identifier. I'd rather add quotes here, too.

Comment 5 Sebastian Zarnekow

2012-03-02 07:57:07 EST

(In reply to comment #4)

> The current rule is not to insert the closing temrinal when the following
> character can be part of a Java identifier. I'd rather add quotes here, too.

+1
I think quotes and numbers should be added there.

Comment 6 Jan Koehnlein

2012-03-02 08:18:11 EST

(In reply to comment #5)
> (In reply to comment #4)
> 
> > The current rule is not to insert the closing temrinal when the following
> > character can be part of a Java identifier. I'd rather add quotes here, too.
> 
> +1
> I think quotes and numbers should be added there.

I am going for
    Character.isJavaIdentifierPart(charAtOffset) 
 || charAtOffset == '\'' || charAtOffset == '\"'

Comment 7 Sven Efftinge

2012-03-02 08:22:10 EST

> (In reply to comment #1)
> > maybe we should simplify the second issue such that it never adds the closing
> > bracket if there's a non WS-char right to the cursor and make it the default
> > for all languages.
> 
> I have tried that and it feels strange, especially adding an opening
> parenthesis before a ';' or a closing parenthesis. 

Would you expect a closing bracket when there's already one right to the cursor?

(In reply to comment #5)
> (In reply to comment #4)
> 
> > The current rule is not to insert the closing temrinal when the following
> > character can be part of a Java identifier. I'd rather add quotes here, too.
> 
> +1
> I think quotes and numbers should be added there.

What about operators?

Comment 8 Jan Koehnlein

2012-03-02 08:24:54 EST

(In reply to comment #3)
> grammar org.xtext.example.mydsl.MyDsl with org.eclipse.xtext.xbase.Xbase
> 
> generate myDsl "http://www.xtext.org/example/mydsl/MyDsl"
> 
> Model:
>     'package' name=QualifiedName '{'|
> ;
> 
> Hit enter at cursor position and you'll get
> 
> grammar org.xtext.example.mydsl.MyDsl with org.eclipse.xtext.xbase.Xbase
> 
> generate myDsl "http://www.xtext.org/example/mydsl/MyDsl"
> 
> DomainModel:
>     'package' name=QualifiedName '{'
>         |
>     }
> ;
> 
> which is _really_ annoying. Set severity to major and scheduled for M6.

Deeper testing reveals that this is likely due to broken partitioning data in
the document. The bug does not show if you just paste the example in a new
document, but it happens when you delete the inserted newline and hit enter
again. I've opened bug 373081 to solve this.

Comment 9 Jan Koehnlein

2012-03-02 08:26:43 EST

(In reply to comment #7)
> > (In reply to comment #1)
> > > maybe we should simplify the second issue such that it never adds the closing
> > > bracket if there's a non WS-char right to the cursor and make it the default
> > > for all languages.
> > 
> > I have tried that and it feels strange, especially adding an opening
> > parenthesis before a ';' or a closing parenthesis. 
> 
> Would you expect a closing bracket when there's already one right to the
> cursor?

Yes.

> (In reply to comment #5)
> > (In reply to comment #4)
> > 
> > > The current rule is not to insert the closing temrinal when the following
> > > character can be part of a Java identifier. I'd rather add quotes here, too.
> > 
> > +1
> > I think quotes and numbers should be added there.
> 
> What about operators?

Not sure. My impression is that if the following char could be something like an argument to a function call, no closing terminal should be added. This would only hold for prefix operators.

Comment 10 Sebastian Zarnekow

2012-03-02 08:37:57 EST

Yes, I think that's something language specific. We should do that for Xbase with the same heuristics as Java does.

Comment 11 Jan Koehnlein

2012-03-02 08:46:01 EST

(In reply to comment #10)
> Yes, I think that's something language specific. We should do that for Xbase
> with the same heuristics as Java does.

Not sure what exactly you're referring to :-) Could you file a separate ticket for Xbase?

I've changed the deafult behavior as stated in comment #6. It's a good sign I only had to adapt a single test our extensive autoedit test suite, that exactly refers to inserting a '(' before a quote.

I've pushed my changes to MASTER, so I assume that this particular ticket is resolved.

Comment 12 Sven Efftinge

2012-03-03 07:48:16 EST

(In reply to comment #9)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > 
> > > > The current rule is not to insert the closing temrinal when the following
> > > > character can be part of a Java identifier. I'd rather add quotes here, too.
> > > 
> > > +1
> > > I think quotes and numbers should be added there.
> > 
> > What about operators?
> 
> Not sure. My impression is that if the following char could be something like
> an argument to a function call, no closing terminal should be added. This would
> only hold for prefix operators.

I agree and think that's the point. A closing parenthesis should never be added if the following character can be the start of an 'expression'. In addition to the mentioned java identifier start, numbers, quotes (i.e. string literals) it should also include prefix operators as well as opening parenthesis. 
So I added '-', '!', '(', '{','[' to the list.

Of course we are assuming some kind of expression language here which might look very different from case to case (i.e. Xtext grammar expressions are very different to Xbase), but I think it's a good match and it's better to not insert anything automatically when in doubt. Let's se how it "feels".

Comment 13 Karsten Thoms

2017-09-19 17:33:43 EDT

Closing all bugs that were set to RESOLVED before Neon.0

Comment 14 Karsten Thoms

2017-09-19 17:44:52 EDT

Closing all bugs that were set to RESOLVED before Neon.0