Added arrow key navigation to Overview Mode

ash/wm/overview/window_selector.cc

Index: ash/wm/overview/window_selector.cc
diff --git a/ash/wm/overview/window_selector.cc b/ash/wm/overview/window_selector.cc
index 04fbccb4283a9010e31878dcca72e0f254e9e2c1..22e462460206a3d24e2dd9da69f60515fdbf6e5a 100644
--- a/ash/wm/overview/window_selector.cc
+++ b/ash/wm/overview/window_selector.cc
@@ -10,11 +10,11 @@
 #include "ash/ash_switches.h"
 #include "ash/metrics/user_metrics_recorder.h"
 #include "ash/root_window_controller.h"
-#include "ash/screen_util.h"
 #include "ash/shell.h"
 #include "ash/shell_window_ids.h"
 #include "ash/switchable_windows.h"
 #include "ash/wm/overview/scoped_transform_overview_window.h"
+#include "ash/wm/overview/window_grid.h"
 #include "ash/wm/overview/window_selector_delegate.h"
 #include "ash/wm/overview/window_selector_item.h"
 #include "ash/wm/overview/window_selector_panels.h"
@@ -30,7 +30,6 @@
 #include "ui/aura/window.h"
 #include "ui/aura/window_event_dispatcher.h"
 #include "ui/aura/window_observer.h"
-#include "ui/compositor/layer_animation_observer.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/events/event.h"
 #include "ui/gfx/screen.h"
@@ -43,81 +42,6 @@ namespace ash {
 
 namespace {
 
-// Conceptually the window overview is a table or grid of cells having this
-// fixed aspect ratio. The number of columns is determined by maximizing the
-// area of them based on the number of windows.
-const float kCardAspectRatio = 4.0f / 3.0f;
-
-// In the conceptual overview table, the window margin is the space reserved
-// around the window within the cell. This margin does not overlap so the
-// closest distance between adjacent windows will be twice this amount.
-const int kWindowMargin = 30;
-
-// The minimum number of cards along the major axis (i.e. horizontally on a
-// landscape orientation).
-const int kMinCardsMajor = 3;
-
-// A comparator for locating a given target window.
-struct WindowSelectorItemComparator
-    : public std::unary_function<WindowSelectorItem*, bool> {
-  explicit WindowSelectorItemComparator(const aura::Window* target_window)
-      : target(target_window) {
-  }
-
-  bool operator()(WindowSelectorItem* window) const {
-    return window->HasSelectableWindow(target);
-  }
-
-  const aura::Window* target;
-};
-
-// An observer which holds onto the passed widget until the animation is
-// complete.
-class CleanupWidgetAfterAnimationObserver : public ui::LayerAnimationObserver {
- public:
-  explicit CleanupWidgetAfterAnimationObserver(
-      scoped_ptr<views::Widget> widget);
-
-  // ui::LayerAnimationObserver:
-  virtual void OnLayerAnimationEnded(
-      ui::LayerAnimationSequence* sequence) OVERRIDE;
-  virtual void OnLayerAnimationAborted(
-      ui::LayerAnimationSequence* sequence) OVERRIDE;
-  virtual void OnLayerAnimationScheduled(
-      ui::LayerAnimationSequence* sequence) OVERRIDE;
-
- private:
-  virtual ~CleanupWidgetAfterAnimationObserver();
-
-  scoped_ptr<views::Widget> widget_;
-
-  DISALLOW_COPY_AND_ASSIGN(CleanupWidgetAfterAnimationObserver);
-};
-
-CleanupWidgetAfterAnimationObserver::CleanupWidgetAfterAnimationObserver(
-    scoped_ptr<views::Widget> widget)
-    : widget_(widget.Pass()) {
-  widget_->GetNativeWindow()->layer()->GetAnimator()->AddObserver(this);
-}
-
-CleanupWidgetAfterAnimationObserver::~CleanupWidgetAfterAnimationObserver() {
-  widget_->GetNativeWindow()->layer()->GetAnimator()->RemoveObserver(this);
-}
-
-void CleanupWidgetAfterAnimationObserver::OnLayerAnimationEnded(
-    ui::LayerAnimationSequence* sequence) {
-  delete this;
-}
-
-void CleanupWidgetAfterAnimationObserver::OnLayerAnimationAborted(
-    ui::LayerAnimationSequence* sequence) {
-  delete this;
-}
-
-void CleanupWidgetAfterAnimationObserver::OnLayerAnimationScheduled(
-    ui::LayerAnimationSequence* sequence) {
-}
-
 // A comparator for locating a selectable window given a targeted window.
 struct WindowSelectorItemTargetComparator
     : public std::unary_function<WindowSelectorItem*, bool> {
@@ -164,7 +88,8 @@ WindowSelector::WindowSelector(const WindowList& windows,
     : delegate_(delegate),
       restore_focus_window_(aura::client::GetFocusClient(
           Shell::GetPrimaryRootWindow())->GetFocusedWindow()),
-      ignore_activations_(false) {
+      ignore_activations_(false),
+      root_index_(0) {
   DCHECK(delegate_);
 
   if (restore_focus_window_)
@@ -283,8 +208,52 @@ void WindowSelector::OnKeyEvent(ui::KeyEvent* event) {
   if (event->type() != ui::ET_KEY_PRESSED)
     return;
 
-  if (event->key_code() == ui::VKEY_ESCAPE)
-    CancelSelection();
+  // Dummy value so that the compiler does not complain about uninitialized

[tdanderson, 2014/05/28 16:01:19]

Comment not needed.

[Nina, 2014/05/29 17:12:24]

Removed.

+  // values.
+  Direction direction = WindowSelector::UP;
+  bool moving = false;
+  switch (event->key_code()) {
+    case ui::VKEY_ESCAPE:
+      CancelSelection();
+      break;
+    case ui::VKEY_UP:
+      direction = WindowSelector::UP;

[tdanderson, 2014/05/28 16:01:19]

Consider defining a private helper with a single parameter of type Direction,
and moving lines 243-256 out of OnKeyEvent() and into the new helper. This would
eliminate the need for the variables |direction| and |moving|, and would also
eliminate the need to initialize something to a dummy value (which is nice to
avoid whenever possible to reduce the risk of introducing new bugs).

[Nina, 2014/05/29 17:12:24]

Yep, this is much nicer. Done.

+      moving = true;
+      break;
+    case ui::VKEY_DOWN:
+      direction = WindowSelector::DOWN;
+      moving = true;
+      break;
+    case ui::VKEY_RIGHT:
+      direction = WindowSelector::RIGHT;
+      moving = true;
+      break;
+    case ui::VKEY_LEFT:
+      direction = WindowSelector::LEFT;
+      moving = true;
+      break;
+    case ui::VKEY_RETURN:
+      SelectWindow(
+          grid_list_[root_index_]->SelectedWindow()->SelectionWindow());
+      break;
+    default:
+      // Not a key we are interested in.
+      break;
+  }
+  if (moving) {

[tdanderson, 2014/05/28 16:01:19]

It seems you can do a bit of simplification here by removing the |moving| flag
and using return instead of break for each case that did not set |moving| to
true.

[Nina, 2014/05/29 17:12:24]

Refactoring made this not necessary anymore.

+    bool change_root = false;

[tdanderson, 2014/05/28 16:01:19]

|change_root| is not necessary. Just say if (grid_list_...) { ... }

[Nina, 2014/05/29 17:12:24]

I thought it was a bit harder to read this way, but a better placed comment
solves this. Done.

+    change_root =
+        grid_list_[root_index_]->Move(direction);
+    if (change_root) {

[tdanderson, 2014/05/28 16:01:19]

Your documentation for WindowGrid::Move() says that it "Moves the selection
widget to the specified |direction|. Returns false if, after moving, the widget
is out of bounds."

Doesn't that imply that you should enter this conditional when
!grid_list_[root_index_]->Move(direction) instead? (i.e., the function call to
Move() returned false, which means we're switching root windows)?

[Nina, 2014/05/29 17:12:24]

Modified the comment.

+      if (root_index_ >= grid_list_.size() - 1)
+        root_index_ = 0;
+      else
+        root_index_++;
+      // The grid reported that the movement command corresponds to the next
+      // root window, call Move() on it to initialize the selection widget.
+      grid_list_[root_index_]->Move(direction);

[tdanderson, 2014/05/28 16:01:19]

I don't like the idea of calling Move() for the purpose of initializing the
selection widget. Is it possible for you to take the logic that initializes the
selection widget out of Move() and put it into its own helper function? Then you
can assume Move() has the precondition that the selection widget has already
been initialized (it wouldn't hurt to add a DCHECK() for that too).

[Nina, 2014/05/29 17:12:24]

Done.

+    }
+  }
 }
 
 void WindowSelector::OnMouseEvent(ui::MouseEvent* event) {
@@ -353,30 +322,31 @@ void WindowSelector::OnWindowAdded(aura::Window* new_window) {
 }
 
 void WindowSelector::OnWindowDestroying(aura::Window* window) {
-  // window is one of a container, the restore_focus_window and/or
-  // one of the selectable windows in overview.
-  ScopedVector<WindowSelectorItem>::iterator iter =
-      std::find_if(windows_.begin(), windows_.end(),
-                   WindowSelectorItemComparator(window));
+  WindowSelectorItem* removed_item = NULL;
+
+  ScopedVector<WindowGrid>::iterator grid_iter;
+  for (ScopedVector<WindowGrid>::iterator iter = grid_list_.begin();
+       iter != grid_list_.end() && !removed_item; ++iter) {
+    removed_item = (*iter)->RemoveWindow(window);

[tdanderson, 2014/05/28 16:01:19]

I prefer the previous approach: using find_if to determine whether or not the
item is found within the grid_list_, and then only calling RemoveWindow() if it
is.

Is there a reason why you can't use a similar approach here?

[Nina, 2014/05/29 17:12:24]

Well, if I do it like that I think I'll have to find the window two times on the
grid. I will change it to search for the root_window, though, avoiding confusion
and making it a little more efficient.

+    grid_iter = iter;
+  }
+
   window->RemoveObserver(this);
   observed_windows_.erase(window);
   if (window == restore_focus_window_)
     restore_focus_window_ = NULL;
-  if (iter == windows_.end())
-    return;
 
-  (*iter)->RemoveWindow(window);
-  // If there are still windows in this selector entry then the overview is
-  // still active and the active selection remains the same.
-  if (!(*iter)->empty())
+  if (!removed_item)
     return;
 
-  windows_.erase(iter);
-  if (windows_.empty()) {
-    CancelSelection();
-    return;
+  windows_.erase(std::find(windows_.begin(), windows_.end(), removed_item));
+
+  if (grid_iter != grid_list_.end() && (*grid_iter)->empty()) {
+    grid_list_.erase(grid_iter);
+    root_index_ = 0;
+    if (grid_list_.size() == 0)
+      CancelSelection();
   }
-  PositionWindows(true);

[tdanderson, 2014/05/28 16:01:19]

Why was this removed?

[Nina, 2014/05/29 17:12:24]

Because RemoveWindow rearranges the windows on its grid if an item is removed.
The previous approach rearranged on every grid for each window removal. I added
a comment on RemoveWindow() to clarify this.

 }
 
 void WindowSelector::OnWindowBoundsChanged(aura::Window* window,
@@ -426,7 +396,7 @@ void WindowSelector::StartOverview() {
        iter != windows_.end(); ++iter) {
     (*iter)->PrepareForOverview();
   }
-  PositionWindows(/* animate */ true);
+  PositionWindows(true);
   DCHECK(!windows_.empty());
   cursor_client_ = aura::client::GetCursorClient(
       windows_.front()->GetRootWindow());
@@ -451,57 +421,17 @@ void WindowSelector::StartOverview() {
 }
 
 void WindowSelector::PositionWindows(bool animate) {
+  grid_list_.clear();
   aura::Window::Windows root_window_list = Shell::GetAllRootWindows();
-  for (size_t i = 0; i < root_window_list.size(); ++i)
-    PositionWindowsFromRoot(root_window_list[i], animate);
-}
-
-void WindowSelector::PositionWindowsFromRoot(aura::Window* root_window,
-                                             bool animate) {
-  std::vector<WindowSelectorItem*> windows;
-  for (WindowSelectorItemList::iterator iter = windows_.begin();
-       iter != windows_.end(); ++iter) {
-    if ((*iter)->GetRootWindow() == root_window)
-      windows.push_back(*iter);
-  }
-
-  if (windows.empty())
-    return;
-
-  gfx::Size window_size;
-  gfx::Rect total_bounds = ScreenUtil::ConvertRectToScreen(
-      root_window,
-      ScreenUtil::GetDisplayWorkAreaBoundsInParent(
-          Shell::GetContainer(root_window, kShellWindowId_DefaultContainer)));
-
-  // Find the minimum number of windows per row that will fit all of the
-  // windows on screen.
-  size_t columns = std::max(
-      total_bounds.width() > total_bounds.height() ? kMinCardsMajor : 1,
-      static_cast<int>(ceil(sqrt(total_bounds.width() * windows.size() /
-                                 (kCardAspectRatio * total_bounds.height())))));
-  size_t rows = ((windows.size() + columns - 1) / columns);
-  window_size.set_width(std::min(
-      static_cast<int>(total_bounds.width() / columns),
-      static_cast<int>(total_bounds.height() * kCardAspectRatio / rows)));
-  window_size.set_height(window_size.width() / kCardAspectRatio);
-
-  // Calculate the X and Y offsets necessary to center the grid.
-  int x_offset = total_bounds.x() + ((windows.size() >= columns ? 0 :
-      (columns - windows.size()) * window_size.width()) +
-      (total_bounds.width() - columns * window_size.width())) / 2;
-  int y_offset = total_bounds.y() + (total_bounds.height() -
-      rows * window_size.height()) / 2;
-  for (size_t i = 0; i < windows.size(); ++i) {
-    gfx::Transform transform;
-    int column = i % columns;
-    int row = i / columns;
-    gfx::Rect target_bounds(window_size.width() * column + x_offset,
-                            window_size.height() * row + y_offset,
-                            window_size.width(),
-                            window_size.height());
-    target_bounds.Inset(kWindowMargin, kWindowMargin);
-    windows[i]->SetBounds(root_window, target_bounds, animate);
+  for (size_t i = 0; i < root_window_list.size(); ++i) {
+    std::vector<WindowSelectorItem*> windows;
+    for (WindowSelectorItemList::iterator iter = windows_.begin();
+         iter != windows_.end(); ++iter) {
+      if ((*iter)->GetRootWindow() == root_window_list[i])
+        windows.push_back(*iter);
+    }
+    if (!windows.empty())
+      grid_list_.push_back(new WindowGrid(root_window_list[i], windows));
   }
 }