From d3db73aa6737e3f03742bb05380b24f94e892a50 Mon Sep 17 00:00:00 2001
From: Tom Morris <tfmorris@gmail.com>
Date: Thu, 2 Jul 2020 15:42:55 -0400
Subject: [PATCH] Remove shortest-column-name ordering

Refs #2863
The tree importer sorts columns/column groups by how populated
they are, which is of arguable utility, but the tie-breaker
of ordering by shortest column name is completely silly.

This change removes that and, in conjunction with a stable sort
algorithm, will preserve the original order of the columns.
---
 .../refine/importers/tree/TreeImportUtilities.java   |  9 ++++-----
 .../google/refine/importers/XmlImporterTests.java    | 12 +++++++-----
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/main/src/com/google/refine/importers/tree/TreeImportUtilities.java b/main/src/com/google/refine/importers/tree/TreeImportUtilities.java
index c7e37fff8..0ed0beece 100644
--- a/main/src/com/google/refine/importers/tree/TreeImportUtilities.java
+++ b/main/src/com/google/refine/importers/tree/TreeImportUtilities.java
@@ -73,8 +73,7 @@ public abstract class TreeImportUtilities {
                     return o1.blankOnFirstRow ? 1 : -1;
                 }
 
-                int c = o2.nonBlankCount - o1.nonBlankCount;
-                return c != 0 ? c : (o1.name.length() - o2.name.length());
+                return o2.nonBlankCount - o1.nonBlankCount;
             }
         });
 
@@ -85,6 +84,7 @@ public abstract class TreeImportUtilities {
             project.columnModel.columns.add(column);
         }
 
+        // The LinkedHashMap iterator will guaranteed that the list is arranged in order found
         List<ImportColumnGroup> subgroups = new ArrayList<ImportColumnGroup>(columnGroup.subgroups.values());
         Collections.sort(subgroups, new Comparator<ImportColumnGroup>() {
             @Override
@@ -96,9 +96,8 @@ public abstract class TreeImportUtilities {
                 // TODO: From a human factors point of view, we probably want
                 // to try to preserve the order that we found things in the XML
                 
-                // Sort by most populated first, then shortest name
-                int c = o2.nonBlankCount - o1.nonBlankCount;
-                return c != 0 ? c : (o1.name.length() - o2.name.length());
+                // Sort by most populated first, but leave order unchanged if they're equal
+                return o2.nonBlankCount - o1.nonBlankCount;
             }
         });
 
diff --git a/main/tests/server/src/com/google/refine/importers/XmlImporterTests.java b/main/tests/server/src/com/google/refine/importers/XmlImporterTests.java
index 63bb20828..e18441f9d 100644
--- a/main/tests/server/src/com/google/refine/importers/XmlImporterTests.java
+++ b/main/tests/server/src/com/google/refine/importers/XmlImporterTests.java
@@ -51,6 +51,7 @@ import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.google.refine.importers.XmlImporter;
 import com.google.refine.importers.tree.TreeImportingParserBase;
 import com.google.refine.importing.ImportingJob;
+import com.google.refine.model.ColumnGroup;
 import com.google.refine.model.Row;
 import com.google.refine.util.JSONUtilities;
 import com.google.refine.util.ParsingUtilities;
@@ -197,11 +198,12 @@ public class XmlImporterTests extends ImporterTest {
         assertProjectCreated(project, 5, 6);
 
         Assert.assertEquals(project.columnModel.columnGroups.size(),1);
-        Assert.assertEquals(project.columnModel.columnGroups.get(0).keyColumnIndex, 2);
-        Assert.assertEquals(project.columnModel.columnGroups.get(0).startColumnIndex, 2);
-        Assert.assertNull(project.columnModel.columnGroups.get(0).parentGroup);
-        Assert.assertEquals(project.columnModel.columnGroups.get(0).subgroups.size(),0);
-        Assert.assertEquals(project.columnModel.columnGroups.get(0).columnSpan,2);
+        ColumnGroup cg0 = project.columnModel.columnGroups.get(0);
+        Assert.assertEquals(cg0.keyColumnIndex, 1);
+        Assert.assertEquals(cg0.startColumnIndex, 1);
+        Assert.assertNull(cg0.parentGroup);
+        Assert.assertEquals(cg0.subgroups.size(),0);
+        Assert.assertEquals(cg0.columnSpan,2);
     }
 
     //------------helper methods---------------