Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2375843003:
views: add Harmony dialog width support (Closed)
|
Created:
4 years, 2 months ago by Elly Fong-Jones Modified:
4 years ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionviews: add Harmony dialog width support
This change:
1) Adds DialogDelegate::{set_,}dialog_width() to get/set the Harmony width for a dialog;
2) Adds an override of DialogDelegateView::GetPreferredSize() to honor the
Harmony width in secondary-ui-md mode;
3) Changes the collected cookies dialog to use the Harmony medium width.
BUG=635173
Patch Set 1 #
Total comments: 9
Patch Set 2 : round 1 #
Total comments: 8
Patch Set 3 : remove GetDialogWidth #
Total comments: 12
Patch Set 4 : move DialogWidth and such #Patch Set 5 : GetPreferredSize -> DialogClientView #
Total comments: 3
Messages
Total messages: 21 (3 generated)
Description was changed from ========== views: add Harmony dialog width support This change: 1) Adds DialogDelegate::GetDefaultDialogWidth() to return the desired Harmony width for a dialog; 2) Adds an override of DialogDelegateView::GetPreferredSize() to honor the Harmony width in secondary-ui-md mode; 3) Changes the collected cookies dialog to use the Harmony medium width. BUG=635173 ========== to ========== views: add Harmony dialog width support This change: 1) Adds DialogDelegate::GetDefaultDialogWidth() to return the desired Harmony width for a dialog; 2) Adds an override of DialogDelegateView::GetPreferredSize() to honor the Harmony width in secondary-ui-md mode; 3) Changes the collected cookies dialog to use the Harmony medium width. BUG=635173 ==========
ellyjones@chromium.org changed reviewers: + estade@chromium.org, sky@chromium.org
estade, sky: ptal? :)
https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:277: int height = View::GetPreferredSize().height(); you should get the width in the switch statement below, then apply GetHeightForWidth https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:285: default: return View::GetPreferredSize(); nit: better not to have a default --- explicitly list all enum values. That way when you add a new one, the compiler will complain about an unhandled value. https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:285: default: return View::GetPreferredSize(); don't we want to enforce small/medium/large? If we support anything outside this set of three sizes, it should be the exception rather than the default. https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.h:100: virtual DialogWidth GetDefaultDialogWidth() const; nit: I don't think you need "Default" in this fn name.
estade: ptal? :) https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:277: int height = View::GetPreferredSize().height(); On 2016/09/28 16:14:37, Evan Stade wrote: > you should get the width in the switch statement below, then apply > GetHeightForWidth Done. https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:285: default: return View::GetPreferredSize(); On 2016/09/28 16:14:37, Evan Stade wrote: > don't we want to enforce small/medium/large? If we support anything outside this > set of three sizes, it should be the exception rather than the default. We do, but we can't right now, until every DialogDelegate has a GetDefaultDialogWidth that returns a useful value. I'll add a TODO to remove the WIDTH_DEFAULT case, though. https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:285: default: return View::GetPreferredSize(); On 2016/09/28 16:14:37, Evan Stade wrote: > nit: better not to have a default --- explicitly list all enum values. That way > when you add a new one, the compiler will complain about an unhandled value. Done. https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.h:100: virtual DialogWidth GetDefaultDialogWidth() const; On 2016/09/28 16:14:37, Evan Stade wrote: > nit: I don't think you need "Default" in this fn name. Done.
https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2375843003/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.cc:285: default: return View::GetPreferredSize(); On 2016/09/28 18:44:09, Elly Fong-Jones wrote: > On 2016/09/28 16:14:37, Evan Stade wrote: > > nit: better not to have a default --- explicitly list all enum values. That > way > > when you add a new one, the compiler will complain about an unhandled value. > > Done. if every dialogdelegate is going to need to explicitly set this (rather than defaulting to WIDTH_LARGE or something) we should probably not make it an overrideable method, because that's a very verbose way to specify the width. It would be less verbose to pass a param to the ctor or to have a setter (a la set_margins, set_anchor_view_insets, etc.) https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:277: DialogDelegate::DialogWidth width = DialogDelegate::DIALOG_WIDTH_DEFAULT; maybe cleaner (especially when it comes time to remove the old way of doing things): if (!ui::MaterialDesignController::IsSecondaryUiMaterial() || width_factor_ == DIALOG_WIDTH_UNSPECIFIED) return View::GetPreferredSize(); // Harmony stuff https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:284: case DialogDelegate::DIALOG_WIDTH_DEFAULT: nit: maybe DIALOG_WIDTH_UNSPECIFIED? https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:287: case DialogDelegate::DIALOG_WIDTH_SMALL: perhaps the enum should be: enum WidthFactor { DIALOG_WIDTH_SMALL = 10, DIALOG_WIDTH_MEDIUM = 14, DIALOG_WIDTH_LARGE = 16, } then this code is pretty simple: int width_px = 32 * width_factor_; https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:290: case DialogDelegate::DIALOG_WIDTH_MEDIUM: nit: don't need to qualify these constants with DialogDelegate::
Description was changed from
==========
views: add Harmony dialog width support
This change:
1) Adds DialogDelegate::GetDefaultDialogWidth() to return the desired Harmony
width for a dialog;
2) Adds an override of DialogDelegateView::GetPreferredSize() to honor the
Harmony width in secondary-ui-md mode;
3) Changes the collected cookies dialog to use the Harmony medium width.
BUG=635173
==========
to
==========
views: add Harmony dialog width support
This change:
1) Adds DialogDelegate::{set_,}dialog_width() to get/set the Harmony width for a
dialog;
2) Adds an override of DialogDelegateView::GetPreferredSize() to honor the
Harmony width in secondary-ui-md mode;
3) Changes the collected cookies dialog to use the Harmony medium width.
BUG=635173
==========
estade: ptal? :) https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:277: DialogDelegate::DialogWidth width = DialogDelegate::DIALOG_WIDTH_DEFAULT; On 2016/09/28 21:12:38, Evan Stade wrote: > maybe cleaner (especially when it comes time to remove the old way of doing > things): > > if (!ui::MaterialDesignController::IsSecondaryUiMaterial() || width_factor_ == > DIALOG_WIDTH_UNSPECIFIED) > return View::GetPreferredSize(); > > // Harmony stuff Done. https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:284: case DialogDelegate::DIALOG_WIDTH_DEFAULT: On 2016/09/28 21:12:38, Evan Stade wrote: > nit: maybe DIALOG_WIDTH_UNSPECIFIED? Done. https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:287: case DialogDelegate::DIALOG_WIDTH_SMALL: On 2016/09/28 21:12:38, Evan Stade wrote: > perhaps the enum should be: > > enum WidthFactor { > DIALOG_WIDTH_SMALL = 10, > DIALOG_WIDTH_MEDIUM = 14, > DIALOG_WIDTH_LARGE = 16, > } > > then this code is pretty simple: > > int width_px = 32 * width_factor_; Oh, that's really clever! Done. https://codereview.chromium.org/2375843003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:290: case DialogDelegate::DIALOG_WIDTH_MEDIUM: On 2016/09/28 21:12:38, Evan Stade wrote: > nit: don't need to qualify these constants with DialogDelegate:: Done.
lgtm https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:274: int width_px = dialog_width() * 32; actually dp, not px. I'd suggest leaving the unit off as GetPreferredSize always expects the same thing (dp). https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:34: // These values are in the 32px units the Harmony spec uses. nit: I don't really think we need to mention "harmony" or "spec" in comments that are meant to persist. (When "harmony" is default, it will be implicit.) I'd just as soon leave this comment off completely. https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:36: enum DialogWidth { protected? https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:128: DialogWidth dialog_width() const { return dialog_width_; } this getter doesn't seem necessary https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:136: // The Harmony width to use for this dialog. s/Harmony //
https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:36: enum DialogWidth { enum class and remove DIALOG_WIDTH from each name? Also, it seems unusual to have these constants defined here, but used only by DialogDelegateView. Should the constants move to DialogDelegateView?
https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:36: enum DialogWidth { On 2016/10/03 21:24:02, sky wrote: > enum class and remove DIALOG_WIDTH from each name? Also, it seems unusual to > have these constants defined here, but used only by DialogDelegateView. Should > the constants move to DialogDelegateView? ahhhh, I forgot about this distinction. We do have some dialogs which are DialogDelegates but *not* DialogDelegateViews, and we probably want them to also be sized accordingly. This makes me think we need to move this logic to DialogClientView. DialogClientView sizes the dialog[1] but how it does so should change. DialogDelegate probably should get a new function DialogWidth GetWidthFactor() and then DCV can use that to size the contents. This will cover both DialogDelegateView and any other DialogDelegate implementor. [1] https://cs.chromium.org/chromium/src/ui/views/window/dialog_client_view.cc?rc...
ok, I moved the sizing logic to DialogClientView and the width factor to DialogDelegate. estade, sky: ptal? :) https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:274: int width_px = dialog_width() * 32; On 2016/10/03 19:04:19, Evan Stade wrote: > actually dp, not px. I'd suggest leaving the unit off as GetPreferredSize > always expects the same thing (dp). Done. https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:34: // These values are in the 32px units the Harmony spec uses. On 2016/10/03 19:04:19, Evan Stade wrote: > nit: I don't really think we need to mention "harmony" or "spec" in comments > that are meant to persist. (When "harmony" is default, it will be implicit.) I'd > just as soon leave this comment off completely. But even after we're done, I think people may want to figure out what these values are / where they came from, and the spec will still be named Harmony then, so it seems better to reference it by name here. https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:36: enum DialogWidth { On 2016/10/04 00:13:46, Evan Stade wrote: > On 2016/10/03 21:24:02, sky wrote: > > enum class and remove DIALOG_WIDTH from each name? Also, it seems unusual to > > have these constants defined here, but used only by DialogDelegateView. Should > > the constants move to DialogDelegateView? > > ahhhh, I forgot about this distinction. We do have some dialogs which are > DialogDelegates but *not* DialogDelegateViews, and we probably want them to also > be sized accordingly. This makes me think we need to move this logic to > DialogClientView. > > DialogClientView sizes the dialog[1] but how it does so should change. > DialogDelegate probably should get a new function DialogWidth GetWidthFactor() > and then DCV can use that to size the contents. This will cover both > DialogDelegateView and any other DialogDelegate implementor. > > [1] > https://cs.chromium.org/chromium/src/ui/views/window/dialog_client_view.cc?rc... I don't understand this suggestion - aren't many dialogs not DialogClientViews? like the collected cookies dialog, for example. I did enum class and removed the DIALOG_WIDTH prefixes, though. https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:128: DialogWidth dialog_width() const { return dialog_width_; } On 2016/10/03 19:04:19, Evan Stade wrote: > this getter doesn't seem necessary It was only necessary because the member was private, not protected, and I needed it in DialogDelegateView. It can go now :)
https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2375843003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:36: enum DialogWidth { On 2016/10/05 13:20:31, Elly Fong-Jones wrote: > On 2016/10/04 00:13:46, Evan Stade wrote: > > On 2016/10/03 21:24:02, sky wrote: > > > enum class and remove DIALOG_WIDTH from each name? Also, it seems unusual to > > > have these constants defined here, but used only by DialogDelegateView. > Should > > > the constants move to DialogDelegateView? > > > > ahhhh, I forgot about this distinction. We do have some dialogs which are > > DialogDelegates but *not* DialogDelegateViews, and we probably want them to > also > > be sized accordingly. This makes me think we need to move this logic to > > DialogClientView. > > > > DialogClientView sizes the dialog[1] but how it does so should change. > > DialogDelegate probably should get a new function DialogWidth GetWidthFactor() > > and then DCV can use that to size the contents. This will cover both > > DialogDelegateView and any other DialogDelegate implementor. > > > > [1] > > > https://cs.chromium.org/chromium/src/ui/views/window/dialog_client_view.cc?rc... > > I don't understand this suggestion - aren't many dialogs not DialogClientViews? > like the collected cookies dialog, for example. > > I did enum class and removed the DIALOG_WIDTH prefixes, though. I assume you meant to delete this comment https://codereview.chromium.org/2375843003/diff/80001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2375843003/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:180: size = gfx::Size(width_factor * 32, size.height()); you still need to use GetHeightForWidth gfx::Size contents_size; if (width_factor != 0) { int width = width_factor * 32; contents_size = gfx::Size(width, contents_view()->GetHeightForWidth(width); } else { contents_size = contents_view()->GetPreferredSize(); } https://codereview.chromium.org/2375843003/diff/80001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2375843003/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:118: int GetWidthFactor() const; I would expect this to either return an int that is the width in px (factor * 32) or an instance of DialogWidth https://codereview.chromium.org/2375843003/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_delegate.h:124: // These values are in the 32px units the Harmony spec uses. a/units/increments
How common is it going to be to set a width based on the harmony constants? I would rather we add a SetMinimizeSize(gfx::size) to DialogClientView and not bake these values into views.
On 2016/10/06 16:13:54, sky wrote: > How common is it going to be to set a width based on the harmony constants? I > would rather we add a SetMinimizeSize(gfx::size) to DialogClientView and not > bake these values into views. The plan is for every dialog to use them.
Could this knowledge move to chrome? As mentioned a SetMinimizeSize(gfx::Size) on DialogClientView seems reasonable, but I would prefer if the harmony constant could live in chrome. -Scott On Thu, Oct 6, 2016 at 9:16 AM, <ellyjones@chromium.org> wrote: > On 2016/10/06 16:13:54, sky wrote: >> How common is it going to be to set a width based on the harmony >> constants? I >> would rather we add a SetMinimizeSize(gfx::size) to DialogClientView and >> not >> bake these values into views. > > The plan is for every dialog to use them. > > https://codereview.chromium.org/2375843003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/06 21:15:51, sky wrote: > Could this knowledge move to chrome? As mentioned a > SetMinimizeSize(gfx::Size) on DialogClientView seems reasonable, but I > would prefer if the harmony constant could live in chrome. > > -Scott > > On Thu, Oct 6, 2016 at 9:16 AM, <mailto:ellyjones@chromium.org> wrote: > > On 2016/10/06 16:13:54, sky wrote: > >> How common is it going to be to set a width based on the harmony > >> constants? I > >> would rather we add a SetMinimizeSize(gfx::size) to DialogClientView and > >> not > >> bake these values into views. > > > > The plan is for every dialog to use them. > > > > https://codereview.chromium.org/2375843003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > I don't think it would be very easy to use DialogClientView::SetMinimumSize because you can only call GetDialogClientView after creating the widget. I guess I don't see that as conceptually different from the other parts of DialogDelegate which allow you to specify details about the appearance of a dialog, or the other parts of Harmony which live in ui/views (i.e. controls).
Aren't most delegates a DialogDelegateView, so that they could do this->SetMinimumSize? I agree the distinction as to what we end up putting in ui/views and not is hazy. Given that ui/views doesn't really need the size in anyway it seems best to separate the logic as much as possible. Arguably we could have done that with other md effects (ink drop and focus ring come to mind) and would result in making ui/views more general and less tied to the current ui. -Scott On Thu, Oct 6, 2016 at 2:25 PM, <estade@chromium.org> wrote: > On 2016/10/06 21:15:51, sky wrote: >> Could this knowledge move to chrome? As mentioned a >> SetMinimizeSize(gfx::Size) on DialogClientView seems reasonable, but I >> would prefer if the harmony constant could live in chrome. >> >> -Scott >> >> On Thu, Oct 6, 2016 at 9:16 AM, <mailto:ellyjones@chromium.org> wrote: >> > On 2016/10/06 16:13:54, sky wrote: >> >> How common is it going to be to set a width based on the harmony >> >> constants? I >> >> would rather we add a SetMinimizeSize(gfx::size) to DialogClientView >> >> and >> >> not >> >> bake these values into views. >> > >> > The plan is for every dialog to use them. >> > >> > https://codereview.chromium.org/2375843003/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > I don't think it would be very easy to use DialogClientView::SetMinimumSize > because you can only call GetDialogClientView after creating the widget. I > guess > I don't see that as conceptually different from the other parts of > DialogDelegate which allow you to specify details about the appearance of a > dialog, or the other parts of Harmony which live in ui/views (i.e. > controls). > > https://codereview.chromium.org/2375843003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
FYI I'm holding off on updating this CL until we resolve the mailing list thread about where this kind of extension should go. On 2016/10/06 22:32:17, sky wrote: > Aren't most delegates a DialogDelegateView, so that they could do > this->SetMinimumSize? > > I agree the distinction as to what we end up putting in ui/views and > not is hazy. Given that ui/views doesn't really need the size in > anyway it seems best to separate the logic as much as possible. > Arguably we could have done that with other md effects (ink drop and > focus ring come to mind) and would result in making ui/views more > general and less tied to the current ui. > > -Scott > > On Thu, Oct 6, 2016 at 2:25 PM, <mailto:estade@chromium.org> wrote: > > On 2016/10/06 21:15:51, sky wrote: > >> Could this knowledge move to chrome? As mentioned a > >> SetMinimizeSize(gfx::Size) on DialogClientView seems reasonable, but I > >> would prefer if the harmony constant could live in chrome. > >> > >> -Scott > >> > >> On Thu, Oct 6, 2016 at 9:16 AM, <mailto:ellyjones@chromium.org> wrote: > >> > On 2016/10/06 16:13:54, sky wrote: > >> >> How common is it going to be to set a width based on the harmony > >> >> constants? I > >> >> would rather we add a SetMinimizeSize(gfx::size) to DialogClientView > >> >> and > >> >> not > >> >> bake these values into views. > >> > > >> > The plan is for every dialog to use them. > >> > > >> > https://codereview.chromium.org/2375843003/ > >> > >> -- > >> You received this message because you are subscribed to the Google Groups > >> "Chromium-reviews" group. > >> To unsubscribe from this group and stop receiving emails from it, send an > > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > I don't think it would be very easy to use DialogClientView::SetMinimumSize > > because you can only call GetDialogClientView after creating the widget. I > > guess > > I don't see that as conceptually different from the other parts of > > DialogDelegate which allow you to specify details about the appearance of a > > dialog, or the other parts of Harmony which live in ui/views (i.e. > > controls). > > > > https://codereview.chromium.org/2375843003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
On 2016/10/06 22:32:17, sky wrote: > Aren't most delegates a DialogDelegateView, so that they could do > this->SetMinimumSize? most, but far from all. The contents of a dialog are sized by DialogClientView. I don't think we can avoid modifying that code. I don't believe View::GetMinimumSize is used in this case, and it seems unlikely to be exactly what we want anyway, which is a hardcoded width and an expandable height. > > I agree the distinction as to what we end up putting in ui/views and > not is hazy. Given that ui/views doesn't really need the size in > anyway it seems best to separate the logic as much as possible. > Arguably we could have done that with other md effects (ink drop and > focus ring come to mind) and would result in making ui/views more > general and less tied to the current ui. > > -Scott > > On Thu, Oct 6, 2016 at 2:25 PM, <mailto:estade@chromium.org> wrote: > > On 2016/10/06 21:15:51, sky wrote: > >> Could this knowledge move to chrome? As mentioned a > >> SetMinimizeSize(gfx::Size) on DialogClientView seems reasonable, but I > >> would prefer if the harmony constant could live in chrome. > >> > >> -Scott > >> > >> On Thu, Oct 6, 2016 at 9:16 AM, <mailto:ellyjones@chromium.org> wrote: > >> > On 2016/10/06 16:13:54, sky wrote: > >> >> How common is it going to be to set a width based on the harmony > >> >> constants? I > >> >> would rather we add a SetMinimizeSize(gfx::size) to DialogClientView > >> >> and > >> >> not > >> >> bake these values into views. > >> > > >> > The plan is for every dialog to use them. > >> > > >> > https://codereview.chromium.org/2375843003/ > >> > >> -- > >> You received this message because you are subscribed to the Google Groups > >> "Chromium-reviews" group. > >> To unsubscribe from this group and stop receiving emails from it, send an > > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > I don't think it would be very easy to use DialogClientView::SetMinimumSize > > because you can only call GetDialogClientView after creating the widget. I > > guess > > I don't see that as conceptually different from the other parts of > > DialogDelegate which allow you to specify details about the appearance of a > > dialog, or the other parts of Harmony which live in ui/views (i.e. > > controls). > > > > https://codereview.chromium.org/2375843003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
Can we put the SetMinimumSize on DialogClientView? On Mon, Oct 10, 2016 at 10:24 AM, <estade@chromium.org> wrote: > On 2016/10/06 22:32:17, sky wrote: >> Aren't most delegates a DialogDelegateView, so that they could do >> this->SetMinimumSize? > > most, but far from all. > > The contents of a dialog are sized by DialogClientView. I don't think we can > avoid modifying that code. I don't believe View::GetMinimumSize is used in > this > case, and it seems unlikely to be exactly what we want anyway, which is a > hardcoded width and an expandable height. > > >> >> I agree the distinction as to what we end up putting in ui/views and >> not is hazy. Given that ui/views doesn't really need the size in >> anyway it seems best to separate the logic as much as possible. >> Arguably we could have done that with other md effects (ink drop and >> focus ring come to mind) and would result in making ui/views more >> general and less tied to the current ui. >> >> -Scott >> >> On Thu, Oct 6, 2016 at 2:25 PM, <mailto:estade@chromium.org> wrote: >> > On 2016/10/06 21:15:51, sky wrote: >> >> Could this knowledge move to chrome? As mentioned a >> >> SetMinimizeSize(gfx::Size) on DialogClientView seems reasonable, but I >> >> would prefer if the harmony constant could live in chrome. >> >> >> >> -Scott >> >> >> >> On Thu, Oct 6, 2016 at 9:16 AM, <mailto:ellyjones@chromium.org> wrote: >> >> > On 2016/10/06 16:13:54, sky wrote: >> >> >> How common is it going to be to set a width based on the harmony >> >> >> constants? I >> >> >> would rather we add a SetMinimizeSize(gfx::size) to DialogClientView >> >> >> and >> >> >> not >> >> >> bake these values into views. >> >> > >> >> > The plan is for every dialog to use them. >> >> > >> >> > https://codereview.chromium.org/2375843003/ >> >> >> >> -- >> >> You received this message because you are subscribed to the Google >> >> Groups >> >> "Chromium-reviews" group. >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> > >> > I don't think it would be very easy to use >> > DialogClientView::SetMinimumSize >> > because you can only call GetDialogClientView after creating the widget. >> > I >> > guess >> > I don't see that as conceptually different from the other parts of >> > DialogDelegate which allow you to specify details about the appearance >> > of a >> > dialog, or the other parts of Harmony which live in ui/views (i.e. >> > controls). >> > >> > https://codereview.chromium.org/2375843003/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/2375843003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
