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 (!window->bounds().IsEmpty() && wm::IsWindowPositionManaged(window));

[sky, 2012/10/18 16:36:10]

What made you add the !IsEmpty() check? Is there really code that is trying to
hide/show an empty window? Even if there is, does it effect this code in anyway?

[Mr4D (OOO till 08-26), 2012/10/19 20:46:05]

Think about this: Suddenly your window moves to the right but nothing comes on
the left. I have found this check for empty somewhere else and I think it is a
good check. Essentially this window is as invisible as a window with a
TargetVisibility of 0 and should therefore not have any impact on the placement.
Don't you agree? Since the test was somewhere else I am fairly certain that
there might be special cases where this can get a problem. And since this is
true for all windows (added/removed/remaining) this test is in here and not the
other function.

[sky, 2012/10/19 22:00:32]

I'm reminded of your comment earlier today about AOL forcing 29 paints just to
be sure. The problem with this line of thinking is where do you draw the line?
Why not a NULL check for window here too? There are tons of other bad states
things could get into and this is just one.

In general we don't code defensively. There are good arguments to be made in
both directions, but that happens to be style we have.

A 0x0 window is a bug some where and shouldn't happen. We shouldn't have to
resort to tons of errors checks for cases that 'could' come up.

[Mr4D (OOO till 08-26), 2012/10/19 22:54:41]

I don't think that the example fits here. This code is preventing bad things
from happening not intentionally doing things multiple times to make sure that
the result might be now correct.

I was looking over the code and looked for IsEmpty() references and have only
found them in conjunction with the layout manager (which looks for setting the
bounds recursively if they are empty), the tray and a few other places. I think
therefore that it must be safe to remove this. Done.

+}
+
+// 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 window location management. If there is more then one windows,

[sky, 2012/10/18 16:36:10]

windows -> window

[Mr4D (OOO till 08-26), 2012/10/19 20:46:05]

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
+// 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;
+}
+
+} // namespace
+
+void RearrangeVisibleWindowOnHide(const aura::Window* removed_window) {
+  // 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;
+
+  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->bounds()) {

[sky, 2012/10/18 16:36:10]

Do you really need the bounds != bounds check here? If the bounds is the same
won't this effectively result in nothing? Yes, you might try to animate but it
shouldn't cause a problem, right?

[Mr4D (OOO till 08-26), 2012/10/19 20:46:05]

You told me to add this because otherwise I might end up with a double SetBounds
and the animation would get aborted. Isn't that true anymore? Sure it could get
folded into the next if to suppress the animation and set the bounds instead
directly, but that would make no sense.

[sky, 2012/10/19 22:00:32]

Remember bounds is the current bounds. If you really want to check this (see my
earlier comments about my thinking here) then you need to check the target
bounds, not the current ones.

[Mr4D (OOO till 08-26), 2012/10/19 22:54:41]

That answers that question I had in my back of my mind for a while. Done.

+      if (!other_shown_window->GetProperty(

[sky, 2012/10/18 16:36:10]

Simplify this by creating a function like:

base::TimeDelta AdjustAnimationTime(int time_ms) {
  if (other_shown_window->GetProperty(aura::client::kAnimationsDisabledKey) ||
     
CommandLine::ForCurrentProcess()->HasSwitch(ash::switches::kAshWindowAnimationsDisabled)
    return base::TimeDelta();
  return base::TimeDelta::FromMilliseconds(time_ms);
}

In fact you can put this in window_animations if you want.

That way you can remove the conditional branch and always create the animator.
Same comment below.

[sky, 2012/10/18 23:47:14]

The other approach that avoids unnecessary allocation is:

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

scoped_ptr<ui::ScopedLayerAnimationSettings> settings(CreateScoped...);
other_shown_window->SetBounds(bounds);

You could make the first one return a scoped_ptr to make ownership clearer, but
you get the idea.

[Mr4D (OOO till 08-26), 2012/10/19 22:54:41]

Done.

+              aura::client::kAnimationsDisabledKey) &&
+          !CommandLine::ForCurrentProcess()->HasSwitch(
+               ash::switches::kAshWindowAnimationsDisabled)) {
+        ui::LayerAnimator* animator =
+            other_shown_window->layer()->GetAnimator();
+        ui::ScopedLayerAnimationSettings animation_setter(animator);
+        animation_setter.SetTransitionDuration(
+            base::TimeDelta::FromMilliseconds(kWindowAutoMoveDurationMS));
+        // SetBounds needs to be in this closure, so don't combine with the
+        // else case.
+        other_shown_window->SetBounds(bounds);
+      } else
+        other_shown_window->SetBounds(bounds);
+    }
+  }
+}
+
+void RearrangeVisibleWindowOnShow(aura::Window* added_window) {
+  // Early bail out if the window isn't visible.
+  if (!added_window->TargetVisibility())
+    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;
+  }
+
+  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->bounds()) {
+        if (!other_shown_window->GetProperty(
+                aura::client::kAnimationsDisabledKey) &&
+            !CommandLine::ForCurrentProcess()->HasSwitch(
+                 ash::switches::kAshWindowAnimationsDisabled)) {
+          ui::LayerAnimator* animator =
+              other_shown_window->layer()->GetAnimator();
+          ui::ScopedLayerAnimationSettings animation_setter(animator);
+          animation_setter.SetTransitionDuration(
+              base::TimeDelta::FromMilliseconds(kWindowAutoMoveDurationMS));
+          // SetBounds needs to be in this closure, so don't combine with the
+          // else case.
+          other_shown_window->SetBounds(other_bounds);
+        } else
+          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
+