Community
Participate
Working Groups
I20081012 1. get org.eclipse.jdt.internal.ui.preferences.ProblemSeveritiesConfigurationBlock rev. 1.53 2. selection from beginning of line 155 to line 158 3. Ctrl+Shift+F ==> broken indentation
Created attachment 114952 [details] My formatter preferences
You may not believe me but... it works as designed! First point, as your selection does not include the new line characters after the opening brace, it isn't removed by the formatter. That explains why your formatted text still starts the line after the opening brace... Second point, as your selection includes the new line after the last constant of the array initializer and your preferences says not include a new line before the closing brace of the array initializer, then this new line is removed by the formatter. However, you still get the tabulations before the closing brace because they were not in your initial selection... So, to have the correct formatting using the selection, you need to select the entire body of the array initializer and you'll see the expected output... OK to close as INVALID?
hmmm, finally, there may be something to do in this area. The test case I used was: public class X { public static String[] getKeys() { return new String[] { "PREF_PB_COMPARING_IDENTICAL", "PREF_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD", "PREF_PB_MISSING_HASHCODE_METHOD", "PREF_PB_DEAD_CODE" }; } } and I selected the two lines. That's why the behavior looks ok for me as the selection starts at the first expression of the array initializer... But with the following test case: public class X { public static String[] getKeys() { return new String[] { "TEST", "PREF_PB_COMPARING_IDENTICAL", "PREF_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD", "PREF_PB_MISSING_HASHCODE_METHOD", "PREF_PB_DEAD_CODE" }; } } and still selecting the last two lines, then I agree that it seems odd to remove the indentation of the beginning of the selection and replace it with only one space because the selection does not start on the first expression of the array initializer... So, I won't close it as WONTFIX...
>You may not believe me but... it works as designed! Sorry, but then the design needs to get fixed ;-) Seeing comment 3 I guess you already realized this. Just think of being the user/client and not the coder of the formatter.
(In reply to comment #4) > >You may not believe me but... it works as designed! > Sorry, but then the design needs to get fixed ;-) > The design I was talking about was: while formatting a region, never modify (replace, delete) characters which are outside the given region... and I guess you do not want to see it changed... I was referring to it to explain why the cr/lf before the selection were not removed... > Seeing comment 3 I guess you already realized this. Just think of being the > user/client and not the coder of the formatter. > I just missed the key point here, but fixed this error by writing comment 3... I did not try to forget users to ease my pain...
>The design I was talking about was OK, makes it more clear and I agree.
Hum, I finally think that there's nothing we can do on our side... I had some troubles to retrieve all problem information, hence let me first explain it again... Here's the initial test case, similar than the comment 0 one (but easier to fill in a bugzilla comment...): public class B { public static String[] getKeys() { return new String[] { "TEST", "PREF_PB_COMPARING_IDENTICAL", "PREF_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD", "PREF_PB_MISSING_HASHCODE_METHOD", "PREF_PB_DEAD_CODE" }; } } I select the 3 last lines of the array initializer (the selection is shown by the brackets): public class B { public static String[] getKeys() { return new String[] { "TEST", "PREF_PB_COMPARING_IDENTICAL", [ "PREF_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD", "PREF_PB_MISSING_HASHCODE_METHOD", "PREF_PB_DEAD_CODE" ] }; } } Formatting with the attached formatter profile, I get: public class B { public static String[] getKeys() { return new String[] { "TEST", "PREF_PB_COMPARING_IDENTICAL", "PREF_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD", "PREF_PB_MISSING_... } } Note that the 3 selected lines where joined by the formatter in only one but due to bugzilla comment wrapping, I cannot display it entirely... The problem is that the leading tabulation (4 tabs or 12 spaces) has been broken and replaced with a single space! OK, now that the problem is set, why it happens...!? Simply because the selection includes the leading tabulations. Hence, as I said in previous comments, the formatter "has the authorization to..." replace it with the corresponding white spaces it has in the overlapping edit. In this case, as the array initializer line wrapping policy is "Wrap when necessary" and the max line width is 200, then the entire array initializer would have been formatted in only one line (verify it by formatting the entire class file...). So, the edit at the beginning of the selected region is just a single space between two items of the array initializer. As, this edit has the same offset than the selected region, the formatter does the right thing when it replaces the indentation at the beginning of the selected region by this single space. So, that shows the problem is in fact more in the formatter preferences or in the selection itself... If the formatter preferences did introduce at that offset an indentation (e.g. by using the "Wrap all elements, every element on a new line" + "Force split"), then the leading indentation would be replaced by... a leading indentation and there wouldn't be any problem. If the selection did not include the leading indentation, then the formatter wouldn't be allowed to replace it and there wouldn't be any problem either. I tried to verify this last possible solution using the editor but, unfortunately, the editor always select the beginning of the line while calling the formatter with a selected area... Hence that apparently does not change anything :-( However, I will attach a formatter test which will show that without changing the selected area, the formatter behaves correctly and does not break the indentation when the selection does not include the leading indentation... Dani, what should I do of this bug? Put it back in JDT/Text land because there's no way to give the formatter the correct selection (I cannot retrieve any existing bug about this issue), finally close it as wontfix or do you still think there's a real JDT/Core formatter bug here?
Created attachment 185856 [details] Test cases illustrating the formatter correct behavior The test case showing that the formatter behavior is correct with the right selection is testBug250616_selection_ok.
Olivier, Do you think I should release the attached tests?
(In reply to comment #7) > Hum, I finally think that there's nothing we can do on our side... > > I had some troubles to retrieve all problem information, hence let me first > explain it again... > > Here's the initial test case, similar than the comment 0 one (but easier to > fill in a bugzilla comment...): > public class B { > > public static String[] getKeys() { > return new String[] { > "TEST", > "PREF_PB_COMPARING_IDENTICAL", > "PREF_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD", > "PREF_PB_MISSING_HASHCODE_METHOD", > "PREF_PB_DEAD_CODE" > }; > } > } > > I select the 3 last lines of the array initializer (the selection is shown by > the brackets): > public class B { > > public static String[] getKeys() { > return new String[] { > "TEST", > "PREF_PB_COMPARING_IDENTICAL", > [ "PREF_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD", > "PREF_PB_MISSING_HASHCODE_METHOD", > "PREF_PB_DEAD_CODE" > ] }; > } > } > > Formatting with the attached formatter profile, I get: > public class B { > > public static String[] getKeys() { > return new String[] { > "TEST", > "PREF_PB_COMPARING_IDENTICAL", > "PREF_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD", "PREF_PB_MISSING_... > } > } > > Dani, > what should I do of this bug? > ... > or do you still think there's a real JDT/Core formatter bug here? As you might have guessed already - yes - from a client side I still consider this a bug in JDT Core as the behavior is useless for a customer (or do you claim to be happy with the above result? ;-). Giving the formatter the entire lines that are expected to be formatted must be enough to produce a correct result.
(In reply to comment #10) > > Dani, > > what should I do of this bug? > > ... > > or do you still think there's a real JDT/Core formatter bug here? > As you might have guessed already - yes - from a client side I still consider > this a bug in JDT Core as the behavior is useless for a customer (or do you > claim to be happy with the above result? ;-). Giving the formatter the entire > lines that are expected to be formatted must be enough to produce a correct > result. You should have put the entire paragraph: > Dani, > what should I do of this bug? Put it back in JDT/Text land because there's no > way to give the formatter the correct selection (I cannot retrieve any > existing bug about this issue), finally close it as wontfix or do you still > think there's a real JDT/Core formatter bug here? But I was surely not clear enough, as usual... I didn't say there's no bug! Of course I cannot claim to be happy with the current behavior... What I was saying is that I do not think the problem is in JDT/Core land. The problem is that JDT/Text changes the selection to the entire line even if I (as from the client side) haven't selected all the line to have the correct behavior. So, my personal feeling would be to put back this bug into JDT/Text land. As I already explained, the current formatter behavior is due to the selected preferences, hence if you do not agree to assign this bug to JDT/Text, the other solution I see is to close it as invalid due to incorrect preferences setting...
>As I already explained, the current formatter behavior is due to the selected >preferences, Which formatter preference? I would rather expect bad/wrong formatting if I don't give you the entire line and actually in the past it didn't work correctly with partial selection and we were asked to give the full lines. Bummer!
(In reply to comment #12) > >As I already explained, the current formatter behavior is due to the selected > >preferences, > Which formatter preference? > See the following paragraphs of comment #7: ... >> In this case, as the array initializer line wrapping policy is "Wrap when >> necessary" and the max line width is 200, then the entire array initializer >> would have been formatted in only one line (verify it by formatting the >> entire class file...). So, the edit at the beginning of the selected region >> is just a single space between two items of the array initializer. >> As, this edit has the same offset than the selected region, the formatter >> does the right thing when it replaces the indentation at the beginning of >> the selected region by this single space. >> >> So, that shows the problem is in fact more in the formatter preferences or >> in the selection itself... >> >> If the formatter preferences did introduce at that offset an indentation >> (e.g. by using the "Wrap all elements, every element on a new line" + >> "Force split"), then the leading indentation would be replaced by... >> a leading indentation and there wouldn't be any problem. ... > I would rather expect bad/wrong formatting if I don't give you the entire line > and actually in the past it didn't work correctly with partial selection and > we were asked to give the full lines. Bummer! IMO, this is the real issue here. I understand that partial selections could not work properly in the past... but this is no longer the case and I think that JDT/Text has now to take this into account. For now, I'll put this bug into the jdt-core-triaged inbox in case we want to restart this discussion later...
> > I would rather expect bad/wrong formatting if I don't give you the entire line > > and actually in the past it didn't work correctly with partial selection and > > we were asked to give the full lines. Bummer! > > IMO, this is the real issue here. I understand that partial selections could > not work properly in the past... but this is no longer the case and I think > that JDT/Text has now to take this into account. As you can imagine I disagree. Why should *all* clients have to deal with this if it can be done at the service provide layer?
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
*** Bug 426494 has been marked as a duplicate of this bug. ***
New Gerrit change created: https://git.eclipse.org/r/153838
Gerrit change https://git.eclipse.org/r/153838 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=ba73b313fde63d8ccedec4bd1f0dfefd65dc4e33
Now the formatter will recognize line wraps outside formatting regions and even adapt to invalid indentation.
(In reply to Eclipse Genie from comment #18) > Gerrit change https://git.eclipse.org/r/153838 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=ba73b313fde63d8ccedec4bd1f0dfefd65dc4e33 This caused severe regression, see bug 559006.
