views: implement dialog width snapping

ui/views/window/dialog_client_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/window/dialog_client_view.h"
 
 #include <algorithm>
 
 #include "build/build_config.h"
 #include "ui/base/material_design/material_design_controller.h"
@@ skipping 23 matching lines @@
 #else
 const bool kIsOkButtonOnLeftSide = false;
 #endif
 
 // Returns true if the given view should be shown (i.e. exists and is
 // visible).
 bool ShouldShow(View* view) {
   return view && view->visible();
 }
 
+// Snaps the width of |size| up to the next multiple of |unit|.
+gfx::Size SnapWidthToMultipleOf(gfx::Size size, int unit) {
+  size.Enlarge(unit - 1, 0);

[tapted, 2017/03/15 22:26:11]

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;

[Peter Kasting, 2017/03/21 19:17:03]

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.

+  size.set_width(size.width() - (size.width() % unit));
+  return size;
+}
+
 // Returns the bounding box required to contain |size1| and |size2|, placed one
 // atop the other.

[Peter Kasting, 2017/03/21 19:17:03]

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.

 gfx::Size GetBoundingSizeForVerticalStack(const gfx::Size& size1,

[tapted, 2017/03/15 22:26:11]

rename -> GetSnappedBoundingSize?

                                           const gfx::Size& size2) {
-  return gfx::Size(std::max(size1.width(), size2.width()),
-                   size1.height() + size2.height());
+  return SnapWidthToMultipleOf(
+      gfx::Size(std::max(size1.width(), size2.width()),
+                size1.height() + size2.height()),
+      ViewsDelegate::GetInstance()->GetDistanceMetric(
+          views::DistanceMetric::DIALOG_WIDTH_SNAPPING_UNIT));

[tapted, 2017/03/15 22:26:11]

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

 }
 
 }  // namespace
 
 // Simple container to bubble child view changes up the view hierarchy.
 class DialogClientView::ButtonRowContainer : public View {
  public:
   explicit ButtonRowContainer(DialogClientView* owner) : owner_(owner) {}
 
   // View:
@@ skipping 63 matching lines @@
 }
 
 const DialogClientView* DialogClientView::AsDialogClientView() const {
   return this;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // DialogClientView, View overrides:
 
 gfx::Size DialogClientView::GetPreferredSize() const {
   return GetBoundingSizeForVerticalStack(

[tapted, 2017/03/15 22:26:11]

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.

[Peter Kasting, 2017/03/21 19:17:03]

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.

       ClientView::GetPreferredSize(),
       button_row_container_->GetPreferredSize());
 }
 
 gfx::Size DialogClientView::GetMinimumSize() const {
   return GetBoundingSizeForVerticalStack(
       ClientView::GetMinimumSize(), button_row_container_->GetMinimumSize());
 }
 
 gfx::Size DialogClientView::GetMaximumSize() const {
@@ skipping 269 matching lines @@
 
   if (extra_view_)
     return;
 
   extra_view_ = GetDialogDelegate()->CreateExtraView();
   if (extra_view_)
     extra_view_->SetGroup(kButtonGroup);
 }
 
 }  // namespace views