Link to undisplayed inline comments

Summary:
We currently don't link to comments which aren't visible. Link to the
appropriate diff in a new window, indicating where the comment lives.

Test Plan: Clicked visible, not-so-visible comments.

Reviewers: btrahan, jungejason, davidreuss

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Maniphest Tasks: T555, T449

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

src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php

@@ -210,6 +210,7 @@ class DifferentialRevisionViewController extends DifferentialController {
     $comment_view->setChangesets($all_changesets);
     $comment_view->setUser($user);
     $comment_view->setTargetDiff($target);
+    $comment_view->setVersusDiffID($diff_vs);
 
     $changeset_view = new DifferentialChangesetListView();
     $changeset_view->setChangesets($visible_changesets);

src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php

@@ -27,6 +27,7 @@ final class DifferentialRevisionCommentView extends AphrontView {
   private $target;
   private $commentNumber;
   private $user;
+  private $versusDiffID;
 
   public function setComment($comment) {
     $this->comment = $comment;
@@ -61,6 +62,12 @@ final class DifferentialRevisionCommentView extends AphrontView {
 
   public function setTargetDiff($target) {
     $this->target = $target;
+    return $this;
+  }
+
+  public function setVersusDiffID($diff_vs) {
+    $this->versusDiffID = $diff_vs;
+    return $this;
   }
 
   public function setCommentNumber($comment_number) {
@@ -181,15 +188,60 @@ final class DifferentialRevisionCommentView extends AphrontView {
                      ($inline->getLineNumber() + $inline->getLineLength());
           }
 
-          if (!$this->target ||
-              $changeset->getDiffID() === $this->target->getID()) {
+          $on_target = ($this->target) &&
+                       ($this->target->getID() == $changeset->getDiffID());
+
+          $is_visible = false;
+          if ($inline->getIsNewFile()) {
+            // This comment is on the right side of the versus diff, and visible
+            // on the left side of the page.
+            if ($this->versusDiffID) {
+              if ($changeset->getDiffID() == $this->versusDiffID) {
+                $is_visible = true;
+              }
+            }
+
+            // This comment is on the right side of the target diff, and visible
+            // on the right side of the page.
+            if ($on_target) {
+              $is_visible = true;
+            }
+          } else {
+            // Ths comment is on the left side of the target diff, and visible
+            // on the left side of the page.
+            if (!$this->versusDiffID) {
+              if ($on_target) {
+                $is_visible = true;
+              }
+            }
+
+            // TODO: We still get one edge case wrong here, when we have a
+            // versus diff and the file didn't exist in the old version. The
+            // comment is visible because we show the left side of the target
+            // diff when there's no corresponding file in the versus diff, but
+            // we incorrectly link it off-page.
+          }
+
+          $where = null;
+          if ($is_visible) {
             $lines = phutil_render_tag(
               'a',
               array(
-                'href' => '#inline-'.$inline->getID(),
-                'class' => 'num',
+                'href'    => '#inline-'.$inline->getID(),
+                'class'   => 'num',
               ),
               $lines);
+          } else {
+            $diff_id = $changeset->getDiffID();
+            $lines = phutil_render_tag(
+              'a',
+              array(
+                'href'    => '?id='.$diff_id.'#inline-'.$inline->getID(),
+                'class'   => 'num',
+                'target'  => '_blank',
+              ),
+              $lines." \xE2\x86\x97");
+            $where = '(On Diff #'.$diff_id.')';
           }
 
           $inline_content = $inline->getContent();
@@ -212,6 +264,7 @@ final class DifferentialRevisionCommentView extends AphrontView {
           $inline_render[] =
             '<tr>'.
               '<td class="inline-line-number">'.$lines.'</td>'.
+              '<td class="inline-which-diff">'.$where.'</td>'.
               '<td>'.
                 '<div class="phabricator-remarkup">'.
                   $inline_content.

src/applications/differential/view/revisioncommentlist/DifferentialRevisionCommentListView.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.
@@ -24,6 +24,7 @@ final class DifferentialRevisionCommentListView extends AphrontView {
   private $changesets;
   private $user;
   private $target;
+  private $versusDiffID;
 
   public function setComments(array $comments) {
     $this->comments = $comments;
@@ -52,6 +53,12 @@ final class DifferentialRevisionCommentListView extends AphrontView {
 
   public function setTargetDiff(DifferentialDiff $target) {
     $this->target = $target;
+    return $this;
+  }
+
+  public function setVersusDiffID($diff_vs) {
+    $this->versusDiffID = $diff_vs;
+    return $this;
   }
 
   public function render() {
@@ -75,6 +82,7 @@ final class DifferentialRevisionCommentListView extends AphrontView {
       $view->setInlineComments(idx($inlines, $comment->getID(), array()));
       $view->setChangesets($this->changesets);
       $view->setTargetDiff($this->target);
+      $view->setVersusDiffID($this->versusDiffID);
       $view->setCommentNumber($num++);
 
       $html[] = $view->render();

webroot/rsrc/css/application/differential/revision-comment.css

@@ -191,6 +191,11 @@
   width: 70px; /* Need lots of width for 23,950-23,951 */
 }
 
+.differential-inline-summary td.inline-which-diff {
+  padding: 0 8px 0 0;
+  color: #666666;
+}
+
 .differential-inline-summary td.inline-line-number a:hover {
   background: #3b5998;
   color: white;