Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2146033003:
TableView: 3-phase ToggleSortOrder (sorted/reversed/unsorted) (Closed)
|
Created:
4 years, 5 months ago by ncarter (slow) Modified:
4 years, 5 months ago CC:
chromium-reviews, tfarina, Ben Goodger (Google) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTableView: Make ToggleSortOrder a three-phase operation that cycles through
(ascending -> descending -> no sort) if the column is initially ascending,
or (descending -> ascending -> no sort) if the column is initially
descending.
Expose a set_sort_descriptors() method so that TaskManagerTableModel can
apply its saved user preference directly. Previously that worked
by calling ToggleSortOrder the right number of times, which was weird,
since ToggleSortOrder is basically a click event handler.
Expand unittests to verify the implementation.
BUG=616897, 54909
Committed: https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a
Cr-Commit-Position: refs/heads/master@{#407607}
Patch Set 1 #Patch Set 2 : Expand unittests. #Patch Set 3 : Back out some unneeded renaming. #Patch Set 4 : <utility> for std::move #Patch Set 5 : Fix bug. #
Total comments: 19
Patch Set 6 : sky's fixes. #
Messages
Total messages: 29 (21 generated)
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which seemed like a tenuous behavior dependency. Rename TableViewDelegate to TaskManagerTableModel::Delegate, since that's the idiom actually used here. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which seemed like a tenuous behavior dependency. Rename TableViewDelegate to TaskManagerTableModel::Delegate, since that's the idiom actually used here. BUG=616897,54909 ==========
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which seemed like a tenuous behavior dependency. Rename TableViewDelegate to TaskManagerTableModel::Delegate, since that's the idiom actually used here. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which seemed like a tenuous behavior dependency in light of these changes. Rename TableViewDelegate to TaskManagerTableModel::Delegate, since that's the idiom actually used here. Expand unittests to verify the implementation. BUG=616897,54909 ==========
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which seemed like a tenuous behavior dependency in light of these changes. Rename TableViewDelegate to TaskManagerTableModel::Delegate, since that's the idiom actually used here. Expand unittests to verify the implementation. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which seemed like a tenuous behavior dependency in light of these changes. Expand unittests to verify the implementation. BUG=616897,54909 ==========
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which seemed like a tenuous behavior dependency in light of these changes. Expand unittests to verify the implementation. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which seems unnecessarily indirect. Expand unittests to verify the implementation. BUG=616897,54909 ==========
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which seems unnecessarily indirect. Expand unittests to verify the implementation. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird. Expand unittests to verify the implementation. BUG=616897,54909 ==========
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved-as-a-preference sort order directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird. Expand unittests to verify the implementation. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved user preference directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird. Expand unittests to verify the implementation. BUG=616897,54909 ==========
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved user preference directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird. Expand unittests to verify the implementation. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved user preference directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird, since ToggleSortOrder is basically a click handler. Expand unittests to verify the implementation. BUG=616897,54909 ==========
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved user preference directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird, since ToggleSortOrder is basically a click handler. Expand unittests to verify the implementation. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved user preference directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird, since ToggleSortOrder is basically a click event handler. Expand unittests to verify the implementation. BUG=616897,54909 ==========
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nick@chromium.org changed reviewers: + ben@chromium.org
Ben, here's a CL to make it possible to get back to the model ordering in the TaskManager.
nick@chromium.org changed reviewers: + afakhry@chromium.org, sky@chromium.org - ben@chromium.org
Scott: you seem like the best reviewer for a TableView change (when you get back). The goal here is to have some way to get back to the model ordering. Since the TM persists the sort descriptors, it's actually impossible currently.
https://codereview.chromium.org/2146033003/diff/80001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.h (right): https://codereview.chromium.org/2146033003/diff/80001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.h:48: virtual void SetSortDescriptor(TableSortDescriptor sort_descriptor) = 0; const TableSortDescriptor& ? https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_view.cc (right): https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view.cc:237: const auto& column = visible_columns_[visible_column_index].column; nit: auto->ui::TableColumn for improved readability. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view.cc:247: sort.clear(); Shouldn't this only remove the first item? https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_view.h (left): https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view.h:231: void SetSortDescriptors(const SortDescriptors& sort_descriptors); Move implementation to match new location. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_view.h (right): https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view.h:167: void SetSortDescriptors(SortDescriptors descriptors); nit: const SortDescriptors& . I realize what you have is likely just as efficient, but seeing the value type requires more thought than had you just used a const&. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_view_unittest.cc (right): https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view_unittest.cc:164: std::string GetRowsInViewOrderAsString(TableView* table) { add a description of the format of the returned string. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view_unittest.cc:167: std::string separator = ""; nit: no = "", just std::string separator; https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view_unittest.cc:175: separator = ", "; nit: reusing separator for the inner loop makes this code mildly confusing. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view_unittest.cc:358: ASSERT_EQ(0u, table_->sort_descriptors().size()); nit: use empty
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2146033003/diff/80001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.h (right): https://codereview.chromium.org/2146033003/diff/80001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.h:48: virtual void SetSortDescriptor(TableSortDescriptor sort_descriptor) = 0; On 2016/07/18 16:18:58, sky wrote: > const TableSortDescriptor& ? Done. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_view.cc (right): https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view.cc:237: const auto& column = visible_columns_[visible_column_index].column; On 2016/07/18 16:18:58, sky wrote: > nit: auto->ui::TableColumn for improved readability. Done. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view.cc:247: sort.clear(); On 2016/07/18 16:18:58, sky wrote: > Shouldn't this only remove the first item? 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. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_view.h (left): https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view.h:231: void SetSortDescriptors(const SortDescriptors& sort_descriptors); On 2016/07/18 16:18:58, sky wrote: > Move implementation to match new location. Done. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_view.h (right): https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view.h:167: void SetSortDescriptors(SortDescriptors descriptors); On 2016/07/18 16:18:58, sky wrote: > nit: const SortDescriptors& . I realize what you have is likely just as > efficient, but seeing the value type requires more thought than had you just > used a const&. Done. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_view_unittest.cc (right): https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view_unittest.cc:164: std::string GetRowsInViewOrderAsString(TableView* table) { On 2016/07/18 16:18:58, sky wrote: > add a description of the format of the returned string. Done. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view_unittest.cc:167: std::string separator = ""; On 2016/07/18 16:18:58, sky wrote: > nit: no = "", just std::string separator; Done (switched to index checking). https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view_unittest.cc:175: separator = ", "; On 2016/07/18 16:18:58, sky wrote: > nit: reusing separator for the inner loop makes this code mildly confusing. You're right, this was confusing. I switched to using index-based iteration, and special-casing iteration zero. https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view_unittest.cc:358: ASSERT_EQ(0u, table_->sort_descriptors().size()); On 2016/07/18 16:18:58, sky wrote: > nit: use empty Done.
LGTM https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_view.cc (right): https://codereview.chromium.org/2146033003/diff/80001/ui/views/controls/table... ui/views/controls/table/table_view.cc:247: sort.clear(); On 2016/07/25 20:04:01, ncarter wrote: > On 2016/07/18 16:18:58, sky wrote: > > Shouldn't this only remove the first item? > > 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. Fair enough, go with what you have.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved user preference directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird, since ToggleSortOrder is basically a click event handler. Expand unittests to verify the implementation. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved user preference directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird, since ToggleSortOrder is basically a click event handler. Expand unittests to verify the implementation. BUG=616897,54909 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved user preference directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird, since ToggleSortOrder is basically a click event handler. Expand unittests to verify the implementation. BUG=616897,54909 ========== to ========== TableView: Make ToggleSortOrder a three-phase operation that cycles through (ascending -> descending -> no sort) if the column is initially ascending, or (descending -> ascending -> no sort) if the column is initially descending. Expose a set_sort_descriptors() method so that TaskManagerTableModel can apply its saved user preference directly. Previously that worked by calling ToggleSortOrder the right number of times, which was weird, since ToggleSortOrder is basically a click event handler. Expand unittests to verify the implementation. BUG=616897,54909 Committed: https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a Cr-Commit-Position: refs/heads/master@{#407607} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cb592d4b10cd8677aeac0694c5befd489b262a7a Cr-Commit-Position: refs/heads/master@{#407607} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
