Fix column removal in reorder leaves undeleted hidden cells.

main/src/com/google/refine/model/changes/CellAtRowCellIndex.java

@@ -0,0 +1,84 @@
+/*
+
+Copyright 2010, Google Inc.
+All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are
+met:
+
+    * Redistributions of source code must retain the above copyright
+notice, this list of conditions and the following disclaimer.
+    * Redistributions in binary form must reproduce the above
+copyright notice, this list of conditions and the following disclaimer
+in the documentation and/or other materials provided with the
+distribution.
+    * Neither the name of Google Inc. nor the names of its
+contributors may be used to endorse or promote products derived from
+this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+*/
+
+package com.google.refine.model.changes;
+
+import java.io.IOException;
+import java.io.Writer;
+import java.util.Properties;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import com.google.refine.model.Cell;
+import com.google.refine.util.Pool;
+
+public class CellAtRowCellIndex {
+
+        final public int row;
+        final public int cellIndex;
+        final public Cell cell;
+        final static private Pattern semicolonPattern = Pattern.compile(";");
+
+        public CellAtRowCellIndex(int row, int cellIndex, Cell cell) {
+                this.row = row;
+                this.cell = cell;
+                this.cellIndex = cellIndex;
+        }
+
+        public void save(Writer writer, Properties options) throws IOException {
+                writer.write(Integer.toString(row));
+                writer.write(';');
+                writer.write(Integer.toString(cellIndex));
+                writer.write(';');
+                if (cell != null) {
+                        cell.save(writer, options);
+                }
+        }
+
+        static public CellAtRowCellIndex load(String s, Pool pool) throws Exception {
+
+                Matcher m = semicolonPattern.matcher(s);
+
+                m.find();
+                int semicolon = m.start();
+                m.find();
+                int nextSemicolon = m.start();
+
+                int row = Integer.parseInt(s.substring(0, semicolon));
+                int cellIndex = Integer.parseInt(s.substring(semicolon + 1, nextSemicolon));
+                Cell cell = nextSemicolon < s.length() - 1 ? Cell.loadStreaming(s.substring(nextSemicolon + 1), pool)
+                                : null;
+
+                return new CellAtRowCellIndex(row, cellIndex, cell);
+        }
+}

main/src/com/google/refine/model/changes/ColumnReorderChange.java

@@ -42,16 +42,20 @@ import java.util.List;
 import java.util.Properties;
 
 import com.google.refine.history.Change;
+import com.google.refine.model.Cell;
 import com.google.refine.model.Column;
 import com.google.refine.model.ColumnGroup;
 import com.google.refine.model.Project;
+import com.google.refine.model.Row;
 import com.google.refine.util.Pool;
 
 public class ColumnReorderChange extends ColumnChange {
     final protected List<String>    _columnNames;
     protected List<Column>          _oldColumns;
     protected List<Column>          _newColumns;
+    protected List<Column>          _removedColumns;
     protected List<ColumnGroup>     _oldColumnGroups;
+    protected CellAtRowCellIndex[]  _oldCells;
     
     public ColumnReorderChange(List<String> columnNames) {
         _columnNames = columnNames;
@@ -74,6 +78,43 @@ public class ColumnReorderChange extends ColumnChange {
                 _oldColumnGroups = new ArrayList<ColumnGroup>(project.columnModel.columnGroups);
             }
             
+            if (_removedColumns == null) {
+            	_removedColumns = new ArrayList<Column>();
+	            for (String n : project.columnModel.getColumnNames()) {
+	            	Column oldColumn = project.columnModel.getColumnByName(n);
+	            	if(!_newColumns.contains(oldColumn)) {
+	            		_removedColumns.add(oldColumn);
+	            	}
+	            }
+            }
+
+            if (_oldCells == null) {
+	            _oldCells = new CellAtRowCellIndex[project.rows.size() * _removedColumns.size()];
+
+	            int count = 0;
+	            for (int i = 0; i < project.rows.size(); i++) {
+	                for (int j = 0; j < _removedColumns.size(); j++) {
+	            		int cellIndex = _removedColumns.get(j).getCellIndex();
+	            		Row row = project.rows.get(i);
+
+	            		Cell oldCell = null;
+	            		if (cellIndex < row.cells.size()) {
+	            			oldCell = row.cells.get(cellIndex);
+	            		}
+	            		_oldCells[count++] = new CellAtRowCellIndex(i, cellIndex,oldCell);
+	            	}
+	            }
+            }
+
+            //Clear cells on removed columns.
+            for (int i = 0; i < project.rows.size(); i++) {
+                for (int j = 0; j < _removedColumns.size(); j++) {
+            		int cellIndex = _removedColumns.get(j).getCellIndex();
+            		Row row = project.rows.get(i);
+            		row.setCell(cellIndex, null);
+            	}
+            }
+
             project.columnModel.columns.clear();
             project.columnModel.columns.addAll(_newColumns);
             project.columnModel.columnGroups.clear();
@@ -91,6 +132,12 @@ public class ColumnReorderChange extends ColumnChange {
             project.columnModel.columnGroups.clear();
             project.columnModel.columnGroups.addAll(_oldColumnGroups);
 
+
+            for (int i = 0; i < _oldCells.length; i++) {
+                Row row = project.rows.get(_oldCells[i].row);
+                row.setCell(_oldCells[i].cellIndex,_oldCells[i].cell);
+            }
+
             project.update();
         }
     }
@@ -112,6 +159,17 @@ public class ColumnReorderChange extends ColumnChange {
             c.save(writer);
             writer.write('\n');
         }
+        writer.write("removedColumnCount="); writer.write(Integer.toString(_removedColumns.size())); writer.write('\n');
+        for (Column c : _removedColumns) {
+            c.save(writer);
+            writer.write('\n');
+        }
+        writer.write("oldCellCount="); writer.write(Integer.toString(_oldCells.length)); writer.write('\n');
+        for (CellAtRowCellIndex c : _oldCells) {
+            c.save(writer, options);
+            writer.write('\n');
+        }
+
         writeOldColumnGroups(writer, options, _oldColumnGroups);
         writer.write("/ec/\n"); // end of change marker
     }
@@ -120,6 +178,8 @@ public class ColumnReorderChange extends ColumnChange {
         List<String> columnNames = new ArrayList<String>();
         List<Column> oldColumns = new ArrayList<Column>();
         List<Column> newColumns = new ArrayList<Column>();
+        List<Column> removedColumns = new ArrayList<Column>();
+        CellAtRowCellIndex[] oldCells = new CellAtRowCellIndex[0];
         List<ColumnGroup> oldColumnGroups = null;
         
         String line;
@@ -151,6 +211,24 @@ public class ColumnReorderChange extends ColumnChange {
                         newColumns.add(Column.load(line));
                     }
                 }
+            } else if ("removedColumnCount".equals(field)) {
+                int count = Integer.parseInt(line.substring(equal + 1));
+                for (int i = 0; i < count; i++) {
+                    line = reader.readLine();
+                    if (line != null) {
+                    	removedColumns.add(Column.load(line));
+                    }
+                }
+            } else if ("oldCellCount".equals(field)) {
+                int oldCellCount = Integer.parseInt(line.substring(equal + 1));
+
+                oldCells = new CellAtRowCellIndex[oldCellCount];
+                for (int i = 0; i < oldCellCount; i++) {
+                    line = reader.readLine();
+                    if (line != null) {
+                        oldCells[i] = CellAtRowCellIndex.load(line, pool);
+                    }
+                }
             } else if ("oldColumnGroupCount".equals(field)) {
                 int oldColumnGroupCount = Integer.parseInt(line.substring(equal + 1));
                 
@@ -161,6 +239,8 @@ public class ColumnReorderChange extends ColumnChange {
         ColumnReorderChange change = new ColumnReorderChange(columnNames);
         change._oldColumns = oldColumns;
         change._newColumns = newColumns;
+        change._removedColumns = removedColumns;
+        change._oldCells = oldCells;
         change._oldColumnGroups = oldColumnGroups != null ?
                 oldColumnGroups : new LinkedList<ColumnGroup>();
         

main/tests/server/src/com/google/refine/tests/operations/column/ColumnReorderOperationTests.java

@@ -27,24 +27,39 @@
 package com.google.refine.tests.operations.column;
 
 import java.util.Arrays;
+import java.util.Properties;
 
+import org.testng.Assert;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.BeforeSuite;
 import org.testng.annotations.Test;
 
 import com.google.refine.model.AbstractOperation;
+import com.google.refine.model.Project;
 import com.google.refine.operations.OperationRegistry;
 import com.google.refine.operations.column.ColumnReorderOperation;
+import com.google.refine.process.Process;
 import com.google.refine.tests.RefineTest;
 import com.google.refine.tests.util.TestUtils;
 
 
 public class ColumnReorderOperationTests extends RefineTest {
 
+    Project project;
+
     @BeforeSuite
     public void setUp() {
         OperationRegistry.registerOperation(getCoreModule(), "column-reorder", ColumnReorderOperation.class);
     }
 
+    @BeforeMethod
+    public void createProject() {
+        project = createCSVProject(
+                        "a,b,c\n"+
+                        "1|2,d,e\n"+
+                        "3,f,g\n");
+    }
+
     @Test
     public void serializeColumnReorderOperation() {
         AbstractOperation op = new ColumnReorderOperation(Arrays.asList("b","c","a"));
@@ -52,4 +67,32 @@ public class ColumnReorderOperationTests extends RefineTest {
                 + "\"description\":\"Reorder columns\","
                 + "\"columnNames\":[\"b\",\"c\",\"a\"]}");
     }
+
+    @Test
+    public void testEraseCellsOnRemovedColumns() throws Exception {
+
+        int aCol = project.columnModel.getColumnByName("a").getCellIndex();
+        int bCol = project.columnModel.getColumnByName("b").getCellIndex();
+        int cCol = project.columnModel.getColumnByName("c").getCellIndex();
+
+        Assert.assertEquals(project.rows.get(0).getCellValue(aCol), "1|2");
+        Assert.assertEquals(project.rows.get(0).getCellValue(bCol), "d");
+        Assert.assertEquals(project.rows.get(0).getCellValue(cCol), "e");
+        Assert.assertEquals(project.rows.get(1).getCellValue(aCol), "3");
+        Assert.assertEquals(project.rows.get(1).getCellValue(bCol), "f");
+        Assert.assertEquals(project.rows.get(1).getCellValue(cCol), "g");
+
+
+        AbstractOperation op = new ColumnReorderOperation(Arrays.asList("a"));
+        Process process = op.createProcess(project, new Properties());
+        process.performImmediate();
+
+        Assert.assertEquals(project.rows.get(0).getCellValue(aCol), "1|2");
+        Assert.assertEquals(project.rows.get(0).getCellValue(bCol), null);
+        Assert.assertEquals(project.rows.get(0).getCellValue(cCol), null);
+        Assert.assertEquals(project.rows.get(1).getCellValue(aCol), "3");
+        Assert.assertEquals(project.rows.get(1).getCellValue(bCol), null);
+        Assert.assertEquals(project.rows.get(1).getCellValue(cCol), null);
+
+    }
 }