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

ui/views/controls/table/table_view.cc

Index: ui/views/controls/table/table_view.cc
diff --git a/ui/views/controls/table/table_view.cc b/ui/views/controls/table/table_view.cc
index 9ef4fe95a81979c279f87de6411fcd19cdd168a3..748fc4d21d21b7203c190cf3f9adf4866644f497 100644
--- a/ui/views/controls/table/table_view.cc
+++ b/ui/views/controls/table/table_view.cc
@@ -234,15 +234,20 @@ void TableView::SetColumnVisibility(int id, bool is_visible) {
 void TableView::ToggleSortOrder(int visible_column_index) {
   DCHECK(visible_column_index >= 0 &&
          visible_column_index < static_cast<int>(visible_columns_.size()));
-  if (!visible_columns_[visible_column_index].column.sortable)
+  const auto& column = visible_columns_[visible_column_index].column;

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

nit: auto->ui::TableColumn for improved readability.

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

Done.

+  if (!column.sortable)
     return;
-  const int column_id = visible_columns_[visible_column_index].column.id;
   SortDescriptors sort(sort_descriptors_);
-  if (!sort.empty() && sort[0].column_id == column_id) {
-    sort[0].ascending = !sort[0].ascending;
+  if (!sort.empty() && sort[0].column_id == column.id) {
+    if (sort[0].ascending == column.initial_sort_is_ascending) {
+      // First toggle inverts the order.
+      sort[0].ascending = !sort[0].ascending;
+    } else {
+      // Second toggle clears the sort.
+      sort.clear();

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

Shouldn't this only remove the first item?

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

I had puzzled over this too. I don't think we should -- that would change the
sort back to the secondary sort column, which behavior would I think surprise
the user, since there's no visual indication of the secondary sort column. So
the sort arrow going back there would feel like it came out of nowhere, right?

The goal of this CL is to give the user a meaningful way to clear the sort
column and get back to the model order. Even with this clear, it is still
possible to obtain any combination of primary/secondary sort key, by clicking
columns in the right order.

As an alternative to making column sort be three-phase, I had also proposed
making the first column of the TaskManager have the model order. Others objected
to this because of the perceived value of being able to sort by title when
trying to find a particular tab, and possible user surprise. The consensus on
the chrome-ui-review thread ("Site Isolation UI signoff") seemed to be in favor
of the three-phase approach. I am still flexible, if you think one of these
other approaches is better.

[sky, 2016/07/25 20:23:21]

Fair enough, go with what you have.

+    }
   } else {
-    SortDescriptor descriptor(column_id, visible_columns_[
-        visible_column_index].column.initial_sort_is_ascending);
+    SortDescriptor descriptor(column.id, column.initial_sort_is_ascending);
     sort.insert(sort.begin(), descriptor);
     // Only persist two sort descriptors.
     if (sort.size() > 2)
@@ -640,8 +645,8 @@ void TableView::NumRowsChanged() {
   SchedulePaint();
 }
 
-void TableView::SetSortDescriptors(const SortDescriptors& sort_descriptors) {
-  sort_descriptors_ = sort_descriptors;
+void TableView::SetSortDescriptors(SortDescriptors sort_descriptors) {
+  sort_descriptors_ = std::move(sort_descriptors);
   SortItemsAndUpdateMapping();
   if (header_)
     header_->SchedulePaint();