Community
Participate
Working Groups
Created attachment 256100 [details] Javadoc params formatter setting AFAIK we in Eclipse try to follow the official Oracle code conventions as much as possible, and AFAICS Oracle and Google code conversions suggest to not add line breaks after @param tags Links: http://www.oracle.com/technetwork/articles/java/index-137868.html https://google.github.io/styleguide/javaguide.html#s7.1.2-javadoc-paragraphs
Jay, Manoj whats your option here?
Just tried out on my workspace and interestingly, I always see the @param tag and summary in a single line no matter what the option is set to. Am I missing something here?
I tested with JavaResolutionFactory from pde.ui With lines breaks: /** * Creates and returns an IJavaCompletionProposal for the given * AbstractManifestChange with the given relevance. * * @param change * the modification which should be performed by the proposal * @param relevance * the relevance of the IJavaCompletionProposal * @since 3.4 * @see AbstractManifestChange */ public final static IJavaCompletionProposal createJavaCompletionProposal(final AbstractManifestChange change, final int relevance) { If I remove the "New line after @param tags" setting in the formatter: /** * Creates and returns an IJavaCompletionProposal for the given * AbstractManifestChange with the given relevance. * * @param change the modification which should be performed by the proposal * @param relevance the relevance of the IJavaCompletionProposal * @since 3.4 * @see AbstractManifestChange */ public final static IJavaCompletionProposal createJavaCompletionProposal(final AbstractManifestChange change,
Looking at the preview window, the "New line after @param" option seems to be working correctly in both master and maintenance branch. I don't know what could go wrong in your case, Jay.
(In reply to Mateusz Matela from comment #4) > Looking at the preview window, the "New line after @param" option seems to > be working correctly in both master and maintenance branch. I don't know > what could go wrong in your case, Jay. The problem could have been my eyes. I no longer see this problem, but was certain the sample code wouldn't change when I played around with the relevant options. Anyway, coming back to Lar's request: Lars, I agree with your point, but do we really want to break the existing users? People may not like it if the code changes without them making any change.
Maybe we could come up with a solution that wouldn't change the behavior for workspaces migrating from previous versions? For example, if a workspace has the active profile "Eclipse [built-in]", it would save all settings as a new profile, "Eclipse 4.6" or something like that.
(In reply to Jay Arthanareeswaran from comment #5) > Anyway, coming back to Lar's request: Lars, I agree with your point, but do > we really want to break the existing users? People may not like it if the > code changes without them making any change. We have adjusted the Eclipse profile over the last years. I think we should be open to improve our "Eclipse" profile. If the customer wants a stable profile he should create his own profile, for example by copying one of the existing profiles.
+1 for this change. As long as only the built-in profiles change and pre-existing custom profiles stay the same after migration, it shouldn't be a problem. If someone didn't bother to create their own profile, they probably don't care that much. We've already introduced such a change in bug 514017.
(In reply to Mateusz Matela from comment #8) > +1 for this change. > As long as only the built-in profiles change and pre-existing custom > profiles stay the same after migration, it shouldn't be a problem. If > someone didn't bother to create their own profile, they probably don't care > that much. > > We've already introduced such a change in bug 514017. Can we proceed with this change? Do you need a Gerrit?
(In reply to Lars Vogel from comment #9) > Can we proceed with this change? Do you need a Gerrit? Without further objections from Jay, I assume we can :) Yes please, a Gerrit wyould be helpful.
New Gerrit change created: https://git.eclipse.org/r/112049
Of course, changing just one line is not enough :) Are you up for updating the failing tests? (probably best to change the setting to true in each one)
(In reply to Mateusz Matela from comment #12) > Of course, changing just one line is not enough :) > Are you up for updating the failing tests? (probably best to change the > setting to true in each one) Please also run the JDT UI tests and make sure they don't fail.
(In reply to Mateusz Matela from comment #12) > Of course, changing just one line is not enough :) That is why the patch has the commit message: ;-) --------- [WIP] Bug 475823 - "Eclipse" default formatter should not add line breaks to @params in Javadoc WIP, as I also need to adjust the tests ------- > Are you up for updating the failing tests? (probably best to change the > setting to true in each one) Thanks for the pointer, I was unsure if I should adjust the test or override the setting for the test. I will try to update the test. We have an Eclipse hackathon planned for 11.12 and I will try to pick this for the hackathon.
(In reply to Mateusz Matela from comment #12) > (probably best to change the setting to true in each one) I did this for one test, can you check if the change is in line with your suggestion? https://git.eclipse.org/r/#/c/112049/3/org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/formatter/FormatterCommentsTests.java Once I get your OK, I will adjust the other 146 tests.
Note that such changes can be a big pain for our users as they will get unexpected changes when saving a file with auto-format enabled. I'm -1 for that unless there are many other +1 votes.
(In reply to Lars Vogel from comment #15) > (In reply to Mateusz Matela from comment #12) > > (probably best to change the setting to true in each one) > > I did this for one test, can you check if the change is in line with your > suggestion? > https://git.eclipse.org/r/#/c/112049/3/org.eclipse.jdt.core.tests.model/src/ > org/eclipse/jdt/core/tests/formatter/FormatterCommentsTests.java > > Once I get your OK, I will adjust the other 146 tests. I was thinking about something similar to the setPageWidth80() method in FormatterRegressionTesets - a separate method makes it more clear that the preference change is related to modified defaults, not an essential part of what given test is checking. (In reply to Dani Megert from comment #16) > Note that such changes can be a big pain for our users as they will get > unexpected changes when saving a file with auto-format enabled. > > I'm -1 for that unless there are many other +1 votes. You do remember it will only affect people who don't use their own formatter profile, right? Can you explain how this case is different from e.g. bug 514017, where you gave +1?
+1 for changing this default behaviour.
Here's a thought: bug 128653 is about aligning @param descriptions, a feature which would bring the format even closer to Oracle conventions. So we may want to add that feature first and turn it on by default too. This way there will be more good reasons to change the default profile and we'll make sure the javadoc formatting is affected only once. Bug 128653 shouldn't be hard to fix, but I prefer to wait until bug 462940 is closed before adding new features.
+1 for the default behaviour
(In reply to Florian Gruner from comment #20) > +1 for the default behaviour Sorry, this was unclear. +1 for not adding line breaks.
I'd say +1 for this change in order spare unnecessary lines of code.
+1 for removing the line breaks
IMHO this improves our default formatting and we should always try to deliver the best out-of-box experience for our customers. As it is easy for customers to adjust their profile I suggest to merge this for M6. Manoj, please review the Gerrit change.
(In reply to Lars Vogel from comment #24) > > Manoj, please review the Gerrit change. @Lars: Matuesz will be the better person to review the change. @Mateusz: Can you please review? TIA
I did the review in Gerrit. What do you think about comment 19? If we want to change the default alignment style, it would be good to change these defaults together. Bug 128653 is on the top of my list and I should manage to finish it next weekend, but if I don't, maybe it would be better to wait another year with this change rather than make two changes year after year that could cause reformatting of all files.
(In reply to Mateusz Matela from comment #26) > I did the review in Gerrit. Thanks, I adjust the Gerrit as soon as I can. > but if I don't, maybe it would be better to wait another year I would prefer not to wait another year.
New Gerrit change created: https://git.eclipse.org/r/117871
(In reply to Eclipse Genie from comment #28) > New Gerrit change created: https://git.eclipse.org/r/117871 This is for JDT UI CleanUpStressTest
(In reply to Manoj Palat from comment #25) > @Mateusz: Can you please review? TIA Mateusz gave his +1 on the changes. Manoj, can we merge the changes? I'm waiting since 2015 for this.
(In reply to Lars Vogel from comment #30) Manoj or Mateusz can we merge this, so that we can test it in M6?
If I read https://wiki.eclipse.org/Photon/Simultaneous_Release_Plan correctly, we still have almost 2 weeks left. I'm pretty close to completing the Javadoc align feature and I think we could turn it on by default in this bug.
(In reply to Mateusz Matela from comment #32) > If I read https://wiki.eclipse.org/Photon/Simultaneous_Release_Plan > correctly, we still have almost 2 weeks left. According to Danis email, we only have this week left, next week is testing week. https://dev.eclipse.org/mhonarc/lists/eclipse-dev/msg10624.html > I'm pretty close to completing the Javadoc align feature and I think we > could turn it on by default in this bug. Would be great to see this also in. I hope you manage to finish it this week.
OK, done. See bug 531821 for UI changes needed to see the new preferences. Now we have to choose the best default Javadoc alignment. I think "Align descriptions, group by type" is a good balance between cleanness and saving space. We should also disable the "Indent wrapped @param/@throws descriptions" setting (AKA "Ident description after @param"). @Lars can you take care of changing these defaults and updating the affected tests?
(In reply to Mateusz Matela from comment #34) > OK, done. See bug 531821 for UI changes needed to see the new preferences. > > Now we have to choose the best default Javadoc alignment. I think "Align > descriptions, group by type" is a good balance between cleanness and saving > space. > We should also disable the "Indent wrapped @param/@throws descriptions" > setting (AKA "Ident description after @param"). > > @Lars can you take care of changing these defaults and updating the affected > tests? Could you please do it? I'm currently working for a customer and I'm not familiar with the new settings.
Gerrit change https://git.eclipse.org/r/112049 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=b694860ae5a78596d1ae841b2bb014eeee2be87c
Gerrit change https://git.eclipse.org/r/117871 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=494f1bc84d589a3e9b8f55b84878317d3e523de0
Awesome Mateusz, thanks a bunch. Shall I add it to the noteworthy?
Not sure, AFAIR we've never advertised it in N&N when the formatter defaults changed. I've mentioned the changed defaults for Javadoc tags in N&N note for bug 531821.
(In reply to Mateusz Matela from comment #39) > Not sure, AFAIR we've never advertised it in N&N when the formatter defaults > changed. I've mentioned the changed defaults for Javadoc tags in N&N note > for bug 531821. Ok, thanks again for the merge.
Verified for 4.8 M6 using I20180306-2000 build
Mateusz, is there a way to mass update "only" the formatter settings in all eclipse.platform projects? The changes in the default profile are only applied to a project, if I go to the project properties -> Formatter and press the Edit... button. Unfortunately this cannot be done for multiple projects AFAIK and copied in the jdt ui and jdt core .settings files around would override other settings.
(In reply to Lars Vogel from comment #42) > Mateusz, is there a way to mass update "only" the formatter settings in all > eclipse.platform projects? I've never done this, but it should be enough to use find/replace on .settings/org.eclipse.jdt.core.prefs files. The relevant prefs are: -org.eclipse.jdt.core.formatter.comment.indent_parameter_description -org.eclipse.jdt.core.formatter.comment.indent_root_tags -org.eclipse.jdt.core.formatter.comment.insert_new_line_for_parameter -org.eclipse.jdt.core.formatter.comment.align_tags_descriptions_grouped (new one to set to true) Compare with the change to DefaultCodeFormatterOptions in this commit.
Actually there's a bug related to upgrading with unmanaged profiles, see bug 532964. Not sure if this is what affected you, Lars.
Thanks for the info but I don't think we are affected as we do not use unmanaged profile. As for the adjustment of settings, I plan to do it manually
