Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2440723003:
Tweaks to ToggleButton details. (Closed)
|
Created:
4 years, 2 months ago by Evan Stade Modified:
4 years, 1 month ago CC:
chromium-reviews, tfarina, varkha Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTweaks to ToggleButton details.
- fix the color of the track and take more colors from the
theme. The spec states 0.16a for the track whether off or
on, but the color of the mock-up is actual 0.32a for the
on version. I take this to be a bug in the text of the
spec.
- make the thumb the correct size. The actual white circle
is supposed to be 15dip rather than 16dip (this is another
thing confusing about the spec, as it states 16dp but
where it measures from includes 1 dip of shadow). Note
that to maintain crispness, we must align to pixel
boundaries and must maintain a centered position, so
there's a bit of rounding here depending on scale factor.
BUG=626827
Committed: https://crrev.com/b58c6c3057e95b0d27097821743dcb7653e3dbaa
Cr-Commit-Position: refs/heads/master@{#429682}
Patch Set 1 #Patch Set 2 : size of thumb #Patch Set 3 : colors #
Total comments: 12
Patch Set 4 : no new layer #
Total comments: 5
Patch Set 5 : rearrange fn #
Total comments: 8
Patch Set 6 : invert #
Total comments: 4
Patch Set 7 : Offset #
Total comments: 2
Patch Set 8 : review feedback #Patch Set 9 : remove a little more #
Total comments: 1
Messages
Total messages: 48 (22 generated)
Description was changed from ========== Tweaks to ToggleButton. Eliminate some TODOs around theming, reuse default InkDropHostView methods. BUG=none ========== to ========== Tweaks to ToggleButton details. - fix the color of the track and take more colors from the theme. The spec states 0.16a for the track whether off or on, but the color of the mock-up is actual 0.32a for the on version. I take this to be a bug in the text of the spec. - make the thumb the correct size. The actual white circle is supposed to be 15dip rather than 16dip (this is another thing confusing about the spec, as it states 16dp but where it measures from includes 1 dip of shadow). Note that to maintain crispness, we must align to pixel boundaries and must maintain a centered position, so there's a bit of rounding here depending on scale factor. BUG=626827 ==========
estade@chromium.org changed reviewers: + sadrul@chromium.org
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Description was changed from ========== Tweaks to ToggleButton details. - fix the color of the track and take more colors from the theme. The spec states 0.16a for the track whether off or on, but the color of the mock-up is actual 0.32a for the on version. I take this to be a bug in the text of the spec. - make the thumb the correct size. The actual white circle is supposed to be 15dip rather than 16dip (this is another thing confusing about the spec, as it states 16dp but where it measures from includes 1 dip of shadow). Note that to maintain crispness, we must align to pixel boundaries and must maintain a centered position, so there's a bit of rounding here depending on scale factor. BUG=626827 ========== to ========== Tweaks to ToggleButton details. - fix the color of the track and take more colors from the theme. The spec states 0.16a for the track whether off or on, but the color of the mock-up is actual 0.32a for the on version. I take this to be a bug in the text of the spec. - make the thumb the correct size. The actual white circle is supposed to be 15dip rather than 16dip (this is another thing confusing about the spec, as it states 16dp but where it measures from includes 1 dip of shadow). Note that to maintain crispness, we must align to pixel boundaries and must maintain a centered position, so there's a bit of rounding here depending on scale factor. BUG=626827 ==========
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.
varkha@chromium.org changed reviewers: + varkha@chromium.org
https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:36: SetPaintToLayer(true); The old implementation was more lines but it was only using extra layers when the toggle is interacted with (and flattened otherwise). The new impl is always using an extra layer - something we were trying to minimize before with the ripple. Something probably only noticeable when there are many toggle buttons at the same time but still worth consideration. https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:146: thumb_bounds.Inset(-ThumbView::GetShadowInsets()); Thank you! https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.h:52: SkColor GetTrackColor(bool is_on) const; nit: I think the style guide asked for the overrides to be after local methods.
https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:36: SetPaintToLayer(true); On 2016/10/21 00:35:18, varkha wrote: > The old implementation was more lines but it was only using extra layers when > the toggle is interacted with (and flattened otherwise). The new impl is always > using an extra layer - something we were trying to minimize before with the > ripple. Something probably only noticeable when there are many toggle buttons at > the same time but still worth consideration. It's not clear to me that creating and destroying this layer on every ink drop is a performance improvement, but the real reason I added it is because the shadow extends slightly beyond the bounds of the view. That said, it's not truly necessary if we leave the vertical margin constants alone, I mainly changed those to match the overall height Sebastien spec'd. https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.h:52: SkColor GetTrackColor(bool is_on) const; On 2016/10/21 00:35:18, varkha wrote: > nit: I think the style guide asked for the overrides to be after local methods. I don't think it specifies[1] and I believe (without substantiation) it's more common in Chrome to put overrides first. [1] https://google.github.io/styleguide/cppguide.html#Declaration_Order
varkha@chromium.org changed reviewers: + bruthig@chromium.org
https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:36: SetPaintToLayer(true); On 2016/10/21 17:19:15, Evan Stade wrote: > On 2016/10/21 00:35:18, varkha wrote: > > The old implementation was more lines but it was only using extra layers when > > the toggle is interacted with (and flattened otherwise). The new impl is > always > > using an extra layer - something we were trying to minimize before with the > > ripple. Something probably only noticeable when there are many toggle buttons > at > > the same time but still worth consideration. > > It's not clear to me that creating and destroying this layer on every ink drop > is a performance improvement, but the real reason I added it is because the > shadow extends slightly beyond the bounds of the view. That said, it's not > truly necessary if we leave the vertical margin constants alone, I mainly > changed those to match the overall height Sebastien spec'd. +bruthig@ and sadrul@ to see if they have stronger opinions here. I think this is what we are doing with the rest of the ripple-enabled buttons because we tried to keep texture memory down as a concern. https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.h:52: SkColor GetTrackColor(bool is_on) const; On 2016/10/21 17:19:15, Evan Stade wrote: > On 2016/10/21 00:35:18, varkha wrote: > > nit: I think the style guide asked for the overrides to be after local > methods. > > I don't think it specifies[1] and I believe (without substantiation) it's more > common in Chrome to put overrides first. > > [1] https://google.github.io/styleguide/cppguide.html#Declaration_Order No strong opinion here. Just needs to be consistent with the rest of order (which I have changed because I had this suggested in my CL - https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto...).
https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:36: SetPaintToLayer(true); On 2016/10/21 17:55:36, varkha wrote: > On 2016/10/21 17:19:15, Evan Stade wrote: > > On 2016/10/21 00:35:18, varkha wrote: > > > The old implementation was more lines but it was only using extra layers > when > > > the toggle is interacted with (and flattened otherwise). The new impl is > > always > > > using an extra layer - something we were trying to minimize before with the > > > ripple. Something probably only noticeable when there are many toggle > buttons > > at > > > the same time but still worth consideration. > > > > It's not clear to me that creating and destroying this layer on every ink drop > > is a performance improvement, but the real reason I added it is because the > > shadow extends slightly beyond the bounds of the view. That said, it's not > > truly necessary if we leave the vertical margin constants alone, I mainly > > changed those to match the overall height Sebastien spec'd. > > +bruthig@ and sadrul@ to see if they have stronger opinions here. I think this > is what we are doing with the rest of the ripple-enabled buttons because we > tried to keep texture memory down as a concern. Even though the ThumbView is always painting to a Layer the ink drop layers will be cleaned up when not active. Considering that the ShelfButtons paint to their own layer I don't have any concerns with having 1 extra layer per toggle. Having said that, if it's easy enough to not require a layer then why not eliminate the layer... In summary, no strong opinion ;) FTR I'd wait for sadrul@ to give an opinion too. https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:193: SchedulePaint(); varkha@, IF the ThumbView is always painting to a layer are these SchedulePaint() calls still necessary on lines 193 and 198?
On 2016/10/21 18:36:40, bruthig wrote: > https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... > File ui/views/controls/button/toggle_button.cc (right): > > https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... > ui/views/controls/button/toggle_button.cc:36: SetPaintToLayer(true); > On 2016/10/21 17:55:36, varkha wrote: > > On 2016/10/21 17:19:15, Evan Stade wrote: > > > On 2016/10/21 00:35:18, varkha wrote: > > > > The old implementation was more lines but it was only using extra layers > > when > > > > the toggle is interacted with (and flattened otherwise). The new impl is > > > always > > > > using an extra layer - something we were trying to minimize before with > the > > > > ripple. Something probably only noticeable when there are many toggle > > buttons > > > at > > > > the same time but still worth consideration. > > > > > > It's not clear to me that creating and destroying this layer on every ink > drop > > > is a performance improvement, but the real reason I added it is because the > > > shadow extends slightly beyond the bounds of the view. That said, it's not > > > truly necessary if we leave the vertical margin constants alone, I mainly > > > changed those to match the overall height Sebastien spec'd. > > > > +bruthig@ and sadrul@ to see if they have stronger opinions here. I think this > > is what we are doing with the rest of the ripple-enabled buttons because we > > tried to keep texture memory down as a concern. > > Even though the ThumbView is always painting to a Layer the ink drop layers will > be cleaned up when not active. Considering that the ShelfButtons paint to their > own layer I don't have any concerns with having 1 extra layer per toggle. > Having said that, if it's easy enough to not require a layer then why not > eliminate the layer... > > In summary, no strong opinion ;) > > FTR I'd wait for sadrul@ to give an opinion too. > > https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... > ui/views/controls/button/toggle_button.cc:193: SchedulePaint(); > varkha@, IF the ThumbView is always painting to a layer are these > SchedulePaint() calls still necessary on lines 193 and 198? ping sadrul
https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:36: SetPaintToLayer(true); On 2016/10/21 18:36:40, bruthig wrote: > On 2016/10/21 17:55:36, varkha wrote: > > On 2016/10/21 17:19:15, Evan Stade wrote: > > > On 2016/10/21 00:35:18, varkha wrote: > > > > The old implementation was more lines but it was only using extra layers > > when > > > > the toggle is interacted with (and flattened otherwise). The new impl is > > > always > > > > using an extra layer - something we were trying to minimize before with > the > > > > ripple. Something probably only noticeable when there are many toggle > > buttons > > > at > > > > the same time but still worth consideration. > > > > > > It's not clear to me that creating and destroying this layer on every ink > drop > > > is a performance improvement, but the real reason I added it is because the > > > shadow extends slightly beyond the bounds of the view. That said, it's not > > > truly necessary if we leave the vertical margin constants alone, I mainly > > > changed those to match the overall height Sebastien spec'd. > > > > +bruthig@ and sadrul@ to see if they have stronger opinions here. I think this > > is what we are doing with the rest of the ripple-enabled buttons because we > > tried to keep texture memory down as a concern. > > Even though the ThumbView is always painting to a Layer the ink drop layers will > be cleaned up when not active. Considering that the ShelfButtons paint to their > own layer I don't have any concerns with having 1 extra layer per toggle. > Having said that, if it's easy enough to not require a layer then why not > eliminate the layer... > > In summary, no strong opinion ;) > > FTR I'd wait for sadrul@ to give an opinion too. Always having the layer seem unrelated to the bugs being fixed. Can you move the layer-change to a separate CL so we can discuss that separately, and not block the bug fix? https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.h:52: SkColor GetTrackColor(bool is_on) const; On 2016/10/21 17:55:36, varkha wrote: > On 2016/10/21 17:19:15, Evan Stade wrote: > > On 2016/10/21 00:35:18, varkha wrote: > > > nit: I think the style guide asked for the overrides to be after local > > methods. > > > > I don't think it specifies[1] and I believe (without substantiation) it's more > > common in Chrome to put overrides first. > > > > [1] https://google.github.io/styleguide/cppguide.html#Declaration_Order > > No strong opinion here. Just needs to be consistent with the rest of order > (which I have changed because I had this suggested in my CL - > https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto...). In ui code, I think we fairly consistently put non-overrides before overrides.
On 2016/10/24 14:46:12, sadrul wrote: > https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... > File ui/views/controls/button/toggle_button.cc (right): > > https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... > ui/views/controls/button/toggle_button.cc:36: SetPaintToLayer(true); > On 2016/10/21 18:36:40, bruthig wrote: > > On 2016/10/21 17:55:36, varkha wrote: > > > On 2016/10/21 17:19:15, Evan Stade wrote: > > > > On 2016/10/21 00:35:18, varkha wrote: > > > > > The old implementation was more lines but it was only using extra layers > > > when > > > > > the toggle is interacted with (and flattened otherwise). The new impl is > > > > always > > > > > using an extra layer - something we were trying to minimize before with > > the > > > > > ripple. Something probably only noticeable when there are many toggle > > > buttons > > > > at > > > > > the same time but still worth consideration. > > > > > > > > It's not clear to me that creating and destroying this layer on every ink > > drop > > > > is a performance improvement, but the real reason I added it is because > the > > > > shadow extends slightly beyond the bounds of the view. That said, it's > not > > > > truly necessary if we leave the vertical margin constants alone, I mainly > > > > changed those to match the overall height Sebastien spec'd. > > > > > > +bruthig@ and sadrul@ to see if they have stronger opinions here. I think > this > > > is what we are doing with the rest of the ripple-enabled buttons because we > > > tried to keep texture memory down as a concern. > > > > Even though the ThumbView is always painting to a Layer the ink drop layers > will > > be cleaned up when not active. Considering that the ShelfButtons paint to > their > > own layer I don't have any concerns with having 1 extra layer per toggle. > > Having said that, if it's easy enough to not require a layer then why not > > eliminate the layer... > > > > In summary, no strong opinion ;) > > > > FTR I'd wait for sadrul@ to give an opinion too. > > Always having the layer seem unrelated to the bugs being fixed. Can you move the > layer-change to a separate CL so we can discuss that separately, and not block > the bug fix? > > https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... > File ui/views/controls/button/toggle_button.h (right): > > https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... > ui/views/controls/button/toggle_button.h:52: SkColor GetTrackColor(bool is_on) > const; > On 2016/10/21 17:55:36, varkha wrote: > > On 2016/10/21 17:19:15, Evan Stade wrote: > > > On 2016/10/21 00:35:18, varkha wrote: > > > > nit: I think the style guide asked for the overrides to be after local > > > methods. > > > > > > I don't think it specifies[1] and I believe (without substantiation) it's > more > > > common in Chrome to put overrides first. > > > > > > [1] https://google.github.io/styleguide/cppguide.html#Declaration_Order > > > > No strong opinion here. Just needs to be consistent with the rest of order > > (which I have changed because I had this suggested in my CL - > > > https://codereview.chromium.org/2396133005/diff/80001/ui/views/controls/butto...). > > In ui code, I think we fairly consistently put non-overrides before overrides. ok, will get back to this on Thursday.
https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/40001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:36: SetPaintToLayer(true); On 2016/10/24 14:46:12, sadrul wrote: > On 2016/10/21 18:36:40, bruthig wrote: > > On 2016/10/21 17:55:36, varkha wrote: > > > On 2016/10/21 17:19:15, Evan Stade wrote: > > > > On 2016/10/21 00:35:18, varkha wrote: > > > > > The old implementation was more lines but it was only using extra layers > > > when > > > > > the toggle is interacted with (and flattened otherwise). The new impl is > > > > always > > > > > using an extra layer - something we were trying to minimize before with > > the > > > > > ripple. Something probably only noticeable when there are many toggle > > > buttons > > > > at > > > > > the same time but still worth consideration. > > > > > > > > It's not clear to me that creating and destroying this layer on every ink > > drop > > > > is a performance improvement, but the real reason I added it is because > the > > > > shadow extends slightly beyond the bounds of the view. That said, it's > not > > > > truly necessary if we leave the vertical margin constants alone, I mainly > > > > changed those to match the overall height Sebastien spec'd. > > > > > > +bruthig@ and sadrul@ to see if they have stronger opinions here. I think > this > > > is what we are doing with the rest of the ripple-enabled buttons because we > > > tried to keep texture memory down as a concern. > > > > Even though the ThumbView is always painting to a Layer the ink drop layers > will > > be cleaned up when not active. Considering that the ShelfButtons paint to > their > > own layer I don't have any concerns with having 1 extra layer per toggle. > > Having said that, if it's easy enough to not require a layer then why not > > eliminate the layer... > > > > In summary, no strong opinion ;) > > > > FTR I'd wait for sadrul@ to give an opinion too. > > Always having the layer seem unrelated to the bugs being fixed. Can you move the > layer-change to a separate CL so we can discuss that separately, and not block > the bug fix? Done. https://codereview.chromium.org/2440723003/diff/60001/ui/gfx/geometry/insets.h File ui/gfx/geometry/insets.h (right): https://codereview.chromium.org/2440723003/diff/60001/ui/gfx/geometry/insets.... ui/gfx/geometry/insets.h:123: GFX_EXPORT Insets operator+(const Insets& insets, const gfx::Vector2d& vector); I've wanted this before --- I think it will get use.
lgtm with a couple nits. https://codereview.chromium.org/2440723003/diff/60001/ui/gfx/geometry/insets.h File ui/gfx/geometry/insets.h (right): https://codereview.chromium.org/2440723003/diff/60001/ui/gfx/geometry/insets.... ui/gfx/geometry/insets.h:123: GFX_EXPORT Insets operator+(const Insets& insets, const gfx::Vector2d& vector); On 2016/10/27 17:12:14, Evan Stade wrote: > I've wanted this before --- I think it will get use. Does it make sense to add a short doc here to avoid the need to look in impl what exactly this does? https://codereview.chromium.org/2440723003/diff/60001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2440723003/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.h:52: SkColor GetTrackColor(bool is_on) const; Do you want to move this to be with other non-overrides?
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...
https://codereview.chromium.org/2440723003/diff/60001/ui/gfx/geometry/insets.h File ui/gfx/geometry/insets.h (right): https://codereview.chromium.org/2440723003/diff/60001/ui/gfx/geometry/insets.... ui/gfx/geometry/insets.h:123: GFX_EXPORT Insets operator+(const Insets& insets, const gfx::Vector2d& vector); On 2016/10/27 19:02:00, varkha wrote: > On 2016/10/27 17:12:14, Evan Stade wrote: > > I've wanted this before --- I think it will get use. > > Does it make sense to add a short doc here to avoid the need to look in impl > what exactly this does? I don't think so. We don't do it for any other operators in ui/gfx/geo and it seems obvious enough to me, the author :). But sadrul is owner here. https://codereview.chromium.org/2440723003/diff/60001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.h (right): https://codereview.chromium.org/2440723003/diff/60001/ui/views/controls/butto... ui/views/controls/button/toggle_button.h:52: SkColor GetTrackColor(bool is_on) const; On 2016/10/27 19:02:00, varkha wrote: > Do you want to move this to be with other non-overrides? sorry, yes
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sadrul@chromium.org changed reviewers: + danakj@chromium.org
+danakj@ for the geometry change (I feel like Insets + Vector2d is a little bit confusing, and the impl is actually doing the reverse of what I would expect. So off to geometry owner) https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:84: // unscale back to dip bounds. This is pretty weird. Can you use Canvas::UndoDeviceScaleFactor() and draw in pixel-space instead?
On 2016/10/28 02:58:14, sadrul wrote: > +danakj@ for the geometry change (I feel like Insets + Vector2d is a little bit > confusing, and the impl is actually doing the reverse of what I would expect. So > off to geometry owner) you're right, I had it backwards --- I got twisted around regarding the meaning of positive vs. negative Inset values. It worked because both the operation and the use in ToggleButton were inverted. https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:84: // unscale back to dip bounds. On 2016/10/28 02:58:14, sadrul wrote: > This is pretty weird. Can you use Canvas::UndoDeviceScaleFactor() and draw in > pixel-space instead? That makes it harder to deal with the shadows, which are defined in DIP and use integers rather than floats (so it wouldn't work well for dsf 1.5). Shadows aside, this part doesn't get much less weird when unscaling: gfx::RectF thumb_bounds(GetLocalBounds()); thumb_bounds.Inset(-GetShadowInsets()); thumb_bounds.Inset(gfx::InsetsF(0.5f)); // We want the circle to have an integer pixel diameter and to be aligned // with pixel boundaries, so we scale dip bounds to pixel bounds and round. gfx::ScopedCanvas scoped(canvas); float dsf = canvas->UndoDeviceScaleFactor(); gfx::RectF bounds_in_px = thumb_bounds; bounds_in_px.Scale(dsf); bounds_in_px = gfx::RectF(gfx::ToEnclosingRect(bounds_in_px)); canvas->DrawCircle(bounds_in_px.CenterPoint(), bounds_in_px.height() / 2.f, thumb_paint);
https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:84: // unscale back to dip bounds. On 2016/10/28 17:49:39, Evan Stade wrote: > On 2016/10/28 02:58:14, sadrul wrote: > > This is pretty weird. Can you use Canvas::UndoDeviceScaleFactor() and draw in > > pixel-space instead? > > That makes it harder to deal with the shadows, which are defined in DIP and use > integers rather than floats (so it wouldn't work well for dsf 1.5). Shadows > aside, this part doesn't get much less weird when unscaling: > > gfx::RectF thumb_bounds(GetLocalBounds()); > thumb_bounds.Inset(-GetShadowInsets()); > thumb_bounds.Inset(gfx::InsetsF(0.5f)); > // We want the circle to have an integer pixel diameter and to be aligned > // with pixel boundaries, so we scale dip bounds to pixel bounds and round. > gfx::ScopedCanvas scoped(canvas); > float dsf = canvas->UndoDeviceScaleFactor(); > gfx::RectF bounds_in_px = thumb_bounds; > bounds_in_px.Scale(dsf); > bounds_in_px = gfx::RectF(gfx::ToEnclosingRect(bounds_in_px)); > canvas->DrawCircle(bounds_in_px.CenterPoint(), > bounds_in_px.height() / 2.f, thumb_paint); UndoDSF() mostly lets you round to physical pixels so you don't get unintended blurriness at scales like 1.5. https://codereview.chromium.org/2440723003/diff/100001/ui/gfx/geometry/insets.h File ui/gfx/geometry/insets.h (right): https://codereview.chromium.org/2440723003/diff/100001/ui/gfx/geometry/insets... ui/gfx/geometry/insets.h:123: GFX_EXPORT Insets operator+(const Insets& insets, const gfx::Vector2d& vector); So this moves the top/bottom and left/right insets in the same direction based on the vector? I think the operation of + isn't super obvious for this and I'd prefer a member method with a name that describes it like OffsetBy() or something? wdyt?
https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:84: // unscale back to dip bounds. On 2016/10/28 18:44:59, danakj wrote: > On 2016/10/28 17:49:39, Evan Stade wrote: > > On 2016/10/28 02:58:14, sadrul wrote: > > > This is pretty weird. Can you use Canvas::UndoDeviceScaleFactor() and draw > in > > > pixel-space instead? > > > > That makes it harder to deal with the shadows, which are defined in DIP and > use > > integers rather than floats (so it wouldn't work well for dsf 1.5). Shadows > > aside, this part doesn't get much less weird when unscaling: > > > > gfx::RectF thumb_bounds(GetLocalBounds()); > > thumb_bounds.Inset(-GetShadowInsets()); > > thumb_bounds.Inset(gfx::InsetsF(0.5f)); > > // We want the circle to have an integer pixel diameter and to be aligned > > // with pixel boundaries, so we scale dip bounds to pixel bounds and > round. > > gfx::ScopedCanvas scoped(canvas); > > float dsf = canvas->UndoDeviceScaleFactor(); > > gfx::RectF bounds_in_px = thumb_bounds; > > bounds_in_px.Scale(dsf); > > bounds_in_px = gfx::RectF(gfx::ToEnclosingRect(bounds_in_px)); > > canvas->DrawCircle(bounds_in_px.CenterPoint(), > > bounds_in_px.height() / 2.f, thumb_paint); > > UndoDSF() mostly lets you round to physical pixels so you don't get unintended > blurriness at scales like 1.5. I don't think that addresses the problem with shadows here. We'd have to scale the shadow values by the dsf. That's not easy to do. https://codereview.chromium.org/2440723003/diff/100001/ui/gfx/geometry/insets.h File ui/gfx/geometry/insets.h (right): https://codereview.chromium.org/2440723003/diff/100001/ui/gfx/geometry/insets... ui/gfx/geometry/insets.h:123: GFX_EXPORT Insets operator+(const Insets& insets, const gfx::Vector2d& vector); On 2016/10/28 18:44:59, danakj wrote: > So this moves the top/bottom and left/right insets in the same direction based > on the vector? I think the operation of + isn't super obvious for this and I'd > prefer a member method with a name that describes it like OffsetBy() or > something? wdyt? assuming it returns an Inset (unlike gfx::Rect::Offset, which acts in-place which I usually find less useful), sure.
https://codereview.chromium.org/2440723003/diff/100001/ui/gfx/geometry/insets.h File ui/gfx/geometry/insets.h (right): https://codereview.chromium.org/2440723003/diff/100001/ui/gfx/geometry/insets... ui/gfx/geometry/insets.h:123: GFX_EXPORT Insets operator+(const Insets& insets, const gfx::Vector2d& vector); On 2016/10/28 19:22:03, Evan Stade wrote: > On 2016/10/28 18:44:59, danakj wrote: > > So this moves the top/bottom and left/right insets in the same direction based > > on the vector? I think the operation of + isn't super obvious for this and I'd > > prefer a member method with a name that describes it like OffsetBy() or > > something? wdyt? > > assuming it returns an Inset (unlike gfx::Rect::Offset, which acts in-place > which I usually find less useful), sure. That would match the behaviour of Scale() so sure
https://codereview.chromium.org/2440723003/diff/100001/ui/gfx/geometry/insets.h File ui/gfx/geometry/insets.h (right): https://codereview.chromium.org/2440723003/diff/100001/ui/gfx/geometry/insets... ui/gfx/geometry/insets.h:123: GFX_EXPORT Insets operator+(const Insets& insets, const gfx::Vector2d& vector); On 2016/10/28 19:46:56, danakj wrote: > On 2016/10/28 19:22:03, Evan Stade wrote: > > On 2016/10/28 18:44:59, danakj wrote: > > > So this moves the top/bottom and left/right insets in the same direction > based > > > on the vector? I think the operation of + isn't super obvious for this and > I'd > > > prefer a member method with a name that describes it like OffsetBy() or > > > something? wdyt? > > > > assuming it returns an Inset (unlike gfx::Rect::Offset, which acts in-place > > which I usually find less useful), sure. > > That would match the behaviour of Scale() so sure done (unlike Scale, the impl is in the .cc file to reduce transitive includes)
https://codereview.chromium.org/2440723003/diff/120001/ui/gfx/geometry/insets.h File ui/gfx/geometry/insets.h (right): https://codereview.chromium.org/2440723003/diff/120001/ui/gfx/geometry/insets... ui/gfx/geometry/insets.h:98: Insets Offset(const gfx::Vector2d& vector) const; Can you leave a comment explaining in more detail what it does too? And add a unit test showing/verifying the behaviour of this method?
https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:84: // unscale back to dip bounds. On 2016/10/28 17:49:39, Evan Stade wrote: > On 2016/10/28 02:58:14, sadrul wrote: > > This is pretty weird. Can you use Canvas::UndoDeviceScaleFactor() and draw in > > pixel-space instead? > > That makes it harder to deal with the shadows, which are defined in DIP and use > integers rather than floats (so it wouldn't work well for dsf 1.5). Shadows > aside, this part doesn't get much less weird when unscaling: > > gfx::RectF thumb_bounds(GetLocalBounds()); > thumb_bounds.Inset(-GetShadowInsets()); > thumb_bounds.Inset(gfx::InsetsF(0.5f)); > // We want the circle to have an integer pixel diameter and to be aligned > // with pixel boundaries, so we scale dip bounds to pixel bounds and round. > gfx::ScopedCanvas scoped(canvas); > float dsf = canvas->UndoDeviceScaleFactor(); > gfx::RectF bounds_in_px = thumb_bounds; > bounds_in_px.Scale(dsf); > bounds_in_px = gfx::RectF(gfx::ToEnclosingRect(bounds_in_px)); > canvas->DrawCircle(bounds_in_px.CenterPoint(), > bounds_in_px.height() / 2.f, thumb_paint); We avoid going pixel -> dip (and possibly avoid more rounding issues?). I think that's still better. (You shouldn't need the ScopedCanvas either, since View::Paint calls OnPaint with a scoped-canvas, so the undoDSF wouldn't affect other views.) Looks like ShadowValue already has a Scale() method that you would use. So dealing with shadows shouldn't be bad either? https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:84: // unscale back to dip bounds. On 2016/10/28 19:22:03, Evan Stade wrote: > On 2016/10/28 18:44:59, danakj wrote: > > On 2016/10/28 17:49:39, Evan Stade wrote: > > > On 2016/10/28 02:58:14, sadrul wrote: > > > > This is pretty weird. Can you use Canvas::UndoDeviceScaleFactor() and draw > > in > > > > pixel-space instead? > > > > > > That makes it harder to deal with the shadows, which are defined in DIP and > > use > > > integers rather than floats (so it wouldn't work well for dsf 1.5). Shadows > > > aside, this part doesn't get much less weird when unscaling: > > > > > > gfx::RectF thumb_bounds(GetLocalBounds()); > > > thumb_bounds.Inset(-GetShadowInsets()); > > > thumb_bounds.Inset(gfx::InsetsF(0.5f)); > > > // We want the circle to have an integer pixel diameter and to be > aligned > > > // with pixel boundaries, so we scale dip bounds to pixel bounds and > > round. > > > gfx::ScopedCanvas scoped(canvas); > > > float dsf = canvas->UndoDeviceScaleFactor(); > > > gfx::RectF bounds_in_px = thumb_bounds; > > > bounds_in_px.Scale(dsf); > > > bounds_in_px = gfx::RectF(gfx::ToEnclosingRect(bounds_in_px)); > > > canvas->DrawCircle(bounds_in_px.CenterPoint(), > > > bounds_in_px.height() / 2.f, thumb_paint); > > > > UndoDSF() mostly lets you round to physical pixels so you don't get unintended > > blurriness at scales like 1.5. > > I don't think that addresses the problem with shadows here. We'd have to scale > the shadow values by the dsf. That's not easy to do. Can you clarify why scaling the shadow values is hard?
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...
https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:84: // unscale back to dip bounds. On 2016/10/31 18:28:50, sadrul wrote: > On 2016/10/28 19:22:03, Evan Stade wrote: > > On 2016/10/28 18:44:59, danakj wrote: > > > On 2016/10/28 17:49:39, Evan Stade wrote: > > > > On 2016/10/28 02:58:14, sadrul wrote: > > > > > This is pretty weird. Can you use Canvas::UndoDeviceScaleFactor() and > draw > > > in > > > > > pixel-space instead? > > > > > > > > That makes it harder to deal with the shadows, which are defined in DIP > and > > > use > > > > integers rather than floats (so it wouldn't work well for dsf 1.5). > Shadows > > > > aside, this part doesn't get much less weird when unscaling: > > > > > > > > gfx::RectF thumb_bounds(GetLocalBounds()); > > > > thumb_bounds.Inset(-GetShadowInsets()); > > > > thumb_bounds.Inset(gfx::InsetsF(0.5f)); > > > > // We want the circle to have an integer pixel diameter and to be > > aligned > > > > // with pixel boundaries, so we scale dip bounds to pixel bounds and > > > round. > > > > gfx::ScopedCanvas scoped(canvas); > > > > float dsf = canvas->UndoDeviceScaleFactor(); > > > > gfx::RectF bounds_in_px = thumb_bounds; > > > > bounds_in_px.Scale(dsf); > > > > bounds_in_px = gfx::RectF(gfx::ToEnclosingRect(bounds_in_px)); > > > > canvas->DrawCircle(bounds_in_px.CenterPoint(), > > > > bounds_in_px.height() / 2.f, thumb_paint); > > > > > > UndoDSF() mostly lets you round to physical pixels so you don't get > unintended > > > blurriness at scales like 1.5. > > > > I don't think that addresses the problem with shadows here. We'd have to scale > > the shadow values by the dsf. That's not easy to do. > > Can you clarify why scaling the shadow values is hard? mainly because of a lack of awareness of ShadowValue::Scale :) https://codereview.chromium.org/2440723003/diff/80001/ui/views/controls/butto... ui/views/controls/button/toggle_button.cc:84: // unscale back to dip bounds. On 2016/10/31 18:28:50, sadrul wrote: > On 2016/10/28 17:49:39, Evan Stade wrote: > > On 2016/10/28 02:58:14, sadrul wrote: > > > This is pretty weird. Can you use Canvas::UndoDeviceScaleFactor() and draw > in > > > pixel-space instead? > > > > That makes it harder to deal with the shadows, which are defined in DIP and > use > > integers rather than floats (so it wouldn't work well for dsf 1.5). Shadows > > aside, this part doesn't get much less weird when unscaling: > > > > gfx::RectF thumb_bounds(GetLocalBounds()); > > thumb_bounds.Inset(-GetShadowInsets()); > > thumb_bounds.Inset(gfx::InsetsF(0.5f)); > > // We want the circle to have an integer pixel diameter and to be aligned > > // with pixel boundaries, so we scale dip bounds to pixel bounds and > round. > > gfx::ScopedCanvas scoped(canvas); > > float dsf = canvas->UndoDeviceScaleFactor(); > > gfx::RectF bounds_in_px = thumb_bounds; > > bounds_in_px.Scale(dsf); > > bounds_in_px = gfx::RectF(gfx::ToEnclosingRect(bounds_in_px)); > > canvas->DrawCircle(bounds_in_px.CenterPoint(), > > bounds_in_px.height() / 2.f, thumb_paint); > > We avoid going pixel -> dip (and possibly avoid more rounding issues?). I think > that's still better. (You shouldn't need the ScopedCanvas either, since > View::Paint calls OnPaint with a scoped-canvas, so the undoDSF wouldn't affect > other views.) Looks like ShadowValue already has a Scale() method that you would > use. So dealing with shadows shouldn't be bad either? Done. https://codereview.chromium.org/2440723003/diff/120001/ui/gfx/geometry/insets.h File ui/gfx/geometry/insets.h (right): https://codereview.chromium.org/2440723003/diff/120001/ui/gfx/geometry/insets... ui/gfx/geometry/insets.h:98: Insets Offset(const gfx::Vector2d& vector) const; On 2016/10/29 00:45:53, danakj wrote: > Can you leave a comment explaining in more detail what it does too? And add a > unit test showing/verifying the behaviour of this method? Done. https://codereview.chromium.org/2440723003/diff/160001/ui/views/controls/butt... File ui/views/controls/button/toggle_button.cc (right): https://codereview.chromium.org/2440723003/diff/160001/ui/views/controls/butt... ui/views/controls/button/toggle_button.cc:177: // round up to pixel boundaries. added this improvement as well (visible with dsf=1.5)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping sadrul, danakj
gfx LGTM
lgtm
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org Link to the patchset: https://codereview.chromium.org/2440723003/#ps160001 (title: "remove a little more")
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 ========== Tweaks to ToggleButton details. - fix the color of the track and take more colors from the theme. The spec states 0.16a for the track whether off or on, but the color of the mock-up is actual 0.32a for the on version. I take this to be a bug in the text of the spec. - make the thumb the correct size. The actual white circle is supposed to be 15dip rather than 16dip (this is another thing confusing about the spec, as it states 16dp but where it measures from includes 1 dip of shadow). Note that to maintain crispness, we must align to pixel boundaries and must maintain a centered position, so there's a bit of rounding here depending on scale factor. BUG=626827 ========== to ========== Tweaks to ToggleButton details. - fix the color of the track and take more colors from the theme. The spec states 0.16a for the track whether off or on, but the color of the mock-up is actual 0.32a for the on version. I take this to be a bug in the text of the spec. - make the thumb the correct size. The actual white circle is supposed to be 15dip rather than 16dip (this is another thing confusing about the spec, as it states 16dp but where it measures from includes 1 dip of shadow). Note that to maintain crispness, we must align to pixel boundaries and must maintain a centered position, so there's a bit of rounding here depending on scale factor. BUG=626827 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Tweaks to ToggleButton details. - fix the color of the track and take more colors from the theme. The spec states 0.16a for the track whether off or on, but the color of the mock-up is actual 0.32a for the on version. I take this to be a bug in the text of the spec. - make the thumb the correct size. The actual white circle is supposed to be 15dip rather than 16dip (this is another thing confusing about the spec, as it states 16dp but where it measures from includes 1 dip of shadow). Note that to maintain crispness, we must align to pixel boundaries and must maintain a centered position, so there's a bit of rounding here depending on scale factor. BUG=626827 ========== to ========== Tweaks to ToggleButton details. - fix the color of the track and take more colors from the theme. The spec states 0.16a for the track whether off or on, but the color of the mock-up is actual 0.32a for the on version. I take this to be a bug in the text of the spec. - make the thumb the correct size. The actual white circle is supposed to be 15dip rather than 16dip (this is another thing confusing about the spec, as it states 16dp but where it measures from includes 1 dip of shadow). Note that to maintain crispness, we must align to pixel boundaries and must maintain a centered position, so there's a bit of rounding here depending on scale factor. BUG=626827 Committed: https://crrev.com/b58c6c3057e95b0d27097821743dcb7653e3dbaa Cr-Commit-Position: refs/heads/master@{#429682} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b58c6c3057e95b0d27097821743dcb7653e3dbaa Cr-Commit-Position: refs/heads/master@{#429682} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
