ESC accelerator and consistent overview mode for Athena homecard

athena/home/home_card_impl.cc

 // Copyright 2014 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 "athena/home/home_card_impl.h"
 
 #include <cmath>
 #include <limits>
 
 #include "athena/env/public/athena_env.h"
@@ skipping 208 matching lines @@
   scoped_ptr<HomeCardGestureManager> gesture_manager_;
   HomeCardGestureManager::Delegate* gesture_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(HomeCardView);
 };
 
 HomeCardImpl::HomeCardImpl(AppModelBuilder* model_builder)
     : model_builder_(model_builder),
       state_(HIDDEN),
       original_state_(VISIBLE_MINIMIZED),
+      overview_state_(VISIBLE_BOTTOM),
       home_card_widget_(NULL),
       home_card_view_(NULL),
       layout_manager_(NULL),
       activation_client_(NULL) {
   DCHECK(!instance);
   instance = this;
   WindowManager::GetInstance()->AddObserver(this);
 }
 
 HomeCardImpl::~HomeCardImpl() {
@@ skipping 56 matching lines @@
 
 void HomeCardImpl::InstallAccelerators() {
   const AcceleratorData accelerator_data[] = {
       {TRIGGER_ON_PRESS, ui::VKEY_L, ui::EF_CONTROL_DOWN,
        COMMAND_SHOW_HOME_CARD, AF_NONE},
   };
   AcceleratorManager::Get()->RegisterAccelerators(
       accelerator_data, arraysize(accelerator_data), this);
 }
 
+// Use when switching internally between "shown" and "not shown" so that
+// we enter and exit overview mode appropriately
+void HomeCardImpl::SetStateAndOverview(HomeCard::State state) {

[Jun Mukai, 2014/09/25 21:53:41]

This method name is unclear.  SetStateAndToggleOverview?  But it doesn't toggle
overview state at some time.

Based on my comments below, I believe this function is simply two line of:
SetState(state);
WindowManager::Get()->ToggleOverview();

[Greg Levin, 2014/10/02 21:56:09]

Done (mostly... see below)

+  if (state_ == state)
+    return;
+
+  bool show = (state == VISIBLE_BOTTOM || state == VISIBLE_CENTERED);
+  if (show == WindowManager::GetInstance()->IsOverviewModeActive()) {
+      SetState(state); // overview mode already matches state
+  } else {
+    overview_state_ = state;
+    WindowManager::GetInstance()->ToggleOverview();
+  }
+}
+
 void HomeCardImpl::SetState(HomeCard::State state) {
   if (state_ == state)
     return;
 
   // Update |state_| before changing the visibility of the widgets, so that
   // LayoutManager callbacks get the correct state.
   HomeCard::State old_state = state_;
   state_ = state;
   original_state_ = state;
 
@@ skipping 46 matching lines @@
 
 bool HomeCardImpl::IsCommandEnabled(int command_id) const {
   return true;
 }
 
 bool HomeCardImpl::OnAcceleratorFired(int command_id,
                                       const ui::Accelerator& accelerator) {
   DCHECK_EQ(COMMAND_SHOW_HOME_CARD, command_id);
 
   if (state_ == VISIBLE_CENTERED && original_state_ != VISIBLE_BOTTOM)
-    SetState(VISIBLE_MINIMIZED);
+    SetStateAndOverview(VISIBLE_MINIMIZED);
   else if (state_ == VISIBLE_MINIMIZED)
-    SetState(VISIBLE_CENTERED);
+    SetStateAndOverview(VISIBLE_CENTERED);
   return true;
 }
 
 void HomeCardImpl::OnGestureEnded(State final_state, bool is_fling) {
   home_card_view_->ClearGesture();
   if (state_ != final_state &&
       (state_ == VISIBLE_MINIMIZED || final_state == VISIBLE_MINIMIZED)) {
     SetState(final_state);
     WindowManager::GetInstance()->ToggleOverview();
   } else {
@@ skipping 24 matching lines @@
       GetBoundsForState(screen_bounds, to_state)));
 
   home_card_view_->SetStateProgress(from_state, to_state, progress);
 
   // TODO(mukai): signals the update to the window manager so that it shows the
   // intermediate visual state of overview mode.
 }
 
 void HomeCardImpl::OnOverviewModeEnter() {
   if (state_ == HIDDEN || state_ == VISIBLE_MINIMIZED)
-    SetState(VISIBLE_BOTTOM);
+    SetState(overview_state_);

[Jun Mukai, 2014/09/25 21:53:41]

I am not sure why overview_state_ is necessary.  It's used here, right?

I once introduced similar field, and some reviewer (oshima?) pointed out that
it's okay to run SetState() -> ToggleOverview() in this order.  The if-clause of
the previous line was introduced for this purpose -- it prevents setting state
back to BOTTOM from CENTERED.

I believe you can go with the same way.

[Greg Levin, 2014/10/02 21:56:09]

ToggleOverview() was a little imprecise for use here, so I created
WindowManager::ActivateOverview(), and am using SetState() -> ActivateOverview()
instead.

 }
 
 void HomeCardImpl::OnOverviewModeExit() {
   SetState(VISIBLE_MINIMIZED);
+  overview_state_ = VISIBLE_BOTTOM;
 }
 
 void HomeCardImpl::OnSplitViewModeEnter() {
 }
 
 void HomeCardImpl::OnSplitViewModeExit() {
 }
 
 void HomeCardImpl::OnWindowActivated(aura::Window* gained_active,
                                      aura::Window* lost_active) {
@@ skipping 17 matching lines @@
   instance = NULL;
 }
 
 // static
 HomeCard* HomeCard::Get() {
   DCHECK(instance);
   return instance;
 }
 
 }  // namespace athena