Improving window auto management between workspaces

ash/wm/workspace/auto_window_management.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 "ash/wm/workspace/auto_window_management.h"
+
+#include "ash/shell.h"
+#include "ash/wm/window_animations.h"
+#include "ash/wm/window_util.h"
+#include "ui/aura/window.h"
+#include "ui/compositor/layer_animator.h"
+#include "ui/compositor/scoped_layer_animation_settings.h"
+#include "ui/gfx/screen.h"
+
+namespace ash {
+namespace internal {
+
+namespace {
+
+// The time which should be used to visually move a window through an
+// automatic "intelligent" window management option.
+const base::TimeDelta kWindowAutoMoveDuration =

[sky, 2012/10/16 17:25:34]

This triggers a static initializer, increasing startup time. We don't want
those. Use an int instead.

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+    base::TimeDelta::FromMilliseconds(125);
+
+// Check if the given |window| is visible on the screen and has therefore

[sky, 2012/10/16 17:25:34]

This doesn't check visibility.

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+// an impact on the position manage ability. Note: The minimized/maximized
+// state is not included in this test since a window which just got changed
+// states might have an impact on the window management system.
+bool WindowHasImpactOnPositioning(const aura::Window* window) {
+  return (!window->bounds().IsEmpty() && wm::IsWindowPositionManaged(window));
+}
+
+// Check if a given |window| is considered to be an auto location manageable

[sky, 2012/10/16 17:25:34]

Clarify how this and WindowHasImpactOnPositioning differ. It would be good if
you could name them better too. Right now when I look at the names they both
mean the same thing to me.

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+// window or not.
+bool IsWindowPositionManageable(const aura::Window* window) {
+  return (WindowHasImpactOnPositioning(window) &&
+          !wm::IsWindowMinimized(window) &&
+          !wm::IsWindowMaximized(window) &&
+          !wm::HasUserChangedWindowPositionOrSize(window));
+}
+
+// Given a |window|, return the |work_area| of the desktop on which it is
+// located as well as the only |other_window| which has an impact on the
+// automated window location management. If there are more then one windows,

[sky, 2012/10/16 17:25:34]

'there are' -> 'there is'

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+// false get returned, but the |other_window| will be set to the first one

[sky, 2012/10/16 17:25:34]

get -> is

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+// found.
+// If the return value is true a single window was found.
+// Note: |work_area| is undefined if false is returned and |other_window| is
+// NULL.
+bool GetOtherVisibleAndManageableWindow(const aura::Window* window,
+                                        gfx::Rect* work_area,
+                                        aura::Window** other_window) {
+  *other_window = NULL;
+  // If the given window was not manageable, it cannot have caused a management
+  // operation earlier.
+  if (!WindowHasImpactOnPositioning(window))
+    return false;
+
+  // Get our work area.
+  *work_area = window->parent()->bounds();

[sky, 2012/10/16 17:25:34]

*work_area = gfx::Rect(window->parent()->bounds().size());

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

I have the feeling that this (including the removal below) might bite us later.
Done.

+  work_area->Inset(Shell::GetScreen()->GetDisplayMatching(
+      *work_area).GetWorkAreaInsets());
+
+  const aura::Window::Windows& windows = window->parent()->children();
+  // Find a single open browser window.

[sky, 2012/10/16 17:25:34]

browser -> managed

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+  for (size_t i = 0; i < windows.size(); i++) {
+    aura::Window* j = windows[i];

[sky, 2012/10/16 17:25:34]

nit: use something more descriptive than 'j'. 'j' is typically used for
iterators (as you have with i above).

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+    if (window != j && j->type() == aura::client::WINDOW_TYPE_NORMAL &&
+        work_area->Intersects(j->GetBoundsInScreen()) &&

[sky, 2012/10/16 17:25:34]

You're comparing coordinates in different coordinate system. You know j is a
child and we force windows on screen, so I think you can remove this line
entirely.

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+        j->TargetVisibility() && WindowHasImpactOnPositioning(j)) {
+      // Bail if we find a second usable window.
+      if (*other_window)
+        return false;
+      *other_window = j;
+    }
+  }
+  return *other_window != NULL;
+}
+
+// Move the given |bounds| on the available |parent_width| to the
+// direction. If |move_right| is true, the rectangle gets moved to the right
+// corner, otherwise to the left one.
+bool MoveRectToOneSide(int parent_width, gfx::Rect& bounds, bool move_right) {

[sky, 2012/10/16 17:25:34]

gfx::Rect& -> gfx::Rect* and it should be the last parameter.

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+  if (move_right) {
+    if (parent_width > bounds.right()) {
+      bounds.set_x(parent_width - bounds.width());
+      return true;
+    }
+  } else {
+    if (0 < bounds.x()) {
+      bounds.set_x(0);
+      return true;
+    }
+  }
+  return false;
+}
+
+} // namespace
+
+// Check if after removal of the given |removed_window| an automated desktop
+// location management can be performed and rearrange accordingly
+void RearrangeVisibleWindowOnHide(const aura::Window* removed_window) {
+  // Find a single open browser window.
+  aura::Window* shown_window = NULL;
+  gfx::Rect work_area;
+  if (!GetOtherVisibleAndManageableWindow(removed_window,
+                                          &work_area,
+                                          &shown_window))
+      return;
+
+  if (IsWindowPositionManageable(shown_window)) {
+    // Center the window (only in x).

[sky, 2012/10/16 17:25:34]

How come only in x?
Shouldn't this restore to the position the window was at before the
auto-arrange? Might that not be centered?

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Because we only center in X. There was no mentioning of centering in Y
explicitly in this algorithm. BUT... the window will remain automatically Y
centered if it was created the way.

+    gfx::Rect bounds = shown_window->bounds();
+    bounds.set_x((work_area.width() - bounds.width()) / 2);
+    if (bounds != shown_window->bounds()) {
+      ui::LayerAnimator* animator = shown_window->layer()->GetAnimator();
+      ui::ScopedLayerAnimationSettings animation_setter(animator);
+      animation_setter.SetTransitionDuration(kWindowAutoMoveDuration);
+      shown_window->SetBounds(bounds);
+    }
+  }
+}
+
+// Check if after insertion  of the given |added_window| an automated desktop

[sky, 2012/10/16 17:25:34]

Don't duplicate comments here.

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+// location management can be performed and rearrange accordingly.
+void RearrangeVisibleWindowOnShow(aura::Window* added_window) {
+  // Find a single open browser window.

[sky, 2012/10/16 17:25:34]

browser->managed

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Done.

+  aura::Window* shown_window = NULL;
+  gfx::Rect work_area;
+  if (!GetOtherVisibleAndManageableWindow(added_window,
+                                          &work_area,
+                                          &shown_window)) {
+    // It could be that this window is the first window joining the workspace.
+    if (!IsWindowPositionManageable(added_window) || shown_window)
+      return;
+    // If so we have to make sure it is centered.

[sky, 2012/10/16 17:25:34]

How come you don't vertically center?

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Again - see comment above.

+    gfx::Rect bounds = added_window->bounds();
+    bounds.set_x((work_area.width() - bounds.width()) / 2);
+    added_window->SetBounds(bounds);
+    return;
+  }
+
+  if (IsWindowPositionManageable(shown_window)) {
+    // Push away the other window.
+    gfx::Rect other_bounds = shown_window->bounds();
+    bool move_right = other_bounds.CenterPoint().x() < work_area.width() / 2;
+    if (MoveRectToOneSide(work_area.width(), other_bounds, move_right)) {
+      if (other_bounds != shown_window->bounds()) {
+        ui::LayerAnimator* animator = shown_window->layer()->GetAnimator();

[sky, 2012/10/16 17:25:34]

Don't animate if either of the are set:

      window->GetProperty(aura::client::kAnimationsDisabledKey) ||
      CommandLine::ForCurrentProcess()->HasSwitch(
          ash::switches::kAshWindowAnimationsDisabled)) {

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

Sorry - isn't this something which belongs into the animation class? Somehow I
really expected that to be there. Done.

[sky, 2012/10/16 22:01:14]

Different packages.

+        ui::ScopedLayerAnimationSettings animation_setter(animator);
+        animation_setter.SetTransitionDuration(kWindowAutoMoveDuration);
+        shown_window->SetBounds(other_bounds);
+      }
+    }
+    // Push the new window also to the opposite location (if needed).
+    // Since it is just coming into view, we do not need to animate it.
+    gfx::Rect added_bounds = added_window->bounds();
+    if (MoveRectToOneSide(work_area.width(), added_bounds, !move_right))
+      added_window->SetBounds(added_bounds);

[sky, 2012/10/16 17:25:34]

Don't you want to animate this too?

[Mr4D (OOO till 08-26), 2012/10/16 19:00:28]

No. As you can read in the comment above, this window gets dropped into the
desktop at this time and there is no reason to animate it from a 'fictive' point
where it might even never had been in the past. Animations should only be used
where they help the user understand what is going on - this animation would only
be confusing.

+  }
+}
+
+} // namespace internal
+} // namespace ash
+