From de7aa2186ce23c02e4188ea1e871b4036011e205 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 3 Feb 2012 15:26:47 -0800 Subject: [PATCH] Resolve great internal confusion about left vs right inline comments Summary: This code was just all kinds of wrong, but got all the common cases anyone cares about correct. - In edit-inline-comments.js, if isOnRight() is true, use data.right, not data.left (derp). - Set data.left correctly, not to the same value as data.right (derp derp). - Set "isNewFile" based on $is_new, not $on_right (derp derp derp). Test Plan: - Added JS debugging code to print "OLD" vs "NEW" and "LEFT" vs "RIGHT". Clicked the left and right sides of diff-vs-base and diff-vs-diff diffs, verified output was accurate in all cases. - Added comments to the left-display-side of a diff-of-diffs, saved them, they showed up where I put them. Reviewers: btrahan, vrana Reviewed By: btrahan CC: aran, epriestley Maniphest Tasks: T543 Differential Revision: https://secure.phabricator.com/D1567 --- src/__celerity_resource_map__.php | 80 +++++++++---------- ...ifferentialInlineCommentEditController.php | 4 +- .../DifferentialRevisionViewController.php | 1 + .../DifferentialChangesetDetailView.php | 14 +++- .../view/changesetdetailview/__init__.php | 1 + .../DifferentialChangesetListView.php | 11 +++ .../behavior-edit-inline-comments.js | 4 +- 7 files changed, 70 insertions(+), 45 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 4dabf8a5a8..8e4d133aff 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -487,7 +487,7 @@ celerity_register_resource_map(array( ), 'javelin-behavior-differential-edit-inline-comments' => array( - 'uri' => '/res/c24338ce/rsrc/js/application/differential/behavior-edit-inline-comments.js', + 'uri' => '/res/31a8ef7b/rsrc/js/application/differential/behavior-edit-inline-comments.js', 'type' => 'js', 'requires' => array( @@ -1729,6 +1729,30 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/03ef179e/diffusion.pkg.css', 'type' => 'css', ), + '15c4a6dd' => + array( + 'name' => 'differential.pkg.js', + 'symbols' => + array( + 0 => 'phabricator-drag-and-drop-file-upload', + 1 => 'phabricator-shaped-request', + 2 => 'javelin-behavior-differential-feedback-preview', + 3 => 'javelin-behavior-differential-edit-inline-comments', + 4 => 'javelin-behavior-differential-populate', + 5 => 'javelin-behavior-differential-show-more', + 6 => 'javelin-behavior-differential-diff-radios', + 7 => 'javelin-behavior-differential-accept-with-errors', + 8 => 'javelin-behavior-differential-comment-jump', + 9 => 'javelin-behavior-differential-add-reviewers-and-ccs', + 10 => 'javelin-behavior-differential-keyboard-navigation', + 11 => 'javelin-behavior-aphront-drag-and-drop', + 12 => 'javelin-behavior-aphront-drag-and-drop-textarea', + 13 => 'javelin-behavior-phabricator-object-selector', + 14 => 'differential-inline-comment-editor', + ), + 'uri' => '/res/pkg/15c4a6dd/differential.pkg.js', + 'type' => 'js', + ), '46547a92' => array( 'name' => 'core.pkg.js', @@ -1808,30 +1832,6 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/9fd6210b/core.pkg.css', 'type' => 'css', ), - 'a6562582' => - array( - 'name' => 'differential.pkg.js', - 'symbols' => - array( - 0 => 'phabricator-drag-and-drop-file-upload', - 1 => 'phabricator-shaped-request', - 2 => 'javelin-behavior-differential-feedback-preview', - 3 => 'javelin-behavior-differential-edit-inline-comments', - 4 => 'javelin-behavior-differential-populate', - 5 => 'javelin-behavior-differential-show-more', - 6 => 'javelin-behavior-differential-diff-radios', - 7 => 'javelin-behavior-differential-accept-with-errors', - 8 => 'javelin-behavior-differential-comment-jump', - 9 => 'javelin-behavior-differential-add-reviewers-and-ccs', - 10 => 'javelin-behavior-differential-keyboard-navigation', - 11 => 'javelin-behavior-aphront-drag-and-drop', - 12 => 'javelin-behavior-aphront-drag-and-drop-textarea', - 13 => 'javelin-behavior-phabricator-object-selector', - 14 => 'differential-inline-comment-editor', - ), - 'uri' => '/res/pkg/a6562582/differential.pkg.js', - 'type' => 'js', - ), 'b164acea' => array( 'name' => 'javelin.pkg.js', @@ -1866,7 +1866,7 @@ celerity_register_resource_map(array( 'aphront-typeahead-control-css' => '9fd6210b', 'differential-changeset-view-css' => '80580cea', 'differential-core-view-css' => '80580cea', - 'differential-inline-comment-editor' => 'a6562582', + 'differential-inline-comment-editor' => '15c4a6dd', 'differential-local-commits-view-css' => '80580cea', 'differential-revision-add-comment-css' => '80580cea', 'differential-revision-comment-css' => '80580cea', @@ -1877,20 +1877,20 @@ celerity_register_resource_map(array( 'diffusion-commit-view-css' => '03ef179e', 'javelin-behavior' => 'b164acea', 'javelin-behavior-aphront-basic-tokenizer' => '540effd7', - 'javelin-behavior-aphront-drag-and-drop' => 'a6562582', - 'javelin-behavior-aphront-drag-and-drop-textarea' => 'a6562582', + 'javelin-behavior-aphront-drag-and-drop' => '15c4a6dd', + 'javelin-behavior-aphront-drag-and-drop-textarea' => '15c4a6dd', 'javelin-behavior-aphront-form-disable-on-submit' => '46547a92', - 'javelin-behavior-differential-accept-with-errors' => 'a6562582', - 'javelin-behavior-differential-add-reviewers-and-ccs' => 'a6562582', - 'javelin-behavior-differential-comment-jump' => 'a6562582', - 'javelin-behavior-differential-diff-radios' => 'a6562582', - 'javelin-behavior-differential-edit-inline-comments' => 'a6562582', - 'javelin-behavior-differential-feedback-preview' => 'a6562582', - 'javelin-behavior-differential-keyboard-navigation' => 'a6562582', - 'javelin-behavior-differential-populate' => 'a6562582', - 'javelin-behavior-differential-show-more' => 'a6562582', + 'javelin-behavior-differential-accept-with-errors' => '15c4a6dd', + 'javelin-behavior-differential-add-reviewers-and-ccs' => '15c4a6dd', + 'javelin-behavior-differential-comment-jump' => '15c4a6dd', + 'javelin-behavior-differential-diff-radios' => '15c4a6dd', + 'javelin-behavior-differential-edit-inline-comments' => '15c4a6dd', + 'javelin-behavior-differential-feedback-preview' => '15c4a6dd', + 'javelin-behavior-differential-keyboard-navigation' => '15c4a6dd', + 'javelin-behavior-differential-populate' => '15c4a6dd', + 'javelin-behavior-differential-show-more' => '15c4a6dd', 'javelin-behavior-phabricator-keyboard-shortcuts' => '46547a92', - 'javelin-behavior-phabricator-object-selector' => 'a6562582', + 'javelin-behavior-phabricator-object-selector' => '15c4a6dd', 'javelin-behavior-phabricator-watch-anchor' => '46547a92', 'javelin-behavior-refresh-csrf' => '46547a92', 'javelin-behavior-workflow' => '46547a92', @@ -1915,12 +1915,12 @@ celerity_register_resource_map(array( 'phabricator-core-buttons-css' => '9fd6210b', 'phabricator-core-css' => '9fd6210b', 'phabricator-directory-css' => '9fd6210b', - 'phabricator-drag-and-drop-file-upload' => 'a6562582', + 'phabricator-drag-and-drop-file-upload' => '15c4a6dd', 'phabricator-keyboard-shortcut' => '46547a92', 'phabricator-keyboard-shortcut-manager' => '46547a92', 'phabricator-object-selector-css' => '80580cea', 'phabricator-remarkup-css' => '9fd6210b', - 'phabricator-shaped-request' => 'a6562582', + 'phabricator-shaped-request' => '15c4a6dd', 'phabricator-standard-page-view' => '9fd6210b', 'syntax-highlighting-css' => '9fd6210b', ), diff --git a/src/applications/differential/controller/inlinecommentedit/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/inlinecommentedit/DifferentialInlineCommentEditController.php index eb968856cc..63eb1424e0 100644 --- a/src/applications/differential/controller/inlinecommentedit/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/inlinecommentedit/DifferentialInlineCommentEditController.php @@ -1,7 +1,7 @@ setAuthorPHID($user->getPHID()) ->setLineNumber($number) ->setLineLength($length) - ->setIsNewFile($on_right) + ->setIsNewFile($is_new) ->setContent($text) ->save(); diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index d0338c5b45..328278153e 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -242,6 +242,7 @@ class DifferentialRevisionViewController extends DifferentialController { $changeset_view->setRevision($revision); $changeset_view->setDiff($target); $changeset_view->setRenderingReferences($rendering_references); + $changeset_view->setVsMap($vs_map); $changeset_view->setWhitespace($whitespace); if ($repository) { $changeset_view->setRepository($repository, $target); diff --git a/src/applications/differential/view/changesetdetailview/DifferentialChangesetDetailView.php b/src/applications/differential/view/changesetdetailview/DifferentialChangesetDetailView.php index c623b8a07c..a3aed7f45b 100644 --- a/src/applications/differential/view/changesetdetailview/DifferentialChangesetDetailView.php +++ b/src/applications/differential/view/changesetdetailview/DifferentialChangesetDetailView.php @@ -23,6 +23,7 @@ class DifferentialChangesetDetailView extends AphrontView { private $revisionID; private $symbolIndex; private $id; + private $vsChangesetID; public function setChangeset($changeset) { $this->changeset = $changeset; @@ -51,6 +52,15 @@ class DifferentialChangesetDetailView extends AphrontView { return $this->id; } + public function setVsChangesetID($vs_changeset_id) { + $this->vsChangesetID = $vs_changeset_id; + return $this; + } + + public function getVsChangesetID() { + return $this->vsChangesetID; + } + public function render() { require_celerity_resource('differential-changeset-view-css'); require_celerity_resource('syntax-highlighting-css'); @@ -91,7 +101,9 @@ class DifferentialChangesetDetailView extends AphrontView { array( 'sigil' => 'differential-changeset', 'meta' => array( - 'left' => $this->changeset->getID(), + 'left' => nonempty( + $this->getVsChangesetID(), + $this->changeset->getID()), 'right' => $this->changeset->getID(), ), 'class' => $class, diff --git a/src/applications/differential/view/changesetdetailview/__init__.php b/src/applications/differential/view/changesetdetailview/__init__.php index 783a68af76..cd6eeaa9a5 100644 --- a/src/applications/differential/view/changesetdetailview/__init__.php +++ b/src/applications/differential/view/changesetdetailview/__init__.php @@ -12,6 +12,7 @@ phutil_require_module('phabricator', 'infrastructure/javelin/markup'); phutil_require_module('phabricator', 'view/base'); phutil_require_module('phutil', 'markup'); +phutil_require_module('phutil', 'utils'); phutil_require_source('DifferentialChangesetDetailView.php'); diff --git a/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php b/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php index f1d0cb8234..4f149e3f70 100644 --- a/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php +++ b/src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php @@ -29,6 +29,7 @@ class DifferentialChangesetListView extends AphrontView { private $symbolIndexes = array(); private $repository; private $diff; + private $vsMap; public function setChangesets($changesets) { $this->changesets = $changesets; @@ -85,6 +86,15 @@ class DifferentialChangesetListView extends AphrontView { return $this; } + public function setVsMap(array $vs_map) { + $this->vsMap = $vs_map; + return $this; + } + + public function getVsMap() { + return $this->vsMap; + } + public function render() { require_celerity_resource('differential-changeset-view-css'); @@ -167,6 +177,7 @@ class DifferentialChangesetListView extends AphrontView { $detail->setChangeset($changeset); $detail->addButton($detail_button); $detail->setSymbolIndex(idx($this->symbolIndexes, $key)); + $detail->setVsChangesetID(idx($this->vsMap, $changeset->getID())); $uniq_id = celerity_generate_unique_node_id(); $detail->appendChild( diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js index 8dd896dcaf..e17d87826c 100644 --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js @@ -87,9 +87,9 @@ JX.behavior('differential-edit-inline-comments', function(config) { var data = e.getNodeData('differential-changeset'); if (isOnRight(target)) { - changeset = data.left; - } else { changeset = data.right; + } else { + changeset = data.left; } updateReticle();