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

@@ -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',
   ),

src/applications/differential/controller/inlinecommentedit/DifferentialInlineCommentEditController.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -112,7 +112,7 @@ class DifferentialInlineCommentEditController extends DifferentialController {
           ->setAuthorPHID($user->getPHID())
           ->setLineNumber($number)
           ->setLineLength($length)
-          ->setIsNewFile($on_right)
+          ->setIsNewFile($is_new)
           ->setContent($text)
           ->save();
 

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);

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,

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');

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(

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();