Community
Participate
Working Groups
Build Identifier: 20100218-1602 There are three options for how to deal with newlines with respect to annotations -- these are not enough. I want a newline after a method annotation, but not after a field annotation. Reproducible: Always Steps to Reproduce: 1. Open Preferences 2. Go to Java -> Code Style -> Formatter 3. Edit a Profile 4. Go to the New Lines tab 5. There is an option for 'members', but it does not distinguish between 'fields' and 'methods'
Issues and enhancements dealing with the Java formatter belongs in the JDT bucket.
This cannot be done for 3.6 as it would require new API constants. Deferring for 3.7.
*** Bug 317784 has been marked as a duplicate of this bug. ***
Olivier, do you agree to target this enhancement for 3.7?
Sure.
Created attachment 172780 [details] Proposed patch This patch adds options for new lines after annotations on: - types - fields - methods - constructors - packages Not sure the last two ones were required... Of course, they can be easily removed if we do not want to introduce them.
- Javadoc of DefaultCodeFormatterConstants# FORMATTER_INSERT_NEW_LINE_AFTER_ANNOTATION_ON_MEMBER is missing: * {@link #FORMATTER_INSERT_NEW_LINE_AFTER_ANNOTATION_ON_CONSTRUCTOR} But I would drop the 'on_constructor' preference altogether. The setting on package makes sense, since it would otherwise be unspecified, but constructors are just a subspecies of methods, and I don't think it makes sense to single them out, but throw abstract, static, and instance methods into one bucket. - Problem in jdt.core compatibility layer: With project-specific settings ... org.eclipse.jdt.core.formatter.insert_new_line_after_annotation_on_local_variable=insert org.eclipse.jdt.core.formatter.insert_new_line_after_annotation_on_member=do not insert org.eclipse.jdt.core.formatter.insert_new_line_after_annotation_on_parameter=do not insert ..., I now get new lines after annotations in members. AFAICS, the problem is that JavaProject#getOptions(boolean) drops options that are not in the optionNames set. The formatter preference dialog also comes up with wrong settings in some cases (with and without the patch for bug 318010). We probably also need to tweak the ProfileVersioner. I'll look at that once the Core bugs are squashed.
Created attachment 176332 [details] New proposed patch This patch removes the constructor preference. For the compatibility issue, I observed that: 1) we got the same issue while deprecating the FORMATTER_INSERT_NEW_LINE_AFTER_ANNOTATION preference on 3.4 version and nobody complains about it since then... 2) I have added two specific tests which uses deprecated options and they behave correctly (see test727 and test728 in FormatterRegressionTests in the provided patch). JDT/Core behaves correctly in term of backward compatibility as the used deprecated option value is well set onto the new formatter options. Hence, IMO, the change needs to be done in ProfileVersioner while initializing the profiles. Deprecated options need to be converted similarly as they are in DefaultCodeFormatterOptions.setDeprecatedOptions(Map). Then the options map keys will correct when got from the project or from JavaCore. Note that these options are internal since preferences are used in JDT/Core and so they are not supposed to contain any deprecated keys...
Created attachment 176715 [details] example project In this example project, I20100810-0800 behaves properly (only inserts new line in local variable). But with the patch, I get newlines everywhere except for parameters. I think the problem of your tests is that they don't read the options from the preferences file, so they don't see the bug. See comment 7 paragraph -2.
Created attachment 177102 [details] New proposed patch + additional test (In reply to comment #9) > Created an attachment (id=176715) [details] > example project > > In this example project, I20100810-0800 behaves properly (only inserts new > line in local variable). But with the patch, I get newlines everywhere > except for parameters. > > I think the problem of your tests is that they don't read the options from the > preferences file, so they don't see the bug. See comment 7 paragraph -2. I've added test729() in FormatterRegressionTest reading a formatter profile which has the old FORMATTER_INSERT_NEW_LINE_AFTER_ANNOTATION_ON_MEMBER (i.e. "org.eclipse.jdt.core.formatter.insert_new_line_after_annotation_on_member") set to "do not insert". The result is the correct expected behavior: the formatter only inserts a new line in local variable. So, again, the problem is not in JDT/Core which already has the mechanism for backward compatibility. Debugging a little further, it seems that the problem is in ProfileVersioner.updateAndComplete(Map, int). As version 11 is the last up-to-date version, all keys which are not in the newSettings (i.e. keys got from the DefaultCodeFormatterConstants.getEclipseDefaultSettings() map) are removed without any preservation of the oldSettings value. Hence, the returned map from the updateAndComplete method contains the new keys ("..._on_field", "..._on_method", "..._on_package" and "..._on_type") with their default value ("insert") which was different than the old key value... So, the conclusion is that the formatter profile needs to be upgraded to version 12 and a new conversion method version11to12(Map) added to preserve the org.eclipse.jdt.core.formatter.insert_new_line_after_annotation_on_member key value coming from older profiles.
(In reply to comment #10) Sure, the UI (including the ProfileVersioner) needs to be adapted to the new options. Until that's done, you can't use the UI to change or view options. But it should be possible for a project to check in a .settings folder in 3.6, and then run with these settings in a 3.7 workspace with no changes. As long as the formatter properties page is not saved, the old settings should still work, but they don't. In this scenario, ProfileVersioner is never used. All we do is: ToolFactory.createCodeFormatter(.., ToolFactory.M_FORMAT_EXISTING).format(..) BTW: When trying this again, I noticed that the attached example project has a bug, which makes the project look like it's using the "Eclipse [built-in]" profile (but this doesn't affect my scenario where the user just formats): In .settings/org.eclipse.jdt.ui.prefs, line 3 should be: formatter_profile=_bug308000
Created attachment 177845 [details] Final proposed patch This patch addresses the backward compatibility issue described in comment 11 scenario. Note that to fix it, I needed to add an option migration method on JavaModelManager as it has also to be done while getting workspace options... Olivier, could you have a look on this proposal and let me know your thoughts about it? TIA
(In reply to comment #12) > Olivier, could you have a look on this proposal and let me know your thoughts > about it? TIA +1. This should cover all possible cases for annotation new lines.
Created attachment 178168 [details] Final proposed patch + doc Same patch + added documentation to build notes...
(In reply to comment #14) > Created an attachment (id=178168) [details] > Final proposed patch + doc > > Same patch + added documentation to build notes... Released for 3.7M2 in HEAD stream.
Thanks a lot guys! When will 3.7M2 be part of http://eclipse.org/downloads/ ?
(In reply to comment #16) > When will 3.7M2 be part of http://eclipse.org/downloads/ ? 3.7M2 is scheduled for September 17th
Created attachment 178171 [details] Additional patch This additional patch moves the getCompatibleConstants method to JavaModelManager to avoid an unnecessary new API addition...
(In reply to comment #18) > Created an attachment (id=178171) [details] > Additional patch > Released for 3.7M2 in HEAD stream.
(In reply to comment #19) In the test cases and the example, I see package annotations on the package declaration in the file itself. As I understand the package annotations should be in package-info.java. Am I missing anything?
I tried to acknowledge this in the preview with the comment: // only allowed in package-info.java I should probably expand this to: // annotation on package is only allowed in package-info.java => See bug 325221. But the formatter should not care about whether an annotation on a package is in package-info.java or not. Formatting is often done on fragments without clear context, and it doesn't hurt to format things correctly even if the compiler rejects them.
Verified for 3.7M2 using I20100914-0100. Remaining issue will be fixed as part of bug 324987.
(In reply to comment #21) > I tried to acknowledge this in the preview with the comment: > // only allowed in package-info.java > Thanks, I missed this in the preview.
Verified for 3.7RC0 using build I20110428-0848.
