TableView: 3-phase ToggleSortOrder (sorted/reversed/unsorted)

ui/views/controls/table/table_view_unittest.cc

Index: ui/views/controls/table/table_view_unittest.cc
diff --git a/ui/views/controls/table/table_view_unittest.cc b/ui/views/controls/table/table_view_unittest.cc
index 9fb6bb0fc2dbc37511011e00aa633a732b2d8cd7..7f89c941343d4c256e92796ae6ba20bf75ec1fea 100644
--- a/ui/views/controls/table/table_view_unittest.cc
+++ b/ui/views/controls/table/table_view_unittest.cc
@@ -161,6 +161,25 @@ std::string GetModelToViewAsString(TableView* table) {
   return result;
 }
 
+std::string GetRowsInViewOrderAsString(TableView* table) {

[sky, 2016/07/18 16:18:58]

add a description of the format of the returned string.

[ncarter (slow), 2016/07/25 20:04:02]

Done.

+  std::string result;
+
+  std::string separator = "";

[sky, 2016/07/18 16:18:58]

nit: no = "", just std::string separator;

[ncarter (slow), 2016/07/25 20:04:02]

Done (switched to index checking).

+  for (int row = 0; row < table->RowCount(); ++row) {
+    result += separator;
+    separator = "[";
+    for (auto column : table->visible_columns()) {
+      result += separator;
+      result += base::UTF16ToUTF8(
+          table->model()->GetText(table->ViewToModel(row), column.column.id));
+      separator = ", ";

[sky, 2016/07/18 16:18:58]

nit: reusing separator for the inner loop makes this code mildly confusing.

[ncarter (slow), 2016/07/25 20:04:01]

You're right, this was confusing. I switched to using index-based iteration, and
special-casing iteration zero.

+    }
+    result += "]";
+    separator = ", ";
+  }
+  return result;
+}
+
 class TestTableView : public TableView {
  public:
   TestTableView(ui::TableModel* model,
@@ -335,6 +354,13 @@ TEST_F(TableViewTest, ResizeViaGesture) {
 
 // Assertions for table sorting.
 TEST_F(TableViewTest, Sort) {
+  // Initial ordering.
+  ASSERT_EQ(0u, table_->sort_descriptors().size());

[sky, 2016/07/18 16:18:58]

nit: use empty

[ncarter (slow), 2016/07/25 20:04:02]

Done.

+  EXPECT_EQ("0 1 2 3", GetViewToModelAsString(table_));
+  EXPECT_EQ("0 1 2 3", GetModelToViewAsString(table_));
+  EXPECT_EQ("[0, 1], [1, 1], [2, 2], [3, 0]",
+            GetRowsInViewOrderAsString(table_));
+
   // Toggle the sort order of the first column, shouldn't change anything.
   table_->ToggleSortOrder(0);
   ASSERT_EQ(1u, table_->sort_descriptors().size());
@@ -342,46 +368,119 @@ TEST_F(TableViewTest, Sort) {
   EXPECT_TRUE(table_->sort_descriptors()[0].ascending);
   EXPECT_EQ("0 1 2 3", GetViewToModelAsString(table_));
   EXPECT_EQ("0 1 2 3", GetModelToViewAsString(table_));
+  EXPECT_EQ("[0, 1], [1, 1], [2, 2], [3, 0]",
+            GetRowsInViewOrderAsString(table_));
 
-  // Invert the sort (first column descending).
+  // Toggle the sort (first column descending).
   table_->ToggleSortOrder(0);
   ASSERT_EQ(1u, table_->sort_descriptors().size());
   EXPECT_EQ(0, table_->sort_descriptors()[0].column_id);
   EXPECT_FALSE(table_->sort_descriptors()[0].ascending);
   EXPECT_EQ("3 2 1 0", GetViewToModelAsString(table_));
   EXPECT_EQ("3 2 1 0", GetModelToViewAsString(table_));
+  EXPECT_EQ("[3, 0], [2, 2], [1, 1], [0, 1]",
+            GetRowsInViewOrderAsString(table_));
 
-  // Change cell 0x3 to -1, meaning we have 0, 1, 2, -1 (in the first column).
+  // Change the [3, 0] cell to [-1, 0]. This should move it to the back of
+  // the current sort order.
   model_->ChangeRow(3, -1, 0);
   ASSERT_EQ(1u, table_->sort_descriptors().size());
   EXPECT_EQ(0, table_->sort_descriptors()[0].column_id);
   EXPECT_FALSE(table_->sort_descriptors()[0].ascending);
   EXPECT_EQ("2 1 0 3", GetViewToModelAsString(table_));
   EXPECT_EQ("2 1 0 3", GetModelToViewAsString(table_));
+  EXPECT_EQ("[2, 2], [1, 1], [0, 1], [-1, 0]",
+            GetRowsInViewOrderAsString(table_));
+
+  // Toggle the sort again, to clear the sort and restore the model ordering.
+  table_->ToggleSortOrder(0);
+  ASSERT_EQ(0u, table_->sort_descriptors().size());
+  EXPECT_EQ("0 1 2 3", GetViewToModelAsString(table_));
+  EXPECT_EQ("0 1 2 3", GetModelToViewAsString(table_));
+  EXPECT_EQ("[0, 1], [1, 1], [2, 2], [-1, 0]",
+            GetRowsInViewOrderAsString(table_));
 
-  // Invert sort again (first column ascending).
+  // Toggle the sort again (first column ascending).
   table_->ToggleSortOrder(0);
   ASSERT_EQ(1u, table_->sort_descriptors().size());
   EXPECT_EQ(0, table_->sort_descriptors()[0].column_id);
   EXPECT_TRUE(table_->sort_descriptors()[0].ascending);
   EXPECT_EQ("3 0 1 2", GetViewToModelAsString(table_));
   EXPECT_EQ("1 2 3 0", GetModelToViewAsString(table_));
+  EXPECT_EQ("[-1, 0], [0, 1], [1, 1], [2, 2]",
+            GetRowsInViewOrderAsString(table_));
 
-  // Add a row so that model has 0, 3, 1, 2, -1.
+  // Add a row that's second in the model order, but last in the active sort
+  // order.
   model_->AddRow(1, 3, 4);
   ASSERT_EQ(1u, table_->sort_descriptors().size());
   EXPECT_EQ(0, table_->sort_descriptors()[0].column_id);
   EXPECT_TRUE(table_->sort_descriptors()[0].ascending);
   EXPECT_EQ("4 0 2 3 1", GetViewToModelAsString(table_));
   EXPECT_EQ("1 4 2 3 0", GetModelToViewAsString(table_));
+  EXPECT_EQ("[-1, 0], [0, 1], [1, 1], [2, 2], [3, 4]",
+            GetRowsInViewOrderAsString(table_));
 
-  // Delete the first row, ending up with 3, 1, 2, -1.
-  model_->RemoveRow(0);
+  // Add a row that's last in the model order but second in the the active sort
+  // order.
+  model_->AddRow(5, -1, 20);
   ASSERT_EQ(1u, table_->sort_descriptors().size());
   EXPECT_EQ(0, table_->sort_descriptors()[0].column_id);
   EXPECT_TRUE(table_->sort_descriptors()[0].ascending);
-  EXPECT_EQ("3 1 2 0", GetViewToModelAsString(table_));
-  EXPECT_EQ("3 1 2 0", GetModelToViewAsString(table_));
+  EXPECT_EQ("4 5 0 2 3 1", GetViewToModelAsString(table_));
+  EXPECT_EQ("2 5 3 4 0 1", GetModelToViewAsString(table_));
+  EXPECT_EQ("[-1, 0], [-1, 20], [0, 1], [1, 1], [2, 2], [3, 4]",
+            GetRowsInViewOrderAsString(table_));
+
+  // Click the first column again, then click the second column. This should
+  // yield an ordering of second column ascending, with the first column
+  // descending as a tiebreaker.
+  table_->ToggleSortOrder(0);
+  table_->ToggleSortOrder(1);
+  ASSERT_EQ(2u, table_->sort_descriptors().size());
+  EXPECT_EQ(1, table_->sort_descriptors()[0].column_id);
+  EXPECT_TRUE(table_->sort_descriptors()[0].ascending);
+  EXPECT_EQ(0, table_->sort_descriptors()[1].column_id);
+  EXPECT_FALSE(table_->sort_descriptors()[1].ascending);
+  EXPECT_EQ("4 2 0 3 1 5", GetViewToModelAsString(table_));
+  EXPECT_EQ("2 4 1 3 0 5", GetModelToViewAsString(table_));
+  EXPECT_EQ("[-1, 0], [1, 1], [0, 1], [2, 2], [3, 4], [-1, 20]",
+            GetRowsInViewOrderAsString(table_));
+
+  // Toggle the current column to change from ascending to descending. This
+  // should result in an almost-reversal of the previous order, except for the
+  // two rows with the same value for the second column.
+  table_->ToggleSortOrder(1);
+  ASSERT_EQ(2u, table_->sort_descriptors().size());
+  EXPECT_EQ(1, table_->sort_descriptors()[0].column_id);
+  EXPECT_FALSE(table_->sort_descriptors()[0].ascending);
+  EXPECT_EQ(0, table_->sort_descriptors()[1].column_id);
+  EXPECT_FALSE(table_->sort_descriptors()[1].ascending);
+  EXPECT_EQ("5 1 3 2 0 4", GetViewToModelAsString(table_));
+  EXPECT_EQ("4 1 3 2 5 0", GetModelToViewAsString(table_));
+  EXPECT_EQ("[-1, 20], [3, 4], [2, 2], [1, 1], [0, 1], [-1, 0]",
+            GetRowsInViewOrderAsString(table_));
+
+  // Delete the [0, 1] row from the model. It's at model index zero.
+  model_->RemoveRow(0);
+  ASSERT_EQ(2u, table_->sort_descriptors().size());
+  EXPECT_EQ(1, table_->sort_descriptors()[0].column_id);
+  EXPECT_FALSE(table_->sort_descriptors()[0].ascending);
+  EXPECT_EQ(0, table_->sort_descriptors()[1].column_id);
+  EXPECT_FALSE(table_->sort_descriptors()[1].ascending);
+  EXPECT_EQ("4 0 2 1 3", GetViewToModelAsString(table_));
+  EXPECT_EQ("1 3 2 4 0", GetModelToViewAsString(table_));
+  EXPECT_EQ("[-1, 20], [3, 4], [2, 2], [1, 1], [-1, 0]",
+            GetRowsInViewOrderAsString(table_));
+
+  // Toggle the current sort column again. This should clear both the primary
+  // and secondary sort descriptor.
+  table_->ToggleSortOrder(1);
+  ASSERT_EQ(0u, table_->sort_descriptors().size());
+  EXPECT_EQ("0 1 2 3 4", GetViewToModelAsString(table_));
+  EXPECT_EQ("0 1 2 3 4", GetModelToViewAsString(table_));
+  EXPECT_EQ("[3, 4], [1, 1], [2, 2], [-1, 0], [-1, 20]",
+            GetRowsInViewOrderAsString(table_));
 }
 
 // Verfies clicking on the header sorts.
@@ -501,7 +600,13 @@ TEST_F(TableViewTest, Grouping) {
   EXPECT_EQ("0 1 2 3", GetViewToModelAsString(table_));
   EXPECT_EQ("0 1 2 3", GetModelToViewAsString(table_));
 
-  // Toggle to ascending sort.
+  // Toggle to clear the sort.
+  table_->ToggleSortOrder(0);
+  ASSERT_EQ(0u, table_->sort_descriptors().size());
+  EXPECT_EQ("0 1 2 3", GetViewToModelAsString(table_));
+  EXPECT_EQ("0 1 2 3", GetModelToViewAsString(table_));
+
+  // Toggle again to effect an ascending sort.
   table_->ToggleSortOrder(0);
   ASSERT_EQ(1u, table_->sort_descriptors().size());
   EXPECT_EQ(0, table_->sort_descriptors()[0].column_id);