Tablet WM : Swiping on system tray bubble.

ash/system/tray/system_tray.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 "ash/system/tray/system_tray.h"
 
 #include <algorithm>
 #include <map>
 #include <vector>
 
@@ skipping 32 matching lines @@
 #include "ash/system/tray/tray_bubble_wrapper.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_container.h"
 #include "ash/system/tray_accessibility.h"
 #include "ash/system/tray_caps_lock.h"
 #include "ash/system/tray_tracing.h"
 #include "ash/system/update/tray_update.h"
 #include "ash/system/user/tray_user.h"
 #include "ash/system/web_notification/web_notification_tray.h"
 #include "ash/wm/container_finder.h"
+#include "ash/wm/maximize_mode/maximize_mode_controller.h"
 #include "ash/wm/widget_finder.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/timer/timer.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/compositor/layer.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
@@ skipping 102 matching lines @@
   // WmActivationObserver:
   void OnWindowActivated(ActivationReason reason,
                          aura::Window* gained_active,
                          aura::Window* lost_active) override {
     if (!tray_->HasSystemBubble() || !gained_active)
       return;
 
     int container_id = wm::GetContainerForWindow(gained_active)->id();
 
     // Don't close the bubble if a popup notification is activated.
-    if (container_id == kShellWindowId_StatusContainer)
+    if (container_id == kShellWindowId_StatusContainer ||
+        container_id == kShellWindowId_SettingBubbleContainer) {
       return;
+    }
 
     views::Widget* bubble_widget =
         tray_->GetSystemBubble()->bubble_view()->GetWidget();
     // Don't close the bubble if a transient child is gaining or losing
     // activation.
     if (bubble_widget == GetInternalWidgetForWindow(gained_active) ||
         ::wm::HasTransientAncestor(gained_active,
                                    bubble_widget->GetNativeWindow()) ||
         (lost_active && ::wm::HasTransientAncestor(
                             lost_active, bubble_widget->GetNativeWindow()))) {
       return;
     }
 
     tray_->CloseSystemBubble();
   }
 
  private:
   SystemTray* tray_;
 
   DISALLOW_COPY_AND_ASSIGN(ActivationObserver);
 };
 
 // SystemTray
 
-SystemTray::SystemTray(Shelf* shelf) : TrayBackgroundView(shelf) {
+SystemTray::SystemTray(Shelf* shelf)
+    : TrayBackgroundView(shelf), shelf_(shelf) {
   SetInkDropMode(InkDropMode::ON);
 
   // Since user avatar is on the right hand side of System tray of a
   // horizontal shelf and that is sufficient to indicate separation, no
   // separator is required.
   set_separator_visibility(false);
 }
 
 SystemTray::~SystemTray() {
   // Destroy any child views that might have back pointers before ~View().
@@ skipping 411 matching lines @@
 void SystemTray::ActivateBubble() {
   TrayBubbleView* bubble_view = GetSystemBubble()->bubble_view();
   // If system tray bubble is in the process of closing, do not try to activate
   // bubble.
   if (bubble_view->GetWidget()->IsClosed())
     return;
   bubble_view->set_can_activate(true);
   bubble_view->GetWidget()->Activate();
 }
 
+void SystemTray::OnGestureEvent(ui::GestureEvent* event) {
+  if (Shell::Get()
+          ->maximize_mode_controller()
+          ->IsMaximizeModeWindowManagerEnabled() &&
+      shelf_->IsHorizontalAlignment() && ProcessGestureEvent(*event)) {
+    event->StopPropagation();

[tdanderson, 2017/06/20 22:42:27]

I think that event->SetHandled() should do the trick here, is there a reason why
you need to use StopPropagation() specifically?

[minch1, 2017/06/22 17:42:11]

Done.

+  } else {
+    TrayBackgroundView::OnGestureEvent(event);
+  }
+}
+
+gfx::Rect SystemTray::GetWorkAreaBoundsInScreen() const {
+  return shelf_->GetUserWorkAreaBounds();
+}
+
 bool SystemTray::PerformAction(const ui::Event& event) {
   // If we're already showing a full system tray menu, either default or
   // detailed menu, hide it; otherwise, show it (and hide any popup that's
   // currently shown).
   if (HasSystemBubble() && full_system_tray_menu_) {
     system_bubble_->bubble()->Close();
   } else {
     ShowDefaultView(BUBBLE_CREATE_NEW);
     if (event.IsKeyEvent() || (event.flags() & ui::EF_TOUCH_ACCESSIBILITY))
       ActivateBubble();
   }
   return true;
 }
 
+bool SystemTray::ProcessGestureEvent(const ui::GestureEvent& event) {
+  if (event.type() == ui::ET_GESTURE_SCROLL_BEGIN)
+    return StartGestureDrag(event);
+
+  if (!HasSystemBubble())
+    return false;

[xdai1, 2017/06/20 17:00:31]

Why don't put this if-clause at the top?

[xdai1, 2017/06/20 17:02:53]

I think I get it...

+
+  if (event.type() == ui::ET_GESTURE_SCROLL_UPDATE) {
+    UpdateGestureDrag(event);
+    return true;
+  }
+
+  if (event.type() == ui::ET_GESTURE_SCROLL_END ||
+      event.type() == ui::ET_SCROLL_FLING_START) {
+    CompleteGestureDrag(event);
+    return true;
+  }
+  return false;
+}
+
+bool SystemTray::StartGestureDrag(const ui::GestureEvent& gesture) {
+  if ((HasSystemBubble() && full_system_tray_menu_) ||
+      IsDraggingDownOnShelf(gesture)) {

[tdanderson, 2017/06/20 22:42:24]

Now I understand what IsDraggingDownOnShelf() is doing. Since that function is
only called in one place and its implementation is very short (just a few
lines), for readability I suggest just inlining the implementation of
StartGestureDrag() as follows:

bool SystemTray::StartGestureDrag(...) {
  DCHECK_EQ(gesture.type(), ui::ET_GESTURE_SCROLL_BEGIN);

  // <add some comment here>
  if (HasSystemBubble() && full_system_tray_menu_)
    return false;

  // If the scroll sequence begins to scroll downward, return false
  // so that the event will instead by handled by the shelf.
  gfx::Point gesture_location = ...;
  View::ConvertPointToScreen(...);
  if (gesture_location.y() >= shelf_->GetIdealBounds().y() &&
      gesture.details().scroll_y_hint() > 0) {
    return false;
  }

  gesture_drag_amount_ = 0.f;
  ...
  return true;
}


NOTE: I think it should be sufficient to just check that
gesture.details().scroll_y_hint() > 0, which checks that scrolling has started
in the downward direction. I don't think you need to compare against the shelf
location. Can you please verify this? If so, then your code will be simpler
since you don't need to store the |shelf_| member variable or convert the event
location to a different coordinate space.

[minch1, 2017/06/22 17:42:11]

Yes, you are right, thanks.

+    return false;
+  }
+
+  gesture_drag_amount_ = 0.f;

[tdanderson, 2017/06/20 22:42:23]

It looks like you are only resetting |gesture_drag_amount_| once a new
scroll-sequence begins, which means that this member could be storing a
meaningless value whenever a scroll is not in progress. This is potentially
dangerous, as someone could query this value and infer that a drag was in
progress when it wasn't.

For this CL, I suggest that you also reset this member to 0 in
CompleteGestureDrag() as well as right above line 683.

[minch1, 2017/06/22 17:42:11]

Done.

+  ShowDefaultView(BUBBLE_CREATE_NEW);
+  system_tray_bubble_bounds_ =
+      system_bubble_->bubble_view()->GetWidget()->GetWindowBoundsInScreen();
+  UpdateBoundsOnDragging(gesture);
+  return true;
+}
+
+void SystemTray::UpdateGestureDrag(const ui::GestureEvent& gesture) {
+  UpdateBoundsOnDragging(gesture);
+  gesture_drag_amount_ += gesture.details().scroll_y();
+}
+
+void SystemTray::CompleteGestureDrag(const ui::GestureEvent& gesture) {
+  gfx::Rect target_bounds = system_tray_bubble_bounds_;

[tdanderson, 2017/06/20 22:42:25]

Consider simplifying the implementation of this function as follows:

const bool hide_bubble = !ShouldShowSystemBubbleForDragging(gesture);
gfx::Rect target_bounds = system_tray_bubble_bounds_;

if (hide_bubble)
  target_bounds.set_y(shelf_->GetIdealBounds().y());

system_bubble_->bubble()->UpdateBounds(target_bounds, hide_bubble);
  

[minch1, 2017/06/22 17:42:11]

Done.

+  if (ShouldShowSystemBubbleForDragging(gesture)) {
+    system_bubble_->bubble()->UpdateBounds(target_bounds, false);
+  } else {
+    target_bounds.set_y(shelf_->GetIdealBounds().y());
+    system_bubble_->bubble()->UpdateBounds(target_bounds, true);
+  }
+}
+
+bool SystemTray::IsDraggingDownOnShelf(const ui::GestureEvent& gesture) {
+  gfx::Point gesture_location = gesture.location();
+  View::ConvertPointToScreen(this, &gesture_location);
+
+  if (gesture_location.y() >= shelf_->GetIdealBounds().y() &&

[tdanderson, 2017/06/20 22:42:23]

In StartGestureDrag() I suggest just inlining this implementation. However if
you do keep this as a separate named helper function, lines 719-723 can just be
replaced with:

return gesture_location.y() >= shelf_->GetIdealBounds().y() &&
       gesture.details().scroll_y_hint() > 0;

[minch1, 2017/06/22 17:42:11]

Acknowledged.

+      gesture.details().scroll_y_hint() > 0) {
+    return true;
+  }
+  return false;
+}
+
+void SystemTray::UpdateBoundsOnDragging(const ui::GestureEvent& gesture) {

[tdanderson, 2017/06/20 22:42:26]

Since the only property of |gesture| you use is its location, I suggest just
passing in the gfx::Point of the location instead of the whole GestureEvent.
Then you can re-name to something such as SetBubbleBounds() or similar.

[minch1, 2017/06/22 17:42:11]

Done.

+  gfx::Point gesture_location = gesture.location();
+  View::ConvertPointToScreen(this, &gesture_location);
+
+  // System tray bubble should not be dragged higher than its original height.
+  if (gesture_location.y() < system_tray_bubble_bounds_.y())
+    gesture_location.set_y(system_tray_bubble_bounds_.y());
+
+  gfx::Rect bounds_on_gesture(
+      system_tray_bubble_bounds_.x(), gesture_location.y(),
+      system_tray_bubble_bounds_.width(), system_tray_bubble_bounds_.height());
+  system_bubble_->bubble_view()->GetWidget()->SetBounds(bounds_on_gesture);
+}
+
+bool SystemTray::ShouldShowSystemBubbleForDragging(
+    const ui::GestureEvent& gesture) {
+  const float kFlingVelocity = 100.0f;
+  if (gesture.type() == ui::ET_SCROLL_FLING_START &&

[tdanderson, 2017/06/20 22:42:20]

Suggestion for inline documentation here: "If the scroll sequence terminates
with a fling, show the system menu if the fling was fast enough and in the
correct direction."

[minch1, 2017/06/22 17:42:11]

Done.

+      fabs(gesture.details().velocity_y()) > kFlingVelocity) {
+    return gesture.details().velocity_y() < 0;
+  }
+

[tdanderson, 2017/06/20 22:42:20]

Consider adding a DCHECK here to verify that the gesture type is a
GESTURE_SCROLL_END.

[minch1, 2017/06/22 17:42:11]

It is not correct to add a DCHECK to verify that the gesture type is
GESTURE_SCROLL_END here. Since gesture type here can be both GESTURE_SCROLL_END
and SCROLL_FLING_START. If the velocity of the fling event is not large enough
it should consider the position of the gesture.

[tdanderson, 2017/06/22 20:21:42]

Ah yes, you're right. Then perhaps add a DCHECK to verify (type == SCROLL_END ||
type == SCROLL_FLING_START).

[minch1, 2017/06/23 22:39:56]

Done.

+  return -gesture_drag_amount_ >= system_tray_bubble_bounds_.height() / 3.0;

[tdanderson, 2017/06/20 22:42:21]

Suggestion for inline documentation here: "Show the system menu if it is already
at least one-third visible."

[minch1, 2017/06/22 17:42:11]

Done.

+}
+
 void SystemTray::CloseSystemBubbleAndDeactivateSystemTray() {
   activation_observer_.reset();
   key_event_watcher_.reset();
   system_bubble_.reset();
   // When closing a system bubble with the alternate shelf layout, we need to
   // turn off the active tinting of the shelf.
   if (full_system_tray_menu_) {
     SetIsActive(false);
     full_system_tray_menu_ = false;
   }
@@ skipping 20 matching lines @@
           .work_area()
           .height();
   if (work_area_height > 0) {
     UMA_HISTOGRAM_CUSTOM_COUNTS(
         "Ash.SystemMenu.PercentageOfWorkAreaHeightCoveredByMenu",
         100 * bubble_view->height() / work_area_height, 1, 300, 100);
   }
 }
 
 }  // namespace ash