Bug 208541 - [formatter] Formatter does not format whole region/selection
Summary: [formatter] Formatter does not format whole region/selection
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.4   Edit
Hardware: All All
: P2 normal (vote)
Target Milestone: 3.4 M4   Edit
Assignee: Eric Jodet CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-02 06:55 EDT by Dani Megert CLA
Modified: 2008-03-11 10:10 EDT (History)
5 users (show)

See Also:


Attachments
[proposed patch + test cases] on top v824 - all jdt.core tests OK (8.70 KB, patch)
2007-11-15 10:31 EST, Eric Jodet CLA
no flags Details | Diff
New proposal (8.82 KB, patch)
2007-11-15 13:15 EST, Olivier Thomann CLA
no flags Details | Diff
[proposed patch + new test case] on top v824 - all jdt.core tests OK (3.32 KB, patch)
2007-11-19 14:21 EST, Eric Jodet CLA
no flags Details | Diff
[proposed patch + test cases] on top v825 (11.77 KB, patch)
2007-11-22 12:59 EST, Eric Jodet CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2007-11-02 06:55:38 EDT
I20071101-2000.

Test Case:
1. create a CU like this
--- %< ---
public class Test {
                        int i= 1;               
}
--- %< ---
2. select the second line from left to right
3. format (Ctrl+Shift+F)

==>
The indentation is not corrected and trailing space is not removed.

Since we call API that gives JDT Core the full CU plus the correct region I'd expect this line to end up as if the I had formatted the whole CU.


This bug causes special grief when only formatting the changed lines when saving: as we pass the changed lines as an array of regions to the formatter using the new formatter API.
Comment 1 Olivier Thomann CLA 2007-11-02 09:00:24 EDT
Do you include whitespaces in the selection?
Since you pushed to get the API in for M3, you are more than welcome to propose a patch for this one.
Comment 2 Dani Megert CLA 2007-11-02 09:15:49 EDT
>Do you include whitespaces in the selection?
The full line excluding whitespace

>Since you pushed to get the API in for M3, you are more than welcome to propose
>a patch for this one.
Please...
This has *nothing* to do with the new API. Same bug in R3.3 with the old API.
Comment 3 Olivier Thomann CLA 2007-11-02 09:43:49 EDT
Glad to hear that it has nothing to do with the new API. Not obvious with the initial comment.
Comment 4 Dani Megert CLA 2007-11-02 09:47:18 EDT
Sorry about that. Thought it was clear because formatting a single selection is nothing new and you can't have more than one selection in the editor ;-)
Comment 5 Eric Jodet CLA 2007-11-07 05:18:02 EST
Formats ok without selection
Comment 6 Jerome Lanneluc CLA 2007-11-12 02:44:24 EST
Eric, please fix for 3.4M4 (ask Olivier for help if fix is not obvious).
Comment 7 Eric Jodet CLA 2007-11-15 10:31:04 EST
Created attachment 82969 [details]
[proposed patch + test cases] on top v824 - all jdt.core tests OK

all jdt.ui and jdt.text tests OK
Comment 8 Eric Jodet CLA 2007-11-15 10:32:03 EST
(In reply to comment #7)
Olivier: may you please review proposed patch - thanks
Comment 9 Olivier Thomann CLA 2007-11-15 13:15:49 EST
Created attachment 82986 [details]
New proposal

New proposal based on the previous patch.
Small changes to minimize the number of method invocations.
Eric, please review. If ok, then you can release.
Comment 10 Eric Jodet CLA 2007-11-16 07:12:27 EST
Patch released for 3.4M4 in HEAD.
Comment 11 Dani Megert CLA 2007-11-16 07:15:37 EST
Benno please, check whether we can bail out of our workaround code now.
Comment 12 Benno Baumgartner CLA 2007-11-19 04:45:56 EST
(In reply to comment #11)
> Benno please, check whether we can bail out of our workaround code now.

I still have a failing test case when I remove the workaround. Steps to reproduce:
Given:
package test1;
public class Test {
	int i = 1;     

}
1. Select line with field decl
2. Format
Is:
 the spaces after the field decl are not removed
Should:
 be removed

The strange thing here is, if I do the exact same thing but with:
Given:
public class Test {
	int i = 1;     

}
it works as expected (hint the missing package decl)
Comment 13 Benno Baumgartner CLA 2007-11-19 05:03:32 EST
reopen
Comment 14 Eric Jodet CLA 2007-11-19 05:21:39 EST
(In reply to comment #13)
investigating
Comment 15 Eric Jodet CLA 2007-11-19 14:21:48 EST
Created attachment 83270 [details]
[proposed patch + new test case] on top v824 - all jdt.core tests OK

improved patch - all jdt.ui and jdt.text OK.
Olivier, please review - thanks
Comment 16 Benno Baumgartner CLA 2007-11-20 04:37:55 EST
(In reply to comment #15)
> Created an attachment (id=83270) [details]
> [proposed patch + new test case] on top v824 - all jdt.core tests OK
> 
> improved patch - all jdt.ui and jdt.text OK.
> Olivier, please review - thanks
> 

I have two problems with this. First it still does not work if:
Given:
package test1;
public class Test {
        int i = 1;     

}
1. File>Convert Line Delimiters To>Unix
2. Select the line with the field decl
3. Format

Second, you now change the offset and length of regions in the array of Region passed to the formatter, this is unexpected, you must not modify objects passed in as parameters to a method unless this is specified. Modifying this array is a problem for me because I would need to make a copy before I pass it to the formatter, each time, on save.
Comment 17 Eric Jodet CLA 2007-11-22 12:59:35 EST
Created attachment 83555 [details]
[proposed patch + test cases] on top v825

all jdt.core, ui and text OK
Olivier, may you please review - thanks
Comment 18 Eric Jodet CLA 2007-11-23 09:47:45 EST
Patch released for 3.4M4 in HEAD.
Comment 19 Benno Baumgartner CLA 2007-11-26 08:44:48 EST
Introduced bug 210922
Comment 20 David Audel CLA 2007-12-11 07:48:44 EST
Verified for 3.4M4 using build I20071210-1800.
Comment 21 Eric Jodet CLA 2008-03-11 10:10:03 EDT
Also introduced bug 222182