Bug 28241 - Smart edit: curly braces are not so smart
Summary: Smart edit: curly braces are not so smart
Status: RESOLVED INVALID
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Text (show other bugs)
Version: 2.1   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: JDT-Text-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: needinfo
Depends on: 38565
Blocks:
  Show dependency tree
 
Reported: 2002-12-13 04:29 EST by Andre Weinand CLA
Modified: 2007-06-22 10:04 EDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Weinand CLA 2002-12-13 04:29:48 EST
With this piece of code:

if (true)
    System.out.println("hi");

enter a '{'  after the first closing parenthesis.
Result:

if (true) {
}
    System.out.println("hi");

I would expect:

if (true) {
    System.out.println("hi");
}
Comment 1 Bob Foster CLA 2002-12-22 14:22:09 EST
Be careful what you wish for. In M4, with this piece of code:

boolean foo() {
  return false;
}

Now try to insert a conditional statement before the return. You start to type:

boolean foo() {
  if (true)
  return false;
}

enter a '{' after the first closing parenthesis. Result:

boolean foo() {
  if (true) {
  return false;
  }
}

The following line should only be contained in the braces if it is indented with
respect to the conditional. Yes, this is a heuristic, but the current behavior
is horrible at least as often as it is helpful.
Comment 2 Ron Baldwin CLA 2003-02-22 17:21:19 EST
Related to (dup of?) bug 32082.
Comment 3 Bob Foster CLA 2003-02-24 18:33:12 EST
This is still broken in RC1. Here is a typical example:

private boolean newMethod() {
  return false;
}

Now try to insert a conditional in front of the return:

private boolean newMethod() {
  if (condition) {
  return false;
}

Which yields:

private boolean newMethod() {
  if (condition) {
  return false;
  }
}

Generating an error! If you can't do this right (and there are a number of 
cases to consider) please don't do it at all. Here is the only acceptable 
result for the example above:

private boolean newMethod() {
  if (condition) {
    //cursor here
  }
  return false;
}
Comment 4 Mariano Kamp CLA 2003-07-16 09:18:26 EDT
I am using 200307080010 and have similar, if not the same, findings as reported here. 
Bob, I also had a look at your last posting and it seems to me that it is now working 
fine. This might be due to a code change or to the lack of details in what order you 
pressed which key. 
 
When considering this code: 
 
private boolean newMethod() { 
  <a> 
  return false; 
} 
 
Entering "if (condition) {" at the position marked <a> nothing happens. If you press 
enter the code looks like this: 
 
		private boolean newMethod() { 
		  if (condition) { 
		  	 
		  } 
		  return false; 
		} 
 
Which I believe is what you called "the only acceptable behavior", isn't it. 
 
But the following case seems broken to me. Consider you want to add a trace 
statement to the following code: 
 
if (a==1) <a> 
  a=2; 
else 
  a=3; 
 
The result should look like this: 
if (a==1) { 
  System.out.println("a==1"); 
  a=2; 
} 
else 
  a=3; 
 
When entering "{" at <a> nothing happens, but if you press "Enter" here the code looks 
like this: 
if (a==1) { 
  <cursor is here> 
} 
  a=2; 
else 
  a=3; 
 
The result is a syntax error and more work then without smart braces. 
I would expect to put the closing curly brace after a=2; and put the cursor in an empty 
line before or after this line. 
Comment 5 Bob Foster CLA 2003-07-16 16:03:28 EDT
You object to the editor turning:

void foo() {
  if (a==1)
    a = 2;
  else
    a = 3;
}

into

void foo() {
  if (a==1) {
  }
    a = 2;
  else
    a = 3;
}

and would prefer to see

void foo() {
  if (a==1) {
    
    a = 2;
  }
  else
    a = 3;
}

And so would anyone. But beware of simple cases. What about this one?

void foo() {
  if (a==1)
  bar(a);
}

If the editor turns this into:

void foo() {
  if (a==1) {
    
    bar(a);
  }
}

it is no more likely to be right than in the annoying suck-in-the-return case,
even though the result isn't a syntax error.

I previously suggested that the editor should take into account indentation,
since it is the most reliable guide that a following statement is intended to be
subordinate to the if and doesn't just happen to be the statement before which
an if is inserted, but my suggestion was ignored.

If the editor must be dumb about it, I prefer it not to suck in the return, as
this hits me about 90% of the time, whereas I only have to cut and paste a line
into the if about 50% of the time. ;-}
Comment 6 Mariano Kamp CLA 2003-07-16 16:23:51 EDT
>And so would anyone. But beware of simple cases. What about this one? 
 
>void foo() { 
>  if (a==1) 
>  bar(a); 
>} 
 
>If the editor turns this into: 
 
>void foo() { 
>  if (a==1) { 
>     
>    bar(a); 
>  } 
>} 
Sorry, I don't get this - it's probably due to the heavy sun today ;-) 
That seems to be perfect. What's wrong with it? The compiler doesn't complain and it 
preserves the original behavior. 
The example is also a slightly different case as the conditional is already present in the 
existing code. 
 
> I previously suggested that the editor should take into account indentation, 
Hmmh. I don't know .. that doesn't feel safe to me. 
I believe it is already pretty good to rely on the Java syntax. A (one) statement xor 
block following an if belongs to the if.  
 
So it seems pretty straight forward to me: 
1) If enter is pressed after a recently typed opening curly brace then go to the next 
statement, insert a blank line, go after the statement and put the closing curly brace in. 
Don't forget to intend the statement now belonging to an if. 
2) In case the next statement is a "return": Just provide a new blank line and a closing 
curly brace. 
Did I miss something?  
Comment 7 Mariano Kamp CLA 2003-07-16 16:33:50 EDT
I thought again and now believe the missunderstandings root in the missing distinction 
of two cases in this report. 
a) enhancing an existing if statement, by transforming the formerly single statement 
body to a block and 
b) to introduce a new if statement before an existing statement. 
 
The orginal posting was talking about a) and you about b), right? 
Don't know how hard it would be to detect if an existing if statement is to be enhanced 
or a new if statement is being introduced. 
Comment 8 Bob Foster CLA 2003-07-16 17:57:43 EDT
Mariano,

Your second reply seems to contradict your first. Which is it, Java language
rules or the order of edit?

>Don't know how hard it would be to detect if an existing if statement is to be
>enhanced or a new if statement is being introduced.

Unless you or someone proposes an algorithm, I'm going to assume it is hard.

>> I previously suggested that the editor should take into account indentation, 
>Hmmh. I don't know .. that doesn't feel safe to me.

Hmmm is not an argument. ;-} What does "safe" mean? It's a heuristic that will
lead to a satisfying result more often than not for most users. I'm not arguing
that indentation should be part of the language, I'm just saying if one has
information, one should use it.

Bob


Comment 9 Tom Hofmann CLA 2003-12-11 05:34:20 EST
I propose to close this PR due to the points below. Bottomline: don't use
too-smart braces, but offer specific quick fixes for special tasks.

your comments?


- I guess we all agreed that it is hard to come up with a good heuristic that
does not do the wrong thing in 49% of the cases

- There is a new quick fix to turn simple then/loop statements into a block.
These do the job a lot better and let you select the action you really want
without having to rely on the editor guess your mood / indentation.

- The closing brace is never put around an existing statement since quite a
while. Also, the closing brace is only inserted when you enter a new line. No
surprises.

Comment 10 Dani Megert CLA 2004-01-13 08:40:45 EST
Please reopen if you disagree with Tom.
Comment 11 Bob Foster CLA 2004-01-13 12:43:29 EST
I disagree that there aren't still "smart" bugs that should be fixed, but don't
want to be a nuisance.

The original problem has been fixed. What remains is still bad enough to cause
me to turn off curly brace matching. It isn't predictable whether or not the
feature will actually add a brace. Sometimes it does, sometimes it doesn't.

It fails to generate a matching '}' most often when the next non-ws character
after a typed '{' is already a '}'. I'm not sure why this is even considered; a
dumber feature would be a better (more predictable) one.
Comment 12 Dani Megert CLA 2007-06-22 09:59:01 EDT
Get rid of deprecated state.
Comment 13 Dani Megert CLA 2007-06-22 10:04:21 EDT
.