Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 349396 - [1.7][formatter] Line wrapping and indentation options for try with resources
Summary: [1.7][formatter] Line wrapping and indentation options for try with resources
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: 3.7.1   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 349798
  Show dependency tree
 
Reported: 2011-06-15 01:23 EDT by Ayushman Jain CLA
Modified: 2011-08-05 02:54 EDT (History)
4 users (show)

See Also:
Olivier_Thomann: review+


Attachments
proposed fix v1.0 + regression tests (66.43 KB, patch)
2011-06-20 04:18 EDT, Ayushman Jain CLA
no flags Details | Diff
patch to correct spacing (15.62 KB, patch)
2011-06-27 10:58 EDT, Ayushman Jain CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ayushman Jain CLA 2011-06-15 01:23:24 EDT
BETA_JAVA7

For the following case

  void foo() throws Throwable {
        try (FileReader reader1 = new FileReader("file1");
                FileReader reader2 = new FileReader("file2");
                        FileReader reader3 = new FileReader("file3");
                                FileReader reader4 = new FileReader("file4")) {
                            int ch;
                            while ((ch = reader1.read()) != -1) {
                                System.out.println(ch);
                            }
         }
  }

We should give an option to the user under "Line wrapping" to decide how he wants the resources to be wrapped and indented. This would be on the lines of line wrapping for method declaration's parameters.
Comment 1 Ayushman Jain CLA 2011-06-20 04:18:19 EDT
Created attachment 198240 [details]
proposed fix v1.0 + regression tests

The patch introduces support for adding formatter options under the "line wrapping" and "white space" tabs in the formatter preferences. The formatting of the try with resources statement is done via a new method org.eclipse.jdt.internal.formatter.CodeFormatterVisitor.formatTryResources(TryStatement, boolean, boolean, boolean, boolean, boolean, int)

The default formatting preference (and also the Java conventions prefs) for a try with resources statement is "Wrap all elements, every element on a new line" and default indentation is "Default indentation". Added tests in FormatterRegressionTests#test750()... test765().
Comment 2 Ayushman Jain CLA 2011-06-20 04:18:45 EDT
Satyam, can you please review? Thanks!
Comment 3 Satyam Kandula CLA 2011-06-20 07:03:11 EDT
Olivier, Can you review this? I think it is better if you review.
Comment 4 Olivier Thomann CLA 2011-06-24 13:46:58 EDT
Looks good.
Comment 5 Ayushman Jain CLA 2011-06-27 01:41:26 EDT
Released in BETA_JAVA7 branch.
Comment 6 Markus Keller CLA 2011-06-27 06:35:58 EDT
The default for DefaultCodeFormatterConstants#
FORMATTER_INSERT_SPACE_BEFORE_OPENING_PAREN_IN_TRY is wrong (should be INSERT).

The initializations in DefaultCodeFormatterOptions# insert_space_before_opening_paren_in_try also need correction.

These workarounds for this bug in FormatterRegressionTests should also be removed:
this.formatterOptions.put(DefaultCodeFormatterConstants.FORMATTER_INSERT_SPACE_BEFORE_OPENING_PAREN_IN_TRY, JavaCore.INSERT);
Comment 7 Ayushman Jain CLA 2011-06-27 06:47:16 EDT
(In reply to comment #6)
> The default for DefaultCodeFormatterConstants#
> FORMATTER_INSERT_SPACE_BEFORE_OPENING_PAREN_IN_TRY is wrong (should be INSERT).

Any particular reason why you want a space before the opening paren? Currently, in both java conventions and Eclipse built in profiles, we do not insert a space before opening paren in method/construction declarations, method invocations, etc, although we do insert space for expressions such as for, if, etc.
Comment 8 Markus Keller CLA 2011-06-27 06:56:59 EDT
> Any particular reason why you want a space before the opening paren?

Yes, we just follow the Java code conventions:
http://www.oracle.com/technetwork/java/codeconventions-141388.html#682

8.2 Blank Spaces:

A keyword followed by a parenthesis should be separated by a space.
[..]
Note that a blank space should not be used between a method name and its opening parenthesis. This helps to distinguish keywords from method calls.
Comment 9 Markus Keller CLA 2011-06-27 06:56:59 EDT
> Any particular reason why you want a space before the opening paren?

Yes, we just follow the Java code conventions:
http://www.oracle.com/technetwork/java/codeconventions-141388.html#682

8.2 Blank Spaces:

A keyword followed by a parenthesis should be separated by a space.
[..]
Note that a blank space should not be used between a method name and its opening parenthesis. This helps to distinguish keywords from method calls.
Comment 10 Markus Keller CLA 2011-06-27 06:58:17 EDT
(In reply to comment #9)
Wow, does Google Chrome duplicate every comment now?
Comment 11 Ayushman Jain CLA 2011-06-27 10:58:38 EDT
Created attachment 198656 [details]
patch to correct spacing

The patch also fixes a latent bug. I now pass Alignment.R_OUTERMOST as tie breaking strategy while creating the alignment to make sure formatting of the try with resource clause doesn;t get broken by the local declaration statements constituting it.
Comment 12 Ayushman Jain CLA 2011-06-27 11:02:41 EDT
Released
Comment 13 Deepak Azad CLA 2011-07-19 08:08:17 EDT
Verified with v20110714-1300.