From 51df02821b86a7d48ddb708750328ad44279bdc6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 May 2017 07:00:42 -0700 Subject: [PATCH] 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 | 42 ++-- resources/celerity/packages.php | 1 - .../view/DifferentialChangesetListView.php | 7 - webroot/rsrc/externals/javelin/lib/DOM.js | 4 +- .../js/application/diff/DiffChangesetList.js | 149 +++++++++++++ .../behavior-edit-inline-comments.js | 209 ------------------ 6 files changed, 167 insertions(+), 245 deletions(-) delete mode 100644 webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 125568ab64..794b117e82 100644 --- a/resources/celerity/map.php +++ b/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', diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php index 9ea3fa6b35..619a5d6389 100644 --- a/resources/celerity/packages.php +++ b/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', diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 1e40595bd0..aa34b0415f 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/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 { diff --git a/webroot/rsrc/externals/javelin/lib/DOM.js b/webroot/rsrc/externals/javelin/lib/DOM.js index 189d778ad5..e0e3d764e0 100644 --- a/webroot/rsrc/externals/javelin/lib/DOM.js +++ b/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; } diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 6067217419..8c2ff6e90f 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/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 "" 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 + // "" 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 + // "" 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(); } } diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js deleted file mode 100644 index 08c09d6626..0000000000 --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ /dev/null @@ -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 - // 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 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 "" 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 - // "" 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 "" 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(); - }); - -});