Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750063002:
views: implement dialog width snapping (Closed)
|
Created:
3 years, 9 months ago by Elly Fong-Jones Modified:
3 years, 8 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionviews: implement dialog width snapping
Harmony calls for dialogs to grow in units of 16 points. This
change:
1) Adds a DIALOG_WIDTH_SNAPPING_UNIT distance metric to
ViewsDelegate;
2) Implements width snapping in DialogClientView
3) Plumbs DIALOG_WIDTH_SNAPPING_UNIT down from LayoutDelegate
in ChromeViewsDelegate.
BUG=635173
Patch Set 1 #
Total comments: 7
Patch Set 2 : move logic to BubbleFrameView #Patch Set 3 : only snap to harmony units #Patch Set 4 : remove stray logging #
Total comments: 29
Patch Set 5 : width -> min_width and improve tests #
Total comments: 11
Messages
Total messages: 29 (7 generated)
Description was changed from ========== views: implement dialog width snapping Harmony calls for dialogs to grow in units of 16 points. This change: 1) Adds a DIALOG_WIDTH_SNAPPING_UNIT distance metric to ViewsDelegate; 2) Implements width snapping in DialogClientView 3) Plumbs DIALOG_WIDTH_SNAPPING_UNIT down from LayoutDelegate in ChromeViewsDelegate. BUG=635173 ========== to ========== views: implement dialog width snapping Harmony calls for dialogs to grow in units of 16 points. This change: 1) Adds a DIALOG_WIDTH_SNAPPING_UNIT distance metric to ViewsDelegate; 2) Implements width snapping in DialogClientView 3) Plumbs DIALOG_WIDTH_SNAPPING_UNIT down from LayoutDelegate in ChromeViewsDelegate. BUG=635173 ==========
ellyjones@chromium.org changed reviewers: + tapted@chromium.org
tapted: ptal? I am not that pleased with this design because I can't figure out how to test it. I can't make a DialogClientView unit test, because I need the ChromeViewsDelegate installed in order to have a snapping unit > 1. I'm considering making a test-specific ViewsDelegate subclass, but I have no idea how to actually install a new ViewsDelegate. What do you think?
On 2017/03/15 15:37:38, Elly Fong-Jones wrote: > tapted: ptal? > > I am not that pleased with this design because I can't figure out how to test > it. I can't make a DialogClientView unit test, because I need the > ChromeViewsDelegate installed in order to have a snapping unit > 1. I'm > considering making a test-specific ViewsDelegate subclass, but I have no idea > how to actually install a new ViewsDelegate. > > What do you think? yah, it needs a test. You should be able to get the DialogClientViewTest constructor to call set_views_delegate(MakeUnique<DialogTestViewsDelegate>()) (where you declare a DialogTestViewsDelegate that inherits from views::TestViewsDelegate and overrides the metrics you want to test) https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:46: size.Enlarge(unit - 1, 0); I find it a bit weird to manipulate the size in all cases.. E.g. you could do something like if (size.width() % unit == 0) return size; size.set_width(size.width() + unit - size.width() % unit); return size; https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:53: gfx::Size GetBoundingSizeForVerticalStack(const gfx::Size& size1, rename -> GetSnappedBoundingSize? https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:59: views::DistanceMetric::DIALOG_WIDTH_SNAPPING_UNIT)); This is the ClientView - I think we need to take into account the NonClientView's margins as well. (i.e. the parameter passed as |content_margins| to BubbleFrameView's constructor at BubbleDialogDelegateView::CreateNonClientFrameView https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:143: return GetBoundingSizeForVerticalStack( Do we also need to consider the 3 "preferred" dialog sizes here? i.e. LayoutDelegate::DialogWidth::{SMALL, MEDIUM, LARGE} I'd imagined that under harmony a dialog could just say "0" for its preferred size to automatically get `SMALL` (or something bigger if its buttons happen to be really wide). Or SMALL+delta if that's too narrow. If we are planning for that, the logic here might need rethink.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:46: size.Enlarge(unit - 1, 0); On 2017/03/15 22:26:11, tapted wrote: > I find it a bit weird to manipulate the size in all cases.. E.g. you could do > something like > > if (size.width() % unit == 0) > return size; > > size.set_width(size.width() + unit - size.width() % unit); > return size; FWIW, always manipulating reads OK to me, but in my mind the idiomatic way to do this is more like: size.set_width(((size.width() + unit - 1) / unit) * unit); ...which is also shorter. https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:52: // atop the other. I wonder if this function is the wrong place to implement this snapping. Basically, I read the name and comment as simply saying this returns the minimum bounding box to contain these sizes, without regard to how caller code uses them. It's the caller who is using this as "the complete size of the dialog". Accordingly, I would compose this functionality: leave this function alone, and have a separate function which takes a size and snaps it to the correct dialog size. This will help maintenance in the future if e.g. Bret changes icon/title/close buttons to be rendered by a "horizontal row" much like the buttons at the bottom already are. Then we'll want to get the bounding box of three different stacked sizes, and round up the whole thing, which could plausibly be done with two calls to this existing method, plus one to the new method. https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:143: return GetBoundingSizeForVerticalStack( On 2017/03/15 22:26:11, tapted wrote: > Do we also need to consider the 3 "preferred" dialog sizes here? > > i.e. LayoutDelegate::DialogWidth::{SMALL, MEDIUM, LARGE} > > I'd imagined that under harmony a dialog could just say "0" for its preferred > size to automatically get `SMALL` (or something bigger if its buttons happen to > be really wide). Or SMALL+delta if that's too narrow. > > If we are planning for that, the logic here might need rethink. I'm wondering if you're asking about the same thing I'm wondering about, which is that the snapping rule is supposed to be: * Anything smaller than SMALL rounds up to SMALL * Anything bigger than SMALL but smaller than MEDIUM rounds up to MEDIUM * Anything bigger than MEDIUM but smaller than LARGE rounds up to LARGE * Anything bigger than LARGE rounds up to a multiple of the snapping unit Is implementing the first three bullets, and restricting the last to the "bigger than LARGE" case, intended to happen in a separate CL, or this one? It seems like it might be best for it to be this one. At that point, we can hopefully remove references to SMALL/MEDIUM/LARGE from specific dialogs, but they might still need them if for some reason we want to force a dialog to have much more space than it prefers.
On 2017/03/21 19:17:03, Peter Kasting wrote: > https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... > File ui/views/window/dialog_client_view.cc (right): > > https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... > ui/views/window/dialog_client_view.cc:46: size.Enlarge(unit - 1, 0); > On 2017/03/15 22:26:11, tapted wrote: > > I find it a bit weird to manipulate the size in all cases.. E.g. you could do > > something like > > > > if (size.width() % unit == 0) > > return size; > > > > size.set_width(size.width() + unit - size.width() % unit); > > return size; > > FWIW, always manipulating reads OK to me, but in my mind the idiomatic way to do > this is more like: > > size.set_width(((size.width() + unit - 1) / unit) * unit); > > ...which is also shorter. > > https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... > ui/views/window/dialog_client_view.cc:52: // atop the other. > I wonder if this function is the wrong place to implement this snapping. > > Basically, I read the name and comment as simply saying this returns the minimum > bounding box to contain these sizes, without regard to how caller code uses > them. It's the caller who is using this as "the complete size of the dialog". > Accordingly, I would compose this functionality: leave this function alone, and > have a separate function which takes a size and snaps it to the correct dialog > size. > > This will help maintenance in the future if e.g. Bret changes icon/title/close > buttons to be rendered by a "horizontal row" much like the buttons at the bottom > already are. Then we'll want to get the bounding box of three different stacked > sizes, and round up the whole thing, which could plausibly be done with two > calls to this existing method, plus one to the new method. > > https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... > ui/views/window/dialog_client_view.cc:143: return > GetBoundingSizeForVerticalStack( > On 2017/03/15 22:26:11, tapted wrote: > > Do we also need to consider the 3 "preferred" dialog sizes here? > > > > i.e. LayoutDelegate::DialogWidth::{SMALL, MEDIUM, LARGE} > > > > I'd imagined that under harmony a dialog could just say "0" for its preferred > > size to automatically get `SMALL` (or something bigger if its buttons happen > to > > be really wide). Or SMALL+delta if that's too narrow. > > > > If we are planning for that, the logic here might need rethink. > > I'm wondering if you're asking about the same thing I'm wondering about, which > is that the snapping rule is supposed to be: > > * Anything smaller than SMALL rounds up to SMALL > * Anything bigger than SMALL but smaller than MEDIUM rounds up to MEDIUM > * Anything bigger than MEDIUM but smaller than LARGE rounds up to LARGE > * Anything bigger than LARGE rounds up to a multiple of the snapping unit Hm, it seems like I was implementing the wrong thing, then. BubbleFrameView is at least the right place for the logic to live :) but I'll put another version of this up shortly. > Is implementing the first three bullets, and restricting the last to the "bigger > than LARGE" case, intended to happen in a separate CL, or this one? It seems > like it might be best for it to be this one. > > At that point, we can hopefully remove references to SMALL/MEDIUM/LARGE from > specific dialogs, but they might still need them if for some reason we want to > force a dialog to have much more space than it prefers. I don't know if we will or not, but it would be nice to get rid of them.
On 2017/03/24 17:47:26, Elly Fong-Jones wrote: > On 2017/03/21 19:17:03, Peter Kasting wrote: > > > https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... > > File ui/views/window/dialog_client_view.cc (right): > > > > > https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... > > ui/views/window/dialog_client_view.cc:46: size.Enlarge(unit - 1, 0); > > On 2017/03/15 22:26:11, tapted wrote: > > > I find it a bit weird to manipulate the size in all cases.. E.g. you could > do > > > something like > > > > > > if (size.width() % unit == 0) > > > return size; > > > > > > size.set_width(size.width() + unit - size.width() % unit); > > > return size; > > > > FWIW, always manipulating reads OK to me, but in my mind the idiomatic way to > do > > this is more like: > > > > size.set_width(((size.width() + unit - 1) / unit) * unit); > > > > ...which is also shorter. > > > > > https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... > > ui/views/window/dialog_client_view.cc:52: // atop the other. > > I wonder if this function is the wrong place to implement this snapping. > > > > Basically, I read the name and comment as simply saying this returns the > minimum > > bounding box to contain these sizes, without regard to how caller code uses > > them. It's the caller who is using this as "the complete size of the dialog". > > > Accordingly, I would compose this functionality: leave this function alone, > and > > have a separate function which takes a size and snaps it to the correct dialog > > size. > > > > This will help maintenance in the future if e.g. Bret changes icon/title/close > > buttons to be rendered by a "horizontal row" much like the buttons at the > bottom > > already are. Then we'll want to get the bounding box of three different > stacked > > sizes, and round up the whole thing, which could plausibly be done with two > > calls to this existing method, plus one to the new method. > > > > > https://codereview.chromium.org/2750063002/diff/1/ui/views/window/dialog_clie... > > ui/views/window/dialog_client_view.cc:143: return > > GetBoundingSizeForVerticalStack( > > On 2017/03/15 22:26:11, tapted wrote: > > > Do we also need to consider the 3 "preferred" dialog sizes here? > > > > > > i.e. LayoutDelegate::DialogWidth::{SMALL, MEDIUM, LARGE} > > > > > > I'd imagined that under harmony a dialog could just say "0" for its > preferred > > > size to automatically get `SMALL` (or something bigger if its buttons happen > > to > > > be really wide). Or SMALL+delta if that's too narrow. > > > > > > If we are planning for that, the logic here might need rethink. > > > > I'm wondering if you're asking about the same thing I'm wondering about, which > > is that the snapping rule is supposed to be: > > > > * Anything smaller than SMALL rounds up to SMALL > > * Anything bigger than SMALL but smaller than MEDIUM rounds up to MEDIUM > > * Anything bigger than MEDIUM but smaller than LARGE rounds up to LARGE > > * Anything bigger than LARGE rounds up to a multiple of the snapping unit > > Hm, it seems like I was implementing the wrong thing, then. BubbleFrameView is > at least the right place for the logic to live :) but I'll put another version > of this up shortly. > > > Is implementing the first three bullets, and restricting the last to the > "bigger > > than LARGE" case, intended to happen in a separate CL, or this one? It seems > > like it might be best for it to be this one. > > > > At that point, we can hopefully remove references to SMALL/MEDIUM/LARGE from > > specific dialogs, but they might still need them if for some reason we want to > > force a dialog to have much more space than it prefers. > > I don't know if we will or not, but it would be nice to get rid of them. I implemented this behavior in patchset 3. The results are... hm. For example, the bookmark bubble's default width is 335pt, which means we end up snapping it up to 448pt and it is quite wide. I am not really sure what to do about that. pkasting@: thoughts?
On 2017/03/27 17:37:25, Elly Fong-Jones wrote: > I implemented this behavior in patchset 3. The results are... hm. For example, > the bookmark bubble's default width is 335pt, which means we end up snapping it > up to 448pt and it is quite wide. I am not really sure what to do about that. What's making the bookmark bubble want to be 335 px by default if on the mock it's 320? It seems like we must be setting the wrong preferred size of something somewhere in it.
On 2017/03/27 20:17:22, Peter Kasting wrote: > On 2017/03/27 17:37:25, Elly Fong-Jones wrote: > > I implemented this behavior in patchset 3. The results are... hm. For example, > > the bookmark bubble's default width is 335pt, which means we end up snapping > it > > up to 448pt and it is quite wide. I am not really sure what to do about that. > > What's making the bookmark bubble want to be 335 px by default if on the mock > it's 320? It seems like we must be setting the wrong preferred size of > something somewhere in it. I did some investigation, and it turns out that this bubble is quite literally a pessimal layout for us. The bottom row of the GridLayout ends up being like this: [ space ] [ padding0 ] [ Remove ] [ padding1 ] [ Edit ] [ padding2 ] [ Close ] And the respective widths are: space = 40 pts (forced by being the same width as the "Folder:" label in the row above) padding0 = unrelated control horizontal spacing = 12 (pre-Harmony), 16 (post-Harmony) Remove = 81 pts (width needed to hold the text) padding1 = unrelated control large spacing = 20 (pre-Harmony), 16 (post-Harmony) Edit = 66 pts (width needed to hold the text) padding2 = related button horizontal spacing = 6 (pre-Harmony), 8 (post-Harmony) Close = 64 pts (width needed to hold the text) At the moment, this code uses non-Harmony constants for the padding widths, which is something to be fixed, but it does not much affect the issue, which is that the widths given above sum up to 40 + 12 + 81 + 20 + 66 + 6 + 64 = 289 pts, and then when you add the 16pt margin on either side of a bubble's content view, you get 321pts, or exactly 1pt wider than the 320pt Harmony size. :( If you swap the padding widths over to the Harmony values, you get 40 + 16 + 81 + 16 + 66 + 8 + 64 = 291 + 32 = 323.
On 2017/03/28 15:43:52, Elly Fong-Jones wrote: > On 2017/03/27 20:17:22, Peter Kasting wrote: > > On 2017/03/27 17:37:25, Elly Fong-Jones wrote: > > > I implemented this behavior in patchset 3. The results are... hm. For > example, > > > the bookmark bubble's default width is 335pt, which means we end up snapping > > it > > > up to 448pt and it is quite wide. I am not really sure what to do about > that. > > > > What's making the bookmark bubble want to be 335 px by default if on the mock > > it's 320? It seems like we must be setting the wrong preferred size of > > something somewhere in it. > > I did some investigation, and it turns out that this bubble is quite literally a > pessimal layout for us. > > The bottom row of the GridLayout ends up being like this: > > [ space ] [ padding0 ] [ Remove ] [ padding1 ] [ Edit ] [ padding2 ] [ Close ] > > And the respective widths are: > space = 40 pts (forced by being the same width as the "Folder:" label in the row > above) > padding0 = unrelated control horizontal spacing = 12 (pre-Harmony), 16 > (post-Harmony) > Remove = 81 pts (width needed to hold the text) > padding1 = unrelated control large spacing = 20 (pre-Harmony), 16 (post-Harmony) > Edit = 66 pts (width needed to hold the text) > padding2 = related button horizontal spacing = 6 (pre-Harmony), 8 (post-Harmony) > Close = 64 pts (width needed to hold the text) > > At the moment, this code uses non-Harmony constants for the padding widths, > which is something to be fixed, but it does not much affect the issue, which is > that the widths given above sum up to 40 + 12 + 81 + 20 + 66 + 6 + 64 = 289 pts, > and then when you add the 16pt margin on either side of a bubble's content view, > you get 321pts, or exactly 1pt wider than the 320pt Harmony size. :( If you swap > the padding widths over to the Harmony values, you get 40 + 16 + 81 + 16 + 66 + > 8 + 64 = 291 + 32 = 323. This doesn't have to be done in this CL, but let's fix by changing the button layout there to look more like the updated mocks: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... Basically, move to a standard dialog button row, which eliminates the left inset, and just has one left button + one pair of right buttons. This should make the preferred width small enough.
Is there a way to implement this without exposing it on ViewsDelegate? It's not that I'm opposed to exposing something there, it's more that I wonder if doing that is really what we want. Do we want to snap things other than dialogs? If no, then maybe what we really want is to muck with DialogDelegateView to override GetPreferredSize() to return the snapped size of the underlying contents view. Or something to that effect. This isolates the snapping to the place that really cares, while forcing all dialogs to conform to it. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:533: // multiple of the snapping unit. This feels like you're mostly testing the functionality of the test. That is, it would be just as effective for this test to implement GetSnappedDialogWidth() as always returning 5, and then check here that the returned size is 5. Even more bluntly, basically this is a mock test with an EXPECT_CALL; the only part of the non-test code it verifies is that GetPreferredSize() calls GetSnappedDialogWidth(). If that is worth testing, then I would simplify the test to just test that. If that's not worth testing, I'd eliminate this, or rewrite it to test something that is worth testing.
On 2017/03/29 03:51:16, Peter Kasting wrote: > Is there a way to implement this without exposing it on ViewsDelegate? > > It's not that I'm opposed to exposing something there, it's more that I wonder > if doing that is really what we want. Do we want to snap things other than > dialogs? If no, then maybe what we really want is to muck with > DialogDelegateView to override GetPreferredSize() to return the snapped size of > the underlying contents view. Or something to that effect. This isolates the > snapping to the place that really cares, while forcing all dialogs to conform to > it. I had a look at doing this. A problem that appears is that some subclasses of DialogDelegateView (CardUnmaskPromptViews and DesktopMediaPickerDialogView at least, possibly others - I'm not sure how to reliably locate them) are already overriding View::GetPreferredSize() to implement their own default sizes, and we need our snapping to apply after that logic. It's possible that they simply shouldn't be doing this at all, and should always use a snapped size, but that doesn't work well in non-Harmony modes. CardUnmaskPromptViews::GetPreferredSize() has the following ominous comment: // Must hardcode a width so the label knows where to wrap. Which suggests that without that width, the dialog will be extremely wide because it contains an extremely wide label. I think there's a problem with the general idea of making dialogs always be a snapped version of their "natural size", which is that existing dialogs rely on having a hardcoded size to force wrapping of long text (i.e., their natural size is extremely wide) :\. Perhaps they should instead be setting a maximum width on the *label*, but then what happens if they get snapped wider? One idea would be to have, for example, DialogDelegateView::SnapSize(), and have CollectedCookiesViews::GetPreferredSize() return SnapSize(computed_preferred_size)... but then the snapping is not really isolated or forced. :( We could also introduce like DialogDelegateView::GetPreferredSizeX() as a virtual method intended to be overridden by child classes, then make DialogDelegateView::GetPreferredSize() a final method that overrides View::GetPreferredSize() and calls ::GetPreferredSizeX() as part of its body. I don't particularly like that approach either. > https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... > File ui/views/bubble/bubble_frame_view_unittest.cc (right): > > https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... > ui/views/bubble/bubble_frame_view_unittest.cc:533: // multiple of the snapping > unit. > This feels like you're mostly testing the functionality of the test. That is, > it would be just as effective for this test to implement GetSnappedDialogWidth() > as always returning 5, and then check here that the returned size is 5. Even > more bluntly, basically this is a mock test with an EXPECT_CALL; the only part > of the non-test code it verifies is that GetPreferredSize() calls > GetSnappedDialogWidth(). > > If that is worth testing, then I would simplify the test to just test that. If > that's not worth testing, I'd eliminate this, or rewrite it to test something > that is worth testing. I think deleting this test is probably the way to go. That said, I am kind of doubting the entire concept of this CL now...
On 2017/03/29 18:15:26, Elly Fong-Jones wrote:
> On 2017/03/29 03:51:16, Peter Kasting wrote:
> > Is there a way to implement this without exposing it on ViewsDelegate?
> >
> > It's not that I'm opposed to exposing something there, it's more that I
wonder
> > if doing that is really what we want. Do we want to snap things other than
> > dialogs? If no, then maybe what we really want is to muck with
> > DialogDelegateView to override GetPreferredSize() to return the snapped size
> of
> > the underlying contents view. Or something to that effect. This isolates
the
> > snapping to the place that really cares, while forcing all dialogs to
conform
> to
> > it.
>
> I had a look at doing this. A problem that appears is that some subclasses of
> DialogDelegateView (CardUnmaskPromptViews and DesktopMediaPickerDialogView at
> least, possibly others - I'm not sure how to reliably locate them)
Mark DialogDelegateView::GetPreferredSize() final and see what fails to compile.
> are already
> overriding View::GetPreferredSize() to implement their own default sizes, and
we
> need our snapping to apply after that logic.
Yes.
> It's possible that they simply
> shouldn't be doing this at all, and should always use a snapped size, but that
> doesn't work well in non-Harmony modes.
We should be OK here since non-Harmony will snap to 1 px, so snapping the
underlying preferred size will basically be a no-op in non-Harmony.
> CardUnmaskPromptViews::GetPreferredSize() has the following ominous comment:
>
> // Must hardcode a width so the label knows where to wrap.
>
> Which suggests that without that width, the dialog will be extremely wide
> because it contains an extremely wide label. I think there's a problem with
the
> general idea of making dialogs always be a snapped version of their "natural
> size", which is that existing dialogs rely on having a hardcoded size to force
> wrapping of long text (i.e., their natural size is extremely wide) :\. Perhaps
> they should instead be setting a maximum width on the *label*, but then what
> happens if they get snapped wider?
I think this is solvable.
Compute the preferred width of each wrappable label as
int label_width = std::min(label.GetPreferredSize().width(),
label.font_list().GetExpectedTextWidth(n));
...where n is the maximum number of characters we want to display on a single
line, and is either fixed in the code or set as a localizable constant. Then
compute the preferred height of the label using GetHeightForWidth() with the
width computed above.
When the dialog is actually sized (i.e. when OnBoundsChanged() is called), use
SetMaximumWidth() to clamp the label width based on whatever was really
computed.
This means that GetPreferredSize() is basically const; it will never set maximum
widths on labels, only provide a sizing suggestion. So if we snap to a larger
width, no problem.
If we do this in multiple places, then once it's clear how to do so, we can try
to factor some of the above into common helpers, e.g. a Label subclass that does
this kind of thing automatically when given a constructor arg about the max
number of characters to go on a line.
> One idea would be to have, for example, DialogDelegateView::SnapSize(), and
have
> CollectedCookiesViews::GetPreferredSize() return
> SnapSize(computed_preferred_size)... but then the snapping is not really
> isolated or forced. :(
Right. I'd rather not do this because it's too easy for people to fail to do.
> We could also introduce like
> DialogDelegateView::GetPreferredSizeX() as a virtual method intended to be
> overridden by child classes, then make DialogDelegateView::GetPreferredSize()
a
> final method that overrides View::GetPreferredSize() and calls
> ::GetPreferredSizeX() as part of its body. I don't particularly like that
> approach either.
This is what I had in mind. (I'd call the subclass function
"GetUnsnappedPreferredSize()" or something.) What do you dislike about it?
https://codereview.chromium.org/2750063002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right): https://codereview.chromium.org/2750063002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:85: for (size_t i = 0; i < arraysize(kDefaultWidths); ++i) { for (int snapped_width : kDefaultWidths) (or `width` if the width function arg gets renamed to min_width) https://codereview.chromium.org/2750063002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2750063002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/layout_delegate.h:109: virtual int GetSnappedDialogWidth(int width) const; perhaps width->min_width? or preferred_width? https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:535: ViewsDelegate::GetInstance()->GetSnappedDialogWidth(size.width())); I like the idea of hooking in to BubbleFrameView::GetSizeForClientSize() :) .. (I'm pretty sure that..) the title margins and insets above need to be incorporated into the size calculation, and trying to feed through the correct widths to/from where the snapping gets done might get tricky. E.g. the GetPreferredSnappedWidth(..) virtual method might also need to take arguments for insets/margins so they can be subtracted/added from/to the harmony dialog constants. But Peter is right to be concerned that this may affect things that are not dialogs - I think some tooltips and IME windows also use BubbleFrameView. Extension "PageAction" bubbles and other WebUI dialogs might also need to be considered (we shouldn't be snapping those -- webui may get messed up if we give it a width it doesn't expect). So a method on views::DialogDelegate seems necessary. Sadly, I think tooltips in CrOS/Ash use views::InfoBubble which is a BubbleDialogDelegateView, so just having [Bubble]DialogDelegate[View] as an indicator that "I am a dialog and should be snapped" probably will not work either. One possible heuristic..... is if the dialog has dialog buttons, then it is an "actual" dialog and should be snapped. But there are exceptions - e.g. site info bubble. So dialogs would need a way to override it. (I think some other non-webUI extension bubbles also have a way to report "no buttons", as does the add bookmark bubble [for bad reasons]) Also note there are also dialogs which use views::DialogDelegateView rather than views::BubbleDialogDelegateView (e.g. AppInfo's BaseDialogContainer, which should be snapped). So. What to do. I'm trying to think of a way that "forces" new dialogs to be snapped without having to think about it, and doesn't require us to update some ~180 subclasses of DialogDelegate with their snapping preferences. Note that DialogDelegate::GetContentsView() is "just" a views::View -- it doesn't have to be dialog-specific (but it is _often_ a DialogDelegateView or BubbleDialogDelegateView). It's the vanilla views::View from GetContentsView() that determines GetPreferredSize() -- not the dialog delegate -- and I don't think we can add "snapping" concepts to views::View (e.g. views::Label shouldn't have a "GetUnsnappedPreferredSize()" to support a DialogDelegate that uses a views::Label for its GetContentsView()) I'm currently leaning towards something like *keeping* ViewsDelegate::GetSnappedDialogWidth(..), but also adding `virtual bool DialogDelegate::ShouldSnapFrameWidth() const;`. To get started faster, the default implementation in DialogDelegate could be `return GetDialogButtons() != ui::DIALOG_BUTTON_NONE;`. Origin info bubble and add bookmark bubble would need to override ShouldSnapWidth. Here, in BubbleFrameView::GetSizeForClientSize(), we check: DialogDelegate* as_dialog = GetWidget()->widget_delegate()->AsDialogDelegate(); if (as_dialog && as_dialog->ShouldSnapFrameWidth()) { size.set_width(..GetSnappedDialogWidth()); } (disclaimer: pkasting or sky may hate this idea) (curveball: I see there's ui::mojom::kResizeBehavior on views::WidgetDelegate -- perhaps this could include a bit-flag for "snapping", so we wouldn't also need ShouldSnapFrameWidth()...) A possible alternative may be to overhaul BubbleFrameView::GetSizeForClientSize(). Basically, add DialogDelegate::GetFrameSizeForClientSize(..) [or WidgetDelegate::GetFrameSizeForClientSize], but it would need extra arguments from the FrameView to pass through: e.g. (some or all of) title widths, insets, margins, footnote view size, ... I think this will end up a lot more complex than DialogDelegate::ShouldSnapFrameWidth() + ViewsDelegate::GetSnappedDialogWidth() https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:25: class BubbleFrameViewTest; nit: forward dec not used? https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:32: int GetSnappedDialogWidth(int width) const override { nit: // TestViewsDelegate: https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:41: }; nit: DISALLOW_COPY... https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:47: set_views_delegate(std::unique_ptr<TestViewsDelegate>(views_delegate_)); base::WrapUnique? https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:55: }; nit: DISALLOW_COPY... https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:523: TEST_F(BubbleFrameViewTest, WidthSnapsToUnit) { nit: comment before https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:524: TestBubbleFrameView frame(this); Can we use BubbleDialogDelegateView::CreateBubble() in the test, and compare the dialog size using views::Widget APIs? There are a ton of flags/settings in the codepaths leading up to the creation of the dialog frame -- I feel like this would give us the best reassurance that all elements influencing the dialog size are covered. We might just need some platform-specific code to subtract the "right" shadow, or simply use BubbleDialogDelegateView::set_shadow(BubbleBorder::NO_ASSETS) explicitly so we can assume there is never a shadow.
Note that Elly uploaded a new patch at https://codereview.chromium.org/2785683003/ that I'm in the midst of reviewing. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:535: ViewsDelegate::GetInstance()->GetSnappedDialogWidth(size.width())); On 2017/03/29 23:16:16, tapted wrote: > I like the idea of hooking in to BubbleFrameView::GetSizeForClientSize() :) .. > (I'm pretty sure that..) the title margins and insets above need to be > incorporated into the size calculation, Yes, we need to know how big the title wants to be, and anything else (buttons on the bottom?) that wouldn't be included in the main size. > and trying to feed through the correct > widths to/from where the snapping gets done might get tricky. E.g. the > GetPreferredSnappedWidth(..) virtual method might also need to take arguments > for insets/margins so they can be subtracted/added from/to the harmony dialog > constants. What cases were you thinking of? Naively it seems like we don't need to do anything special here. > But Peter is right to be concerned that this may affect things that are not > dialogs - I think some tooltips and IME windows also use BubbleFrameView. > Extension "PageAction" bubbles and other WebUI dialogs might also need to be > considered (we shouldn't be snapping those -- webui may get messed up if we give > it a width it doesn't expect). Are you sure we shouldn't be snapping those? It seems like we should be snapping all those things. If we should snap All The Things, I think all the rest of your comment goes away because we can avoid solving the problem.
https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:535: ViewsDelegate::GetInstance()->GetSnappedDialogWidth(size.width())); On 2017/03/30 00:18:23, Peter Kasting wrote: > On 2017/03/29 23:16:16, tapted wrote: > > I like the idea of hooking in to BubbleFrameView::GetSizeForClientSize() :) .. > > (I'm pretty sure that..) the title margins and insets above need to be > > incorporated into the size calculation, > > Yes, we need to know how big the title wants to be, and anything else (buttons > on the bottom?) that wouldn't be included in the main size. > > > and trying to feed through the correct > > widths to/from where the snapping gets done might get tricky. E.g. the > > GetPreferredSnappedWidth(..) virtual method might also need to take arguments > > for insets/margins so they can be subtracted/added from/to the harmony dialog > > constants. > > What cases were you thinking of? Naively it seems like we don't need to do > anything special here. Most dialogs are happy using the default panel inset. Some can't use it. E.g. site info/OIB has a line that goes across the entire width (even in Harmony mocks - http://go/tghpy ) -- it sets margins to 0 and increases its preferred width so the line can be included. It does this by convincing the dialog delegate to pass zero-insets to the BubbleFrameView constructor. > > But Peter is right to be concerned that this may affect things that are not > > dialogs - I think some tooltips and IME windows also use BubbleFrameView. > > Extension "PageAction" bubbles and other WebUI dialogs might also need to be > > considered (we shouldn't be snapping those -- webui may get messed up if we > give > > it a width it doesn't expect). > > Are you sure we shouldn't be snapping those? It seems like we should be > snapping all those things. I guess that's a question for a designer :). Tooltips might look weird if they were not snapping to the text string + margin. WebUI stuff might come from extensions - it's hard to say how they will be affected, since we have no control over their layout (e.g. it might be "absolute"). > If we should snap All The Things, I think all the rest of your comment goes away > because we can avoid solving the problem. I'm certainly open to trying it :). It's possibly only an issue on ChromeOS, and we can worry about it later. Or there are enough isolated cases that it's better to address them specifically once we know they are negatively affected.
https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:535: ViewsDelegate::GetInstance()->GetSnappedDialogWidth(size.width())); On 2017/03/30 01:57:13, tapted wrote: > On 2017/03/30 00:18:23, Peter Kasting wrote: > > On 2017/03/29 23:16:16, tapted wrote: > > > I like the idea of hooking in to BubbleFrameView::GetSizeForClientSize() :) > .. > > > (I'm pretty sure that..) the title margins and insets above need to be > > > incorporated into the size calculation, > > > > Yes, we need to know how big the title wants to be, and anything else (buttons > > on the bottom?) that wouldn't be included in the main size. > > > > > and trying to feed through the correct > > > widths to/from where the snapping gets done might get tricky. E.g. the > > > GetPreferredSnappedWidth(..) virtual method might also need to take > arguments > > > for insets/margins so they can be subtracted/added from/to the harmony > dialog > > > constants. > > > > What cases were you thinking of? Naively it seems like we don't need to do > > anything special here. > > Most dialogs are happy using the default panel inset. Some can't use it. E.g. > site info/OIB has a line that goes across the entire width (even in Harmony > mocks - http://go/tghpy ) -- it sets margins to 0 and increases its preferred > width so the line can be included. It does this by convincing the dialog > delegate to pass zero-insets to the BubbleFrameView constructor. As long as GetInsets() returns the real desired insets, though, we can handle this without any funky plumbing, right? Like here, for example, the returned size includes the specified insets, and we snap after those are applied. If those are 0, that still works just fine. I'm not thinking of a case where we have to plumb something special. Maybe I'm missing it; feel free to share pseudocode for the sort of problematic case and solution you're thinking of? > > > But Peter is right to be concerned that this may affect things that are not > > > dialogs - I think some tooltips and IME windows also use BubbleFrameView. > > > Extension "PageAction" bubbles and other WebUI dialogs might also need to be > > > considered (we shouldn't be snapping those -- webui may get messed up if we > > give > > > it a width it doesn't expect). > > > > Are you sure we shouldn't be snapping those? It seems like we should be > > snapping all those things. > > I guess that's a question for a designer :). Tooltips might look weird if they > were not snapping to the text string + margin. Right, if tooltips are affected by this, that's probably bad. Are they? I didn't think they were on Windows at least, I'm not as familiar with, say, CrOS. > > If we should snap All The Things, I think all the rest of your comment goes > away > > because we can avoid solving the problem. > > I'm certainly open to trying it :). It's possibly only an issue on ChromeOS, and > we can worry about it later. Or there are enough isolated cases that it's better > to address them specifically once we know they are negatively affected. I'm fine with that, since if we break something, we should be only breaking it for Harmony, and hopefully with most of this (e.g. tooltips) we'll notice pretty quickly if it does the wrong thing.
https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:535: ViewsDelegate::GetInstance()->GetSnappedDialogWidth(size.width())); On 2017/03/30 04:07:02, Peter Kasting wrote: > On 2017/03/30 01:57:13, tapted wrote: > > On 2017/03/30 00:18:23, Peter Kasting wrote: > > > On 2017/03/29 23:16:16, tapted wrote: > > > > I like the idea of hooking in to BubbleFrameView::GetSizeForClientSize() > :) > > .. > > > > (I'm pretty sure that..) the title margins and insets above need to be > > > > incorporated into the size calculation, > > > > > > Yes, we need to know how big the title wants to be, and anything else > (buttons > > > on the bottom?) that wouldn't be included in the main size. > > > > > > > and trying to feed through the correct > > > > widths to/from where the snapping gets done might get tricky. E.g. the > > > > GetPreferredSnappedWidth(..) virtual method might also need to take > > arguments > > > > for insets/margins so they can be subtracted/added from/to the harmony > > dialog > > > > constants. > > > > > > What cases were you thinking of? Naively it seems like we don't need to do > > > anything special here. > > > > Most dialogs are happy using the default panel inset. Some can't use it. E.g. > > site info/OIB has a line that goes across the entire width (even in Harmony > > mocks - http://go/tghpy ) -- it sets margins to 0 and increases its preferred > > width so the line can be included. It does this by convincing the dialog > > delegate to pass zero-insets to the BubbleFrameView constructor. > > As long as GetInsets() returns the real desired insets, though, we can handle > this without any funky plumbing, right? > > Like here, for example, the returned size includes the specified insets, and we > snap after those are applied. If those are 0, that still works just fine. > > I'm not thinking of a case where we have to plumb something special. Maybe I'm > missing it; feel free to share pseudocode for the sort of problematic case and > solution you're thinking of? We may have had different ideas in our heads about where this CL was headed ;). I'm mostly thinking about the logic that's sitting above this comment, in BubbleFrameView::GetSizeForClientSize(..). e.g. One line uses `title_->GetPreferredSize()`, and that influences the width. So snapping will need to occur after the call to GetSizeForClientSize() -- this could be in BubbleDialogDelegateView::SizeToContents() or BubbleDialogDelegateView::GetBubbleBounds(). However (unless we do a bit more juggling) it can't be in DialogClientView::GetPreferredSize() (or ClientView::GetPreferredSize(), which uses BDDV::GetContentsView() and is how dialogs currently specify their width) since that happens before the call to GetSizeForClientSize(). > > > > > But Peter is right to be concerned that this may affect things that are > not > > > > dialogs - I think some tooltips and IME windows also use BubbleFrameView. > > > > Extension "PageAction" bubbles and other WebUI dialogs might also need to > be > > > > considered (we shouldn't be snapping those -- webui may get messed up if > we > > > give > > > > it a width it doesn't expect). > > > > > > Are you sure we shouldn't be snapping those? It seems like we should be > > > snapping all those things. > > > > I guess that's a question for a designer :). Tooltips might look weird if they > > were not snapping to the text string + margin. > > Right, if tooltips are affected by this, that's probably bad. Are they? I I put a screenshot and code fragment at http://crbug.com/635173#c14 for the types of "tooltip" that could be affected by this. (this one was actually not ChromeOS-only - turns out views::InfoBubble is used elsewhere.) But ash::ShelfTooltipManager::ShelfTooltipBubble is also definitely a BubbleDialogDelegateView, and is the other one I had on the brain.
https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:535: ViewsDelegate::GetInstance()->GetSnappedDialogWidth(size.width())); On 2017/03/30 06:11:49, tapted wrote: > On 2017/03/30 04:07:02, Peter Kasting wrote: > > As long as GetInsets() returns the real desired insets, though, we can handle > > this without any funky plumbing, right? > > > > Like here, for example, the returned size includes the specified insets, and > we > > snap after those are applied. If those are 0, that still works just fine. > > > > I'm not thinking of a case where we have to plumb something special. Maybe > I'm > > missing it; feel free to share pseudocode for the sort of problematic case and > > solution you're thinking of? > > We may have had different ideas in our heads about where this CL was headed ;). > I'm mostly thinking about the logic that's sitting above this comment, in > BubbleFrameView::GetSizeForClientSize(..). > > e.g. One line uses `title_->GetPreferredSize()`, and that influences the width. > > So snapping will need to occur after the call to GetSizeForClientSize() -- this > could be in BubbleDialogDelegateView::SizeToContents() or > BubbleDialogDelegateView::GetBubbleBounds(). > > However (unless we do a bit more juggling) it can't be in > DialogClientView::GetPreferredSize() (or ClientView::GetPreferredSize(), which > uses BDDV::GetContentsView() and is how dialogs currently specify their width) > since that happens before the call to GetSizeForClientSize(). Do all dialogs use a BubbleFrameView? They probably do, right? So maybe we don't need to snap things in DialogClientView::GetPreferredSize() at all (as the other CL does), and we can _just_ snap in the location here. Why do you say "snapping will need to occur after the call to GetSizeForClientSize()"? Why can't it occur at the very end of that function, as it does in this patch? We've taken title, content, insets etc. into account by this point, so it seems like the right place to do this. > > Right, if tooltips are affected by this, that's probably bad. Are they? I > > I put a screenshot and code fragment at http://crbug.com/635173#c14 for the > types of "tooltip" that could be affected by this. (this one was actually not > ChromeOS-only - turns out views::InfoBubble is used elsewhere.) > > But ash::ShelfTooltipManager::ShelfTooltipBubble is also definitely a > BubbleDialogDelegateView, and is the other one I had on the brain. Thanks, that's super helpful. Replied on the bug.
https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:535: ViewsDelegate::GetInstance()->GetSnappedDialogWidth(size.width())); On 2017/03/31 22:09:58, Peter Kasting wrote: > On 2017/03/30 06:11:49, tapted wrote: > > On 2017/03/30 04:07:02, Peter Kasting wrote: > > > As long as GetInsets() returns the real desired insets, though, we can > handle > > > this without any funky plumbing, right? > > > > > > Like here, for example, the returned size includes the specified insets, and > > we > > > snap after those are applied. If those are 0, that still works just fine. > > > > > > I'm not thinking of a case where we have to plumb something special. Maybe > > I'm > > > missing it; feel free to share pseudocode for the sort of problematic case > and > > > solution you're thinking of? > > > > We may have had different ideas in our heads about where this CL was headed > ;). > > I'm mostly thinking about the logic that's sitting above this comment, in > > BubbleFrameView::GetSizeForClientSize(..). > > > > e.g. One line uses `title_->GetPreferredSize()`, and that influences the > width. > > > > So snapping will need to occur after the call to GetSizeForClientSize() -- > this > > could be in BubbleDialogDelegateView::SizeToContents() or > > BubbleDialogDelegateView::GetBubbleBounds(). > > > > However (unless we do a bit more juggling) it can't be in > > DialogClientView::GetPreferredSize() (or ClientView::GetPreferredSize(), which > > uses BDDV::GetContentsView() and is how dialogs currently specify their width) > > since that happens before the call to GetSizeForClientSize(). > > Do all dialogs use a BubbleFrameView? They probably do, right? Most do.. nearly all child classes of DialogDelegate[View] and BubbleDialogDelegateView in the dialog audit https://docs.google.com/spreadsheets/d/1xDrxVl--AysmTowWQ7x5XeNVjpomZzQwGX-lh... (see ParentClass column). Dialogs can override DialogDelegate::ShouldUseCustomFrame() to avoid the BubbleFrameView or override DialogDelegate::CreateNonClientFrameView -- some dialogs do this, but they might be corner cases -- things like task manager, hung renderer dialog, .. Dialogs can also override WidgetDelegate::CreateClientView to avoid getting a DialogClientView (e.g. app info dialog does this). > > So maybe we don't need to snap things in DialogClientView::GetPreferredSize() at > all (as the other CL does), and we can _just_ snap in the location here. > > Why do you say "snapping will need to occur after the call to > GetSizeForClientSize()"? Why can't it occur at the very end of that function, > as it does in this patch? We've taken title, content, insets etc. into account > by this point, so it seems like the right place to do this. This was wrt the comment at https://codereview.chromium.org/2750063002/#msg12 (i.e. around exploring GetPreferredSize() overrides in DialogclientView). I am fine with snapping here :). I think it can work, so long as we have a way to cut out a lot of obscure things that also use BubbleFrameView, but shouldn't be snapped
The CQ bit was checked by ellyjones@chromium.org to run a CQ dry run
tapted, pkasting: ptal? :) https://codereview.chromium.org/2750063002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right): https://codereview.chromium.org/2750063002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:85: for (size_t i = 0; i < arraysize(kDefaultWidths); ++i) { On 2017/03/29 23:16:16, tapted wrote: > for (int snapped_width : kDefaultWidths) > > (or `width` if the width function arg gets renamed to min_width) Done. I actually went one further and made it: for (int width : {320, 448, 512}) Modern C++ is truly marvellous. https://codereview.chromium.org/2750063002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2750063002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/layout_delegate.h:109: virtual int GetSnappedDialogWidth(int width) const; On 2017/03/29 23:16:16, tapted wrote: > perhaps width->min_width? or preferred_width? Done. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:535: ViewsDelegate::GetInstance()->GetSnappedDialogWidth(size.width())); On 2017/04/03 09:53:48, tapted wrote: > On 2017/03/31 22:09:58, Peter Kasting wrote: > > On 2017/03/30 06:11:49, tapted wrote: > > > On 2017/03/30 04:07:02, Peter Kasting wrote: > > > > As long as GetInsets() returns the real desired insets, though, we can > > handle > > > > this without any funky plumbing, right? > > > > > > > > Like here, for example, the returned size includes the specified insets, > and > > > we > > > > snap after those are applied. If those are 0, that still works just fine. > > > > > > > > I'm not thinking of a case where we have to plumb something special. > Maybe > > > I'm > > > > missing it; feel free to share pseudocode for the sort of problematic case > > and > > > > solution you're thinking of? > > > > > > We may have had different ideas in our heads about where this CL was headed > > ;). > > > I'm mostly thinking about the logic that's sitting above this comment, in > > > BubbleFrameView::GetSizeForClientSize(..). > > > > > > e.g. One line uses `title_->GetPreferredSize()`, and that influences the > > width. > > > > > > So snapping will need to occur after the call to GetSizeForClientSize() -- > > this > > > could be in BubbleDialogDelegateView::SizeToContents() or > > > BubbleDialogDelegateView::GetBubbleBounds(). > > > > > > However (unless we do a bit more juggling) it can't be in > > > DialogClientView::GetPreferredSize() (or ClientView::GetPreferredSize(), > which > > > uses BDDV::GetContentsView() and is how dialogs currently specify their > width) > > > since that happens before the call to GetSizeForClientSize(). > > > > Do all dialogs use a BubbleFrameView? They probably do, right? > > Most do.. nearly all child classes of DialogDelegate[View] and > BubbleDialogDelegateView in the dialog audit > https://docs.google.com/spreadsheets/d/1xDrxVl--AysmTowWQ7x5XeNVjpomZzQwGX-lh... > (see ParentClass column). > > Dialogs can override DialogDelegate::ShouldUseCustomFrame() to avoid the > BubbleFrameView or override DialogDelegate::CreateNonClientFrameView -- some > dialogs do this, but they might be corner cases -- things like task manager, > hung renderer dialog, .. > > Dialogs can also override WidgetDelegate::CreateClientView to avoid getting a > DialogClientView (e.g. app info dialog does this). > > > > > > So maybe we don't need to snap things in DialogClientView::GetPreferredSize() > at > > all (as the other CL does), and we can _just_ snap in the location here. > > > > Why do you say "snapping will need to occur after the call to > > GetSizeForClientSize()"? Why can't it occur at the very end of that function, > > as it does in this patch? We've taken title, content, insets etc. into > account > > by this point, so it seems like the right place to do this. > > This was wrt the comment at https://codereview.chromium.org/2750063002/#msg12 > (i.e. around exploring GetPreferredSize() overrides in DialogclientView). I am > fine with snapping here :). I think it can work, so long as we have a way to cut > out a lot of obscure things that also use BubbleFrameView, but shouldn't be > snapped Wrapping up this thread: I ended up adding DialogDelegate::ShouldSnapFrameWidth() and restricting the snapping to BubbleFrameViews that have a DialogDelegate installed as their WidgetDelegate and for which ShouldSnapFrameWidth() returns true. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:25: class BubbleFrameViewTest; On 2017/03/29 23:16:16, tapted wrote: > nit: forward dec not used? Done. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:32: int GetSnappedDialogWidth(int width) const override { On 2017/03/29 23:16:16, tapted wrote: > nit: // TestViewsDelegate: Done. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:41: }; On 2017/03/29 23:16:16, tapted wrote: > nit: DISALLOW_COPY... Done. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:47: set_views_delegate(std::unique_ptr<TestViewsDelegate>(views_delegate_)); On 2017/03/29 23:16:16, tapted wrote: > base::WrapUnique? Done. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:55: }; On 2017/03/29 23:16:16, tapted wrote: > nit: DISALLOW_COPY... Done. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:523: TEST_F(BubbleFrameViewTest, WidthSnapsToUnit) { On 2017/03/29 23:16:16, tapted wrote: > nit: comment before Done. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:524: TestBubbleFrameView frame(this); On 2017/03/29 23:16:16, tapted wrote: > Can we use BubbleDialogDelegateView::CreateBubble() in the test, and compare the > dialog size using views::Widget APIs? There are a ton of flags/settings in the > codepaths leading up to the creation of the dialog frame -- I feel like this > would give us the best reassurance that all elements influencing the dialog size > are covered. Which setup code do you mean? There is some in (eg) TestBubbleFrameView::GetWidget(), but I think I would end up simply duplicating that code in this test... Are you talking about doing something like: unique_ptr<Widget> w(new Widget()); ... make some init params ... w->Init(); BubbleDialogDelegateView d(w); and then calling: BubbleDialogDelegateView::CreateBubble(&d) and checking the resulting widget's size? I don't really understand what that would be testing, since won't ::CreateBubble simply use the widget we created above? I ended up doing something like this, but I am not that pleased with the result. > We might just need some platform-specific code to subtract the "right" shadow, > or simply use BubbleDialogDelegateView::set_shadow(BubbleBorder::NO_ASSETS) > explicitly so we can assume there is never a shadow. https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:533: // multiple of the snapping unit. On 2017/03/29 03:51:16, Peter Kasting wrote: > This feels like you're mostly testing the functionality of the test. That is, > it would be just as effective for this test to implement GetSnappedDialogWidth() > as always returning 5, and then check here that the returned size is 5. Even > more bluntly, basically this is a mock test with an EXPECT_CALL; the only part > of the non-test code it verifies is that GetPreferredSize() calls > GetSnappedDialogWidth(). > > If that is worth testing, then I would simplify the test to just test that. If > that's not worth testing, I'd eliminate this, or rewrite it to test something > that is worth testing. I reworked this test a little bit to be simpler, but I think we do still need it.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
make sure you update the CL description too https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2750063002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:524: TestBubbleFrameView frame(this); On 2017/04/05 18:17:53, Elly Fong-Jones wrote: > On 2017/03/29 23:16:16, tapted wrote: > > Can we use BubbleDialogDelegateView::CreateBubble() in the test, and compare > the > > dialog size using views::Widget APIs? There are a ton of flags/settings in the > > codepaths leading up to the creation of the dialog frame -- I feel like this > > would give us the best reassurance that all elements influencing the dialog > size > > are covered. > > Which setup code do you mean? There is some in (eg) > TestBubbleFrameView::GetWidget(), but I think I would end up simply duplicating > that code in this test... > > Are you talking about doing something like: > > unique_ptr<Widget> w(new Widget()); > ... make some init params ... > w->Init(); > > BubbleDialogDelegateView d(w); > > and then calling: > > BubbleDialogDelegateView::CreateBubble(&d) > > and checking the resulting widget's size? I don't really understand what that > would be testing, since won't ::CreateBubble simply use the widget we created > above? BubbleDialogDelegateView doesn't need a Widget to construct itself -- that's done by BubbleDialogDelegateView::CreateBubble(). The problem is the Widget that CreateBubble creates uses anon::CreateBubbleWidget() which sets a bunch of Widget params that could have subtle influences on the native/non-native nonclientframeviews that are added to the window that is eventually displayed, and what the resulting "width" of the dialog will be when it's shown to the user. https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:121: public TestBubbleFrameViewWidgetDelegate { I don't think we need this - we're not really interested in any the things that TestBubbleFrameViewWidgetDelegate provides -- we can just inherit from a BubbleDialogDelegateView (or DialogDelegateView with a but more work [ShouldUseCustomFrame returning true]) Then so long as we use set_shadow(BubbleBorder::NO_SHADOW) we can verify that the size of the *Widget* matches the expected Harmony sizes. I think that's a lot more compelling than the preferred size of the frame view, since layout code can ignore preferred sizes, and the Widget size will include the non-client frame view, which will be a closer match to the size of the dialog seen by users.
https://codereview.chromium.org/2750063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2750063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/layout_delegate.h:111: // |min_width|. May return 0 if the dialog has no preferred width. Nit: Comment seems outdated. Maybe "Returns the actual width to use for a dialog that requires at least |min_width|. This may be used to e.g. round up to a set of fixed widths." https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:534: DialogDelegate* as_dialog = Nit: I'd name this |dialog| or |dialog_delegate| https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:535: GetWidget()->widget_delegate()->AsDialogDelegate(); Nit: Move this line below the next comment since it's part of that block https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:121: public TestBubbleFrameViewWidgetDelegate { On 2017/04/06 00:24:44, tapted wrote: > I don't think we need this - we're not really interested in any the things that > TestBubbleFrameViewWidgetDelegate provides -- we can just inherit from a > BubbleDialogDelegateView (or DialogDelegateView with a but more work > [ShouldUseCustomFrame returning true]) > > Then so long as we use set_shadow(BubbleBorder::NO_SHADOW) we can verify that > the size of the *Widget* matches the expected Harmony sizes. I think that's a > lot more compelling than the preferred size of the frame view, since layout code > can ignore preferred sizes, and the Widget size will include the non-client > frame view, which will be a closer match to the size of the dialog seen by > users. +1 https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:179: return base::WrapUnique(new TestBubbleFrameViewWidgetDelegate(widget)); Nit: WrapUnique(new -> MakeUnique (2 places) https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:581: int border_width = 10; Nit: This looks like you're matching a shadow/stroke width from elsewhere (either built in to a resource or computed elsewhere). This makes the test fragile; instead expose the necessary values from the code that actually computes them, and then fetch them in this test. https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:584: // multiple of the snapping unit. Nit: Technically, the code below tests only that your GetSnappedDialogWidth() override is called. It doesn't test that this is a "multiple of the snapping unit". If your override returned "the snap_width + 1", that's what we'd get here. Instead, I suggest the following: * Eliminate your views delegate override * Have a dialog that can return a variety of preferred widths * Create an array of pairs (actual width, expected snapped width) * Test that, when you force disable harmony mode, the actual width is the preferred width in all cases * Then, when you force enable harmony mode, the actual width should be the snapped width * Then if the dialog returns false for "should snap", you get the pre-Harmony behavior https://codereview.chromium.org/2750063002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:586: EXPECT_NE(kTestWidth, frame.GetPreferredSize().width() - border_width); Nit: EXPECT_LT? https://codereview.chromium.org/2750063002/diff/80001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2750063002/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:231: return true; Since this patch already makes bubbles-that-aren't-dialogs not snap, do we actually need this method?
https://codereview.chromium.org/2750063002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2750063002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/layout_delegate.h:69: enum class DialogWidth { This enum should disappear. |
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Preview/08-Bookmark#%2F01-Bookmarks_add.png%3Fz=width){kind=link}