Clamp dialog bounds to be fully visible on the nearest display

ui/display/screen.cc

 // Copyright 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/display/screen.h"
 
 #include "ui/display/display.h"
+#include "ui/display/display_finder.h"
 #include "ui/gfx/geometry/rect.h"
 
 namespace display {
 
 namespace {
 
 Screen* g_screen;
 
 }  // namespace
 
 Screen::Screen() {}
 
 Screen::~Screen() {}
 
 // static
 Screen* Screen::GetScreen() {
 #if defined(OS_MACOSX)
   // TODO(scottmg): https://crbug.com/558054
   if (!g_screen)
     g_screen = CreateNativeScreen();
 #endif
   return g_screen;
 }
 
 // static
 void Screen::SetScreenInstance(Screen* instance) {
   g_screen = instance;
 }
 
+//static

[msw, 2016/12/12 23:04:35]

nit: space after slashes

[msw, 2016/12/13 22:52:43]

Ping

[kylix_rd, 2016/12/14 17:35:10]

Done.

+gfx::Rect Screen::MoveScreenRectToNearestDisplay(

[msw, 2016/12/12 23:04:35]

Will this be used elsewhere? Might it make sense to inline this in
UpdateModalDialogPosition, or move it as a helper to
constrained_window_views.cc?

[kylix_rd, 2016/12/13 14:17:44]

I'd originally put this code in that file but this function should be
universally useful. I don't have any immediate example at this point.

I spent a while looking for a function such as this. The logical location was on
the screen/display classes. Should this be put back into
constrained_window_views.cc, the next person may not locate it there and proceed
to write a similar function. By placing this where it logically belongs, it
increases the chance it will be used by the next person needing such a function.

[msw, 2016/12/13 22:52:43]

I'm also a little surprised that I can't find a similar function. Still, this
doesn't belong here without another existing or imminent user (unless a
ui/display owner disagrees).

[kylix_rd, 2016/12/14 17:35:10]

My point was that should this be put elsewhere, it would increase the likelihood
of it being missed and the same or similar function being written again. I think
we should at least try and make things a little easier on those that come in
behind us.

I also expect this function to be used in the near future and the extra code
churn seems a little excessive.

In the end, I'm not going to "go to the mat" for this :)... I'm just trying to
state my case.

[msw, 2016/12/14 18:22:06]

Finding code to reuse later isn't too hard. I searched a little more and found a
few related functions, and I encourage you to extract some common functionality:
AppWindow::AdjustBoundsToBeVisibleOnScreen, GetWindowBoundsForClientBounds (in
ui/platform_window/win/win_window.cc), BubbleFrameView::OffsetArrowIfOffScreen,
hopefully you can find even more.

I'd defer to the advice of a ui/display owner, but in general, we try to avoid
pushing client-specific code to widely used/included base libraries. This
operation can be performed with the publicly-accessible display/screen
interfaces, which strive to strike a balance between minimalism and universal
applicability. This operation is not an intrinsic/essential part of
display::Screen, it's more fitting in a display/screen/window utility file of
grab-bag helpers, and I'd only suggest that if the function had sufficient
complexity or applicability to more than one user (otherwise, it belongs with
its solitary client).

+    const gfx::Rect& screen_rect) {
+  gfx::Rect display_rect = screen_rect;
+  const display::Display* display =
+      display::FindDisplayNearestPoint(GetScreen()->GetAllDisplays(),
+                                       display_rect.CenterPoint());

[msw, 2016/12/14 18:45:11]

It might make sense to use the display closest to the center point of the parent
window, rather than the new dialog, though they're probably often the same thing
(unless a window is split vertically)

+  DCHECK(display);
+  const gfx::Rect work_area = display->work_area();
+  if (!work_area.Contains(display_rect)) {

[msw, 2016/12/12 23:04:35]

q: do we care about making minimal changes to the position? otherwise, would it
make sense to use gfx::Rect::ClampToCenteredSize or similar?

[kylix_rd, 2016/12/13 14:17:44]

If the rect were partially visible, it would be somewhat jarring to the user for
it to "jump" to the center of the screen.

[msw, 2016/12/13 22:52:44]

Hopefully this positioning fixup is applied before the dialog is actually
painted, so it never actually renders partially offscreen then 'jumps'; can you
confirm that? Regardless, it's fine to minimize the positioning delta, but this
code doesn't handle (center) rects larger than the work area in either
dimension, yet another reason this shouldn't be a generalized function.

[kylix_rd, 2016/12/14 17:35:10]

Yes, this is done prior to the dialog is initially made visible.

So, are you saying that the size should also be clamped to the screen size as
well? IMO, that would be a different operation. The name of the function is
*Move*ScreenRectToNearestDisplay, so it shouldn't be confusing. I could add a
parameter to this function or add a different function to do that.

[msw, 2016/12/14 18:22:06]

I'm saying that if a window were larger than the display's work area, this
function would clamp the rect such that the leading (left or top) edge would be
offscreen, rather than centering or shrinking the rect within the work area.
Even if we didn't want to center/shrink the rect, I'd imagine that we'd want to
show the leading/top edge (it's more likely to have the window controls on top,
but perhaps dialog response buttons are on the right so show the trailing
horizontal edge, which depends upon the RTL/LTR system locale, etc.). These
kinds of considerations are especially important if you want to make this a
generalized, widely-applicable solution.

[msw, 2016/12/14 18:45:12]

Perhaps the right function for this job is gfx::Rect::AdjustToFit; other
functions I pointed out could probably use that too.

+    if (display_rect.x() < work_area.x())
+      display_rect.set_x(work_area.x());
+    if (display_rect.right() > work_area.right())

[msw, 2016/12/13 22:52:44]

else if? (or handle rects larger than the work area?)

[kylix_rd, 2016/12/14 17:35:10]

See above comment.

+      display_rect.set_x(work_area.right() - display_rect.width());
+    if (display_rect.y() < work_area.y())
+      display_rect.set_y(work_area.y());
+    if (display_rect.bottom() > work_area.bottom())

[msw, 2016/12/13 22:52:44]

else if? (or handle rects larger than the work area?)

+      display_rect.set_y(work_area.bottom() - display_rect.height());
+  }
+  return display_rect;
+}
+
 gfx::Rect Screen::ScreenToDIPRectInWindow(gfx::NativeView view,
                                           const gfx::Rect& screen_rect) const {
   float scale = GetDisplayNearestWindow(view).device_scale_factor();
   return ScaleToEnclosingRect(screen_rect, 1.0f / scale);
 }
 
 gfx::Rect Screen::DIPToScreenRectInWindow(gfx::NativeView view,
                                           const gfx::Rect& dip_rect) const {
   float scale = GetDisplayNearestWindow(view).device_scale_factor();
   return ScaleToEnclosingRect(dip_rect, scale);
 }
 
 bool Screen::GetDisplayWithDisplayId(int64_t display_id,
                                      display::Display* display) const {
   for (const display::Display& display_in_list : GetAllDisplays()) {
     if (display_in_list.id() == display_id) {
       *display = display_in_list;
       return true;
     }
   }
   return false;
 }
 
 }  // namespace display