Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
736613002:
Makes Separator more configurable. (Closed)
DescriptionMakes Separator more configurable.
Height and color of Separator now can be specified during creation
R=sky@chromium.org
BUG=434395
Committed: https://crrev.com/e488e57d01b79c4fa937a1ba1e26f00b32427e97
Cr-Commit-Position: refs/heads/master@{#304924}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Setters for color/size instead of specifying in constructor #Patch Set 3 : Removed empty line between ctor and dtor #
Total comments: 5
Patch Set 4 : Schedule paint on SetColor and notify that preferred size has changed on size change #Patch Set 5 : Add missing space before { #Patch Set 6 : Use new SetSize to set separator size #Patch Set 7 : Width from layout #
Total comments: 1
Patch Set 8 : :%s/SetSize/SettPreferredSize #Patch Set 9 : Comment to Separator::SetPreferredSize #Patch Set 10 : fix typos #Patch Set 11 : SetPrefferedize and then SetSize of separator #
Total comments: 2
Patch Set 12 : REmoves changes from try_chrome_dialog_view.cc #Messages
Total messages: 30 (7 generated)
Why do you need this flexibility? Is there a crbug.com/ track this? Is it going to be used by some consumer (existing or new) in a follow up CL?
On 2014/11/18 15:44:04, tfarina wrote: > Why do you need this flexibility? Is there a crbug.com/ track this? > > Is it going to be used by some consumer (existing or new) in a follow up CL? Exactly, I need it in follow up CL. I have a use case where I want to use separator for drawing a lines between rows in ColumnSet.
If you make the BUG= section point to a bug describing why you need this you'll avoid questions like this in the future. This is why the template for uploading patches has BUG=. On Tue, Nov 18, 2014 at 7:49 AM, <melandory@chromium.org> wrote: > On 2014/11/18 15:44:04, tfarina wrote: >> >> Why do you need this flexibility? Is there a crbug.com/ track this? > > >> Is it going to be used by some consumer (existing or new) in a follow up >> CL? > > Exactly, I need it in follow up CL. I have a use case where I want to use > separator for drawing a lines between rows > in ColumnSet. > > https://codereview.chromium.org/736613002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/736613002/diff/1/ui/views/controls/separator.h File ui/views/controls/separator.h (right): https://codereview.chromium.org/736613002/diff/1/ui/views/controls/separator.... ui/views/controls/separator.h:28: Separator(Orientation orientation, SkColor color); We generally don't have constructors like this. At one point the style guide frowned upon this, not sure that it does anymore. Don't add any new constructors, instead add setters for the color and height. Also, it shouldn't be called height but rather size.
https://codereview.chromium.org/736613002/diff/1/ui/views/controls/separator.h File ui/views/controls/separator.h (right): https://codereview.chromium.org/736613002/diff/1/ui/views/controls/separator.... ui/views/controls/separator.h:28: Separator(Orientation orientation, SkColor color); On 2014/11/18 18:04:06, sky wrote: > We generally don't have constructors like this. At one point the style guide > frowned upon this, not sure that it does anymore. > > Don't add any new constructors, instead add setters for the color and height. > Also, it shouldn't be called height but rather size. Done.
https://codereview.chromium.org/736613002/diff/40001/ui/views/controls/separa... File ui/views/controls/separator.cc (right): https://codereview.chromium.org/736613002/diff/40001/ui/views/controls/separa... ui/views/controls/separator.cc:32: color_ = color; SchedulePaint. https://codereview.chromium.org/736613002/diff/40001/ui/views/controls/separa... ui/views/controls/separator.cc:36: size_ = size; If size != size_ this should call PreferredSizeChanged.
https://codereview.chromium.org/736613002/diff/40001/ui/views/controls/separa... File ui/views/controls/separator.cc (right): https://codereview.chromium.org/736613002/diff/40001/ui/views/controls/separa... ui/views/controls/separator.cc:32: color_ = color; On 2014/11/18 20:05:33, sky wrote: > SchedulePaint. Done. https://codereview.chromium.org/736613002/diff/40001/ui/views/controls/separa... ui/views/controls/separator.cc:36: size_ = size; On 2014/11/18 20:05:32, sky wrote: > If size != size_ this should call PreferredSizeChanged. Done. Should't SchedulePaint be called also here?
LGTM https://codereview.chromium.org/736613002/diff/40001/ui/views/controls/separa... File ui/views/controls/separator.cc (right): https://codereview.chromium.org/736613002/diff/40001/ui/views/controls/separa... ui/views/controls/separator.cc:36: size_ = size; On 2014/11/18 20:22:40, melandory wrote: > On 2014/11/18 20:05:32, sky wrote: > > If size != size_ this should call PreferredSizeChanged. > > Done. Should't SchedulePaint be called also here? No, you only care about once the bounds actually change, which is separate.
The CQ bit was checked by melandory@chromium.org
The CQ bit was unchecked by melandory@chromium.org
The CQ bit was checked by melandory@chromium.org
The CQ bit was unchecked by melandory@chromium.org
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736613002/80001
The CQ bit was unchecked by melandory@chromium.org
On 2014/11/18 22:05:26, sky wrote: > LGTM Can you look one more time, I've added changes after LGTM.
https://codereview.chromium.org/736613002/diff/120001/chrome/browser/first_ru... File chrome/browser/first_run/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/736613002/diff/120001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:272: separator->SetSize(preferred.width()); Separator::SetSize is much different than View::SetSize, which makes me think we should rename Separators to clear up confusion. How about SetPreferredSize with a clear comment that it's the preferred size of one axis.
On 2014/11/19 16:10:17, sky wrote: > https://codereview.chromium.org/736613002/diff/120001/chrome/browser/first_ru... > File chrome/browser/first_run/try_chrome_dialog_view.cc (right): > > https://codereview.chromium.org/736613002/diff/120001/chrome/browser/first_ru... > chrome/browser/first_run/try_chrome_dialog_view.cc:272: > separator->SetSize(preferred.width()); > Separator::SetSize is much different than View::SetSize, which makes me think we > should rename Separators to clear up confusion. How about SetPreferredSize with > a clear comment that it's the preferred size of one axis. Done
On 2014/11/19 16:10:17, sky wrote: > https://codereview.chromium.org/736613002/diff/120001/chrome/browser/first_ru... > File chrome/browser/first_run/try_chrome_dialog_view.cc (right): > > https://codereview.chromium.org/736613002/diff/120001/chrome/browser/first_ru... > chrome/browser/first_run/try_chrome_dialog_view.cc:272: > separator->SetSize(preferred.width()); > Separator::SetSize is much different than View::SetSize, which makes me think we > should rename Separators to clear up confusion. How about SetPreferredSize with > a clear comment that it's the preferred size of one axis. Done
LGTM https://codereview.chromium.org/736613002/diff/200001/ui/views/controls/separ... File ui/views/controls/separator.h (right): https://codereview.chromium.org/736613002/diff/200001/ui/views/controls/separ... ui/views/controls/separator.h:34: // Preferred size of one axis: height for horizontal separator How about: "Sets the preferred size of the mino axis: height when horizontal, and width when vertical."
Sorry, NO LGTM. No need to change first_run. https://codereview.chromium.org/736613002/diff/200001/chrome/browser/first_ru... File chrome/browser/first_run/try_chrome_dialog_view.cc (left): https://codereview.chromium.org/736613002/diff/200001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:273: separator->SetSize(gfx::Size(preferred.width(), separator_height)); You shouldn't change this code. Leave it as is.
or not lgtm
On 2014/11/19 20:30:19, sky wrote: > Sorry, NO LGTM. No need to change first_run. > > https://codereview.chromium.org/736613002/diff/200001/chrome/browser/first_ru... > File chrome/browser/first_run/try_chrome_dialog_view.cc (left): > > https://codereview.chromium.org/736613002/diff/200001/chrome/browser/first_ru... > chrome/browser/first_run/try_chrome_dialog_view.cc:273: > separator->SetSize(gfx::Size(preferred.width(), separator_height)); > You shouldn't change this code. Leave it as is. Done
On 2014/11/19 20:30:19, sky wrote: > Sorry, NO LGTM. No need to change first_run. > > https://codereview.chromium.org/736613002/diff/200001/chrome/browser/first_ru... > File chrome/browser/first_run/try_chrome_dialog_view.cc (left): > > https://codereview.chromium.org/736613002/diff/200001/chrome/browser/first_ru... > chrome/browser/first_run/try_chrome_dialog_view.cc:273: > separator->SetSize(gfx::Size(preferred.width(), separator_height)); > You shouldn't change this code. Leave it as is. Done
LGTM
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736613002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e488e57d01b79c4fa937a1ba1e26f00b32427e97 Cr-Commit-Position: refs/heads/master@{#304924} |
