Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH.

ui/app_list/views/app_list_view.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 "ui/app_list/views/app_list_view.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/macros.h"
@@ skipping 43 matching lines @@
 
 // The margin from the edge to the speech UI.
 constexpr int kSpeechUIMargin = 12;
 
 // The height/width of the shelf.
 constexpr int kShelfSize = 48;
 
 // The height of the peeking app list.
 constexpr int kPeekingAppListHeight = 320;
 
+// The height of the half app list.
+constexpr int kHalfAppListHeight = 561;

[oshima, 2017/06/13 22:26:04]

Where this half value came from? Won't it be too small for portrait for example?

[newcomer, 2017/06/13 23:00:39]

This value came from the Spec, I just sent it to you.

[oshima, 2017/06/14 00:37:34]

It looks to me that this height includes the shelf side, but the code below
excludes it, when the shelf is on the bottom side.

[newcomer, 2017/06/15 16:55:33]

Fixed! thanks.

[oshima, 2017/06/15 17:51:44]

Looks like this isn't fixed yet? Please see my comment below.

[newcomer, 2017/06/15 18:20:26]

Done.

+
 // The fraction of app list height that the app list must be released at in
 // order to transition to the next state.
 constexpr int kAppListThresholdDenominator = 3;
 
 // The velocity the app list must be dragged in order to transition to the next
 // state, measured in DIPs/event.
 constexpr int kAppListDragVelocityThreshold = 25;
 
+// The DIP distance from the bezel that a drag event must end within to transfer
+// the |app_list_state_|.
+constexpr int kAppListBezelMargin = 50;
+
 // The opacity of the app list background.
 constexpr float kAppListOpacity = 0.8;
 
 // The vertical position for the appearing animation of the speech UI.
 constexpr float kSpeechUIAppearingPosition = 12;
 
 bool IsFullscreenAppListEnabled() {
   // Cache this value to avoid repeated lookup.
   static bool cached_value = features::IsFullscreenAppListEnabled();
   return cached_value;
@@ skipping 127 matching lines @@
     display_observer_.Add(display::Screen::GetScreen());
 }
 
 AppListView::~AppListView() {
   delegate_->GetSpeechUI()->RemoveObserver(this);
   animation_observer_.reset();
   // Remove child views first to ensure no remaining dependencies on delegate_.
   RemoveAllChildViews(true);
 }
 
-void AppListView::Initialize(gfx::NativeView parent, int initial_apps_page) {
+void AppListView::Initialize(gfx::NativeView parent,
+                             int initial_apps_page,
+                             bool is_maximize_mode,
+                             bool is_side_shelf) {
   base::Time start_time = base::Time::Now();
+  is_maximize_mode_ = is_maximize_mode;

[oshima, 2017/06/13 22:26:04]

I'd recommend to just use the maximize_mode_controller to check the state
instead of copying the state and maintain it.

same for is_side_shelf_

[newcomer, 2017/06/13 23:00:39]

Unfortunately the MaximizeModeController is in ash and I cannot include anything
from ash in ui because ui is below ash in the dependency graph. Maybe this can
happen after we move ui/app_list into ash/app_list in the future. 

Until this happens, I'm using ShellObserver overrides in the
AppListPresenterDelegate to notify AppListView when the maximize mode changes. 

As for side shelf, changing the shelf orientation causes the AppListView to get
destroyed. So it should be sufficient to only update is_side_shelf_ on
initialization.

[oshima, 2017/06/14 00:37:33]

Ack. I believe the app list has been moved to ui/app_list because this was used
on other platforms as well. Since they're gone, we can move back to ash/.

Can you file a bug to move it? I think it makes sense now.

[newcomer, 2017/06/15 16:55:33]

Done! crbug.com/733662

+  is_side_shelf_ = is_side_shelf;
   InitContents(parent, initial_apps_page);
   AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
   set_color(kContentsBackgroundColor);
   set_parent_window(parent);
 
-  if (IsFullscreenAppListEnabled())
+  if (features::IsFullscreenAppListEnabled())
     InitializeFullscreen(parent, initial_apps_page);
   else
     InitializeBubble(parent, initial_apps_page);
 
   InitChildWidgets();
   AddChildView(overlay_view_);
 
   if (IsFullscreenAppListEnabled())
     SetState(app_list_state_);
 
@@ skipping 270 matching lines @@
     new_widget_bounds.set_y(new_y_position);
   }
   fullscreen_widget_->SetBounds(new_widget_bounds);
 }
 
 void AppListView::EndDrag(const gfx::Point& location) {
   // Change the app list state based on where the drag ended. If fling velocity
   // was over the threshold, snap to the next state in the direction of the
   // fling.
   int const new_y_position = location.y() - initial_drag_point_.y() +
                              fullscreen_widget_->GetWindowBoundsInScreen().y();

[oshima, 2017/06/13 22:26:04]

is location in screen coordinates?

[newcomer, 2017/06/13 23:00:39]

yes!

[oshima, 2017/06/14 00:37:34]

Looks like this is from event, which means it's ot screen coordinates. This
works because you're using just diff from initial point.

This also explains my question below.

[newcomer, 2017/06/15 16:55:33]

Ah, you're correct. I misinterpreted your question. location is from event and
is therefore not in screen coordinates, new_y_position is in screen coordinates.

   if (std::abs(last_fling_velocity_) > kAppListDragVelocityThreshold) {
     // If the user releases drag with velocity over the threshold, snap to
     // the next state, ignoring the drag release position.
-    if (app_list_state_ == FULLSCREEN) {
-      if (last_fling_velocity_ > 0)
-        SetState(PEEKING);
 
+    if (last_fling_velocity_ > 0) {
+      switch (app_list_state_) {
+        case PEEKING:
+        case HALF:
+        case FULLSCREEN_SEARCH:
+          SetState(CLOSED);
+          break;
+        case FULLSCREEN_ALL_APPS:
+          SetState(is_maximize_mode_ || is_side_shelf_ ? CLOSED : PEEKING);
+          break;
+        case CLOSED:
+          NOTREACHED();
+          break;
+      }
     } else {
-      SetState(last_fling_velocity_ > 0 ? CLOSED : FULLSCREEN);
+      switch (app_list_state_) {
+        case FULLSCREEN_ALL_APPS:
+        case FULLSCREEN_SEARCH:
+          SetState(app_list_state_);
+          break;
+        case HALF:
+          SetState(FULLSCREEN_SEARCH);
+          break;
+        case PEEKING:
+          SetState(FULLSCREEN_ALL_APPS);
+          break;
+        case CLOSED:
+          NOTREACHED();
+          break;
+      }
     }
-    last_fling_velocity_ = 0;

[xiyuan, 2017/06/13 21:38:58]

Who is going to reset |last_fling_velocity_| now?

[newcomer, 2017/06/13 22:22:48]

Assuming that a ui::ET_GESTURE_END event must always be preceded with a
UI::ET_GESTURE_SCROLL_UPDATE, the |last_fling_velocity_| doesn't need to be
reset because it is set in line 738.

   } else {
     int display_height = display::Screen::GetScreen()
                              ->GetDisplayNearestView(parent_window())
                              .work_area()
                              .height();
-    int default_peeking_y = display_height + kShelfSize - kPeekingAppListHeight;
-    // The drag release velocity was too low, so use the release point.
-    int app_list_snap_y =
-        (app_list_state_ == FULLSCREEN) ? 0 : default_peeking_y;
-    // The DIP delta that must be exceeded for the app list to snap to the next
-    // state.
-    int app_list_threshold =
-        (fullscreen_widget_->GetWindowBoundsInScreen().height() + kShelfSize) /
-        kAppListThresholdDenominator;
-    app_list_threshold -=
-        (app_list_state_ == FULLSCREEN ? 0 : kPeekingAppListHeight) /
-        kAppListThresholdDenominator;
+    int app_list_y_for_state = 0;
+    int app_list_height = 0;
+    switch (app_list_state_) {
+      case FULLSCREEN_ALL_APPS:
+      case FULLSCREEN_SEARCH:
+        app_list_y_for_state = 0;
+        app_list_height = display_height;
+        break;
+      case HALF:
+        app_list_y_for_state = display_height - kHalfAppListHeight;
+        app_list_height = kHalfAppListHeight;
+        break;
+      case PEEKING:
+        app_list_y_for_state = display_height - kPeekingAppListHeight;
+        app_list_height = kPeekingAppListHeight;
+        break;
+      case CLOSED:
+        NOTREACHED();
+        break;
+    }
 
-    // If the user releases +/- 1/3 of |app_list_threshold|, snap to the
-    // next state.
-    if (std::abs(app_list_snap_y - new_y_position) < app_list_threshold) {
-      // The drag was not far enough so set the app list bounds to the target
-      // bounds for the current state.
-      SetState(app_list_state_);
-    } else if ((app_list_snap_y + app_list_threshold) < new_y_position) {
-      // The drag was far enough to change states and was a downward drag, so
-      // set the app list bounds to the next state.
-      SetState(app_list_state_ == FULLSCREEN ? PEEKING : CLOSED);
-    } else {
-      // The drag was far enough to change states and was an upward drag, so
-      // set the app list bounds to the next state.
-      SetState(FULLSCREEN);
+    int app_list_threshold = app_list_height / kAppListThresholdDenominator;
+    int drag_delta = app_list_y_for_state - new_y_position;
+    switch (app_list_state_) {
+      case FULLSCREEN_ALL_APPS:
+        if (std::abs(drag_delta) > app_list_threshold)
+          SetState(is_maximize_mode_ || is_side_shelf_ ? CLOSED : PEEKING);
+        else
+          SetState(app_list_state_);

[xiyuan, 2017/06/13 21:38:59]

Why do we need to call SetState with the same state? Wounldn't this be a no-op? 

[newcomer, 2017/06/13 22:22:48]

The widget gets re-positioned as the drag events are being received. If the drag
action fails to transition states then the widget must be reset from it's
intermediate state to the correct y for its current state.

+        break;
+      case FULLSCREEN_SEARCH:
+        if (std::abs(drag_delta) > app_list_threshold)
+          SetState(CLOSED);
+        else
+          SetState(app_list_state_);
+        break;
+      case HALF:
+        if (std::abs(drag_delta) > app_list_threshold) {
+          SetState(drag_delta > 0 ? FULLSCREEN_SEARCH : CLOSED);
+        } else if (location.y() +
+                       fullscreen_widget_->GetWindowBoundsInScreen().y() >=
+                   display_height - kAppListBezelMargin) {
+          // If the user drags to the bezel, close the app list.
+          SetState(CLOSED);
+        } else {
+          SetState(app_list_state_);
+        }
+        break;
+      case PEEKING:
+        if (std::abs(drag_delta) > app_list_threshold) {
+          SetState(drag_delta > 0 ? FULLSCREEN_ALL_APPS : CLOSED);
+        } else if (location.y() +
+                       fullscreen_widget_->GetWindowBoundsInScreen().y() >=
+                   display_height - kAppListBezelMargin) {

[oshima, 2017/06/13 22:26:04]

I didn't fully understand this. why this isn't location.y() >= bezel y?

[newcomer, 2017/06/13 23:00:39]

Because location.y() would be in widget coordinates, we need screen coordinates
here. Should I add a comment or create a variable for |screen_coordinates| for
readability?

[oshima, 2017/06/14 00:37:34]

Yes, please create one and use wm::ConvertPointToScreen to convert.

[newcomer, 2017/06/15 16:55:33]

Done.

+          // If the user drags to the bezel, close the app list.
+          SetState(CLOSED);
+        } else {
+          SetState(app_list_state_);
+        }
+        break;
+      case CLOSED:
+        NOTREACHED();
+        break;
     }
   }
 }
 
+void AppListView::SetStateFromSearchBoxView(bool empty) {
+  switch (app_list_state_) {
+    case PEEKING:
+      if (!empty)
+        SetState(HALF);
+      break;
+    case HALF:
+      if (empty)
+        SetState(PEEKING);
+      break;
+    case FULLSCREEN_SEARCH:
+      if (empty) {
+        SetState(FULLSCREEN_ALL_APPS);
+        app_list_main_view()->contents_view()->SetActiveState(
+            AppListModel::State::STATE_APPS);
+      }
+      break;
+    case FULLSCREEN_ALL_APPS:
+      if (!empty)
+        SetState(FULLSCREEN_SEARCH);
+      break;
+    case CLOSED:
+      NOTREACHED();
+      break;
+  }
+}
+
+void AppListView::OnMaximizeModeChanged(bool started) {

[xiyuan, 2017/06/13 21:38:58]

|started| is different from what is in the header file. |shown| was used there.
Please keep them consistent.

[newcomer, 2017/06/13 22:22:47]

Done!

+  is_maximize_mode_ = started;
+  if (is_maximize_mode_ && !is_fullscreen()) {
+    // Set |app_list_state_| to a maximize mode friendly state.
+    SetState(app_list_state_ == PEEKING ? FULLSCREEN_ALL_APPS
+                                        : FULLSCREEN_SEARCH);
+  }
+}
+
 void AppListView::OnBeforeBubbleWidgetInit(views::Widget::InitParams* params,
                                            views::Widget* widget) const {
   if (!params->native_widget) {
     views::ViewsDelegate* views_delegate = views::ViewsDelegate::GetInstance();
     if (views_delegate && !views_delegate->native_widget_factory().is_null()) {
       params->native_widget =
           views_delegate->native_widget_factory().Run(*params, widget);
     }
   }
   // Apply a WM-provided shadow (see ui/wm/core/).
@@ skipping 112 matching lines @@
   }
 }
 
 void AppListView::SchedulePaintInRect(const gfx::Rect& rect) {
   BubbleDialogDelegateView::SchedulePaintInRect(rect);
   if (GetBubbleFrameView())
     GetBubbleFrameView()->SchedulePaint();
 }
 
 void AppListView::SetState(AppListState new_state) {
   gfx::Rect new_widget_bounds = fullscreen_widget_->GetWindowBoundsInScreen();

[xiyuan, 2017/06/13 21:38:58]

nit: move this down to line 815, closer to where it is first used.

[newcomer, 2017/06/13 22:22:47]

Done.

-  switch (new_state) {
+  AppListState new_state_override = new_state;
+  if (is_side_shelf_ || is_maximize_mode_) {
+    // If side shelf or maximize mode are active, all transitions should be
+    // made to the maximize mode/side shelf friendly versions.
+    if (new_state == PEEKING)
+      new_state_override = FULLSCREEN_ALL_APPS;
+    else if (new_state == HALF)
+      new_state_override = FULLSCREEN_SEARCH;
+  }
+

[xiyuan, 2017/06/13 21:38:58]

nit: Should we bail if new_state_override == app_list_state_ ?

[newcomer, 2017/06/13 22:22:48]

No, because sometimes it is useful to re-set the current state. For example, if
a drag gesture completes but the touch point is not far enough to qualify for a
state transition, we call SetState(app_list_state_) in order to set the widget
back in the proper position.

+  switch (new_state_override) {
     case PEEKING: {
-      int display_height = display::Screen::GetScreen()
-                               ->GetDisplayNearestView(parent_window())
-                               .work_area()
-                               .bottom();
-      int default_peeking_y =
-          display_height + kShelfSize - kPeekingAppListHeight;
-      new_widget_bounds.set_y(default_peeking_y);
+      switch (app_list_state_) {
+        case HALF:
+        case FULLSCREEN_ALL_APPS:
+        case PEEKING: {
+          int display_height = display::Screen::GetScreen()
+                                   ->GetDisplayNearestView(parent_window())
+                                   .work_area()
+                                   .bottom();

[xiyuan, 2017/06/13 21:38:58]

Should we use height() instead of bottom() to get |display_height| ?

[xiyuan, 2017/06/13 21:38:59]

nit: move this out of the switch so that it could be shared with line 843.

[newcomer, 2017/06/13 22:22:48]

msw@ recommended we use bottom() to avoid a case where height() could be 0.

[newcomer, 2017/06/13 22:22:48]

Done.

+          int peeking_app_list_y = display_height - kPeekingAppListHeight;
+          new_widget_bounds.set_y(peeking_app_list_y);
+          app_list_main_view_->contents_view()->SetActiveState(
+              AppListModel::STATE_START);
+          break;
+        }
+        case FULLSCREEN_SEARCH:
+        case CLOSED:
+          NOTREACHED();
+          break;
+      }
       break;
-    }
-    case FULLSCREEN:
+    } break;

[xiyuan, 2017/06/13 21:38:58]

get rid of the extra "break"

[newcomer, 2017/06/13 22:22:48]

Done.

+    case HALF:
+      switch (app_list_state_) {
+        case PEEKING:
+        case HALF: {
+          int display_height = display::Screen::GetScreen()
+                                   ->GetDisplayNearestView(parent_window())
+                                   .work_area()
+                                   .bottom();
+          int half_app_list_y = display_height - kHalfAppListHeight;
+          new_widget_bounds.set_y(half_app_list_y);
+          break;
+        }
+        case FULLSCREEN_SEARCH:
+        case FULLSCREEN_ALL_APPS:
+        case CLOSED:
+          NOTREACHED();
+          break;
+      }
+      break;
+    case FULLSCREEN_ALL_APPS:
+      new_widget_bounds.set_y(0);
+      app_list_main_view_->contents_view()->SetActiveState(
+          AppListModel::STATE_APPS);
+      break;
+    case FULLSCREEN_SEARCH:
       new_widget_bounds.set_y(0);
       break;
     case CLOSED:
       app_list_main_view_->Close();
       delegate_->Dismiss();
       break;
   }
   fullscreen_widget_->SetBounds(new_widget_bounds);
-  app_list_state_ = new_state;
+  app_list_state_ = new_state_override;
 }
 
 void AppListView::OnWidgetDestroying(views::Widget* widget) {
   BubbleDialogDelegateView::OnWidgetDestroying(widget);
   if (delegate_ && widget == GetWidget())
     delegate_->ViewClosing();
 }
 
 void AppListView::OnWidgetVisibilityChanged(views::Widget* widget,
                                             bool visible) {
@@ skipping 84 matching lines @@
                        .work_area()
                        .size();
   size.Enlarge(0, kShelfSize);
   fullscreen_widget_->SetSize(size);
 
   // Update the |fullscreen_widget_| bounds to accomodate the new work area.
   SetState(app_list_state_);
 }
 
 }  // namespace app_list