Split Screen mode implementation

athena/wm/bezel_controller.cc

Index: athena/wm/bezel_controller.cc
diff --git a/athena/wm/bezel_controller.cc b/athena/wm/bezel_controller.cc
index fb6215533b87eacf8adeeed7695baaa98ba163d1..1ebb21fbbb78d2ee9e38404dc41f481c72ff3512 100644
--- a/athena/wm/bezel_controller.cc
+++ b/athena/wm/bezel_controller.cc
@@ -4,6 +4,7 @@
 
 #include "athena/wm/bezel_controller.h"
 
+#include "athena/wm/coordinate_conversion.h"
 #include "ui/aura/window.h"
 #include "ui/events/event_handler.h"
 
@@ -15,7 +16,7 @@ namespace {
 // So setting this width fairly high for now.
 const float kBezelWidth = 20.0f;
 
-const float kScrollPositionNone = -100;
+const float kScrollDeltaNone = 0;
 
 bool ShouldProcessGesture(ui::EventType event_type) {
   return event_type == ui::ET_GESTURE_SCROLL_UPDATE ||
@@ -34,25 +35,30 @@ BezelController::BezelController(aura::Window* container)
       left_right_delegate_(NULL) {
 }
 
-float BezelController::GetDistance(const gfx::PointF& position,
+float BezelController::GetDistance(const gfx::PointF& location,
+                                   aura::Window* window,
                                    BezelController::Bezel bezel) {
   DCHECK(bezel == BEZEL_LEFT || bezel == BEZEL_RIGHT);
+  // Convert location from window coordinates to screen coordinates.
+  gfx::Point point_in_screen(location.x(), location.y());

[Jun Mukai, 2014/07/25 20:21:29]

point_in_screen(location) would copy it.

[mfomitchev, 2014/07/25 23:27:38]

Doesn't look like I can pass PointF to a Point constructor...

[Jun Mukai, 2014/07/25 23:44:23]

oops, missed your point. Well, in that case, consider using a function in
ui/gfx/geometry/pointer_conversions.h

[mfomitchev, 2014/07/26 02:08:43]

Cool, will do.

[mfomitchev, 2014/08/05 19:56:57]

Done.

+  ConvertPointToScreen(window, &point_in_screen);
   return bezel == BEZEL_LEFT
-             ? position.x()
-             : position.x() - container_->GetBoundsInScreen().width();
+             ? point_in_screen.x()
+             : point_in_screen.x() - container_->GetBoundsInScreen().width();

[Jun Mukai, 2014/07/25 20:21:29]

Not the thing in this CL, but should this be GetBoundsInScreen().right()
instead?  And the previous line should be x() - GetBoundsInScreen().x()?

The container is spanning fullscreen so this makes no difference for now, but I
think that is more strict.

[mfomitchev, 2014/07/25 23:27:38]

Since this is a bezel gesture recognizer, I think  point_in_screen.x() is right,
but instead of container_->GetBoundsInScreen().width() we want to get the screen
width is not taking up the entire screen. Does this make sense? And if so -
what's a good way to get the screen width?

[Jun Mukai, 2014/07/25 23:44:23]

I don't object to point_in_screen.x(). I'm saying to use right() instead of
width(), and use x() instead of doing nothing, about
container_->GetBoundsInScreen().

That addresses the case that the container's rectangle (i.e. user's work area)
is different from the display/screen's bounds. Although right now I don't hear
any plan of doing so, it might happen eventually.

For example, ash work area originally used the entire screen width, but at some
point we introduced side shelf and docked window which shrinks work area's
width, and suffered from weird corner cases caused by implicit assumptions of
'work area width must match to the screen width'. It's better to address this
kind of thing earlier.

[mfomitchev, 2014/07/26 02:08:43]

Yes, I understand. My point is that this is a bezel controller, which is
supposed to recognize bezel gestures. Bezel gestures by definition are supposed
to start from the edge of the screen, so I think it makes sense to report the
distance from the edge of the screen in the callbacks as well. Now, the current
implementation is still wrong: the first clause (point_in_screen.x()) is okay,
but the second clause (point_in_screen.x() -
container_->GetBoundsInScreen().width()) should be changed to
(point_in_screen.x() - <screen width>).

If we want to account for the possibility that the main container is not full
screen, I think we should do it in SplitViewController: we should take
container_->x() and container_->right() into account when calculating the
windows positions. I can make that change.

[Jun Mukai, 2014/07/26 20:30:37]

+1 to use screen_width.

[mfomitchev, 2014/08/05 19:56:57]

Done.

 }
 
-void BezelController::SetState(BezelController::State state,
-                               const gfx::PointF& scroll_position) {
+void BezelController::SetState(
+    BezelController::State state, float scroll_delta) {
   if (!left_right_delegate_ || state == state_)
     return;
 
-  if (state == BEZEL_SCROLLING_TWO_FINGERS) {
-    float delta = GetDistance(scroll_position, scroll_bezel_);
-    left_right_delegate_->ScrollBegin(scroll_bezel_, delta);
-  } else if (state_ == BEZEL_SCROLLING_TWO_FINGERS) {
-    left_right_delegate_->ScrollEnd();
-  }
+  LOG(ERROR) << "\nBezelController::SetState, state=*** " << state << " ***";
+
+      if (state == BEZEL_SCROLLING_TWO_FINGERS) {
+        left_right_delegate_->ScrollBegin(scroll_bezel_, scroll_delta);
+      } else if (state_ == BEZEL_SCROLLING_TWO_FINGERS) {
+        left_right_delegate_->ScrollEnd();
+      }

[Jun Mukai, 2014/07/25 20:21:29]

remove unnecessary indents

[mfomitchev, 2014/07/25 23:27:38]

Done.

   state_ = state;
   if (state == NONE) {
     scroll_bezel_ = BEZEL_NONE;
@@ -92,11 +98,16 @@ void BezelController::OnGestureEvent(ui::GestureEvent* event) {
   const gfx::PointF& event_location = event->location_f();
   const ui::GestureEventDetails& event_details = event->details();
   int num_touch_points = event_details.touch_points();
+  float scroll_delta = kScrollDeltaNone;
+  if (scroll_bezel_ != BEZEL_NONE) {
+    aura::Window* target_window = static_cast<aura::Window*> (event->target());
+    scroll_delta = GetDistance(
+        event_location, target_window, scroll_bezel_);
+  }
 
   if (type == ui::ET_GESTURE_BEGIN) {
     if (num_touch_points > 2) {
-      SetState(IGNORE_CURRENT_SCROLL,
-               gfx::Point(kScrollPositionNone, kScrollPositionNone));
+      SetState(IGNORE_CURRENT_SCROLL, kScrollDeltaNone);
       return;
     }
     BezelController::Bezel event_bezel = GetBezel(event->location_f());
@@ -104,12 +115,10 @@ void BezelController::OnGestureEvent(ui::GestureEvent* event) {
       case NONE:
         scroll_bezel_ = event_bezel;
         scroll_target_ = event->target();
-        if (event_bezel != BEZEL_LEFT && event_bezel != BEZEL_RIGHT) {
-          SetState(IGNORE_CURRENT_SCROLL,
-                   gfx::Point(kScrollPositionNone, kScrollPositionNone));
-        } else {
-          SetState(BEZEL_GESTURE_STARTED, event_location);
-        }
+        if (event_bezel != BEZEL_LEFT && event_bezel != BEZEL_RIGHT)
+          SetState(IGNORE_CURRENT_SCROLL, kScrollDeltaNone);
+        else
+          SetState(BEZEL_GESTURE_STARTED, kScrollDeltaNone);

[Jun Mukai, 2014/07/25 20:21:29]

I don't like using None all around. Can we make a struct for the state, which
has the enum and the distance.  Then declare two struct constructors: just
receiving enum (distance is None), and receiving enum and distance.

[mfomitchev, 2014/08/05 19:56:57]

I've just created an overloaded SetState method that takes one argument and
added a DCHECK there - this seems simpler and requires less changes to the
header. Let me know if you are ok with this.

[Jun Mukai, 2014/08/05 21:13:38]

SGTM

         break;
       case IGNORE_CURRENT_SCROLL:
         break;
@@ -120,12 +129,11 @@ void BezelController::OnGestureEvent(ui::GestureEvent* event) {
         DCHECK_NE(scroll_bezel_, BEZEL_NONE);
 
         if (event_bezel != scroll_bezel_) {
-          SetState(IGNORE_CURRENT_SCROLL,
-                   gfx::Point(kScrollPositionNone, kScrollPositionNone));
+          SetState(IGNORE_CURRENT_SCROLL, kScrollDeltaNone);
           return;
         }
         if (state_ == BEZEL_SCROLLING_ONE_FINGER)
-          SetState(BEZEL_SCROLLING_TWO_FINGERS, event_location);
+          SetState(BEZEL_SCROLLING_TWO_FINGERS, scroll_delta);
         break;
       case BEZEL_SCROLLING_TWO_FINGERS:
         // Should've exited above
@@ -138,10 +146,9 @@ void BezelController::OnGestureEvent(ui::GestureEvent* event) {
 
     CHECK(scroll_target_);
     if (num_touch_points == 1) {
-      SetState(NONE, gfx::Point(kScrollPositionNone, kScrollPositionNone));
+      SetState(NONE, kScrollDeltaNone);
     } else {
-      SetState(IGNORE_CURRENT_SCROLL,
-               gfx::Point(kScrollPositionNone, kScrollPositionNone));
+      SetState(IGNORE_CURRENT_SCROLL, kScrollDeltaNone);
     }
   } else if (type == ui::ET_GESTURE_SCROLL_BEGIN) {
     DCHECK(state_ == IGNORE_CURRENT_SCROLL || state_ == BEZEL_GESTURE_STARTED);
@@ -149,19 +156,18 @@ void BezelController::OnGestureEvent(ui::GestureEvent* event) {
       return;
 
     if (num_touch_points == 1) {
-      SetState(BEZEL_SCROLLING_ONE_FINGER, event_location);
+      SetState(BEZEL_SCROLLING_ONE_FINGER, scroll_delta);
       return;
     }
 
     DCHECK_EQ(num_touch_points, 2);
-    SetState(BEZEL_SCROLLING_TWO_FINGERS, event_location);
+    SetState(BEZEL_SCROLLING_TWO_FINGERS, scroll_delta);
     if (left_right_delegate_->CanScroll())
       event->SetHandled();
   } else if (type == ui::ET_GESTURE_SCROLL_UPDATE) {
     if (state_ != BEZEL_SCROLLING_TWO_FINGERS)
       return;
 
-    float scroll_delta = GetDistance(event_location, scroll_bezel_);
     left_right_delegate_->ScrollUpdate(scroll_delta);
     if (left_right_delegate_->CanScroll())
       event->SetHandled();