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/ash_switches.h"
+#include "ash/shell.h"
+#include "ash/wm/window_animations.h"
+#include "ash/wm/window_util.h"
+#include "base/command_line.h"
+#include "ui/aura/window.h"
+#include "ui/aura/client/aura_constants.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 in milliseconds which should be used to visually move a window
+// through an automatic "intelligent" window management option.
+const int kWindowAutoMoveDurationMS = 125;
+
+// Check if the given |window| is marked to get managed.
+bool WindowIsManaged(const aura::Window* window) {
+  return wm::IsWindowPositionManaged(window);
+}
+
+// Check if a given |window| can be managed. This includes that it's state is
+// not minimized/maximized/the user has changed it's size by hand already.
+// It furthermore checks for the WindowIsManaged status.
+bool WindowPositionCanBeManaged(const aura::Window* window) {
+  return (WindowIsManaged(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 windows location management. If there is more then one windows,

[sky, 2012/10/22 16:40:50]

windows -> window

[Mr4D (OOO till 08-26), 2012/10/22 20:27:23]

Done.

+// false is returned, but the |other_window| will be set to the first one
+// 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

[sky, 2012/10/22 16:40:50]

This code doesn't even use the work_area. Is there really a reason to have this
function set it? It's especially weird since it only some times uses it.

[Mr4D (OOO till 08-26), 2012/10/22 20:27:23]

After the rest of the code got so drastically changed this isn't really
necessary here anymore. Separated out and moved all invocations into separate
call.

+// 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 (!WindowIsManaged(window))
+    return false;
+
+  // Get our work area.
+  *work_area = gfx::Rect(window->parent()->bounds().size());
+  work_area->Inset(Shell::GetScreen()->GetDisplayMatching(
+      *work_area).GetWorkAreaInsets());
+
+  const aura::Window::Windows& windows = window->parent()->children();
+  // Find a single open managed window.
+  for (size_t i = 0; i < windows.size(); i++) {
+    aura::Window* iterated_window = windows[i];
+    if (window != iterated_window &&
+        iterated_window->type() == aura::client::WINDOW_TYPE_NORMAL &&
+        iterated_window->TargetVisibility() &&
+        WindowIsManaged(iterated_window)) {
+      // Bail if we find a second usable window.
+      if (*other_window)
+        return false;
+      *other_window = iterated_window;
+    }
+  }
+  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, bool move_right, gfx::Rect* bounds) {
+  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;
+}
+
+ui::ScopedLayerAnimationSettings* CreateAnimationSettings(
+    aura::Window* window) {
+  if (!window->GetProperty(aura::client::kAnimationsDisabledKey) &&
+      !CommandLine::ForCurrentProcess()->HasSwitch(
+           ash::switches::kAshWindowAnimationsDisabled)) {
+    ui::LayerAnimator* animator = window->layer()->GetAnimator();
+    return new ui::ScopedLayerAnimationSettings(animator);
+  }
+  return NULL;
+}
+
+} // namespace
+
+void RearrangeVisibleWindowOnHideOrRemove(const aura::Window* removed_window) {
+  // Early bail out if the window isn't visible of the feature is unwanted.
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kAshDisableAutoWindowPlacement))
+    return;
+  // Find a single open browser window.
+  aura::Window* other_shown_window = NULL;
+  gfx::Rect work_area;
+  if (!GetOtherVisibleAndManageableWindow(removed_window,
+                                          &work_area,
+                                          &other_shown_window))
+    return;

[sky, 2012/10/22 16:40:50]

nit: Move !WindowPositionCanBeManaged here too so you early return on both.

[Mr4D (OOO till 08-26), 2012/10/22 20:27:23]

Done.

+
+  if (WindowPositionCanBeManaged(other_shown_window)) {
+    // Center the window (only in x).
+    gfx::Rect bounds = other_shown_window->bounds();
+    bounds.set_x((work_area.width() - bounds.width()) / 2);
+    if (bounds != other_shown_window->GetTargetBounds()) {
+      ui::ScopedLayerAnimationSettings* settings = CreateAnimationSettings(
+          other_shown_window);
+      settings->SetTransitionDuration(

[sky, 2012/10/22 16:40:50]

You need a null check here for settings. Or move SetTransitionDuration to
CreateAnimationSettings.

This leaks settings. Make CreateAnimationSettings return a
scoped_ptr<ScopedLayerAnimationSettings> so that ownership is clearer and less
error prone.

[Mr4D (OOO till 08-26), 2012/10/22 20:27:23]

You know that this contradicts somehow your previous request/suggestion? Done
(differently).

[sky, 2012/10/22 21:26:21]

This suggestion:
ui::ScopedLayerAnimationSettings*
CreateScopedAnimatorAnimationSettings(aura::Window* window) {
  if (animations disabled)
    return NULL;
  return new ...;
}

? If so, the core of the idea is still there. My new suggestion was based on two
things you missed: NULL check and leaking. I'm still fine with original
suggestion, but your patch crashes (and leaks). It seemed good to make it less
error prone.

+          base::TimeDelta::FromMilliseconds(kWindowAutoMoveDurationMS));
+      other_shown_window->SetBounds(bounds);
+    }
+  }
+}
+
+void RearrangeVisibleWindowOnShow(aura::Window* added_window) {
+  // Early bail out if the window isn't visible of the feature is unwanted.

[sky, 2012/10/22 16:40:50]

of -> or
unwanted -> disabled

But this description just matches the code, so I would remove it or describe why
we do nothing when not visible, eg:
// If the window isn't visible it was just added. Invisible windows have no
effect on auto window management.

[Mr4D (OOO till 08-26), 2012/10/22 20:27:23]

Done.

+  if (!added_window->TargetVisibility() ||
+      CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kAshDisableAutoWindowPlacement))
+    return;
+  // Find a single open managed window.
+  aura::Window* other_shown_window = NULL;
+  gfx::Rect work_area;
+  if (!GetOtherVisibleAndManageableWindow(added_window,
+                                          &work_area,
+                                          &other_shown_window)) {
+    // It could be that this window is the first window joining the workspace.
+    if (!WindowPositionCanBeManaged(added_window) || other_shown_window)
+      return;
+    // If so we have to make sure it is centered.
+    gfx::Rect bounds = added_window->bounds();
+    bounds.set_x((work_area.width() - bounds.width()) / 2);
+    added_window->SetBounds(bounds);
+    return;
+  }
+
+  // When going from one to two windows both windows loose their "positioned
+  // by user" flags.
+  ash::wm::SetUserHasChangedWindowPositionOrSize(added_window, false);
+  ash::wm::SetUserHasChangedWindowPositionOrSize(other_shown_window, false);
+
+  if (WindowPositionCanBeManaged(other_shown_window)) {
+    // Push away the other window.
+    gfx::Rect other_bounds = other_shown_window->bounds();
+    bool move_right = other_bounds.CenterPoint().x() < work_area.width() / 2;
+    if (MoveRectToOneSide(work_area.width(), move_right, &other_bounds)) {
+      if (other_bounds != other_shown_window->GetTargetBounds()) {
+        ui::ScopedLayerAnimationSettings* settings = CreateAnimationSettings(
+            other_shown_window);
+        settings->SetTransitionDuration(
+            base::TimeDelta::FromMilliseconds(kWindowAutoMoveDurationMS));
+        other_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(), !move_right, &added_bounds))
+      added_window->SetBounds(added_bounds);
+  }
+}
+
+} // namespace internal
+} // namespace ash
+