Community
Participate
Working Groups
3.6 M7. See bug 59891 for details. Possible name of the preference: [ ] Try to keep nested expressions on one line
I'll re-vote here for the same, +1 for "Try to keep nested expressions on one line" For the sake of avoiding full reading of bug 59891 , the five proposed choices for now are * "Avoid wrapping nested expressions if possible" * "Try to keep nested expressions on one line" * "Allow aesthetic wrapping" (general) * "Improve message sends wrapping" (msg send specific) * "Improve message sends wrapping (tries to keep nested expressions on one line)"
This will need PMC approval as the new preference is an API constant.
>This will need PMC approval as the new preference is an API constant. +1.
(In reply to comment #0) > 3.6 M7. > > See bug 59891 for details. Possible name of the preference: > [ ] Try to keep nested expressions on one line I agree that words like "improve" or "aesthetic" might be not enough objective to precisely describe what is the intended behavior... So, I agree with this proposal. Note that the default value for this preference will be unset, to let users give a try with the new formatter behavior in this area. Users who will not appreciate it or need to keep the old behavior, will need to explicitly check this preference to cancel the fix for this current bug... Will this make everyone happy?
(In reply to comment #4) >... > this preference to cancel the fix for bug 59891... > should be read instead
(In reply to comment #4) > (In reply to comment #0) > > 3.6 M7. > > > > See bug 59891 for details. Possible name of the preference: > > [ ] Try to keep nested expressions on one line > > I agree that words like "improve" or "aesthetic" might be not enough objective > to precisely describe what is the intended behavior... So, I agree with this > proposal. > > Note that the default value for this preference will be unset, to let users > give a try with the new formatter behavior in this area. Users who will not > appreciate it or need to keep the old behavior, will need to explicitly check > this preference to cancel the fix for this current bug... > > Will this make everyone happy? No problem with it being unset by default, when the remaining bugs are fixed these will indeed be seen as improvements by some ... but they must know it even exists in the first place :).
> but they must know it even exists in the first place :). Right. We'll advertise it in the 'What's New' and can also put a note into the readme.
Created attachment 169154 [details] Proposed patch This patch adds the new formatter constant FORMATTER_KEEP_NESTED_EXPRESSIONS_ON_ONE_LINE allowing users to disable the fix for bug 59891 as required. I've added many new tests which are all the tests I changed while implementing fix for bug 59891 but with this new preference set to verify that one retrieves the behavior of v_A45 (ie. before the fix was released...)
Olivier, Dani, could you please review? TIA
Renaming the method the way you did is the simplest way to handle the new preference. +1.
Exclude list has been updated for the API freeze check.
(In reply to comment #8) + * FORMATTER / Option to try to keep nested expressions on one line + * - option id: "org.eclipse.jdt.core.formatter.keep_nested_expressions_on_one_line" + * - possible values: { TRUE, FALSE } + * - default: FALSE Frederic's patch shows that the label "Try to keep nested expressions on one line" can be misunderstood. The way I meant it was that if the option is TRUE, then the formatter will try to put each nested expression on only one line, i.e. it will avoid wrapping nested expressions to multiple lines (if possible). The consequence of that is that it wraps *earlier* than the old strategy, with the implicit benefit that less wrapping is necessary in the nested expressions. The way you apparently understood it was "keep filling lines as much as possible". I think the preference should be TRUE (checked) by default, and it should tell what the new formatter strategy is (and if possible, even explain or imply *why* it is better). It shouldn't describe what the old strategy was. The tricky point about the right wording is that it's not an absolute choice. Even when the option is set, the formatter may still wrap inside nested expressions if the expression is just too long for a single line. More ideas: * Try to avoid wrapping inside nested expressions * Avoid wrapping inside nested expressions * Wrap earlier if that avoids wrapping nested expressions again * Try to keep nested expressions on one line (wrap outer expressions first) * Prefer wrapping outer expressions (keep nested expression on one line)
(In reply to comment #12) > The tricky point about the right wording is that it's not an absolute choice. > Even when the option is set, the formatter may still wrap inside nested > expressions if the expression is just too long for a single line. Then why not something like? "Preserve Eclipse < 3.6 wrapping" Should be off by default and a readme entry should specify that old behavior can be retrieved by enabling it.
(In reply to comment #10) > Renaming the method the way you did is the simplest way to handle the new > preference. > +1. Yes, even if it's not very smart, I did it that way to minimize the changes in the formatter and reduce at the minimum the risk of side effects with a so late change in the game...
(In reply to comment #12) > > Frederic's patch shows that the label "Try to keep nested expressions on one > line" can be misunderstood. The way I meant it was that if the option is TRUE, > then the formatter will try to put each nested expression on only one line, > i.e. it will avoid wrapping nested expressions to multiple lines (if > possible). > The consequence of that is that it wraps *earlier* than the old strategy, > with the implicit benefit that less wrapping is necessary in the nested > expressions. > > The way you apparently understood it was "keep filling lines as much as > possible". > > I think the preference should be TRUE (checked) by default, and it should tell > what the new formatter strategy is (and if possible, even explain or imply > *why* it is better). It shouldn't describe what the old strategy was. > I understand better now and that sounds reasonable to me. I'll prepare a new patch tomorrow early morning and with a more worded Javadoc to explain the new strategy... > The tricky point about the right wording is that it's not an absolute choice. > Even when the option is set, the formatter may still wrap inside nested > expressions if the expression is just too long for a single line. > > More ideas: > * Try to avoid wrapping inside nested expressions > * Avoid wrapping inside nested expressions > * Wrap earlier if that avoids wrapping nested expressions again > * Try to keep nested expressions on one line (wrap outer expressions first) > * Prefer wrapping outer expressions (keep nested expression on one line) The last one sounds perfect for me. This is exactly what the new strategy intends to do... And it's clear that it's only a best guess, if this strategy fails to fit in the max line width, then the formatter might decide to wrap nested expressions first.
(In reply to comment #13) > (In reply to comment #12) > > The tricky point about the right wording is that it's not an absolute choice. > > Even when the option is set, the formatter may still wrap inside nested > > expressions if the expression is just too long for a single line. > Then why not something like? > "Preserve Eclipse < 3.6 wrapping" > > Should be off by default and a readme entry should specify that old behavior > can be retrieved by enabling it. I was also tempted by something like that, but I guess it's better to explain the formatter new strategy as suggested by Markus in comment 12... But in fact, I think you're right at some point and that we should inform users of this new preference that disabling it will put back the formatter to behave as it did in versions before 3.6... I'll add something about this point in the Javadoc.
(In reply to comment #16) > I was also tempted by something like that, but I guess it's better to explain > the formatter new strategy as suggested by Markus in comment 12... What I don't like with Markus's suggestion is that it is not clear the relationship between this new preference and the Eclipse wrapping policy prior to 3.6. Since the point of adding it is to preserve former line wrapping, I believe this should be reflected in the preference description.
>> * Prefer wrapping outer expressions (keep nested expression on one line) >The last one sounds perfect for me. Me too. Using '3.6' in the preference doesn't mean anything for new users but we should point out in the 'What's New' and readme that this preference can be used to restore the 3.6 behavior.
> >> * Prefer wrapping outer expressions (keep nested expression on one line) > >The last one sounds perfect for me. > Me too. I'd third that as the one we chose previously can be misunderstood as pointed out in comment 12.
I think you don't care, however since I voted also in the previous occasion, I would like to say that I also like the latest Markus's proposal :-)
Created attachment 169285 [details] New proposed patch + doc This patch takes into account all remarks. I hope this is clear enough now and ready to be released for the today's integration build. All JDT/Core formatter tests are OK, all JDT/Core tests are currently running (but as they are not impacted by this changes, there's no reason not to be ok as well...).
Markus, Srikanth, could you review this new patch? TIA
Dani, could you review the patch? TIA
<(In reply to comment #22) > Markus, Srikanth, could you review this new patch? > TIA Markus is busy with other stuff. Looking into now and starting our tests on the patch. I assume the new patch doesn't differ much from the first one - hence Olivier's review should still be good.
(In reply to comment #24) > <(In reply to comment #22) > > Markus, Srikanth, could you review this new patch? > > TIA > Markus is busy with other stuff. Looking into now and starting our tests on the > patch. I assume the new patch doesn't differ much from the first one - hence > Olivier's review should still be good. Yes, that was only refactoring + documentation.
Ooops, As I removed Oliver's + review, I can't set it back, hence he'll need to put it back this afternoon...
Created attachment 169295 [details] Previous patch + incorporated my comments Verified the patch. Some minor issues (fixed in this new patch): - removed the "first" from the preference and updated all code - "set to the {@link #FALSE} value" ==> "set to {@link #FALSE}" - updated the API exclude list
Created attachment 169296 [details] previous patch + missing tests This final patch adds the additional tests to Dani's changes. Those tests were unfortunately missing in my new proposed patch :-(
Srikanth please review, thanks
Looks reasonable to me. Noticed a couple of minor nits in the javadoc: Important notes: 1. This new behavior is automatically activated (ie. the default value for this preference is {@link #TRUE}). * If the backward compatibility regarding previous versions formatter behavior (ie. before 3.6 version) is necessary, ie. ==> should be i.e. (but I suspect it is coded as ie. in many other places too) previous versions formatter ==> previous version's formatter
The buildnotes should reflect the right constant's name.
(In reply to comment #30) > Looks reasonable to me. > > Noticed a couple of minor nits in the javadoc: > > Important notes: > 1. This new behavior is automatically activated (ie. the default value for this > preference is {@link #TRUE}). > * If the backward compatibility regarding previous versions formatter > behavior (ie. before 3.6 version) is necessary, > > ie. ==> should be i.e. (but I suspect it is coded as ie. in many other places > too) > It seems that the correct English version is ie. (in French it's i.e.). > previous versions formatter ==> previous version's formatter In fact it was: previous versions' formatter => fixed and put in v_A54 Thanks Srikanth (In reply to comment #31) > The buildnotes should reflect the right constant's name. Thanks Oliver => fixed and put in v_A54
Released for 3.6RC2 in HEAD stream.
> It seems that the correct English version is ie. (in French it's i.e.). Nope, it's also i.e., see e.g. http://www.merriam-webster.com/dictionary/i.e.
(In reply to comment #34) > > It seems that the correct English version is ie. (in French it's i.e.). > > Nope, it's also i.e., see e.g. http://www.merriam-webster.com/dictionary/i.e. I opened bug 313706 to clean this.
Opened bug 313708 for the readme entry.
Verified for 3.6RC2 using build I20100520-1744
Verified.
