Community
Participate
Working Groups
The Code Formatter has an option (Indentation -> Use tab character) to use tabs for indentation, but it appears to assume a tab always has the same width as the indentation amount, which the same preference panel calls 'Tab size'. It specifically ignores the Preferences -> Java -> Editor -> Appearance -> Displayed tab width setting. In Unix and GNU environments it is traditional to use tab essentially for compression, with the assumption that tabs are always 8 characters. It is common to indent code by 2, 3, or 4 columns, while also using tabs to correspond to 8 spaces. One might argue whether a such convention is a good idea where most programmers use IDEs and editors following different conventions; however the fact is that a lot of code follows these "Unix/GNU" conventions. This is perhaps less an issue for Java (though my "Kawa" code does follow this convention, for now), but it is probably a bigger issue for C/C++, and hence for CDT. I'm calling this an "enhancement" because it's not quite clear what the right behavior should be. You need a little bit of UI-design. The simplest would be to rename "Tab size" by "Indentation columns", and then if Use tab character is set use the Displayed tab width setting to compress initial spaces. The Emacs editor by default supports the descripted "Unix/GNU" standard; see http://www.gnu.org/software/emacs/manual/html_node/Display-Custom.html
>It specifically ignores the Preferences -> Java -> Editor -> Appearance -> >Displayed tab width setting. Of course. This is UI code which defines how a tab is rendered. It has nothing to do with how the file gets changed during formatting. Moving to J Core to comment.
> Of course. This is UI code which defines how a tab is rendered. It has nothing > to do with how the file gets changed during formatting. Right - but should it? Or rather: perhaps the code formatter needs two parameters: "indentation amount in columns", and "if using tabs, number of initial columsn to represent by a tab". If so, should the latter default to or be tied to the "Displayed tab width" setting? It's hard to think of a use case where you'd want these to be different. On the other hand, is the current behaviour (where "indentation amount in columns" and "if using tabs, number of initial columsn to represent by a tab" are tied to each other, regardless of the "Displayed tab width" setting) useful/sensible?
Created attachment 17059 [details] jdt-core_mixed-indents.diff The patches implements mixed settings for tab length and indentation size. ========== The only changes to the formatter itself are in Scribe and Alignment. Alignment.java: This is straight forward - all I did is replace its own tab-length computation by delegation to Scribe. Scribe.java: - added Scribe.indentationSize, which tells the amount of space-equivalents per indentation unit. - Scribe.indentationLevel is always space-equivalent based (e.g. if useTab && tabSize==2 && column==(4+1), then indentationLevel == 4, not 2*tab). Most changes are due to all the places I got rid of tabSize conversions. - Scribe.printIndentationIfNecessary() does some trickery finding out whether it should add a tab or not. It uses getNextIndentationLevel. - added Scribe.snapToTab - if this is true, all indentations are made to align with tab stops. It is set to true if tabs are used and the tabsize and indentation size are equal. To be honest, this is really to be compatible with the regression tests, which I guess are not alway 100% consistent when aligning parameters. - Scribe.getNextIndentationLevel computes the next tab stop considering snapToTab ========== RIPPLES: DefaultCodeFormatterConstants.java There are quite some clients of DefaultCodeFormatterConstants.FORMATTER_TAB_SIZE. Most clients use either one of the two previous meanings of the option. Some need the visual tab length (e.g. the editors that perform tab conversion and inform StyledText about the tab length), others the indentation size. I assume that the latter are the vast majority. Especially the clients that use TAB_SIZE together with TAB_CHAR to compute the indentation are definitely interested in the indentation size, not the tabulator length. This accounts for most of our test cases. I therefore let FORMATTER_TAB_SIZE mean FORMATTER_INDENTATION_SIZE (which replaces the deprecated FORMATTER_TAB_SIZE), and introduce the new option FORMATTER_TAB_LENGTH for the visual equivalent of one tab. DefaultCodeFormatterOptions.java - added the 'indentation_size' option - modified set/get accordingly - for compatibility, if the options passed to set() do not contain a value for FORMATTER_TAB_LENGTH, the indentation_size value is taken - modified default and javaconvention settings accordingly (java conventions are really with tab=8,indent=4, but I left that to not break the tests) CodeFormatter.java - added legacy conversion FormatterRegressionTests.java - this is covered by the compatibility code - except for one place where the tab_size is explicitely set to 3, I set the indentation_size to 3 There are more ripples where they are not so easy to resolve. In JDT Core, I mainly see org.eclipse.jdt.internal.core.dom.rewrite.Indents.java There are a couple of ripples in JDT-UI land, but these should be handleable. There may be some malfunctions for users that start to use mixed indentation settings, before everything is fully in place, but most of it should work right away.
Created attachment 17060 [details] jdt-core-tests-model_mixed-indents.diff
Created attachment 17061 [details] jdt-ui_mixed-indents.diff JDT-UI first cut of changes.
Great! Looking forward to trying this once it's stable.
Olivier - pls look into these proposed changes.
I will review these patches.
With the patches, I end up with a mix of tabs and spaces. Need to find out what is wrong.
Olivier, I'm not 100% sure but I think that's the idea and that's also the Java coding conventions default. AFAIK you find some hints and examples there.
As Dani has said, this is the entire point of the patch. Some people (don't count me in) like to use TAB as a character-saver on their files, where a TAB replaces any run of eight (or whathever tab-length is) SPACEs. Any indentation that is not a multiple of tab-length gets filled up with SPACEs. Unfortunately, this is the java coding standard... Example (indentation is 4, tab-length is 8): A single indentation will be 4 SPACE (.), double indentation 1 TAB (->-), triple 1 TAB 4 SPACE etc: class A { ....void method() { ---->---if (true) ---->---....return; ....} }
*** Bug 81776 has been marked as a duplicate of this bug. ***
The Sun JDK is among the existing code that assumes that a TAB is 8 characters. Thus, unless one reformats the JDK source, or chooses to indent one's own Java code by 8 spaces, the JDK classes appear incorrectly formatted when viewed in Eclipse 3.1 M4.
This should be a new mode to format code. It should not replace the existing tab formatting. it would be the default mode in java conventions settings. But I think it should still be possible to format using only tabs. I will try to integrate the patch into HEAD contents.
(In reply to comment #14) > This should be a new mode to format code. It should not replace the existing tab > formatting. it would be the default mode in java conventions settings. But I > think it should still be possible to format using only tabs. Agreed - I think this is the case as long as the indentation size and tab size are kept equal, as then there will be no odd spaces to add.
(In reply to comment #15) Yes, but it should also be possible to use only tabs, even if the indentation size and the tab size are different. I have several changes in the code formatter since the patch has been done. I am trying to integrate everything.
(In reply to comment #16) Do you mean an option to "ignore display tab width" when indenting, and set the "indentation amount" to "1 tab" rather than "N columns"? The question I see is the user interface. If you can add such an option without complicting the UI then I guess it's harmless. But it doesn't add any extra functionality, since people can achieve the same effect by setting the display tab width to the same as the indentation width and selecting "use tabs". If you want "indentation amount" to be "1 tab" it would be silly (i.e. confusing) to set set the tab width and indentation width different, and if they're the same you'd automatically indent only using tabs. Having an extra explicit option would be redundant and hence confusing. Perhaps you could lay out the UI like: Indentation between levels: * a tab (currently displayed as T columns) * [I] columns * using spaces only * using tabs (currently T columns) and spaces Here I is a fillable field. T should ideally be a link to the appropriate preference pane for display tab width. Alternatively: Identation between levels: [I] * use spaces only * use tabs (currently T columns) and spaces * use tabs only [warning: doesn't match current tab width T] The [warning] would only appear when I != T. The 2nd choice might be grayed out when I == T.
I integrated the patch, but I found issue with Scribe.snapToTab. If tab_length == indentation_size, the behavior should be similar to what we have now and this is not the case. I need to fix it before I can release.
Oh, I think know I understand your comment 14 and comment 16! Your concern is to avoid mixed indentations and you want the user to be able to say so. I think that the problem only arises for deep-alignment cases where parameter lists, field names etc. are aligned with a certain marker (an opening parenthesis for example): From FormatterRegressionTests#114: format: -- public void somehd(String argument1, String argument2,String argument3) {} -- expects (note the tab after the opening parenthesis): -- public void somehd(>String argument1,\n -->|-->|-->|-->|-->|String argument2,\n -->|-->|-->|-->|-->|String argument3) {\n } -- This is what you would expect in tab-only mode. If tab-only mode is off, you would want to get -- public void somehd(String argument1,\n -->|-->|-->|-->|...String argument2,\n -->|-->|-->|-->|...String argument3) {\n } -- The question to ask the user is whether the deep-alignment should be made to snap to an indentation level, or should use the odd indentation of the parenthesis? To get what you want, you can essentially make Scribe.snapToTabs a user preference instead of deriving it from the tabSize and indentationSize settings. This would avoid getting mixed indents as long as (indentationSize % tabSize == 0) holds. If this condition is false, there is no way to avoid mixed indents anyway. Also, Scribe.getNextIndentationLevel is wrong in my patch - it should round to the next *indentationSize*, not the next *tabStop*. It should read: --- public int getNextIndentationLevel(int someColumn) { int indent= someColumn - 1; if (indent == 0) return this.indentationLevel; if (snapToTabs) { int rem= indent % this.indentationSize; int addition= rem == 0 ? 0 : this.indentationSize - rem; // round to superior return indent + addition; } else { return indent; } } --- One thing to consider in this discussion is also bug 49896 which requests to *only use spaces* when doing deep-alignment...
I added a snapToTabs option, but I think this should be enabled only if the formatter is using tabs. The bug 49896 would be fixed by using tabs without enabling snapToTabs, I believe. Do you think this could solve this issue? When can we synchronize to release this code? I'd like to try it in a nightly build first. I don't want to break an integration build.
(In reply to comment #20) > I added a snapToTabs option, but I think this should be enabled only if the > formatter is using tabs. I agree it doesn't make much sense otherwise. > The bug 49896 would be fixed by using tabs without enabling snapToTabs, I believe. I don't think so - the bug requests that any deep alignment is done using spaces. Any standard indents (including double indentation for continuations) would continue to use tabs (if so configured). Everything that you call INDENT_ON_COLUMN would use spaces only. This way, everyone can choose the basic indent he likes, but aligning parameters still works. class X { -->|int method(int param1, -->|...........int param2) { -->|-->|return param1 + param2; -->|} } But that is a separate issue really... > When can we synchronize to release > this code? I'd like to try it in a nightly build first. I don't want to break an > integration build. I see - plus the next I-Build is really a milestone... I think we should try as soon as possible. + I have patches for jdt-ui/-text. + As long as the user does not change the tab setting to something different than the indentation setting, everything will be as it is now, so I don't think the risk is unreasonable high. + If we find a serious problem, we can always disable the new option in the UI so users can't set the indentation size to something other than tab size. So, from our standpoint (checked with Dani & Dirk): go for it if you can add it tonight or tomorrow. This way, we can at least check it out on friday and monday.
Changes in: dom/org/eclipse/jdt/internal/core/dom/rewrite/ASTRewriteFormatter.java formatter/org/eclipse/jdt/core/formatter/DefaultCodeFormatterConstants.java formatter/org/eclipse/jdt/internal/formatter/DefaultCodeFormatterOptions.java formatter/org/eclipse/jdt/internal/formatter/Scribe.java formatter/org/eclipse/jdt/internal/formatter/align/Alignment.java formatter/org/eclipse/jdt/internal/formatter/old/CodeFormatter.java model/org/eclipse/jdt/internal/core/CreateTypeMemberOperation.java model/org/eclipse/jdt/internal/core/JavaCorePreferenceInitializer.java src/org/eclipse/jdt/core/tests/formatter/FormatterRegressionTests.java src/org/eclipse/jdt/core/tests/rewrite/describing/ASTRewritingTest.java src/org/eclipse/jdt/core/tests/rewrite/modifying/ASTRewritingModifyingTest.java workspace/Formatter/test458/A_out.java The code will be released shortly to let you adapt the UI and Text tomorrow.
Fixed and released in HEAD. Regression tests updated.
cool, thanks!
Using this version of the formatter, I am seeing strange behavior with a tab size of 4. Statements are no longer properly indented; when manually setting tab size to 8, it looks fine.
Created attachment 17818 [details] My formatting options
Philippe, the nightly build does not yet contain the JDT UI code that does the formatter profile conversion - I only released it this morning. I will attach a jdt-ui preview that should work for you. This will fix all clients that use a custom formatter profile. We're also discussing changing the default setting for tab-length in the JavaConventionSettings back to 4 so existing clients always see the same behavior before and after the formatter change.
We will revert this fix for now. Too close to M5 and we have some compatibility issues with 3.0 clients. This will be worked on immediately after M5.
*** Bug 82090 has been marked as a duplicate of this bug. ***
*** Bug 87055 has been marked as a duplicate of this bug. ***
In order to fix this we propose the following changes: We would like to add a new value for the FORMATTER_TAB_CHAR option. /** * <pre> * FORMATTER / Option to specify the tabulation size * - option id: "org.eclipse.jdt.core.formatter.tabulation.size" * - possible values: { TAB, SPACE } * - default: TAB * </pre> * @see JavaCore#TAB * @see JavaCore#SPACE * @since 3.0 */ public static final String FORMATTER_TAB_CHAR = JavaCore.PLUGIN_ID + ".formatter.tabulation.char"; //$NON-NLS-1$ This is an API breaking change, because the API doesn't specify that other values can be defined. We can also claim that it doesn't explicitely say that the only possible values are SPACE and TAB. However the source compatibility is not preserved if someone is using our option. if (myValue.equals(JavaCore.TAB)) { ... } else { // this is implicitely SPACE ... } But with a third value this could be wrong. The code would need to be changed for: if (myValue.equals(JavaCore.TAB)) { ... } else if (myValue.equals(JavaCore.SPACE)) { // this is implicitely SPACE ... } So it is trivial to explain how to port existing code once a new value is added. The only client of this API right now is JDT/UI. I don't believe there is anybody else using these APIs. With this change, the existing UI would not be broken because they explicitely set a value to be either TAB or SPACE. Jim, let me know if I missed anything.
Olivier, Please include the modified Javadoc spec for FORMATTER_TAB_CHAR that mentions the new third value and explains more values may be added in future.
Here is the modified javadoc: /** * <pre> * FORMATTER / Option to specify the tabulation size * - option id: "org.eclipse.jdt.core.formatter.tabulation.char" * - possible values: { TAB, SPACE, MIXED } * - default: TAB * </pre> * More values may be added in the future. * * @see JavaCore#TAB * @see JavaCore#SPACE * @see #MIXED * @since 3.0 */ I will attached a patch with the modifications.
Created attachment 18667 [details] Modified javadoc. Apply on HEAD
Request forwarded to PMC. Risks seem very low. Please proceed for 3.1M6 under assumption that PMC will approve. Be sure to add an entry in Incompatibilities section of the 3.1M6 Migration Guide (in the /org.eclipse.platform.doc.isv/porting/3.1/incompatibilities.html). And let me know if you'd like me to polish your draft.
Fixed and released in HEAD. Regression test added in FormatterRegressionTests.test549/550/551/552/555. The following API has been added: org.eclipse.jdt.core.formatter.DefaultCodeFormatterConstants.FORMATTER_INDENTATION_SIZE This is used to set the size of one indentation in term of spaces. It is used only if the org.eclipse.jdt.core.formatter.DefaultCodeFormatterConstants.FORMATTER_TAB_CHAR value is org.eclipse.jdt.core.formatter.DefaultCodeFormatterConstants.MIXED. Tom, Could you please review the changes?
Looking good.
Is this fix supposed to be in: eclipse-SDK-I20050329-2000-linux-gtk.tar.gz? I assume not - I don't see any difference. I've got Preferences -> Text Editors -> Displayed tab width = 8, That seems to have no effect; instead it uses the Preferences -> Java -> Formatter -> Indentation: tab size to *override* the display tab width. Also, I don't see any change in the UI.
As far as I know, this is only at the core level, not the ui level yet.
UI support will not make it into M6, as there are a couple of places that need adaption.
But it has been fixed in HEAD? Should I try the next nightly build (rather than a stream integration build) to try this out? Tom Eicher wrote there are "acouple of places that need adaption" which to me implies there are still some issues. So perhaps marking this as "resolved fixed" was premature?
It is resolved and fixed from the JDT/Core stand point. This is the component attached to this PR. This doesn't mean the JDT/UI is in line with what core provides. We needed a new API to fix it and it had to be done for M6. JDT/UI can however adapt after M6 since they don't need to add a new API.
I filed bug 89593 to track the UI side of it and added you to the cc list.
API is in place and regression tests passed. Verified in 20050330-0500
*** Bug 89823 has been marked as a duplicate of this bug. ***
