Move the "select a line range" inline code to DiffInline

Summary: Ref T12616. This makes line range selection use the new code, and removes the remainder of the old "hover a line number" / "select a line range" code.

Test Plan: Hovered line numbers; selected line ranges.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17927

resources/celerity/map.php

@@ -10,10 +10,10 @@ return array(
     'conpherence.pkg.css' => 'ff161f2d',
     'conpherence.pkg.js' => 'b5b51108',
     'core.pkg.css' => 'ee5f28cd',
-    'core.pkg.js' => '8c5f913d',
+    'core.pkg.js' => '0f87a6eb',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => 'ea471cb0',
-    'differential.pkg.js' => 'ae6f5198',
+    'differential.pkg.js' => '85c19957',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -237,7 +237,7 @@ return array(
     'rsrc/externals/javelin/ext/view/__tests__/ViewInterpreter.js' => '7a94d6a5',
     'rsrc/externals/javelin/ext/view/__tests__/ViewRenderer.js' => '6ea96ac9',
     'rsrc/externals/javelin/lib/Cookie.js' => '62dfea03',
-    'rsrc/externals/javelin/lib/DOM.js' => '805b806a',
+    'rsrc/externals/javelin/lib/DOM.js' => '4976858c',
     'rsrc/externals/javelin/lib/History.js' => 'd4505101',
     'rsrc/externals/javelin/lib/JSON.js' => '69adf288',
     'rsrc/externals/javelin/lib/Leader.js' => '7f243deb',
@@ -391,12 +391,11 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
     'rsrc/js/application/diff/DiffChangeset.js' => '68758d99',
-    'rsrc/js/application/diff/DiffChangesetList.js' => '57c491b5',
+    'rsrc/js/application/diff/DiffChangesetList.js' => '842e2676',
     'rsrc/js/application/diff/DiffInline.js' => '1afe9760',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
     'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76',
     'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
-    'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '1c6bc8cf',
     'rsrc/js/application/differential/behavior-populate.js' => '5e41c819',
     'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb',
     'rsrc/js/application/differential/behavior-user-select.js' => 'a8d8459d',
@@ -619,7 +618,6 @@ return array(
     'javelin-behavior-device' => 'bb1dd507',
     'javelin-behavior-diff-preview-link' => '051c7832',
     'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
-    'javelin-behavior-differential-edit-inline-comments' => '1c6bc8cf',
     'javelin-behavior-differential-feedback-preview' => 'b064af76',
     'javelin-behavior-differential-populate' => '5e41c819',
     'javelin-behavior-differential-toggle-files' => 'ca3f91eb',
@@ -715,7 +713,7 @@ return array(
     'javelin-color' => '7e41274a',
     'javelin-cookie' => '62dfea03',
     'javelin-diffusion-locate-file-source' => 'c93358e3',
-    'javelin-dom' => '805b806a',
+    'javelin-dom' => '4976858c',
     'javelin-dynval' => 'f6555212',
     'javelin-event' => '2ee659ce',
     'javelin-fx' => '54b612ba',
@@ -780,7 +778,7 @@ return array(
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
     'phabricator-diff-changeset' => '68758d99',
-    'phabricator-diff-changeset-list' => '57c491b5',
+    'phabricator-diff-changeset-list' => '842e2676',
     'phabricator-diff-inline' => '1afe9760',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
@@ -1040,13 +1038,6 @@ return array(
       'javelin-request',
       'javelin-uri',
     ),
-    '1c6bc8cf' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-util',
-      'javelin-vector',
-    ),
     '1def2711' => array(
       'javelin-install',
       'javelin-dom',
@@ -1250,6 +1241,13 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
+    '4976858c' => array(
+      'javelin-magical-init',
+      'javelin-install',
+      'javelin-util',
+      'javelin-vector',
+      'javelin-stratcom',
+    ),
     '49ae8328' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1331,9 +1329,6 @@ return array(
       'javelin-vector',
       'javelin-dom',
     ),
-    '57c491b5' => array(
-      'javelin-install',
-    ),
     '58dea2fa' => array(
       'javelin-install',
       'javelin-util',
@@ -1529,13 +1524,6 @@ return array(
       'javelin-vector',
       'javelin-stratcom',
     ),
-    '805b806a' => array(
-      'javelin-magical-init',
-      'javelin-install',
-      'javelin-util',
-      'javelin-vector',
-      'javelin-stratcom',
-    ),
     '834a1173' => array(
       'javelin-behavior',
       'javelin-scrollbar',
@@ -1544,6 +1532,9 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
+    '842e2676' => array(
+      'javelin-install',
+    ),
     '8499b6ab' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2421,7 +2412,6 @@ return array(
       'phabricator-drag-and-drop-file-upload',
       'phabricator-shaped-request',
       'javelin-behavior-differential-feedback-preview',
-      'javelin-behavior-differential-edit-inline-comments',
       'javelin-behavior-differential-populate',
       'javelin-behavior-differential-diff-radios',
       'javelin-behavior-aphront-drag-and-drop-textarea',

resources/celerity/packages.php

@@ -193,7 +193,6 @@ return array(
     'phabricator-shaped-request',
 
     'javelin-behavior-differential-feedback-preview',
-    'javelin-behavior-differential-edit-inline-comments',
     'javelin-behavior-differential-populate',
     'javelin-behavior-differential-diff-radios',
     'javelin-behavior-aphront-drag-and-drop-textarea',

src/applications/differential/view/DifferentialChangesetListView.php

@@ -274,13 +274,6 @@ final class DifferentialChangesetListView extends AphrontView {
       ),
     ));
 
-    if ($this->inlineURI) {
-      Javelin::initBehavior('differential-edit-inline-comments', array(
-        'uri' => $this->inlineURI,
-        'stage' => 'differential-review-stage',
-      ));
-    }
-
     if ($this->header) {
       $header = $this->header;
     } else {

webroot/rsrc/externals/javelin/lib/DOM.js

@@ -891,7 +891,7 @@ JX.install('DOM', {
      * it.
      *
      * @param   Node    Node to look above.
-     * @param   string  Tag name, like 'a' or 'textarea'.
+     * @param   string  Optional tag name, like 'a' or 'textarea'.
      * @param   string  Optionally, sigil which selected node must have.
      * @return  Node    Matching node.
      *
@@ -911,7 +911,7 @@ JX.install('DOM', {
         if (!result) {
           break;
         }
-        if (JX.DOM.isType(result, tagname)) {
+        if (!tagname || JX.DOM.isType(result, tagname)) {
           if (!sigil || JX.Stratcom.hasSigil(result, sigil)) {
             break;
           }

webroot/rsrc/js/application/diff/DiffChangesetList.js

@@ -62,6 +62,21 @@ JX.install('DiffChangesetList', {
       ['mouseover', 'mouseout'],
       'differential-inline-comment',
       onhover);
+
+    var onrangedown = JX.bind(this, this._ifawake, this._onrangedown);
+    JX.Stratcom.listen(
+      'mousedown',
+      ['differential-changeset', 'tag:th'],
+      onrangedown);
+
+    var onrangemove = JX.bind(this, this._ifawake, this._onrangemove);
+    JX.Stratcom.listen(
+      ['mouseover', 'mouseout'],
+      ['differential-changeset', 'tag:th'],
+      onrangemove);
+
+    var onrangeup = JX.bind(this, this._ifawake, this._onrangeup);
+    JX.Stratcom.listen('mouseup', null, onrangeup);
   },
 
   properties: {
@@ -85,6 +100,10 @@ JX.install('DiffChangesetList', {
     _hoverOrigin: null,
     _hoverTarget: null,
 
+    _rangeActive: false,
+    _rangeOrigin: null,
+    _rangeTarget: null,
+
     sleep: function() {
       this._asleep = true;
 
@@ -953,8 +972,18 @@ JX.install('DiffChangesetList', {
       this._redrawHover();
     },
 
+    _setHoverRange: function(origin, target) {
+      this._hoverOrigin = origin;
+      this._hoverTarget = target;
+
+      this._redrawHover();
+    },
+
     resetHover: function() {
       this._setHoverInline(null);
+
+      this._hoverOrigin = null;
+      this._hoverTarget = null;
     },
 
     _redrawHover: function() {
@@ -1056,6 +1085,126 @@ JX.install('DiffChangesetList', {
 
     getDisplaySideFromHeader: function(th) {
       return (th.parentNode.firstChild != th) ? 'right' : 'left';
+    },
+
+    _onrangedown: function(e) {
+      if (!e.isNormalMouseEvent()) {
+        return;
+      }
+
+      if (e.getIsTouchEvent()) {
+        return;
+      }
+
+      if (this._rangeActive) {
+        return;
+      }
+
+      var target = e.getTarget();
+      var number = this.getLineNumberFromHeader(target);
+      if (!number) {
+        return;
+      }
+
+      e.kill();
+      this._rangeActive = true;
+
+      this._rangeOrigin = target;
+      this._rangeTarget = target;
+
+      this._setHoverRange(this._rangeOrigin, this._rangeTarget);
+    },
+
+    _onrangemove: function(e) {
+      if (e.getIsTouchEvent()) {
+        return;
+      }
+
+      var target = e.getTarget();
+
+      // Don't update the range if this "<th />" doesn't correspond to a line
+      // number. For instance, this may be a dead line number, like the empty
+      // line numbers on the left hand side of a newly added file.
+      var number = this.getLineNumberFromHeader(target);
+      if (!number) {
+        return;
+      }
+
+      if (this._rangeActive) {
+        var origin = this._hoverOrigin;
+
+        // Don't update the reticle if we're selecting a line range and the
+        // "<th />" under the cursor is on the wrong side of the file. You can
+        // only leave inline comments on the left or right side of a file, not
+        // across lines on both sides.
+        var origin_side = this.getDisplaySideFromHeader(origin);
+        var target_side = this.getDisplaySideFromHeader(target);
+        if (origin_side != target_side) {
+          return;
+        }
+
+        // Don't update the reticle if we're selecting a line range and the
+        // "<th />" under the cursor corresponds to a different file. You can
+        // only leave inline comments on lines in a single file, not across
+        // multiple files.
+        var origin_table = JX.DOM.findAbove(origin, 'table');
+        var target_table = JX.DOM.findAbove(target, 'table');
+        if (origin_table != target_table) {
+          return;
+        }
+      }
+
+      var is_out = (e.getType() == 'mouseout');
+      if (is_out) {
+        if (this._rangeActive) {
+          // If we're dragging a range, just leave the state as it is. This
+          // allows you to drag over something invalid while selecting a
+          // range without the range flickering or getting lost.
+        } else {
+          // Otherwise, clear the current range.
+          this.resetHover();
+        }
+        return;
+      }
+
+      if (this._rangeActive) {
+        this._rangeTarget = target;
+      } else {
+        this._rangeOrigin = target;
+        this._rangeTarget = target;
+      }
+
+      this._setHoverRange(this._rangeOrigin, this._rangeTarget);
+    },
+
+    _onrangeup: function(e) {
+      if (!this._rangeActive) {
+        return;
+      }
+
+      e.kill();
+
+      var origin = this._rangeOrigin;
+      var target = this._rangeTarget;
+
+      // If the user dragged a range from the bottom to the top, swap the node
+      // order around.
+      if (JX.$V(origin).y > JX.$V(target).y) {
+        var tmp = target;
+        target = origin;
+        origin = tmp;
+      }
+
+      var node = JX.DOM.findAbove(origin, null, 'differential-changeset');
+      var changeset = this.getChangesetForNode(node);
+
+      changeset.newInlineForRange(origin, target);
+
+      this._rangeActive = false;
+      this._rangeOrigin = null;
+      this._rangeTarget = null;
+
+      this.resetHover();
     }
   }
 

webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js

@@ -1,209 +0,0 @@
-/**
- * @provides javelin-behavior-differential-edit-inline-comments
- * @requires javelin-behavior
- *           javelin-stratcom
- *           javelin-dom
- *           javelin-util
- *           javelin-vector
- */
-
-JX.behavior('differential-edit-inline-comments', function(config) {
-
-  var selecting = false;
-  var reticle = JX.$N('div', {className: 'differential-reticle'});
-  JX.DOM.hide(reticle);
-
-  var origin = null;
-  var target = null;
-  var root   = null;
-  var changeset = null;
-
-  function updateReticle() {
-    JX.DOM.getContentFrame().appendChild(reticle);
-
-    var top = origin;
-    var bot = target;
-    if (JX.$V(top).y > JX.$V(bot).y) {
-      var tmp = top;
-      top = bot;
-      bot = tmp;
-    }
-
-    // Find the leftmost cell that we're going to highlight: this is the next
-    // <td /> in the row. In 2up views, it should be directly adjacent. In
-    // 1up views, we may have to skip over the other line number column.
-    var l = top;
-    while (JX.DOM.isType(l, 'th')) {
-      l = l.nextSibling;
-    }
-
-    // Find the rightmost cell that we're going to highlight: this is the
-    // farthest consecutive, adjacent <td /> in the row. Sometimes the left
-    // and right nodes are the same (left side of 2up view); sometimes we're
-    // going to highlight several nodes (copy + code + coverage).
-    var r = l;
-    while (r.nextSibling && JX.DOM.isType(r.nextSibling, 'td')) {
-      r = r.nextSibling;
-    }
-
-    var pos = JX.$V(l)
-      .add(JX.Vector.getAggregateScrollForNode(l));
-
-    var dim = JX.$V(r)
-      .add(JX.Vector.getAggregateScrollForNode(r))
-      .add(-pos.x, -pos.y)
-      .add(JX.Vector.getDim(r));
-
-    var bpos = JX.$V(bot)
-      .add(JX.Vector.getAggregateScrollForNode(bot));
-    dim.y = (bpos.y - pos.y) + JX.Vector.getDim(bot).y;
-
-    pos.setPos(reticle);
-    dim.setDim(reticle);
-
-    JX.DOM.show(reticle);
-  }
-
-  function hideReticle() {
-    JX.DOM.hide(reticle);
-  }
-
-  function isOnRight(node) {
-    return node.parentNode.firstChild != node;
-  }
-
-  function getRowNumber(th_node) {
-    try {
-      return parseInt(th_node.id.match(/^C\d+[ON]L(\d+)$/)[1], 10);
-    } catch (x) {
-      return undefined;
-    }
-  }
-
-  JX.Stratcom.listen(
-    'mousedown',
-    ['differential-changeset', 'tag:th'],
-    function(e) {
-      if (e.isRightButton() ||
-          getRowNumber(e.getTarget()) === undefined) {
-        return;
-      }
-
-      if (selecting) {
-        return;
-      }
-
-      selecting = true;
-      root = e.getNode('differential-changeset');
-
-      origin = target = e.getTarget();
-
-      var data = e.getNodeData('differential-changeset');
-      if (isOnRight(target)) {
-        changeset = data.right;
-      } else {
-        changeset = data.left;
-      }
-
-      updateReticle();
-
-      e.kill();
-    });
-
-  JX.Stratcom.listen(
-    ['mouseover', 'mouseout'],
-    ['differential-changeset', 'tag:th'],
-    function(e) {
-      if (e.getIsTouchEvent()) {
-        return;
-      }
-
-      if (getRowNumber(e.getTarget()) === undefined) {
-        // Don't update the reticle if this "<th />" doesn't correspond to a
-        // line number. For instance, this may be a dead line number, like the
-        // empty line numbers on the left hand side of a newly added file.
-        return;
-      }
-
-      if (selecting) {
-        if (isOnRight(e.getTarget()) != isOnRight(origin)) {
-          // Don't update the reticle if we're selecting a line range and the
-          // "<th />" under the cursor is on the wrong side of the file. You
-          // can only leave inline comments on the left or right side of a
-          // file, not across lines on both sides.
-          return;
-        }
-
-        if (e.getNode('differential-changeset') !== root) {
-          // Don't update the reticle if we're selecting a line range and
-          // the "<th />" under the cursor corresponds to a different file.
-          // You can only leave inline comments on lines in a single file,
-          // not across multiple files.
-          return;
-        }
-      }
-
-      if (e.getType() == 'mouseout') {
-        if (selecting) {
-          // Don't hide the reticle if we're selecting, since we want to
-          // keep showing the line range that will be used if the mouse is
-          // released.
-          return;
-        }
-        hideReticle();
-      } else {
-        target = e.getTarget();
-        if (!selecting) {
-          // If we're just hovering the mouse and not selecting a line range,
-          // set the origin to the current row so we highlight it.
-          origin = target;
-        }
-
-        updateReticle();
-      }
-    });
-
-  JX.Stratcom.listen(
-    'mouseup',
-    null,
-    function(e) {
-      if (!selecting) {
-        return;
-      }
-
-      var o = getRowNumber(origin);
-      var t = getRowNumber(target);
-
-      var insert;
-      var len;
-      if (t < o) {
-        len = (o - t);
-        o = t;
-        insert = origin.parentNode;
-      } else {
-        len = (t - o);
-        insert = target.parentNode;
-      }
-
-      var view = JX.DiffChangeset.getForNode(root);
-
-      view.newInlineForRange(origin, target);
-
-      selecting = false;
-      origin = null;
-      target = null;
-
-      e.kill();
-    });
-
-  JX.Stratcom.listen(
-    ['mouseover', 'mouseout'],
-    'differential-inline-comment',
-    function(e) {
-      if (e.getIsTouchEvent()) {
-        return;
-      }
-      hideReticle();
-    });
-
-});