Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2453133002:
[ash-md] Makes Wi-Fi header row sticky when network list is scrolled (Closed)
Description[ash-md] Makes Wi-Fi header row sticky when network list is scrolled
Adds support for sticky header rows to tray icon scroll views. When
marked as sticky the headers will not scroll above the top of the
visible viewport unless there is another sticky header row just below.
Multiple rows can be indicated as header rows by setting View::id_.
BUG=631831
Committed: https://crrev.com/b55cb3ec546bf8e0749d08f512d7ab5f3f5b7c4e
Cr-Commit-Position: refs/heads/master@{#430214}
Patch Set 1 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (cleanup) #Patch Set 2 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (rebased) #
Total comments: 3
Patch Set 3 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (fixed targeting) #
Total comments: 26
Patch Set 4 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (avoid set_id()) #
Total comments: 10
Patch Set 5 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (based on PS2 + event targetin… #Patch Set 6 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (comments) #
Total comments: 34
Patch Set 7 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (comments) #Patch Set 8 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (cleanup) #Patch Set 9 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (comments) #
Total comments: 8
Patch Set 10 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (nits) #
Total comments: 15
Patch Set 11 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (nits) #Patch Set 12 : [ash-md] Makes Wi-Fi header row sticky when network list is scrolled (back to range) #
Messages
Total messages: 69 (36 generated)
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@, can you please take a first look? There is still some scaffolding in place so you can try this CL (it adds more WiFi rows and marks some of them as headers. This will go away before landing this - I just wanted to have a case to showcase multiple headers which we will have later. Thanks!
https://codereview.chromium.org/2453133002/diff/1/ash/common/system/tray/hove... File ash/common/system/tray/hover_highlight_view.cc (right): https://codereview.chromium.org/2453133002/diff/1/ash/common/system/tray/hove... ash/common/system/tray/hover_highlight_view.cc:282: ActionableView::OnPaintBackground(canvas); Note: scaffolding - the change will be removed in final patch. https://codereview.chromium.org/2453133002/diff/1/chromeos/dbus/fake_shill_ma... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2453133002/diff/1/chromeos/dbus/fake_shill_ma... chromeos/dbus/fake_shill_manager_client.cc:41: int s_extra_wifi_networks = 20; Note: scaffolding - the change will be removed in final patch. https://codereview.chromium.org/2453133002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2453133002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_list_md.cc:10: #include "base/strings/utf_string_conversions.h" Note: scaffolding - the change will be removed in final patch. https://codereview.chromium.org/2453133002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_list_md.cc:443: } Note: This block is scaffolding - the change will be removed in final patch.
The CQ bit was checked by varkha@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/2453133002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2453133002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.cc:445: } Note: scaffolding - the change will be removed in final patch. This simply makes the fake header rows opaque. https://codereview.chromium.org/2453133002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/hover_highlight_view.cc (right): https://codereview.chromium.org/2453133002/diff/40001/ash/common/system/tray/... ash/common/system/tray/hover_highlight_view.cc:282: ActionableView::OnPaintBackground(canvas); Note: scaffolding - the change will be removed in final patch. This simply makes the fake header rows opaque. https://codereview.chromium.org/2453133002/diff/40001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2453133002/diff/40001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_manager_client.cc:41: int s_extra_wifi_networks = 20; Note: scaffolding - the change will be removed in final patch. This adds more Wi-Fi rows to have a scrollbar.
The CQ bit was checked by varkha@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...
bruthig@ noticed a flaw with this approach. Event targeting follows the child ordering rather than paint ordering when there's occlusion so this needs to be addressed first.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/26 22:56:46, varkha wrote: > bruthig@ noticed a flaw with this approach. Event targeting follows the child > ordering rather than paint ordering when there's occlusion so this needs to be > addressed first. OK, please re-ping when you've changed that and this CL is ready for me to look at.
varkha@chromium.org changed reviewers: + estade@chromium.org
estade@, Can you please take a look here? The basic idea that you helped me with works visually but as bruthig@ noticed it has a hole with events - the views are targeted according to their reverse order in View::children_ and so changing visual z-order in paint does not affect the event targeting. I've tried to play with custom targeting but it gets a bit complicated. I am now trying to make the targeting work by keeping a placeholder for the header rows in the original ScrollContents but having another child of the scroller with the headers that would be above ScrollContents and so will be targeted first. The headers then would be layed out in the same place in both containers when they are visible or made to stick to the top. Do you have a better idea while I am working this through?
The CQ bit was checked by varkha@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 varkha@chromium.org
The CQ bit was checked by varkha@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...
varkha@chromium.org changed reviewers: + sadrul@chromium.org
tdanderson@, back to you for the first review now with the fixed targeting. sadrul@, wanted to get your opinions here: - Using SetGroup() and set_id() to maintain the headers <-> rows relationships. - Extending BoxLayout vs. simply adding ability to use header rows to that class maintaining backward compatibility (this should not be changing anything unless you SetGroup() and set_id() on children). - Possibly splitting refactoring in BoxLayout into a separate CL.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
It doesn't seem like changing the default view targeting should be that difficult? You just need to create a view targeter that inherits from the default ViewTargeter and checks coordinate intersection with the headers first then calls up to normal view targeting. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:93: if (child->id() == kHeaderRowId) { I think we discussed an alternative where you would add the headers via a method, e.g. void HeaderListLayout::AddViewAsHeader(views::View*) that would remove the need for this step (also, the way you're currently using IDs is technically in contradiction of the documentation, "ID should be unique within the subtree", although it probably doesn't actually matter much). https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:137: class Header { somewhat confusing that you have to classes both named Header https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:212: // top of the visible viewport. do you have a use case for multiple sticky headers yet?
Generally lg, left some comments below. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:35: const int kHeaderRowId = 1000; Would it be safer to use -1 for |kHeaderRowId| ? https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:91: for (int i = 0, count = host->child_count(); i < count; ++i) { I would find LayoutChildren() to be much more readable if you included documentation above each loop (i.e., lines 91 and 105) as you have already done for lines 100 and 123. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:122: // In the end layout unclaimed headers. nit on wording: just "Layout unclaimed headers" or "Layout remaining headers". https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:162: // views::View. nit: views::View: https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:166: for (int i = 0, count = child_count(); i < count; ++i) { just (int i = 0; i < child_count(); ++i) ? here and elsewhere in this class https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:167: views::View* header = child_at(i); nit: consider naming as |view| instead since you haven't yet determined if it's a header. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:183: headers_.erase(header); Shouldn't you call ScrollChildren() after erasing |header|? https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:213: void ScrollChildren() { Consider a more descriptive name for this, such as PositionHeaderRows()? https://codereview.chromium.org/2453133002/diff/100001/ui/views/layout/box_la... File ui/views/layout/box_layout.h (right): https://codereview.chromium.org/2453133002/diff/100001/ui/views/layout/box_la... ui/views/layout/box_layout.h:101: void LayoutChild(View* child, nit: make private? https://codereview.chromium.org/2453133002/diff/100001/ui/views/layout/box_la... ui/views/layout/box_layout.h:109: // Positions all children. Can be overridden to change the visible order. nit: It would be nice to have some documentation here for the meaning of each parameter
sadrul@, ping for comments on concept. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:93: if (child->id() == kHeaderRowId) { On 2016/10/28 17:15:40, Evan Stade wrote: > I think we discussed an alternative where you would add the headers via a > method, e.g. > > void HeaderListLayout::AddViewAsHeader(views::View*) > > that would remove the need for this step (also, the way you're currently using > IDs is technically in contradiction of the documentation, "ID should be unique > within the subtree", although it probably doesn't actually matter much). I've tried to add setting header rows explicitly. A few rough edges there but it is possible. Before I go with cleaning this up, can you take a look and see if that is what you had in mind? It is a bit more coupling as the child row views will need to know about the scroll contents view and its type but avoids using id().
I created this change based on your patch set 2: https://codereview.chromium.org/2471493002/ It overrides default event targetting (seemingly successfully), and uses about 10 lines to do so. This seems like a simpler overall approach, unless there's something I'm missing.
Is there a vid of the list when the user scrolls past the current 'group' and moves into a new 'group'? The reason I ask is that it may be simpler to have a separate placeholder HeaderView that paints the content of the header for the current group, and routes events back to that header view. This way, you don't have to juggle with the child ordering of the views. https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... File ash/common/system/tray/header_list_scroll_view.cc (right): https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.cc:28: bool operator==(int group_id) { return view->GetGroup() == group_id; } These operator overloads don't actually seem useful. Just explicitly check instead? https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.cc:37: void SetHeaderView(views::View* child) { AddHeaderView https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.cc:54: host->ReorderChildView(header.view, -1); Do you need to do this for all layout? Can this be in LayoutManager::ViewAdded override instead? https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... File ash/common/system/tray/header_list_scroll_view.h (right): https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.h:18: // To indicate that a child is a sticky header row use set_id(kHeaderRowId). Where is kHeaderRowId() defined? https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.h:25: void SetHeaderView(views::View* child); This seems odd with the class level description. There can be more than one sticky headers, right? What does this do? Do you mean 'AddHeaderView()'? If the user should explicitly add the header views, I guess that means the special view-id is not necessary?
Chatted with sadrul@ offline. We have agreed that changing targeting is a better approach in part because it does not change the order of controls in children and doesn't suffer from a wrong tab order that my previous attempt suffered (sadrul@ have noticed that). I'll merge it in this CL and will work on cleaning it up with some of the previous comments.
The CQ bit was checked by varkha@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...
PTAL. Thanks for all the suggestions (and the targeter delegate impl.). I think I was missing the offset when I tried (and abandoned) that approach. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:35: const int kHeaderRowId = 1000; On 2016/10/28 19:42:41, tdanderson wrote: > Would it be safer to use -1 for |kHeaderRowId| ? Done. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:91: for (int i = 0, count = host->child_count(); i < count; ++i) { On 2016/10/28 19:42:41, tdanderson wrote: > I would find LayoutChildren() to be much more readable if you included > documentation above each loop (i.e., lines 91 and 105) as you have already done > for lines 100 and 123. Acknowledged. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:122: // In the end layout unclaimed headers. On 2016/10/28 19:42:41, tdanderson wrote: > nit on wording: just "Layout unclaimed headers" or "Layout remaining headers". Acknowledged. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:137: class Header { On 2016/10/28 17:15:40, Evan Stade wrote: > somewhat confusing that you have to classes both named Header Acknowledged. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:162: // views::View. On 2016/10/28 19:42:41, tdanderson wrote: > nit: views::View: Done. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:166: for (int i = 0, count = child_count(); i < count; ++i) { On 2016/10/28 19:42:41, tdanderson wrote: > just (int i = 0; i < child_count(); ++i) ? here and elsewhere in this class Done. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:167: views::View* header = child_at(i); On 2016/10/28 19:42:41, tdanderson wrote: > nit: consider naming as |view| instead since you haven't yet determined if it's > a header. Done. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:183: headers_.erase(header); On 2016/10/28 19:42:41, tdanderson wrote: > Shouldn't you call ScrollChildren() after erasing |header|? I thought you would get a Layout() call when this happens and you still have any children left. When there are no children left we should be fine as well. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:212: // top of the visible viewport. On 2016/10/28 17:15:40, Evan Stade wrote: > do you have a use case for multiple sticky headers yet? Yes, Cellular networks will have same UI with a sticky header row. https://codereview.chromium.org/2453133002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:213: void ScrollChildren() { On 2016/10/28 19:42:41, tdanderson wrote: > Consider a more descriptive name for this, such as PositionHeaderRows()? Done. https://codereview.chromium.org/2453133002/diff/100001/ui/views/layout/box_la... File ui/views/layout/box_layout.h (right): https://codereview.chromium.org/2453133002/diff/100001/ui/views/layout/box_la... ui/views/layout/box_layout.h:101: void LayoutChild(View* child, On 2016/10/28 19:42:41, tdanderson wrote: > nit: make private? Acknowledged. https://codereview.chromium.org/2453133002/diff/100001/ui/views/layout/box_la... ui/views/layout/box_layout.h:109: // Positions all children. Can be overridden to change the visible order. On 2016/10/28 19:42:41, tdanderson wrote: > nit: It would be nice to have some documentation here for the meaning of each > parameter Acknowledged. https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... File ash/common/system/tray/header_list_scroll_view.cc (right): https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.cc:28: bool operator==(int group_id) { return view->GetGroup() == group_id; } On 2016/11/01 18:50:16, sadrul wrote: > These operator overloads don't actually seem useful. Just explicitly check > instead? Done. Used lambda instead as you have suggested. https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.cc:37: void SetHeaderView(views::View* child) { On 2016/11/01 18:50:16, sadrul wrote: > AddHeaderView Acknowledged. https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.cc:54: host->ReorderChildView(header.view, -1); On 2016/11/01 18:50:16, sadrul wrote: > Do you need to do this for all layout? Can this be in LayoutManager::ViewAdded > override instead? Acknowledged. https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... File ash/common/system/tray/header_list_scroll_view.h (right): https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.h:18: // To indicate that a child is a sticky header row use set_id(kHeaderRowId). On 2016/11/01 18:50:16, sadrul wrote: > Where is kHeaderRowId() defined? Acknowledged. https://codereview.chromium.org/2453133002/diff/120001/ash/common/system/tray... ash/common/system/tray/header_list_scroll_view.h:25: void SetHeaderView(views::View* child); On 2016/11/01 18:50:16, sadrul wrote: > This seems odd with the class level description. There can be more than one > sticky headers, right? What does this do? Do you mean 'AddHeaderView()'? If the > user should explicitly add the header views, I guess that means the special > view-id is not necessary? Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/chro... ash/common/system/chromeos/network/network_list_md.cc:439: if (info->label == base::UTF8ToUTF16("wifi15") || I assume this is for debugging https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... File ash/common/system/tray/hover_highlight_view.cc (right): https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/hover_highlight_view.cc:283: // canvas->DrawColor(hover_ ? highlight_color_ : default_color_); ? https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:25: const int kHeaderRowId = -1; Should this be exposed in tray_details_view.h? https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:30: // the children as sticky header rows. The sticky header rows are not scrolled nit: until the next one "pushes" it up https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:36: ScrollContentsView(ash::TrayDetailsView* tray_details_view) { doesn't look like you need the param https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:65: headers_.push_back(Header(view)); seems like you could/should keep this list up to date in ViewHierarchyChanged? https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:78: headers_.erase(header_it); isn't there a std::remove_if? https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:95: class Header { struct? not sure what this is buying us either way at this point. Can we just call View::y() as needed? https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:97: Header(views::View* header) : view(header), offset(header->bounds().y()) {} you can just do header->y() https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:104: static void ShowHeaderSticky(views::View* header, bool show_sticky) { nit: s/ShowHeaderSticky/DecorateAsSticky/ or SetStickyAppearance or SetLooksSticky or something. "Show" sounds a lot like you're making something visible (like Widget::Show). https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:209: ash::kBorderLightColor); aren't we already in ash https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:317: scroll_content_->SetLayoutManager( perhaps this belongs in ScrollContentsView's ctor now?
Patchset #7 (id:180001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/chro... File ash/common/system/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/chro... ash/common/system/chromeos/network/network_list_md.cc:439: if (info->label == base::UTF8ToUTF16("wifi15") || On 2016/11/02 13:41:27, Evan Stade wrote: > I assume this is for debugging Yes, to be removed just before landing. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... File ash/common/system/tray/hover_highlight_view.cc (right): https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/hover_highlight_view.cc:283: // canvas->DrawColor(hover_ ? highlight_color_ : default_color_); On 2016/11/02 13:41:27, Evan Stade wrote: > ? Yes, to be removed just before landing. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:25: const int kHeaderRowId = -1; On 2016/11/02 13:41:27, Evan Stade wrote: > Should this be exposed in tray_details_view.h? I've moved it to tray_constants.h instead. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:30: // the children as sticky header rows. The sticky header rows are not scrolled On 2016/11/02 13:41:27, Evan Stade wrote: > nit: until the next one "pushes" it up Done. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:36: ScrollContentsView(ash::TrayDetailsView* tray_details_view) { On 2016/11/02 13:41:27, Evan Stade wrote: > doesn't look like you need the param Yes, I've removed it but merging brought it back. Thanks! https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:65: headers_.push_back(Header(view)); On 2016/11/02 13:41:27, Evan Stade wrote: > seems like you could/should keep this list up to date in ViewHierarchyChanged? <Header> list is not just keeping the list of headers but also maintains between the calls to Layout the "original" offset so that a child row can be painted sticky or get re-positioned back to its original location. So I could maintain the membership in the list in hierarchy changes and only update the offset in Layout() but I am not convinced that this would be simpler or more efficient given that I would need to use another container or repeatedly call find before adding to the vector. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:78: headers_.erase(header_it); On 2016/11/02 13:41:27, Evan Stade wrote: > isn't there a std::remove_if? Yes, effectively generalizing this for the case when the elements are not unique in container. Done. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:95: class Header { On 2016/11/02 13:41:27, Evan Stade wrote: > struct? not sure what this is buying us either way at this point. Can we just > call View::y() as needed? I need this to have an offset saved in Layout and still available when OnBoundsChanged is called some time later. This is what Header struct is doing. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:97: Header(views::View* header) : view(header), offset(header->bounds().y()) {} On 2016/11/02 13:41:27, Evan Stade wrote: > you can just do header->y() Done. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:104: static void ShowHeaderSticky(views::View* header, bool show_sticky) { On 2016/11/02 13:41:27, Evan Stade wrote: > nit: s/ShowHeaderSticky/DecorateAsSticky/ > > or SetStickyAppearance > or SetLooksSticky > > or something. "Show" sounds a lot like you're making something visible (like > Widget::Show). I like your first suggestion the most. Done. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:209: ash::kBorderLightColor); On 2016/11/02 13:41:27, Evan Stade wrote: > aren't we already in ash Done. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:317: scroll_content_->SetLayoutManager( On 2016/11/02 13:41:27, Evan Stade wrote: > perhaps this belongs in ScrollContentsView's ctor now? Done.
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/2453133002/diff/160001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:44: PositionHeaderRows(); why not InvalidateLayout()? https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:95: class Header { On 2016/11/02 22:42:34, varkha wrote: > On 2016/11/02 13:41:27, Evan Stade wrote: > > struct? not sure what this is buying us either way at this point. Can we just > > call View::y() as needed? > > I need this to have an offset saved in Layout and still available when > OnBoundsChanged is called some time later. This is what Header struct is doing. this is not at all obvious from the code. This warrants better docs and better naming (of both the class and the offset variable). https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:119: for (auto& header : headers_) { I feel like this loop could be simpler if you iterated backwards, so instead of making a bunch of headers sticky then undoing, you only reposition at most one header. This also (I believe) obviates the need for the Header struct. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:128: header_bounds = previous_header->view->bounds(); I don't see the purpose in reusing header_bounds here. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:130: previous_header->view->SetBoundsRect(header_bounds); this stanza looks equivalent to previous_header->view->SetY(previous_header->offset)
https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:44: PositionHeaderRows(); On 2016/11/02 23:12:25, Evan Stade wrote: > why not InvalidateLayout()? I thought this would be more expensive. Now I am only effectively repositioning one view and keeping the rest of the layout. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:95: class Header { On 2016/11/02 23:12:25, Evan Stade wrote: > On 2016/11/02 22:42:34, varkha wrote: > > On 2016/11/02 13:41:27, Evan Stade wrote: > > > struct? not sure what this is buying us either way at this point. Can we > just > > > call View::y() as needed? > > > > I need this to have an offset saved in Layout and still available when > > OnBoundsChanged is called some time later. This is what Header struct is > doing. > > this is not at all obvious from the code. This warrants better docs and better > naming (of both the class and the offset variable). > Done. I've also moved a bit more into this so this I think is now a legitimate class. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:119: for (auto& header : headers_) { On 2016/11/02 23:12:25, Evan Stade wrote: > I feel like this loop could be simpler if you iterated backwards, so instead of > making a bunch of headers sticky then undoing, you only reposition at most one > header. This also (I believe) obviates the need for the Header struct. Done (although I am still not sure how you would eliminate the need to keep track of the original offset (unless you force Layout() to happen on every scroll increment which I tried to avoid). https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:128: header_bounds = previous_header->view->bounds(); On 2016/11/02 23:12:25, Evan Stade wrote: > I don't see the purpose in reusing header_bounds here. Done. https://codereview.chromium.org/2453133002/diff/160001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:130: previous_header->view->SetBoundsRect(header_bounds); On 2016/11/02 23:12:25, Evan Stade wrote: > this stanza looks equivalent to > > previous_header->view->SetY(previous_header->offset) Done. Here and elsewhere.
I think you forgot to upload a new patch?
On 2016/11/03 18:26:44, Evan Stade wrote: > I think you forgot to upload a new patch? Closed the lid too soon yesterday. Uploaded now.
tray_details_view.cc lgtm https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:97: // calls to Layout() to allow keeping track ofwhich view should be sticky. nit: of which https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:119: const views::View* view() const { return view_; } what do you need this version for? https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:143: views::View* header_view = header->view(); nit: declare in the tightest scope it's needed https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:161: } else { no else after break (I would actually invert the check and continue)
Thanks Evan! https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:97: // calls to Layout() to allow keeping track ofwhich view should be sticky. On 2016/11/03 19:24:31, Evan Stade wrote: > nit: of which Done. https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:119: const views::View* view() const { return view_; } On 2016/11/03 19:24:31, Evan Stade wrote: > what do you need this version for? Line 77 uses it (to keep the iterator const). Line 56 I think would be using it as well. https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:143: views::View* header_view = header->view(); On 2016/11/03 19:24:31, Evan Stade wrote: > nit: declare in the tightest scope it's needed Done. https://codereview.chromium.org/2453133002/diff/240001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:161: } else { On 2016/11/03 19:24:32, Evan Stade wrote: > no else after break (I would actually invert the check and continue) Done.
The CQ bit was checked by varkha@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...
ping tdanderson@ and sadrul@ for OWNERS in ash/common/system.
On 2016/11/03 21:31:50, varkha wrote: > ping tdanderson@ and sadrul@ for OWNERS in ash/common/system. Truthfully I haven't really been following this CL, so I will defer to sadrul@ for his owners review (just chatted with him and he says he will take a look).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some nits. lgtm https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:79: headers_.end()); There should only be one header that matches the child view being removed, right? So you shouldn't need the second param to erase() https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:83: View* TargetForRect(View* root, const gfx::Rect& rect) override { // views::ViewTargeterDelegate: https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:100: Header(views::View* header) explicit https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:134: bool sticky_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:142: for (auto header = headers_.rbegin(); header != headers_.rend(); ++header) { for (auto& header : base::Reversed(headers_)) { ... }
https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:121: return const_cast<views::View*>(const_cast<const Header*>(this)->view()); p.s. this seems crazily verbose instead of just 'return view_;'
https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:79: headers_.end()); On 2016/11/04 15:03:03, sadrul wrote: > There should only be one header that matches the child view being removed, > right? So you shouldn't need the second param to erase() Done. https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:83: View* TargetForRect(View* root, const gfx::Rect& rect) override { On 2016/11/04 15:03:03, sadrul wrote: > // views::ViewTargeterDelegate: Done. https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:100: Header(views::View* header) On 2016/11/04 15:03:03, sadrul wrote: > explicit Done. https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:121: return const_cast<views::View*>(const_cast<const Header*>(this)->view()); On 2016/11/04 15:53:46, Evan Stade wrote: > p.s. this seems crazily verbose instead of just 'return view_;' Done. https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:134: bool sticky_; On 2016/11/04 15:03:03, sadrul wrote: > DISALLOW_COPY_AND_ASSIGN This structure is used in a value array so I think being copyable is OK here. https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:142: for (auto header = headers_.rbegin(); header != headers_.rend(); ++header) { On 2016/11/04 15:03:03, sadrul wrote: > for (auto& header : base::Reversed(headers_)) { > ... > } Done.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2453133002/#ps280001 (title: "[ash-md] Makes Wi-Fi header row sticky when network list is scrolled (nits)")
The CQ bit was unchecked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:134: bool sticky_; On 2016/11/04 18:18:49, varkha wrote: > On 2016/11/04 15:03:03, sadrul wrote: > > DISALLOW_COPY_AND_ASSIGN > > This structure is used in a value array so I think being copyable is OK here. So use emplace_back instead of push_back?
https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... File ash/common/system/tray/tray_details_view.cc (right): https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:79: headers_.end()); On 2016/11/04 15:03:03, sadrul wrote: > There should only be one header that matches the child view being removed, > right? So you shouldn't need the second param to erase() Looked into this some more. While you are right that there is never more than one element matching the predicate there might be none matching in which case you need an empty range (there is no such thing as an empty iterator so the iterator will be pointing to the end() and erase(end()) will simply crash). So restoring the previous syntax. https://codereview.chromium.org/2453133002/diff/260001/ash/common/system/tray... ash/common/system/tray/tray_details_view.cc:134: bool sticky_; On 2016/11/04 19:18:45, Evan Stade wrote: > On 2016/11/04 18:18:49, varkha wrote: > > On 2016/11/04 15:03:03, sadrul wrote: > > > DISALLOW_COPY_AND_ASSIGN > > > > This structure is used in a value array so I think being copyable is OK here. > > So use emplace_back instead of push_back? I will use emplace_back() but that still relies on the copy constructor to be MoveInsertable [1] (or I would need a move constructor at which point it is probably not saving anything in simplicity). [1] http://en.cppreference.com/w/cpp/concept/MoveInsertable
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2453133002/#ps300001 (title: "[ash-md] Makes Wi-Fi header row sticky when network list is scrolled (back to range)")
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.
Committed patchset #12 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Makes Wi-Fi header row sticky when network list is scrolled Adds support for sticky header rows to tray icon scroll views. When marked as sticky the headers will not scroll above the top of the visible viewport unless there is another sticky header row just below. Multiple rows can be indicated as header rows by setting View::id_. BUG=631831 ========== to ========== [ash-md] Makes Wi-Fi header row sticky when network list is scrolled Adds support for sticky header rows to tray icon scroll views. When marked as sticky the headers will not scroll above the top of the visible viewport unless there is another sticky header row just below. Multiple rows can be indicated as header rows by setting View::id_. BUG=631831 Committed: https://crrev.com/b55cb3ec546bf8e0749d08f512d7ab5f3f5b7c4e Cr-Commit-Position: refs/heads/master@{#430214} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/b55cb3ec546bf8e0749d08f512d7ab5f3f5b7c4e Cr-Commit-Position: refs/heads/master@{#430214} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
