Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2487603005:
Adjust TriView. (Closed)
|
Created:
4 years, 1 month ago by Evan Stade Modified:
4 years, 1 month ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdjust TriView.
This fixes the IME keyboard toggle button. This also makes it possible
to properly support extra-tall center rows, which previously didn't
work --- this is needed for the cast row.
BUG=663923
Committed: https://crrev.com/33ed1d1123d26aa0f24125235d05cba1e77ea86f
Cr-Commit-Position: refs/heads/master@{#431664}
Patch Set 1 #
Total comments: 13
Patch Set 2 : merge #
Messages
Total messages: 22 (9 generated)
estade@chromium.org changed reviewers: + bruthig@chromium.org
https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... File ash/common/system/tray/tray_popup_utils.cc (left): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... ash/common/system/tray/tray_popup_utils.cc:50: views::BoxLayout::CROSS_AXIS_ALIGNMENT_STRETCH); The code does not match the comment above the function. To fix the toggle button, all that's needed is to change this STRETCH to CENTER (matching the comment). Having done that, CreateDefaultEndsLayoutManager returns the same thing as TriView's default layout manager, so it's unnecessary and I deleted it. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... File ash/common/system/tray/tray_popup_utils.cc (right): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... ash/common/system/tray/tray_popup_utils.cc:42: void ConfigureDefaultLayout(TriView* tri_view, TriView::Container container) { the rest of the changes in this file allow the center container to grow without displacing the side containers. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... File ash/common/system/tray/tri_view.h (left): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... ash/common/system/tray/tri_view.h:134: views::View* GetContainer(Container container) const; these functions should not be const because they return non-const pointers to member variables. (There's also no need for them to be const.)
(screenshots on bug)
Looks good, just a clarifying question inline. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... File ash/common/system/tray/tray_popup_utils.cc (left): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... ash/common/system/tray/tray_popup_utils.cc:42: // Creates a layout manager that positions Views horizontally. The Views will be Just as a heads up this CL: https://codereview.chromium.org/2482043002 is changing some of these methods around too. Not sure how much of a conflict there will be. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... File ash/common/system/tray/tri_view.cc (right): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... ash/common/system/tray/tri_view.cc:77: views::BoxLayout::CROSS_AXIS_ALIGNMENT_START); Just looking for confirmation, in your comment here https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... you said that is all that is need to fix the toggle? I'm assuming this is also needed so that the Toggle is top aligned. Is that correct or is this not needed? https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... File ash/common/system/tray/tri_view.h (left): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... ash/common/system/tray/tri_view.h:130: std::unique_ptr<views::LayoutManager> CreateDefaultLayoutManager( If you are removing TrayPopupUtils::CreateDefaultLayout() then this function may need to be made available to clients to support the changes in: https://codereview.chromium.org/2482043002 and we probably shouldn't move it to be an anonymous helper. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... ash/common/system/tray/tri_view.h:134: views::View* GetContainer(Container container) const; On 2016/11/09 23:32:15, Evan Stade wrote: > these functions should not be const because they return non-const pointers to > member variables. (There's also no need for them to be const.) Acknowledged, thx for fixing.
Oh one other question, how is the ToggleButtons padding specified?
> Oh one other question, how is the ToggleButtons padding specified? I'm not sure what you mean. In the mocks it's depicted as a certain size with a certain amount of padding above and below. That padding is equivalent to vertically centering the button. As far as horizontal padding, I haven't looked at that, we may need to make tweaks there. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... File ash/common/system/tray/tray_popup_utils.cc (left): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... ash/common/system/tray/tray_popup_utils.cc:42: // Creates a layout manager that positions Views horizontally. The Views will be On 2016/11/10 18:33:40, bruthig wrote: > Just as a heads up this CL: https://codereview.chromium.org/2482043002 is > changing some of these methods around too. Not sure how much of a conflict > there will be. Acknowledged. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... File ash/common/system/tray/tri_view.cc (right): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... ash/common/system/tray/tri_view.cc:77: views::BoxLayout::CROSS_AXIS_ALIGNMENT_START); On 2016/11/10 18:33:40, bruthig wrote: > Just looking for confirmation, in your comment here > https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... > you said that is all that is need to fix the toggle? I'm assuming this is also > needed so that the Toggle is top aligned. Is that correct or is this not > needed? no, all we needed to do to fix the toggle was change the end container's (wrapped) layout manager to center (change STRETCH to CENTER in the deleted block of code at that comment). However since that makes that layout the same as the default one (as defined by CreateDefaultLayoutManager in this file) we were able to just delete that whole function. This change here is needed for the top-alignment of all the containers. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... File ash/common/system/tray/tri_view.h (left): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... ash/common/system/tray/tri_view.h:130: std::unique_ptr<views::LayoutManager> CreateDefaultLayoutManager( On 2016/11/10 18:33:40, bruthig wrote: > If you are removing TrayPopupUtils::CreateDefaultLayout() then this function may > need to be made available to clients to support the changes in: > https://codereview.chromium.org/2482043002 and we probably shouldn't move it to > be an anonymous helper. I don't think so. We just won't set the layout manager for the start/end containers and thus get the TriView defaults for those, which is what we want.
lgtm Thanks for taking the time looking in to a fix we are both happy with. I think this is better than any of the original suggestions either of us proposed. On 2016/11/10 19:18:06, Evan Stade wrote: > > Oh one other question, how is the ToggleButtons padding specified? > > I'm not sure what you mean. In the mocks it's depicted as a certain size with a > certain amount of padding above and below. That padding is equivalent to > vertically centering the button. As far as horizontal padding, I haven't looked > at that, we may need to make tweaks there. Gotcha, so as long as the View (e.g. toggle, label button, etc.) should be vertically centered within the minimum row height it will work. That's great! And this will probably work if the END container has multiple Views in it that are of different height too. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... File ash/common/system/tray/tri_view.cc (right): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... ash/common/system/tray/tri_view.cc:77: views::BoxLayout::CROSS_AXIS_ALIGNMENT_START); On 2016/11/10 19:18:05, Evan Stade wrote: > On 2016/11/10 18:33:40, bruthig wrote: > > Just looking for confirmation, in your comment here > > > https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tray... > > you said that is all that is need to fix the toggle? I'm assuming this is > also > > needed so that the Toggle is top aligned. Is that correct or is this not > > needed? > > no, all we needed to do to fix the toggle was change the end container's > (wrapped) layout manager to center (change STRETCH to CENTER in the deleted > block of code at that comment). However since that makes that layout the same as > the default one (as defined by CreateDefaultLayoutManager in this file) we were > able to just delete that whole function. > > This change here is needed for the top-alignment of all the containers. Acknowledged. https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... File ash/common/system/tray/tri_view.h (left): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... ash/common/system/tray/tri_view.h:130: std::unique_ptr<views::LayoutManager> CreateDefaultLayoutManager( On 2016/11/10 19:18:05, Evan Stade wrote: > On 2016/11/10 18:33:40, bruthig wrote: > > If you are removing TrayPopupUtils::CreateDefaultLayout() then this function > may > > need to be made available to clients to support the changes in: > > https://codereview.chromium.org/2482043002 and we probably shouldn't move it > to > > be an anonymous helper. > > I don't think so. We just won't set the layout manager for the start/end > containers and thus get the TriView defaults for those, which is what we want. I don't think you followed my train of thought, let me try again. Because the audio row (VolumeView) has multiple target-able views the mentioned change is installing the default layout managers used by the START/END containers onto its own container views. Specifically the VolumeView::more_button_ and icon_ buttons. i.e. these default layout managers are being installed directly on non-TriView containers, e.g. buttons, to allow each container to be target-able but have the same layout as if they were added to the TriView. It might make more sense once you try to merge.
https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... File ash/common/system/tray/tri_view.h (left): https://codereview.chromium.org/2487603005/diff/1/ash/common/system/tray/tri_... ash/common/system/tray/tri_view.h:130: std::unique_ptr<views::LayoutManager> CreateDefaultLayoutManager( On 2016/11/10 20:51:16, bruthig wrote: > On 2016/11/10 19:18:05, Evan Stade wrote: > > On 2016/11/10 18:33:40, bruthig wrote: > > > If you are removing TrayPopupUtils::CreateDefaultLayout() then this function > > may > > > need to be made available to clients to support the changes in: > > > https://codereview.chromium.org/2482043002 and we probably shouldn't move it > > to > > > be an anonymous helper. > > > > I don't think so. We just won't set the layout manager for the start/end > > containers and thus get the TriView defaults for those, which is what we want. > > I don't think you followed my train of thought, let me try again. Because the > audio row (VolumeView) has multiple target-able views the mentioned change is > installing the default layout managers used by the START/END containers onto its > own container views. Specifically the VolumeView::more_button_ and icon_ > buttons. i.e. these default layout managers are being installed directly on > non-TriView containers, e.g. buttons, to allow each container to be target-able > but have the same layout as if they were added to the TriView. > > It might make more sense once you try to merge. ah, I only looked at TrayPopupUtils in that CL. Guess I'll see what happens when I merge that change in.
The CQ bit was checked by estade@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...
estade@chromium.org changed reviewers: + tdanderson@chromium.org
+tda for OWNERS
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2487603005/#ps20001 (title: "merge")
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 #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Adjust TriView. This fixes the IME keyboard toggle button. This also makes it possible to properly support extra-tall center rows, which previously didn't work --- this is needed for the cast row. BUG=663923 ========== to ========== Adjust TriView. This fixes the IME keyboard toggle button. This also makes it possible to properly support extra-tall center rows, which previously didn't work --- this is needed for the cast row. BUG=663923 Committed: https://crrev.com/33ed1d1123d26aa0f24125235d05cba1e77ea86f Cr-Commit-Position: refs/heads/master@{#431664} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/33ed1d1123d26aa0f24125235d05cba1e77ea86f Cr-Commit-Position: refs/heads/master@{#431664} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
