Improving window auto management between workspaces

ash/wm/workspace/workspace_layout_manager2_unittest.cc

Index: ash/wm/workspace/workspace_layout_manager2_unittest.cc
diff --git a/ash/wm/workspace/workspace_layout_manager2_unittest.cc b/ash/wm/workspace/workspace_layout_manager2_unittest.cc
index 59cab50ed8e5550ac3c77b824963fad06af2ab6a..17aa0859520ef810da156cfa4c91d47b6a10bbfb 100644
--- a/ash/wm/workspace/workspace_layout_manager2_unittest.cc
+++ b/ash/wm/workspace/workspace_layout_manager2_unittest.cc
@@ -9,6 +9,7 @@
 #include "ash/wm/property_util.h"
 #include "ash/wm/shelf_layout_manager.h"
 #include "ash/wm/window_util.h"
+#include "base/string_number_conversions.h"
 #include "ui/aura/root_window.h"
 #include "ui/aura/test/test_windows.h"
 #include "ui/aura/window.h"
@@ -134,6 +135,212 @@ TEST_F(WorkspaceLayoutManager2Test, WindowShouldBeOnScreenWhenAdded) {
   EXPECT_TRUE(out_window->bounds().Intersects(root_window_bounds));
 }
 
-}  // namespace
+// Test the basic auto placement of one and or two window.

[sky, 2012/10/17 15:59:38]

Since your patch doesn't touch WorkspaceLayoutManager2 these tests shouldn't be
here. Either put them in with the other workspacemanager2 tests, or create a new
file auto_window_management_unittest

[Mr4D (OOO till 08-26), 2012/10/17 18:43:56]

Moved into WorkspaceManager2Test since the WorkspaceManager is also tested with
this (there are calls through it into this auto window manager after all).

+TEST_F(WorkspaceLayoutManager2Test, BasicAutoPlacing) {
+  // Test 1: In case there is no manageable window, no window should shift.
+
+  scoped_ptr<aura::Window> window1(
+      aura::test::CreateTestWindowWithId(0, NULL));
+  window1->SetBounds(gfx::Rect(16, 32, 640, 320));
+  gfx::Rect desktop_area = window1->parent()->bounds();
+
+  scoped_ptr<aura::Window> window2(
+      aura::test::CreateTestWindowWithId(1, NULL));
+  // Note: we need to perform an activation change to trigger the update.

[sky, 2012/10/17 15:59:38]

Then why hide and show? Use Activate() if you need to explicitly activate. That
said, why does the activate matter here? This code only has the desktop
workspace so you shouldn't need to explicitly activate anything. Same comment
for the rest of the files.

[Mr4D (OOO till 08-26), 2012/10/17 18:43:56]

Added comments since the previous comments were not precise enough and the pure
Activation will not work.

+  // As such we initially hide the window and then showing it again.
+  window2->Hide();
+  window2->SetBounds(gfx::Rect(32, 48, 256, 512));
+  window2->Show();
+
+  // Check the initial position of the windows is unchanged.
+  EXPECT_EQ("16,32 640x320", window1->bounds().ToString());
+  EXPECT_EQ("32,48 256x512", window2->bounds().ToString());
+
+  // Remove the second window and make sure that the first window
+  // does NOT get centered.
+  window2.reset();
+  EXPECT_EQ("16,32 640x320", window1->bounds().ToString());
+
+  // Test 2: Set up two managed windows and check their auto positioning.

[sky, 2012/10/17 15:59:38]

Is there a compelling reason to combine so many assertions in a single test? It
makes for a test that is harder to reason about.

[Mr4D (OOO till 08-26), 2012/10/17 18:43:56]

This is a "simulation: of a standard workflow" + it goes over several state
transitions which set up would need a lot of coding otherwise. If you feel
strongly I dissect it into 4 separate tests.

+  ash::wm::SetWindowPositionManaged(window1.get(), true);
+  scoped_ptr<aura::Window> window3(
+      aura::test::CreateTestWindowWithId(2, NULL));
+  ash::wm::SetWindowPositionManaged(window3.get(), true);
+  // Note: we need to perform an activation change to trigger the update.
+  // As such we initially hide the window and then showing it again.
+  window3->Hide();
+  window3->SetBounds(gfx::Rect(32, 48, 256, 512));
+  window3->Show();
+  // |window1| should be flush right and |window3| flush left.
+  EXPECT_EQ(base::IntToString(
+                desktop_area.width() - window1->bounds().width()) +
+            ",32 640x320", window1->bounds().ToString());
+  EXPECT_EQ("0,48 256x512", window3->bounds().ToString());
+
+  // After removing |window3|, |window1| should be centered again.
+  window3.reset();
+  EXPECT_EQ(
+      base::IntToString(
+          (desktop_area.width() - window1->bounds().width()) / 2) +
+      ",32 640x320", window1->bounds().ToString());
+
+  // Test 3: Set up a manageable and a non manageable window and check
+  // positioning.
+  scoped_ptr<aura::Window> window4(
+      aura::test::CreateTestWindowWithId(3, NULL));
+  // Note: we need to perform an activation change to trigger the update.
+  // As such we initially hide the window and then showing it again.
+  window1->Hide();
+  window1->SetBounds(gfx::Rect(16, 32, 640, 320));
+  window4->SetBounds(gfx::Rect(32, 48, 256, 512));
+  window1->Show();
+  // |window1| should be centered and |window4| untouched.
+  EXPECT_EQ(
+      base::IntToString(
+          (desktop_area.width() - window1->bounds().width()) / 2) +
+      ",32 640x320", window1->bounds().ToString());
+  EXPECT_EQ("32,48 256x512", window4->bounds().ToString());
+
+  // Test 4: Once a window gets moved by a user it does not get repositioned.
+  ash::wm::SetUserHasChangedWindowPositionOrSize(window1.get(), true);
+  window1->SetBounds(gfx::Rect(52, 32, 640, 320));
+  // Note: we need to perform an activation change to trigger the update.
+  // As such we initially hide the window and then showing it again.
+  window4->Hide();
+  window4->Show();
+  EXPECT_EQ("52,32 640x320", window1->bounds().ToString());
+  EXPECT_EQ("32,48 256x512", window4->bounds().ToString());
+
+  // Test5: A single manageable window should get centered.
+  window4.reset();
+  ash::wm::SetUserHasChangedWindowPositionOrSize(window1.get(), false);
+  window1->Hide();
+  window1->Show();
+  // |window1| should be centered.
+  EXPECT_EQ(
+      base::IntToString(
+          (desktop_area.width() - window1->bounds().width()) / 2) +
+      ",32 640x320", window1->bounds().ToString());
+}
+
+// Test that a window from normal to minimize will repos the remaining.
+TEST_F(WorkspaceLayoutManager2Test, ToMinimizeRepositionsRemaining) {
+  scoped_ptr<aura::Window> window1(
+      aura::test::CreateTestWindowWithId(0, NULL));
+  ash::wm::SetWindowPositionManaged(window1.get(), true);
+  window1->SetBounds(gfx::Rect(16, 32, 640, 320));
+  gfx::Rect desktop_area = window1->parent()->bounds();
+
+  scoped_ptr<aura::Window> window2(
+      aura::test::CreateTestWindowWithId(1, NULL));
+  ash::wm::SetWindowPositionManaged(window2.get(), true);
+  window2->SetBounds(gfx::Rect(32, 48, 256, 512));
+
+  ash::wm::MinimizeWindow(window1.get());
+
+  // |window2| should be centered now.
+  EXPECT_TRUE(window2->IsVisible());
+  EXPECT_TRUE(ash::wm::IsWindowNormal(window2.get()));
+  EXPECT_EQ(base::IntToString(
+                (desktop_area.width() - window2->bounds().width()) / 2) +
+            ",48 256x512", window2->bounds().ToString());
+
+  ash::wm::RestoreWindow(window1.get());
+  // |window1| should be flush right and |window3| flush left.
+  EXPECT_EQ(base::IntToString(
+                desktop_area.width() - window1->bounds().width()) +
+            ",32 640x320", window1->bounds().ToString());
+  EXPECT_EQ("0,48 256x512", window2->bounds().ToString());
+}
+
+// Test that minimizing an initially maximized window will repos the remaining.
+TEST_F(WorkspaceLayoutManager2Test, MaxToMinRepositionsRemaining) {
+  scoped_ptr<aura::Window> window1(
+      aura::test::CreateTestWindowWithId(0, NULL));
+  ash::wm::SetWindowPositionManaged(window1.get(), true);
+  gfx::Rect desktop_area = window1->parent()->bounds();
+
+  scoped_ptr<aura::Window> window2(
+      aura::test::CreateTestWindowWithId(1, NULL));
+  ash::wm::SetWindowPositionManaged(window2.get(), true);
+  window2->SetBounds(gfx::Rect(32, 48, 256, 512));
 
+  ash::wm::MaximizeWindow(window1.get());
+  ash::wm::MinimizeWindow(window1.get());
+
+  // |window2| should be centered now.
+  EXPECT_TRUE(window2->IsVisible());
+  EXPECT_TRUE(ash::wm::IsWindowNormal(window2.get()));
+  EXPECT_EQ(base::IntToString(
+                (desktop_area.width() - window2->bounds().width()) / 2) +
+            ",48 256x512", window2->bounds().ToString());
+}
+
+// Test that nomral, maximize, minimizing will repos the remaining.
+TEST_F(WorkspaceLayoutManager2Test, NormToMaxToMinRepositionsRemaining) {
+  scoped_ptr<aura::Window> window1(
+      aura::test::CreateTestWindowWithId(0, NULL));
+  window1->SetBounds(gfx::Rect(16, 32, 640, 320));
+  ash::wm::SetWindowPositionManaged(window1.get(), true);
+  gfx::Rect desktop_area = window1->parent()->bounds();
+
+  scoped_ptr<aura::Window> window2(
+      aura::test::CreateTestWindowWithId(1, NULL));
+  ash::wm::SetWindowPositionManaged(window2.get(), true);
+  window2->SetBounds(gfx::Rect(32, 48, 256, 512));
+
+  window1->Hide();
+  window1->Show();
+
+  // |window1| should be flush right and |window3| flush left.
+  EXPECT_EQ(base::IntToString(
+                desktop_area.width() - window1->bounds().width()) +
+            ",32 640x320", window1->bounds().ToString());
+  EXPECT_EQ("0,48 256x512", window2->bounds().ToString());
+
+  ash::wm::MaximizeWindow(window1.get());
+  ash::wm::MinimizeWindow(window1.get());
+
+  // |window2| should be centered now.
+  EXPECT_TRUE(window2->IsVisible());
+  EXPECT_TRUE(ash::wm::IsWindowNormal(window2.get()));
+  EXPECT_EQ(base::IntToString(
+                (desktop_area.width() - window2->bounds().width()) / 2) +
+            ",48 256x512", window2->bounds().ToString());
+}
+
+// Test that nomral, maximize, normal will repos the remaining.
+TEST_F(WorkspaceLayoutManager2Test, NormToMaxToNormRepositionsRemaining) {
+  scoped_ptr<aura::Window> window1(
+      aura::test::CreateTestWindowWithId(0, NULL));
+  window1->SetBounds(gfx::Rect(16, 32, 640, 320));
+  ash::wm::SetWindowPositionManaged(window1.get(), true);
+  gfx::Rect desktop_area = window1->parent()->bounds();
+
+  scoped_ptr<aura::Window> window2(
+      aura::test::CreateTestWindowWithId(1, NULL));
+  ash::wm::SetWindowPositionManaged(window2.get(), true);
+  window2->SetBounds(gfx::Rect(32, 48, 256, 512));
+
+  window1->Hide();
+  window1->Show();
+
+  // |window1| should be flush right and |window3| flush left.
+  EXPECT_EQ(base::IntToString(
+                desktop_area.width() - window1->bounds().width()) +
+            ",32 640x320", window1->bounds().ToString());
+  EXPECT_EQ("0,48 256x512", window2->bounds().ToString());
+
+  ash::wm::MaximizeWindow(window1.get());
+  ash::wm::RestoreWindow(window1.get());
+
+  // |window1| should be flush right and |window2| flush left.
+  EXPECT_EQ(base::IntToString(
+                desktop_area.width() - window1->bounds().width()) +
+            ",32 640x320", window1->bounds().ToString());
+  EXPECT_EQ("0,48 256x512", window2->bounds().ToString());
+}
+
+
+}  // namespace
 }  // namespace ash