Bug 376734 - [Backport] Organize imports wipes comments between statements [code manipulation]
Summary: [Backport] Organize imports wipes comments between statements [code manipulat...
Status: RESOLVED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.6.2   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.6.2+   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 24804 376930
Blocks:
  Show dependency tree
 
Reported: 2012-04-13 11:27 EDT by Marek Chodorowski CLA
Modified: 2012-05-02 07:04 EDT (History)
5 users (show)

See Also:
daniel_megert: pmc_approved+

Attachments
patch v0.1 (13.77 KB, patch)
2012-04-13 11:27 EDT, Marek Chodorowski CLA 		no flags Details | Diff
patch + tests to backport (22.69 KB, patch)
2012-04-23 10:33 EDT, Ayushman Jain CLA 		no flags Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Marek Chodorowski CLA 2012-04-13 11:27:55 EDT 
Created attachment 213982 [details]
patch v0.1

This is a request to backport the fix for bug 24804 to 3.6.2+.
Comment 1 Ayushman Jain CLA 2012-04-13 13:44:38 EDT 
Hi Marek, what's the motivation behind the backport? Note that it was not a regression.
Comment 2 Marek Chodorowski CLA 2012-04-16 02:44:08 EDT 
Hello, this problem was classified as data loss that is why I was asked to
create bugzilla backport request.
Comment 3 Ayushman Jain CLA 2012-04-16 08:49:22 EDT 
(In reply to comment #2)
> Hello, this problem was classified as data loss that is why I was asked to
> create bugzilla backport request.
Yes, I'm reviewing it for backport. Thanks.
Comment 4 Ayushman Jain CLA 2012-04-16 15:20:48 EDT 
The code in org.eclipse.jdt.internal.core.dom.rewrite.ImportRewriteAnalyzer.addExistingImports(CompilationUnit) lines 281 - 297 is not right in the way it calculates ranges of comments before and after an import. However, I've not able to trigger the code that finally uses the two new fields org.eclipse.jdt.internal.core.dom.rewrite.ImportRewriteAnalyzer.ImportDeclEntry.precedingCommentRange and trailingCommentRange via the get..() methods, such that they have non-null values. 
Even the new tests don't trigger that condition. I'll keep trying and open a new bug.
Comment 5 Ayushman Jain CLA 2012-04-17 01:51:19 EDT 
The backport should be done when bug 376930 is fixed.
Comment 6 Ayushman Jain CLA 2012-04-22 10:17:58 EDT 
This patch is good if applied with the fix for bug 376930.

Dani, need your +1 as well.
Comment 7 Dani Megert CLA 2012-04-23 08:55:44 EDT 
(In reply to comment #6)
> This patch is good if applied with the fix for bug 376930.
> 
> Dani, need your +1 as well.

What's the patch to backport? The attached one? It does not include any test cases.
Comment 8 Szymon Ptaszkiewicz CLA 2012-04-23 09:10:27 EDT 
(In reply to comment #7)
> What's the patch to backport? The attached one? It does not include any test
> cases.

Marek and I assumed that tests are not backported. If they are, then I guess the whole patch from bug 24804 should be backported.
Comment 9 Ayushman Jain CLA 2012-04-23 10:33:52 EDT 
Created attachment 214395 [details]
patch + tests to backport

This is the full patch taken from bug 24804. It also contains a JDT.UI test - ImportOrganizeTest.
Comment 10 Dani Megert CLA 2012-04-23 10:36:42 EDT 
(In reply to comment #9)
> Created attachment 214395 [details] [diff]
> patch + tests to backport
> 
> It also contains a JDT.UI test ImportOrganizeTest.
That I'd leave out, because it creates additional work on the JDT UI side for the backport.
Comment 11 Dani Megert CLA 2012-04-30 07:08:15 EDT 
+1 to backport this along with bug 376930, excluding the JDT UI tests.
Comment 12 Ayushman Jain CLA 2012-04-30 08:10:17 EDT 
(In reply to comment #11)
> +1 to backport this along with bug 376930, excluding the JDT UI tests.

I and Dani decided that these tests are necessary to be backported to reflect the new behaviour because of the patch. Hence, raised bug 378068 to track the UI backport.
Comment 13 Ayushman Jain CLA 2012-04-30 08:15:44 EDT 
Backported to R3_7_maintenance via commit ea81eedacd738fc30b5cea61e33c1c4e17dac10d
Comment 15 Dani Megert CLA 2012-05-02 06:45:51 EDT 
Please also backport to 3.6.2+.