Improving window auto management between workspaces

ash/wm/workspace/workspace_manager2.cc

Index: ash/wm/workspace/workspace_manager2.cc
diff --git a/ash/wm/workspace/workspace_manager2.cc b/ash/wm/workspace/workspace_manager2.cc
index 805dd183584e96b98757554ab279df8c7c968a0a..67f1cb6dce3ef16520991713376622b1d7d28a62 100644
--- a/ash/wm/workspace/workspace_manager2.cc
+++ b/ash/wm/workspace/workspace_manager2.cc
@@ -14,6 +14,7 @@
 #include "ash/wm/property_util.h"
 #include "ash/wm/shelf_layout_manager.h"
 #include "ash/wm/window_animations.h"
+#include "ash/wm/window_cycle_controller.h"
 #include "ash/wm/window_properties.h"
 #include "ash/wm/window_util.h"
 #include "ash/wm/workspace/workspace_layout_manager2.h"
@@ -30,6 +31,8 @@
 #include "ui/base/ui_base_types.h"
 #include "ui/compositor/layer.h"
 #include "ui/compositor/layer_animator.h"
+#include "ui/compositor/scoped_layer_animation_settings.h"
+#include "ui/gfx/screen.h"
 
 DECLARE_WINDOW_PROPERTY_TYPE(ash::internal::Workspace2*);
 
@@ -42,6 +45,157 @@ DEFINE_WINDOW_PROPERTY_KEY(Workspace2*, kWorkspaceKey, NULL);
 
 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/11 19:50:54]

Move all this logic out of this class. Maybe into the file window_management or
auto_window_management.

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Done.

+    base::TimeDelta::FromMilliseconds(125);
+
+// Check if the given |window| has an impact on position manageability since it
+// occupies screen space. Note: The minimized state is not included in this

[sky, 2012/10/11 19:50:54]

'since it occupies screen space'?

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Hmm. Yeah, Somehow I have re-written this comment too often. Done.

+// test since a window which just got minimized might have an impact on the
+// window ordering.
+bool HasWindowImpactOnPositionManaging(const aura::Window* window) {

[sky, 2012/10/11 19:50:54]

Name this WindowHasImpactOnPositioning

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Done.

+  return (!wm::IsWindowMaximized(window) &&

[sky, 2012/10/11 19:50:54]

Don't you need to check visibility here? And why the maximized check?

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Visibility check: Made sense, so I have tried it and it did not work. I can only
assume that the windows are in a transitional state when I come here and have to
reposition.

Maximized check: A maximized window makes no sense in this scenario. More so:
Note that this test is also used for the initial check. If the passed window is
maximized there is no need for any further processing.

[sky, 2012/10/12 14:27:04]

Can you point me at a test where what I have suggested fails so that I can
investigate?

[Mr4D (OOO till 08-26), 2012/10/12 23:21:30]

Because when switching from maximize to minimize it will fail. If you still want
to investigate (and I hope my memory does not fail me)
WorkspaceLayoutManager2Test.MaxToMinRepositionsRemaining (and some more will
fail when you try to do it there).

+          !window->bounds().IsEmpty() &&
+          wm::IsWindowPositionManageable(window));
+}
+
+// Check if a given |window| is considered to be an auto location manageable
+// window or not.
+bool IsWindowPositionManageable(const aura::Window* window) {
+  return (HasWindowImpactOnPositionManaging(window) &&
+          !wm::IsWindowMinimized(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,
+// false get 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 (!HasWindowImpactOnPositionManaging(window))
+    return false;
+
+  const std::vector<aura::Window*> windows =
+      ash::WindowCycleController::BuildWindowList(NULL);
+
+  if (windows.empty())
+    return false;
+
+  // Get our work area.
+  *work_area = window->parent()->bounds();
+  work_area->Inset(gfx::Screen::GetDisplayMatching(
+      *work_area).GetWorkAreaInsets());
+
+  // Find a single open browser window.
+  for (size_t i = 0; i < windows.size(); i++) {
+    aura::Window* j = windows[i];
+    if (j && window != j &&

[sky, 2012/10/11 19:50:54]

You should not need a check for validity of j.

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Done.

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

[sky, 2012/10/11 19:50:54]

Why the work_area check?

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

To make sure that the window is on this screen (and not another one) -
WindowCycleController will possibly return all top level windows, no matter on
which screen they are? Unless I misunderstood something here.

[sky, 2012/10/12 14:27:04]

Actually, why are you using the WindowCycleController here? You should be
checking the children of the parent.

[Mr4D (OOO till 08-26), 2012/10/12 23:21:30]

I thought it might be better to only include "noteworthy" windows to begin with.
But I would guess that the introduction of the flag makes this not really
necessary anymore. Done.

+        j->IsVisible() && HasWindowImpactOnPositionManaging(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 |work_area_width| to the

[sky, 2012/10/11 19:50:54]

work_area_width is a misleading name since it isn't the work area.

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Done.

+// direction. If |move_right| is true, the rectangle gets moved to the right
+// corner, otherwise to the left one.
+bool MoveRect(int work_area_width, gfx::Rect& bounds, bool move_right) {

[sky, 2012/10/11 19:50:54]

MoveRect is an extremely generic name. How about MoveToSide.

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Named it "MoveRectToOneSide". Done.

+  if (move_right) {
+    if (work_area_width > bounds.right()) {
+      bounds.set_x(work_area_width - bounds.width());
+      return true;
+    }
+  } else {
+    if (0 < bounds.x()) {
+      bounds.set_x(0);
+      return true;
+    }
+  }
+  return false;
+}
+
+// Check if after removal of the given |removed_window| an automated desktop
+// location management can be performed and rearrange accordingly
+void RearrangeVisibleWindowOnRemove(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 (shown_window && IsWindowPositionManageable(shown_window)) {

[sky, 2012/10/11 19:50:54]

Why the shown_window test? Doesn't GetOtherVisibleAndManageableWindow only
return true if shown_window is set?

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Sorry. My kind of defensive programming. Done.

+    // Center the window (only in x).
+    gfx::Rect bounds = shown_window->bounds();
+    bounds.set_x((work_area.width() - bounds.width()) / 2);
+    ui::LayerAnimator* animator = shown_window->layer()->GetAnimator();
+    animator->set_preemption_strategy(

[sky, 2012/10/11 19:50:54]

Why are you setting the preemption_strategy?

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Because I have seen it being used in a comparable case. And - more importantly -
when I do not set it, no animation is shown.

[sky, 2012/10/12 14:27:04]

You need to investigate this. There should be no need for it and blindly setting
it isn't right. Additionally setting the property like you have done here
changes the animator for this window after the animation is done.

[Mr4D (OOO till 08-26), 2012/10/12 23:21:30]

Yes, I investigated it for 2.5 hours and it was exactly as it is meant to be. I
will send you a separate eMail.

+        ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
+
+    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
+// location management can be performed and rearrange accordingly.
+void RearrangeVisibleWindowOnJoining(aura::Window* added_window) {

[sky, 2012/10/11 19:50:54]

How about RearrangeVisibleWindowOnShow and RearranbleVisibleWindowOnHide for the
names.

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Done.

+  // Find a single open browser window.
+  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)

[sky, 2012/10/11 19:50:54]

How could shown_window be non-null here?

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Read the comment for GetOtherVisibleAndManageableWindow: If there are more then
one suitable windows found, the function returns false to indidcate that the
window should not get managed. However... This one special case (the window
itself needs to be repositioned) has still to be performed.

+      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 (shown_window && 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 (MoveRect(work_area.width(), other_bounds, move_right)) {
+      ui::LayerAnimator* animator = shown_window->layer()->GetAnimator();
+      animator->set_preemption_strategy(
+          ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
+
+      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 (MoveRect(work_area.width(), added_bounds, !move_right))
+      added_window->SetBounds(added_bounds);
+  }
+}
+
 // Changes the parent of |window| and all its transient children to
 // |new_parent|. If |stack_beneach| is non-NULL all the windows are stacked
 // beneath it.
@@ -490,6 +644,7 @@ void WorkspaceManager2::OnWindowAddedToWorkspace(Workspace2* workspace,
 void WorkspaceManager2::OnWillRemoveWindowFromWorkspace(Workspace2* workspace,
                                                         Window* child) {
   child->ClearProperty(kWorkspaceKey);
+  RearrangeVisibleWindowOnRemove(child);
 }
 
 void WorkspaceManager2::OnWindowRemovedFromWorkspace(Workspace2* workspace,
@@ -503,8 +658,13 @@ void WorkspaceManager2::OnWorkspaceChildWindowVisibilityChanged(
     Window* child) {
   if (workspace->ShouldMoveToPending())
     MoveWorkspaceToPendingOrDelete(workspace, NULL, ANIMATE_NEW);
-  else if (workspace == active_workspace_)
+  else if (workspace == active_workspace_) {
     UpdateShelfVisibility();
+    if (child->IsVisible())

[sky, 2012/10/11 19:50:54]

Why make this depend upon whether the workspace is active?

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

There are two reasons:
1.) (the logical one) Why should you re-order windows on the visible desktop
when the state change is happening on a non visible workspace?
2.) (since I was already wondering the same before I was sending out this CL I
have tried it): The result was that in several occasions I have seen odd
behavior - like e.g. on start up some windows shifted around, etc.

[sky, 2012/10/12 14:27:04]

You should rearrange in the desktop the window is in.
Again, this likely indicates another problem that needs to be investigate.

[Mr4D (OOO till 08-26), 2012/10/12 23:21:30]

Changed the algorithm from my "on access" method to the requested "upfront"
change method.

+      RearrangeVisibleWindowOnJoining(child);
+    else
+      RearrangeVisibleWindowOnRemove(child);
+  }
 }
 
 void WorkspaceManager2::OnWorkspaceWindowChildBoundsChanged(
@@ -524,6 +684,11 @@ void WorkspaceManager2::OnWorkspaceWindowShowStateChanged(
   if (wm::IsWindowMinimized(child)) {
     if (workspace->ShouldMoveToPending())
       MoveWorkspaceToPendingOrDelete(workspace, NULL, ANIMATE_NEW);
+    // When a window gets minimized, we have to re-arrange the visible windows
+    // here. This especially applies when you have two normal windows, maximize
+    // one and then minimize it. The other window on the desktop needs then to
+    // get centered.
+    RearrangeVisibleWindowOnRemove(child);

[sky, 2012/10/11 19:50:54]

Minimize also results in OnWorkspaceChildWindowVisibilityChanged being invoked.
Are you sure you need this?

[Mr4D (OOO till 08-26), 2012/10/12 00:29:18]

Yes I am. I was fixing several special cases I have seen with transitioning
between minimized and maximized and normal which caused problems. See comment
above. 

[sky, 2012/10/12 14:27:04]

Can you point me at the tests that fail when this is moved?

[Mr4D (OOO till 08-26), 2012/10/12 23:21:30]

Removed.

     DCHECK(!old_layer);
   } else {
     // Set of cases to deal with: