Bug 570458 - [cleanup & saveaction] Preserve empty lines When "Pulling

down common code from if"

Change-Id: I613ad71f06dc501375c03bf2744909f404bfcc48
Signed-off-by: Fabrice Tiercelin <fabrice.tiercelin@yahoo.fr>

2 files changed, 33 insertions, 20 deletions

diff --git a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java
index 3922bdc846..701072e833 100644
--- a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java
+++ b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest.java
@@ -14028,6 +14028,7 @@ public class CleanUpTest extends CleanUpTestCase {
 
 	@Test
 	public void testControlFlowMerge() throws Exception {
+		// Given
 		IPackageFragment pack= fSourceFolder.createPackageFragment("test1", false, null);
 		String given= "" //
 				+ "package test1;\n" //
@@ -14175,19 +14176,23 @@ public class CleanUpTest extends CleanUpTestCase {
 				+ "        if (b1) {\n" //
 				+ "            // Keep this comment\n" //
 				+ "            i++;\n" //
+				+ "\n" //
 				+ "            j++;\n" //
 				+ "        } else if (b2) {\n" //
 				+ "            i++;\n" //
 				+ "            // Keep this comment\n" //
 				+ "            i++;\n" //
+				+ "\n" //
 				+ "            j++;\n" //
 				+ "        } else if (modifiableList.remove(\"foo\")) {\n" //
 				+ "            // Keep this comment\n" //
 				+ "            i++;\n" //
+				+ "\n" //
 				+ "            j++;\n" //
 				+ "        } else {\n" //
 				+ "            // Keep this comment\n" //
 				+ "            i++;\n" //
+				+ "\n" //
 				+ "            j++;\n" //
 				+ "        }\n" //
 				+ "    }\n" //
@@ -14201,9 +14206,6 @@ public class CleanUpTest extends CleanUpTestCase {
 				+ "        }\n" //
 				+ "    }\n" //
 				+ "}\n";
-		ICompilationUnit cu= pack.createCompilationUnit("E.java", given, false, null);
-
-		enable(CleanUpConstants.CONTROLFLOW_MERGE);
 
 		String expected= "" //
 				+ "package test1;\n" //
@@ -14311,6 +14313,7 @@ public class CleanUpTest extends CleanUpTestCase {
 				+ "        }\n" //
 				+ "        // Keep this comment\n" //
 				+ "        i++;\n" //
+				+ "\n" //
 				+ "        j++;\n" //
 				+ "    }\n" //
 				+ "\n" //
@@ -14322,6 +14325,11 @@ public class CleanUpTest extends CleanUpTestCase {
 				+ "    }\n" //
 				+ "}\n";
 
+		// When
+		ICompilationUnit cu= pack.createCompilationUnit("E.java", given, false, null);
+		enable(CleanUpConstants.CONTROLFLOW_MERGE);
+
+		// Then
 		assertNotEquals("The class must be changed", given, expected);
 		assertGroupCategoryUsed(new ICompilationUnit[] { cu }, new HashSet<>(Arrays.asList(MultiFixMessages.ControlFlowMergeCleanUp_description)));
 		assertRefactoringResultAsExpected(new ICompilationUnit[] { cu }, new String[] { expected });
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/ControlFlowMergeCleanUp.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/ControlFlowMergeCleanUp.java
index acbde1d858..7646ac97ec 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/ControlFlowMergeCleanUp.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/ControlFlowMergeCleanUp.java
@@ -320,18 +320,18 @@ public class ControlFlowMergeCleanUp extends AbstractMultiFix {
 		private final IfStatement visited;
 		private final List<ASTNode> allCases;
 		private final List<List<Statement>> allCasesStatements;
-		private final List<Statement>[] caseStmtsToRemove;
+		private final List<Statement>[] caseStatementsToPullDown;
 		private final List<Integer> casesToRefactor;
 
 		public ControlFlowMergeOperation(final IfStatement visited,
 				final List<ASTNode> allCases,
 				final List<List<Statement>> allCasesStatements,
-				final List<Statement>[] caseStmtsToRemove2,
+				final List<Statement>[] caseStatementsToPullDown,
 				final List<Integer> casesToRefactor) {
 			this.visited= visited;
 			this.allCases= allCases;
 			this.allCasesStatements= allCasesStatements;
-			this.caseStmtsToRemove= caseStmtsToRemove2;
+			this.caseStatementsToPullDown= caseStatementsToPullDown;
 			this.casesToRefactor= casesToRefactor;
 		}
 
@@ -345,18 +345,18 @@ public class ControlFlowMergeCleanUp extends AbstractMultiFix {
 			boolean[] areCasesRemovable= new boolean[allCasesStatements.size()];
 			Arrays.fill(areCasesRemovable, false);
 			removeStmtsFromCases(rewrite, group, areCasesRemovable);
-			List<Statement> oneCaseToRemove= caseStmtsToRemove[casesToRefactor.get(0)];
+			List<Statement> oneCaseToPullDown= caseStatementsToPullDown[casesToRefactor.get(0)];
 
 			if (allRemovable(areCasesRemovable, 0)) {
 				if (ASTNodes.canHaveSiblings(visited)) {
-					insertIdenticalCode(rewrite, group, oneCaseToRemove);
+					insertIdenticalCode(rewrite, group, oneCaseToPullDown);
 
 					ASTNodes.removeButKeepComment(rewrite, visited, group);
 				} else {
-					List<Statement> orderedStatements= new ArrayList<>(oneCaseToRemove.size());
+					List<Statement> orderedStatements= new ArrayList<>(oneCaseToPullDown.size());
 
-					for (Statement stmtToRemove : oneCaseToRemove) {
-						orderedStatements.add(0, ASTNodes.createMoveTarget(rewrite, stmtToRemove));
+					for (Statement statementToPullDown : oneCaseToPullDown) {
+						orderedStatements.add(0, ASTNodes.createMoveTarget(rewrite, statementToPullDown));
 					}
 
 					Block newBlock= ast.newBlock();
@@ -389,11 +389,11 @@ public class ControlFlowMergeCleanUp extends AbstractMultiFix {
 				}
 
 				if (ASTNodes.canHaveSiblings(visited)) {
-					insertIdenticalCode(rewrite, group, oneCaseToRemove);
+					insertIdenticalCode(rewrite, group, oneCaseToPullDown);
 				} else {
-					List<Statement> orderedStatements= new ArrayList<>(oneCaseToRemove.size() + 1);
+					List<Statement> orderedStatements= new ArrayList<>(oneCaseToPullDown.size() + 1);
 
-					for (Statement stmtToRemove : oneCaseToRemove) {
+					for (Statement stmtToRemove : oneCaseToPullDown) {
 						orderedStatements.add(0, ASTNodes.createMoveTarget(rewrite, stmtToRemove));
 					}
 
@@ -405,12 +405,17 @@ public class ControlFlowMergeCleanUp extends AbstractMultiFix {
 			}
 		}
 
-		private void insertIdenticalCode(final ASTRewrite rewrite, final TextEditGroup group, final List<Statement> stmtsToRemove) {
-			ListRewrite listRewrite= rewrite.getListRewrite(visited.getParent(), (ChildListPropertyDescriptor) visited.getLocationInParent());
-
-			for (Statement stmtToRemove : stmtsToRemove) {
-				listRewrite.insertAfter(ASTNodes.createMoveTarget(rewrite, stmtToRemove), visited, group);
+		private void insertIdenticalCode(final ASTRewrite rewrite, final TextEditGroup group, final List<Statement> statementsToPullDown) {
+			ASTNode moveTarget;
+			if (statementsToPullDown.size() == 1) {
+				moveTarget= ASTNodes.createMoveTarget(rewrite, statementsToPullDown.get(0));
+			} else {
+				ListRewrite listRewrite= rewrite.getListRewrite(statementsToPullDown.get(0).getParent(), (ChildListPropertyDescriptor) statementsToPullDown.get(0).getLocationInParent());
+				moveTarget= listRewrite.createMoveTarget(statementsToPullDown.get(statementsToPullDown.size() - 1), statementsToPullDown.get(0));
 			}
+
+			ListRewrite targetListRewrite= rewrite.getListRewrite(visited.getParent(), (ChildListPropertyDescriptor) visited.getLocationInParent());
+			targetListRewrite.insertAfter(moveTarget, visited, group);
 		}
 
 		private boolean allRemovable(final boolean[] areCasesRemovable, final int start) {
@@ -425,7 +430,7 @@ public class ControlFlowMergeCleanUp extends AbstractMultiFix {
 
 		private void removeStmtsFromCases(final ASTRewrite rewrite, final TextEditGroup group, final boolean[] areCasesRemovable) {
 			for (int i : casesToRefactor) {
-				List<Statement> removedStatements= caseStmtsToRemove[i];
+				List<Statement> removedStatements= caseStatementsToPullDown[i];
 				ASTNode parent= allCases.get(i);
 
 				if (removedStatements.containsAll(allCasesStatements.get(i))