Community
Participate
Working Groups
1. Create a blank workspace. 2. Create a Class Test 3. type foo and use ctrl-space for automatic creation of method Before this formatting option was active the comment would have indented to the method body which looked very nice imho. Now the comment is right at the beginning and does not get indented which looks strange for automatically generated bodies especially if the indention is quite high. Also the indention is correct for automatically generated comments in ctrl-space for overriden methods
public class Test5 { public static class InnerClass { private void foo() { // TODO Auto-generated method stub new Runnable() { @Override public void run() { // TODO Auto-generated method stub } }; } } }
You are right, that's quite big problem also for the ASTRewriter. We have to disable the option when formatting generated code or string placeholders passed to the AST rewrite.
There is nothing I can do at the code formatter level. Daniel requested to get the new option true by default. This implies that this is properly formatted from a code formatter standpoint. You would need to either change the default or disable the option while generating the code snippet. The user could also change the default value of this option if he/she doesn't like it.
I think it depends what you format. If you want to format a unformatted, generated piece of code, the option makes less sense that when you format existing code. To be save with clients, it might be a good idea to disable the option by default for 3.3.
Daniel is the one that requested it to be enabled. Don't know if this is a good idea to change it now.
Me and Markus had a longer discussion and we are both worried if this new option has introduced various bugs. There's a difference between formatting newly generated code (which is mostly without any indents) or existing code. For example class A { /* (non-Javadoc) * */ public void foo() { // TODO } } The new option can be fatal for newly generated code. The user would end up with: class A { /* (non-Javadoc) * */ public void foo() { // TODO } } We use the code formatter at so many places that it is hard to find out where problems could have been introduced and it's also hard to disable the given options for generated code. But if we have to do extra work, this means we probably also broke other clients. Right now, not many problems are visible, mostly because we were lucky. For example when creating a type, we use IType.createMethod to add the new method which automatically indents the passed code. So when formatting the whole CU in the end, everything gets correctly indented. Changing the default to disabled is the minimum. We should even consider removing the new option as it isn't as 'No Risk' as initially thought.
(In reply to comment #6) > Right now, not many problems are visible, mostly because we were lucky. For > example when creating a type, we use IType.createMethod to add the new method > which automatically indents the passed code. So when formatting the whole CU in > the end, everything gets correctly indented. > Changing the default to disabled is the minimum. We should even consider > removing the new option as it isn't as 'No Risk' as initially thought. Your team requested the current default value. I am firmly against removing the new option. It does exactly what it is supposed to do. Your current issue is about the default value of the option. If we disable it, this would not prevent the user from enabling it and then getting this kind of issues (if this is an issue. After all, we respect the code formatter settings). The problem comes from the fact that you generate code with line comment starting on the first column. This might not look ideal from the UI standpoint, but from the code formatter standpoint it returns the expected result.
Disabling the option is not a fix as clients that will enable it will immediately get this bug again. Luckly we enabled it by default so we found this early enough.
This being said, there is nothing that can be done at the code formatter level. For once it does what it is supposed to do.
Philippe, Jerome, your opinions? The implementations of the new options is perfectly fine, they seem to work as spec'ed. But we shouldn't have added these options in such a late stage as the give unwanted results for clients that use the code on newly generated code.
What makes this option special comparing to other options ? If the user decides to configure other formatting options so that everything is formatted on 1 line, the resulting generated code is going to look bad. So what makes this new option different from other existing options ?
That the generated code sometimes indents correctly and sometimes not. See run() vs. foo(). If the comment would either be on the first column for both comments or both would be indented I would agree that it is up to the user to decide. But seeing 2 different behaviors is irritating. This has to be fixed at all places where comments/user input is inserted. And disabling the formatting option is just a bad hack to further ignore the problem.
What makes it different, is that these options use the formatting of the input source to decide what to do. 'If its already had no indent, then don't indent it' (Never indent comment on first column) All other formatter options just state rules: 'Have a whitespace after assignment', 'empty line in method bodies', etc. I think the only other option that uses the formatting of the input source is 'Number of empty lines to preserve'. This never hurt us as we would generate code with lots of empty lines. So it seem to me that we need, at some point, are 2 modes when formatting: - format existing source: tells the formatter that it makes sense to look at the formating of the input source - format new source: tells the formatter that the formatting of the existing source shouldn't be used to make conclusions
That's a great one. I was just about to fill a bug about that because I thought something is broken in M7. Actually, the option is good and I can imaging that there are use cases. But in my opinion it could be a breaking change because with this option being enabled by default code in 3.3 formats suddenly different than in 3.2. This also happens to customized profiles. I wonder if such a change classifies as one that should have been introduced before the M5 freeze. Anyway, I'd like to see this option disabled by default. The problem is that it also affects existing custom profiles and not only the Eclipse built-in profile.
Discussing with Martin and Jerome, we came to the conclusion that the new option shouldn't have been enabled for 3.3, as it has some unhappy consequences on existing clients. Looking into 3.4, maybe we should amend the new option (leave comments on column-0) to also consider the initial indentation level (or the kind?). So if original indentationLevel is greater than 0, implicitly there is no more comment on column-0... but this won't solve UI scenario, since it calls the formatter with indentationLevel at 0 (and manages line prefixes on its own afterwards). Another suggestion is to only consider the option, when formatting an entire unit ? (this sounds less defendable). Anyhow, for the short term, the option should be OFF by default, and we need a better solution for 3.4.
So the change needs to be done at the core level. I'll prepare a patch for it.
What about this: New API: /** * Kind used to format a compilation unit where formatter can probe the existing formatting to evaluate the * new formatting to apply. */ public static final int K_COMPILATION_UNIT_WITH_FORMATTING = 0x10; Options DefaultCodeFormatterConstants.FORMATTER_NEVER_INDENT_LINE_COMMENTS_ON_FIRST_COLUMN DefaultCodeFormatterConstants.FORMATTER_NEVER_INDENT_BLOCK_COMMENTS_ON_FIRST_COLUMN are only used when this new kind is selected. This way, clients will only get the behavior of the new options if the intentionally change to the new kind. I think this is the safest approach. Changing the default to off is good, but we (and other clients) still look bad when the user turns it on. I also think this option makes sense for future options like 'preserve line breaks'
Too late for a new API.
>Too late for a new API. Not true. Officially, API can be granted by the PMC.
I like the idea from comment 17.
See comment 15. What made you change your mind ?
Created attachment 67413 [details] Proposed fix Patch to disable the option by default.
(In reply to comment #21) > See comment 15. What made you change your mind ? Its because that all users that turn the options on will sooner or later see the strangely formatted generated code. We're just asking for bug reports and we make other formatter clients angry. Either we really fix it or we remove the options from the UI.
(In reply to comment #17) > /** > * Kind used to format a compilation unit where formatter can probe the > existing formatting to evaluate the > * new formatting to apply. > */ > public static final int K_COMPILATION_UNIT_WITH_FORMATTING = 0x10; Who decides what kind to use? This value would collide with comment kinds. This new value is ok. public static final int K_COMPILATION_UNIT_WITH_FORMATTING = 0x80; I don't understand why you want to add it since it is not clear what formatting will be applied on Ctrl + F for a unit. Also why don't you simply insert a tab or a space before the line comment that you add? If the comment is not on the first column, the option won't be used. Am I missing something?
CTRL + F would use the new constant, all other usages the old one so we have the behavior as before. If we would go though all our code to add indentation before generated comments (which can be in user defined code templates, so we don't really know), all other jdt.core and code template clients would have to do the same. I don't think we should force clients to do that, especially not so late in a release. A second reason is that formatter clients shouldn't really have to know of the different options. Next time we offer an option that preserves all line delimiters or preserves all indentation. We can't adapt all our code again. It seems to me that we need two different kinds of formatting: Formatting newly generated code, formatting already formatted code.
Instead of adding a new API, I propose that the Preferences page uses a UI preference (instead of directly using the JDT/Core option), and the JDT/Core option would be set by the UI when calling Ctrl+F only if the UI preference is set.
Would that be a workaround for 3.3 to avoid new API, or our future story for these kind of settings? The problem of the workaround is: - it will be quite some work to get that right in the formatter profiles. At this late stage this is a risky change. - the UI setting will be project settings, shared in teams so we will have to maintaining them or finding a compatible solution when this moves to core again. The preference story is weak with options that migrate. See the import order settings which we couldn't move down to core. The problem is when teams work with 3.3 and 3.4 on the same shared project. The problem of a permanent solution is: - it complicates the formatter options story a lot. You need to know what the special options are. - its more complicated for clients, as they have to understand the options split as well In general, I'd rather have the UI represent the core options as they are and not play any tricks. From my experience, all special cases cost a lot of time for maintenance. We should try to keep things simple and explicit.
-1 for this as it means that headless clients would need to load JDT UI in order to get the files formatted like in the editor.
If the option is set to never indent comments on first column, then I don't see why auto-generated code would be different. The only change we should make is to keep the existing behavior (indentation) by default. If we introduce a new constant, this doesn't change anything. Why would the user get a different formatting when formatting a compilation unit or a simple code snippet? It is up to the user to decide what he/she wants.
> If the option is set to never indent comments on first column, then I don't see > why auto-generated code would be different. The only change we should make is > to keep the existing behavior (indentation) by default. But as soon as a user enables it, he will get badly formatted new generated code or bad formatted editor templates. He will probably file a bug and eventually turn off the option again. > If we introduce a new constant, this doesn't change anything. Why would the > user get a different formatting when formatting a compilation unit or a simple > code snippet? > It is up to the user to decide what he/she wants. The 'IDE user' can format code only through 'Source > Format'. That's why we would only change the 'Source > Format' action to use the new constant. Internally, the 'Source > Format' action uses the API with K_COMPILATION_UNIT plus source ranges. So the 'Source > Format' user will use the new options which he can control on the formatter preference page. API clients of the JDT/core formatter API (= jdt.ui code generation, AST rewrite, other plugins) stay unchanged and use the existing constants. That means the new options are not used. All API clients continue to function regardless is a JDT/UI user decides to turn on the new option. I think it makes sense to only have the new mode for compilation units, but if we find out later that we should also have the new mode for other kinds (K_CLASS_BODY_DECLARATIONS, K_STATEMENTS, K_UNKNOWN), we easily can add more constants (something like K_CLASS_BODY_DECLARATIONS_WITH_FORMATTING).
I think we need to solve this for 3.3. There may be some clients who are going to be affected by this, like code generators etc... Disabling the option by default is mandatory. Now there are a couple options. 1. Remove the new option from the UI. This is a step back. To be super conservative, we may also want to implicitly disable it at core level (to be safe). 2. Provide a mode for the formatter to tell whether some options reusing existing layout can be used. Duplicating all kinds feels overkill. (a) either provide a new API for formatting, with extra mode (bitmask) so we have more room for later additions. (b) encode the mode in the kind directly. Is this altering the notion of a kind so badly ? e.g. K_REUSE_EXISTING_LAYOUT | K_COMPILATION_UNIT I'd rather vote for 2b, unless there are more pending requests which let us think that we will need more bits, and therefore we cannot hijack the kind any further.
I agree with Philippe's post. I'm fine with both options: 1b) (super conservative approach) or 2b). I think JDT/Core should make the call, depending on how much work it is to implement them.
Same here.
sorry, I didn't mean to change the target milestone
Olivier - pls comment on pending requests in same area (i.e. choosing between 2a and 2b). I know of having an option request for reusing existing line separators, which would fall into the same category (K_REUSE_EXISTING_LAYOUT). Do we have more ?
(In reply to comment #35) > I know of having an option request for reusing existing line separators, which > would fall into the same category (K_REUSE_EXISTING_LAYOUT). Do we have more ? Yes this is one. I don't see another one inside existing open requests. 2a looks more flexible for potential future issues. 2b is technically a breaking API change since http://help.eclipse.org/help32/topic/org.eclipse.jdt.doc.isv/reference/api/org/eclipse/jdt/core/formatter/CodeFormatter.html#format(int,%20java.lang.String,%20int,%20int,%20int,%20java.lang.String) clearly states what kind is accepted.
I am fine either way (2a or 2b). Dani/Martin - final take ?
Discussed with Martin. Both are fine for us. It's your call.
I agree with Olivier that 2b is a breaking change since there can be subclasses of CodeFormatter out there, and these subclasses may not expect a different set of kinds than the one specified. So I would go for 2a.
Good point. Furthermore, since other implementations of CodeFormatter may not have these issues (reusing existing layout), it feels inappropriate to expose these concerns at the level of CodeFormatter. These should rather remain specific to our default code formatter implementation. Thus a fix at the level of ToolFactory feels better. Talking with Jerome we came to the conclusion of adding: ToolFactory#createCodeFormatter(Map options, int mode) where mode can evolve in the future to express more nuances. It would also define a first mode: ToolFactory#M_FORMAT_REUSE_EXISTING_LAYOUT (unless a better spot is identified for the constant)
Created attachment 68172 [details] Better patch Martin, Daniel, please let us know if this is fixing your issue.
Philippe, Jérôme, please review.
In principle OK but I found Philippe's suggestion (M_FORMAT_REUSE_EXISTING_LAYOUT) better understandable than M_FORMAT_USE_EXISTING_COMMENT_POSITIONS.
After discussion, we found that M_FORMAT_REUSE_EXISTING_LAYOUT could lead to some confusion about what it is doing. Some people might believe it also preserves the line breaks which is not the case.
I didn't think of a better name yet but the new one doesn't tell me much more than MODE1 would have done.
M_FORMAT_USE_EXISTING_COMMENT_LAYOUT ?
I didn't want to chime this in before as I know JDT Core likes those pseudo extensible solutions with flags which once again turned out to be not that flexible in reality (see comment 39). So - why not simply add a method like: CodeFormatter createCodeFormatter(Map options, boolean isCodeSnippet) Or if we wanna stick with the flags: M_FORMAT_IGNORE_NEVER_INDENT_FLAGS Just a comment about the code: the default for the NEVER falgs is now FALSE (OK) and depending on the mode FALSE is also set. I could not find the code that sets them to true. Is that OK?
(In reply to comment #47) > I didn't want to chime this in before as I know JDT Core likes those pseudo > extensible solutions with flags which once again turned out to be not that > flexible in reality (see comment 39). So - why not simply add a method like: > CodeFormatter createCodeFormatter(Map options, boolean isCodeSnippet) Should I mention what you said in comment 38? > Or if we wanna stick with the flags: > M_FORMAT_IGNORE_NEVER_INDENT_FLAGS Why not? Philippe, Jérôme, any preference? > Just a comment about the code: the default for the NEVER falgs is now FALSE > (OK) and depending on the mode FALSE is also set. I could not find the code > that sets them to true. Is that OK? They would be set to true inside the options passed to the new API. So either through the UI code formatter preference page or programmatically.
>Should I mention what you said in comment 38? I know ;-) but at that time I didn't want to bring in yet another solution, especially knowing that most of the time JDT Core favorites the (presumed) generic solution. >They would be set to true inside the options passed to the new API. So either >through the UI code formatter preference page or programmatically. I would expect that those are set to by the new method: if M_FORMAT_IGNORE_NEVER_INDENT_FLAGS is set ==> set flags to FALSE else ==> set flags to TRUE I wouldn't push this to every client.
(In reply to comment #49) > >Should I mention what you said in comment 38? > I know ;-) but at that time I didn't want to bring in yet another solution, > especially knowing that most of the time JDT Core favorites the (presumed) > generic solution. Why do you say (presumed) ? > I would expect that those are set to by the new method: > if M_FORMAT_IGNORE_NEVER_INDENT_FLAGS is set > ==> set flags to FALSE > else > ==> set flags to TRUE > I wouldn't push this to every client. It was never question of enabling them. It was question of disabling them using a new API so that existing clients would not be broken. It is up to the caller to pass the desired options.
I don't see *any* reason why this can't be provided to the client.
I don't see any reason why this would be provided to the client. What is difficult in adding a key/value inside a map?
My apologies Olivier, I got tricked by the late hour and the parameter 'mode' which in fact is a filter the way you handle it (i.e. turning off some options). I hoped we could have the *NEVER* flags enabled out of the box in the UI (prefs) but that would mean to set the to TRUE for the 'Eclipse' profile - which we decided not to do for 3.3 - that's fine. So I'm fine with this (e.g. M_FORMAT_IGNORE_NEVER_INDENT_FLAGS). >Why do you say (presumed) ? Because it was not possible to extend the set of flags for the code formatter. For the tool factory it will indeed work because it is not intended to be subclasses and because it is mentioned in the Javadoc that new flags might be added. Little improvement: write what happens if a client uses an not (yet) specified flag.
(In reply to comment #47) > Or if we wanna stick with the flags: > M_FORMAT_IGNORE_NEVER_INDENT_FLAGS Actually, this is the opposite. If the flag is set, then the NEVER_INDENT options are honored. If it is not set, the NEVER_INDENT options are ignored. So what about M_FORMAT_HONOR_NEVER_INDENT_COMMENT_OPTIONS instead ? /** * If set, this mode tells the default code formatter to honor the options that never indent comments on * first column. Note that if these options are disabled, then they remain disabled even if this mode is set. * If this mode is not set, these options are ignored, even if they are enabled. * * @see DefaultCodeFormatterConstants#FORMATTER_NEVER_INDENT_BLOCK_COMMENTS_ON_FIRST_COLUMN * @see DefaultCodeFormatterConstants#FORMATTER_NEVER_INDENT_LINE_COMMENTS_ON_FIRST_COLUMN * @see #createCodeFormatter(Map, int) * @since 3.3 */ public static int M_FORMAT_HONOR_NEVER_INDENT_COMMENT_OPTIONS = 0x01;
Sounds good.
Patch looks good. Two comments: 1. I liked Philippe's constant name suggestion much more than the current. I think we should not be too specific and technical, but rather give give the intent of the option. We should also think ahead of more options that reuse the existing formatting (preserve line delimiters, preserve indentation). It's better if clients don't have to change their code again when we add these. I think it makes sense for clients if they can tell you: I reformat code or I format newly generated code. 2. '0' as a value should be avoided. So better also define a constant for 'mode 0' What about this: static final int M_FORMAT_HONOR_NEVER_INDENT_COMMENT_OPTIONS= XX; static final int REFORMAT_FLAGS = 0; /* open for future flags */; static final int FORMAT_NEW_FLAGS = M_FORMAT_HONOR_NEVER_INDENT_COMMENT_OPTIONS /* | open for future flags */;
it should of course say static final int FORMAT_NEW_FLAGS = 0; /* open for future flags */; static final int REFORMAT_FLAGS = M_FORMAT_HONOR_NEVER_INDENT_COMMENT_OPTIONS /* | open for future flags */;
Martin, I'm having difficulties understanding your proposal. Could you please attach a complete description of the new constants you're introducing and where they are used ? (maybe take Olivier's patch, add your constants, write/change the javadoc, and attach a new patch so that we have a complete picture).
Created attachment 68309 [details] patch for ToolFactory Patch is only for ToolFactory, rest of changes would be the same as in Olivier's patch.
Thanks. I understand now. Unfortunately since M_FORMAT_NEW and M_REFORMAT are constants, their values must be specified. Thus we will never be able to add new modes to these constants in the future. So we're left with M_REFORMAT having the value 0 specified, and M_REFORMAT having the value 1 specified. With this limitation, I don't think introducing these constant is worthwhile.
The value is not a constant here, so no compiler in-lining is possible and the user can't make assumptions about the value. Or is it a JDT/Core API rule that all constants need to have a constant value? Why is that? The pattern used (new Integer(0).toInt()) in the patch is the recommended way of defining constants by Jim, used in JDT/UI , but you can also use a static initializer. There are also several other possibilities to implement the same using constant value constants. Assuming we use a one of these, what do you think is better for clients to reference: something general like M_REFORMAT or something specific so they don't need to update their code again in 3.4 when 'preserve line delimiter' or 'preserve indent' option are added? BTW, if you prefer to add these constants in 3.4, I can also wait for then.
(In reply to comment #61) > BTW, if you prefer to add these constants in 3.4, I can also wait for then. Ok, let's talk about it after 3.3 (otherwise we will never fix this particular bug in time for RC2). Olivier, can you please rename the constant and make it final ? Then attach a new patch and Philippe an I will review it and give the +1. /** * If set, this mode tells the default code formatter to honor the options that never indent comments on * first column. Note that if these options are disabled, then they remain disabled even if this mode is set. * If this mode is not set, these options are ignored, even if they are enabled. * * @see DefaultCodeFormatterConstants#FORMATTER_NEVER_INDENT_BLOCK_COMMENTS_ON_FIRST_COLUMN * @see DefaultCodeFormatterConstants#FORMATTER_NEVER_INDENT_LINE_COMMENTS_ON_FIRST_COLUMN * @see #createCodeFormatter(Map, int) * @since 3.3 */ public static final int M_FORMAT_HONOR_NEVER_INDENT_COMMENT_OPTIONS = 0x01;
Created attachment 68330 [details] New patch with the renamed constant
I will also update the copyright of the ToolFactory class. I missed it in my last patch.
I am not a super big fan of the mode name... but can live with it.
Released for 3.3RC2. Martin, Daniel, please verify the test case in comment 1
Mmm, I find this a bit strange, now we have a flag which basically mirrors the never intent settings, like a super setting or so. Why not defining a new method like createCodeReformatter() and spec that createCodeFormatter returns a formatter that does always ignore the never indent settings?
(In reply to comment #67) > Mmm, I find this a bit strange, now we have a flag which basically mirrors the > never intent settings, like a super setting or so. Why not defining a new > method like createCodeReformatter() and spec that createCodeFormatter returns a > formatter that does always ignore the never indent settings? Note that this is the opposite: when reformatting (if you meant formatting existing code), the never indent setting should be honored, not ignored.
(In reply to comment #68) > Note that this is the opposite: when reformatting (if you meant formatting > existing code), the never indent setting should be honored, not ignored. > Right, just what I said... The existing createCodeFormatter method must ignore the setting, otherwise client will be broken, right?
From the javadoc of createCodeFormatter(Map), it is using: createCodeFormatter(options, 0) This means that the new options (NEVER_....) are disabled.
I am not a big fan of introducing notion of a code REformatter... (which actually turns out to just implement CodeFormatter). But I am not a big fan either of the chosen name for the mode constant. Couldn't the mode simply tell whether we are formatting fresh code (default) or reformatting (reuse existing layout) ? Probably simpler for end-users.
(In reply to comment #66) Verified for 3.3 RC2 using I20070525-0010
Reworking API
Created attachment 68753 [details] New patch after discussion with Martin and Philippe
Philippe, Martin, please review.
1. typo - missing space: "public static final int M_FORMAT_NEW=" 2. older issue in CodeFormatter#createCodeFormatter(Map options, int mode) Isn't the disabling performing a side effect on incoming options map ?
Previous point 2 is actually handled properly. +1
+1
Fixed typo as pointed out by Philippe and released for 3.3RC2 in HEAD.
The javadoc of org/eclipse/jdt/core/ToolFactory.html#createCodeFormatter(java.util.Map, int) has a spelling mistake (I believe). "determine" should take an 's' in the sentence: "The given mode determine what options ...". Following steps in comment 0 I get: public class Test { private void foo() { // TODO Auto-generated method stub } } I would have expected the method body to be indented. This is what happens if I use Ctrl + Shift + F once the method is created. Verified from a JDT/Core standpoint. The new API and the new fields are there. I opened bug 189255 against JDT/Text.
(In reply to comment #80) > The javadoc of > org/eclipse/jdt/core/ToolFactory.html#createCodeFormatter(java.util.Map, int) > has a spelling mistake (I believe). > "determine" should take an 's' in the sentence: > "The given mode determine what options ...". Thanks. This is now fixed in HEAD.
