Support detaching/attaching panels via inter-strip drags.

chrome/browser/ui/panels/panel_drag_controller.cc

Index: chrome/browser/ui/panels/panel_drag_controller.cc
diff --git a/chrome/browser/ui/panels/panel_drag_controller.cc b/chrome/browser/ui/panels/panel_drag_controller.cc
index 2a21cd28b7ffdf9af6b959ce845b07a30c172a51..effcf471886765162645746fa42d580f37312a23 100644
--- a/chrome/browser/ui/panels/panel_drag_controller.cc
+++ b/chrome/browser/ui/panels/panel_drag_controller.cc
@@ -5,30 +5,86 @@
 #include "chrome/browser/ui/panels/panel_drag_controller.h"
 
 #include "base/logging.h"
+#include "chrome/browser/ui/panels/detached_panel_strip.h"
+#include "chrome/browser/ui/panels/docked_panel_strip.h"
 #include "chrome/browser/ui/panels/panel.h"
+#include "chrome/browser/ui/panels/panel_manager.h"
 #include "chrome/browser/ui/panels/panel_strip.h"
 
-PanelDragController::PanelDragController()
-    : dragging_panel_(NULL) {
+namespace {
+
+// The threshold to detach the docked panel to make it free-floating.
+const int kDetachDockedPanelThreshold = -80;

[jennb, 2012/03/01 00:33:38]

Thresholds are usually positive amounts.

[jianli, 2012/03/02 22:42:43]

Done.

+
+// The threshold to dock the detached panel.
+const int kDockDetachedPanelThreshold = -10;
+
+}  // namespace
+
+PanelDragController::PanelDragController(PanelManager* panel_manager)
+    : panel_manager_(panel_manager),
+      dragging_panel_(NULL),
+      dragging_panel_original_strip_(NULL),
+      has_dragged_to_other_strip_(false) {
 }
 
 PanelDragController::~PanelDragController() {
 }
 
-void PanelDragController::StartDragging(Panel* panel) {
+void PanelDragController::StartDragging(Panel* panel,
+                                        const gfx::Point& mouse_location) {
   DCHECK(!dragging_panel_);
   DCHECK(panel->draggable());
 
   dragging_panel_ = panel;
+  dragging_panel_original_strip_ = dragging_panel_->panel_strip();
   dragging_panel_original_position_ = panel->GetBounds().origin();
+  has_dragged_to_other_strip_ = false;
+  total_ommitted_delta_y_ = 0;

[jennb, 2012/03/01 00:33:38]

Seems cleaner to store original mouse location at start of drag, then compute
the difference between current and starting mouse locations when needed, rather
than accumulate delta y's during the drag process.

[jianli, 2012/03/02 22:42:43]

Removed. We now use original mouse location at start of dragging at docked
strip.

 
-  dragging_panel_->panel_strip()->StartDraggingPanel(panel);
+  // When the detached panel is being dragged, it should be always-on-top. This
+  // is to make sure it will not get obscured by other docked panels.
+  if (dragging_panel_original_strip_->type() == PanelStrip::DETACHED)
+    panel->SetAlwaysOnTop(true);

[jennb, 2012/03/01 00:33:38]

Always-on-top should only be set if panel is in the Dock. Don't want the
dragging panel to seem fully visible, only to become partially hidden when the
drag is finished (scenario when the panel overlaps the dock, but does not end up
in the dock).

[jianli, 2012/03/02 22:42:43]

Done.

+
+  dragging_panel_original_strip_->StartDraggingPanel(panel);
+
+  last_mouse_location_ = mouse_location;
 }
 
-void PanelDragController::Drag(int delta_x, int delta_y) {
+void PanelDragController::Drag(const gfx::Point& mouse_location) {
   DCHECK(dragging_panel_);
 
-  dragging_panel_->panel_strip()->DragPanel(dragging_panel_, delta_x, delta_y);
+  int delta_x = mouse_location.x() - last_mouse_location_.x();
+  int delta_y = mouse_location.y() - last_mouse_location_.y();
+
+  PanelStrip* strip_to_drag_to = CanDragToDockedStrip(mouse_location);

[jennb, 2012/03/01 00:33:38]

PanelStrip* target_strip = ComputeDragTarget(mouse_location);
if (target_strip != current strip)
  do stuff
else
  do other stuff

[jianli, 2012/03/02 22:42:43]

Done.

+  if (!strip_to_drag_to)
+    strip_to_drag_to = CanDragToDetachedStrip(mouse_location);
+
+  if (strip_to_drag_to) {
+    has_dragged_to_other_strip_ = true;
+
+    // Compensate all the y-coordinate drag movements that could be omitted
+    // in previous intra-strip drag.
+    gfx::Point new_position = dragging_panel_->GetBounds().origin();
+    new_position.Offset(delta_x, delta_y + total_ommitted_delta_y_);
+    total_ommitted_delta_y_ = 0;
+
+    MoveToStrip(dragging_panel_, strip_to_drag_to, new_position);
+  } else {
+    gfx::Point old_position = dragging_panel_->GetBounds().origin();
+    dragging_panel_->panel_strip()->DragPanel(dragging_panel_,
+                                              delta_x,
+                                              delta_y);
+
+    // Track all the y-coordinate drag movements that could be omitted
+    // in previous intra-strip drag.
+    gfx::Point new_position = dragging_panel_->GetBounds().origin();
+    total_ommitted_delta_y_ += delta_y - new_position.y() + old_position.y();
+  }
+
+  last_mouse_location_ = mouse_location;
 }
 
 void PanelDragController::EndDragging(bool cancelled) {
@@ -39,7 +95,74 @@ void PanelDragController::EndDragging(bool cancelled) {
   // this.
   Panel* panel = dragging_panel_;
   dragging_panel_ = NULL;
+
+  // If the drag is cancelled and the dragging panel is still in the different
+  // strip from the original strip, put it back to the original strip first.
+  if (cancelled && panel->panel_strip() != dragging_panel_original_strip_) {

[jennb, 2012/03/01 00:33:38]

I'm having a hard time picturing what happens here if the original strip is the
docked strip. Panel is moved back to the dock at a position that isn't within
the dock area, then it slides horizontally back to its original position? 

Should it be using original panel position here?

[jianli, 2012/03/02 22:42:43]

Changed per discussed.

+    MoveToStrip(panel,
+                dragging_panel_original_strip_,
+                panel->GetBounds().origin());
+  }
+
   panel->panel_strip()->EndDraggingPanel(panel, cancelled);
+
+  // When the drag ends, the dragging panel will stop staying always-on-top if
+  // it ends with the detached state.
+  if (panel->panel_strip()->type() == PanelStrip::DETACHED)
+    panel->SetAlwaysOnTop(false);

[jennb, 2012/03/01 00:33:38]

Again, it seems like the responsibility of the strip to decide about
always-on-top status.

[jianli, 2012/03/02 22:42:43]

Done.

+}
+
+PanelStrip* PanelDragController::CanDragToDockedStrip(

[jennb, 2012/03/01 00:33:38]

Seems like this function and CanDragToDetachedStrip could be combined into

if (current strip is detached)
  return dock strip if dock strip says panel can be attached
else if docked
  return detached strip if dock strip says panel can be detached

[jianli, 2012/03/02 22:42:43]

I think it is cleaner to split the different checking logic and put them in 2
functions.

+    const gfx::Point& mouse_location) const {
+  // It has to come from the detached strip.
+  if (dragging_panel_->panel_strip()->type() != PanelStrip::DETACHED)
+    return NULL;
+
+  // The bottom of the panel should come very close to or fall below the bottom
+  // of the docked area.
+  int new_panel_bottom = dragging_panel_->GetBounds().bottom() +

[jennb, 2012/03/01 00:33:38]

Would it be cleaner to do computations using starting and current mouse
positions?

[jianli, 2012/03/02 22:42:43]

The triggering condition for attach is to check the bottom of the panel. This
cannot be done purely based on mouse locations.

+      mouse_location.y() - last_mouse_location_.y();
+  int docked_area_bottom =
+      panel_manager_->docked_strip()->display_area().bottom();

[jennb, 2012/03/01 00:33:38]

Should dock strip be the one to decide if a panel can be detached/attached
rather than drag controller digging into display area of the dock?

[jianli, 2012/03/02 22:42:43]

Docked strip can only decide if a docked panel can be detached. It cannot decide
if a detached panel can be docked because this decision is made based on both
knowledge of detached strip (information about last mouse location) and docked
strip (information about docked area). So I rather keep both in drag controller.

+  if (new_panel_bottom - docked_area_bottom < kDockDetachedPanelThreshold)

[jennb, 2012/03/03 02:19:33]

if new_panel_bottom is above the bottom of the dock, this will be negative and
therefore, always less than threshold, even if new_panel_bottom is close to the
bottom of the dock.

+    return NULL;
+
+  return panel_manager_->docked_strip();
+}
+
+PanelStrip* PanelDragController::CanDragToDetachedStrip(
+    const gfx::Point& mouse_location) const {
+  // It has to come from the docked strip.
+  if (dragging_panel_->panel_strip()->type() != PanelStrip::DOCKED)
+    return NULL;
+
+  // The minimized docked panel is not allowed to detach.
+  if (dragging_panel_->expansion_state() != Panel::EXPANDED)
+    return NULL;
+
+  // The panel should be dragged up higher enough to pass certain threshold.
+  int delta_y = total_ommitted_delta_y_ +
+      mouse_location.y() - last_mouse_location_.y();
+  if (delta_y > kDetachDockedPanelThreshold)
+    return NULL;
+
+  return panel_manager_->detached_strip();
+}
+
+void PanelDragController::MoveToStrip(Panel* panel,
+                                      PanelStrip* new_strip,
+                                      const gfx::Point& new_position) {
+  // Stop dragging in old strip. This needs to be done before moving the panel.
+  PanelStrip* old_strip = panel->panel_strip();
+  old_strip->EndDraggingPanel(panel, false);

[jennb, 2012/03/01 00:33:38]

Confusing. Dragging isn't really ended. For instance, the dock still needs to
remember panel's original index in the strip until dragging is truly completed
in case the drag is cancelled.

[jianli, 2012/03/02 22:42:43]

Per discussion, this is what we want.

+
+  // Move the panel to new strip.

[jennb, 2012/03/01 00:33:38]

I'd prefer if moving a panel between strips was always brokered by PanelManager.
See https://chromiumcodereview.appspot.com/9560002/

[jianli, 2012/03/02 22:42:43]

I agree. I would merge with your patch later.

+  old_strip->RemovePanel(panel);
+  panel->set_panel_strip(new_strip);
+  new_strip->AddPanelAtPosition(panel, new_position);
+
+  // Start dragging in new strip.
+  new_strip->StartDraggingPanel(panel);

[jennb, 2012/03/01 00:33:38]

Very weird logic flow:

EndDragging()
MoveToStrip()
strip->StartDragging()

[jianli, 2012/03/02 22:42:43]

Per discussion, this is what we want. Renamed dragging methods to make the whole
logic clearer.

 }
 
 void PanelDragController::OnPanelClosed(Panel* panel) {