Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH.

ui/app_list/views/app_list_view.cc

Index: ui/app_list/views/app_list_view.cc
diff --git a/ui/app_list/views/app_list_view.cc b/ui/app_list/views/app_list_view.cc
index a38c6092e4bd9be4872f4008e16246d1c1d9cb77..7c77ab3e4494bfd8fd842e90d7f3d0b682eaba8d 100644
--- a/ui/app_list/views/app_list_view.cc
+++ b/ui/app_list/views/app_list_view.cc
@@ -61,6 +61,9 @@ 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;
@@ -69,6 +72,10 @@ constexpr int kAppListThresholdDenominator = 3;
 // 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;
 
@@ -216,14 +223,19 @@ AppListView::~AppListView() {
   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);
@@ -518,50 +530,146 @@ void AppListView::EndDrag(const gfx::Point& location) {
   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;
-
-    // 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_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;
+    }
+
+    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) {
@@ -695,18 +803,64 @@ void AppListView::SchedulePaintInRect(const gfx::Rect& rect) {
 
 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:
@@ -715,7 +869,7 @@ void AppListView::SetState(AppListState new_state) {
       break;
   }
   fullscreen_widget_->SetBounds(new_widget_bounds);
-  app_list_state_ = new_state;
+  app_list_state_ = new_state_override;
 }
 
 void AppListView::OnWidgetDestroying(views::Widget* widget) {