Use timeline view in Differential and make inlines somewhat usable again

Summary:
Ref T2222. This gets rid of Differential's custom view and uses a standard view instead.

This also mostly fixes the rendering logic for inlines.

This is headed to the `tmp.differential` branch.

Test Plan: {F112696}

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T1790, T2222

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

src/__phutil_library_map__.php

@@ -437,7 +437,6 @@ phutil_register_library_map(array(
     'DifferentialReviewersFieldSpecification' => 'applications/differential/field/specification/DifferentialReviewersFieldSpecification.php',
     'DifferentialReviewersView' => 'applications/differential/view/DifferentialReviewersView.php',
     'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php',
-    'DifferentialRevisionCommentListView' => 'applications/differential/view/DifferentialRevisionCommentListView.php',
     'DifferentialRevisionCommentView' => 'applications/differential/view/DifferentialRevisionCommentView.php',
     'DifferentialRevisionControlSystem' => 'applications/differential/constants/DifferentialRevisionControlSystem.php',
     'DifferentialRevisionDetailRenderer' => 'applications/differential/controller/DifferentialRevisionDetailRenderer.php',
@@ -465,6 +464,7 @@ phutil_register_library_map(array(
     'DifferentialTransaction' => 'applications/differential/storage/DifferentialTransaction.php',
     'DifferentialTransactionComment' => 'applications/differential/storage/DifferentialTransactionComment.php',
     'DifferentialTransactionQuery' => 'applications/differential/query/DifferentialTransactionQuery.php',
+    'DifferentialTransactionView' => 'applications/differential/view/DifferentialTransactionView.php',
     'DifferentialUnitFieldSpecification' => 'applications/differential/field/specification/DifferentialUnitFieldSpecification.php',
     'DifferentialUnitStatus' => 'applications/differential/constants/DifferentialUnitStatus.php',
     'DifferentialUnitTestResult' => 'applications/differential/constants/DifferentialUnitTestResult.php',
@@ -2951,7 +2951,6 @@ phutil_register_library_map(array(
       5 => 'HarbormasterBuildableInterface',
       6 => 'PhabricatorSubscribableInterface',
     ),
-    'DifferentialRevisionCommentListView' => 'AphrontView',
     'DifferentialRevisionCommentView' => 'AphrontView',
     'DifferentialRevisionDetailView' => 'AphrontView',
     'DifferentialRevisionEditController' => 'DifferentialController',
@@ -2979,6 +2978,7 @@ phutil_register_library_map(array(
     'DifferentialTransaction' => 'PhabricatorApplicationTransaction',
     'DifferentialTransactionComment' => 'PhabricatorApplicationTransactionComment',
     'DifferentialTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'DifferentialTransactionView' => 'PhabricatorApplicationTransactionView',
     'DifferentialUnitFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialViewPolicyFieldSpecification' => 'DifferentialFieldSpecification',
     'DiffusionBranchTableController' => 'DiffusionController',

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -258,15 +258,9 @@ final class DifferentialRevisionViewController extends DifferentialController {
     $revision_detail->setActions($actions);
     $revision_detail->setUser($user);
 
-    $comment_view = new DifferentialRevisionCommentListView();
-    $comment_view->setComments($comments);
-    $comment_view->setHandles($handles);
-    $comment_view->setInlineComments($inlines);
-    $comment_view->setChangesets($all_changesets);
-    $comment_view->setUser($user);
-    $comment_view->setTargetDiff($target);
-    $comment_view->setVersusDiffID($diff_vs);
-    $comment_view->setRevision($revision);
+    $comment_view = $this->buildTransactions(
+      $revision,
+      $changesets);
 
     if ($arc_project) {
       Javelin::initBehavior(
@@ -926,4 +920,28 @@ final class DifferentialRevisionViewController extends DifferentialController {
     return id(new AphrontRedirectResponse())->setURI($file->getBestURI());
 
   }
+
+  private function buildTransactions(
+    DifferentialRevision $revision,
+    array $changesets) {
+    $viewer = $this->getRequest()->getUser();
+
+    $xactions = id(new DifferentialTransactionQuery())
+      ->setViewer($viewer)
+      ->withObjectPHIDs(array($revision->getPHID()))
+      ->needComments(true)
+      ->execute();
+
+    $engine = id(new PhabricatorMarkupEngine())
+      ->setViewer($viewer);
+
+    $timeline = id(new DifferentialTransactionView())
+      ->setUser($viewer)
+      ->setObjectPHID($revision->getPHID())
+      ->setChangesets($changesets)
+      ->setTransactions($xactions);
+
+    return $timeline;
+  }
+
 }

src/applications/differential/storage/DifferentialTransaction.php

@@ -18,4 +18,20 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
     return new DifferentialTransactionComment();
   }
 
+  public function getTitle() {
+    $author_phid = $this->getAuthorPHID();
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    switch ($this->getTransactionType()) {
+      case self::TYPE_INLINE:
+        return pht(
+          '%s added inline comments.',
+          $this->renderHandleLink($author_phid));
+    }
+
+    return parent::getTitle();
+  }
+
 }

src/applications/differential/view/DifferentialRevisionCommentListView.php

@@ -1,209 +0,0 @@
-<?php
-
-final class DifferentialRevisionCommentListView extends AphrontView {
-
-  private $comments;
-  private $handles;
-  private $inlines;
-  private $changesets;
-  private $target;
-  private $versusDiffID;
-  private $id;
-  private $revision;
-
-  public function setRevision(DifferentialRevision $revision) {
-    $this->revision = $revision;
-    return $this;
-  }
-
-  public function getRevision() {
-    return $this->revision;
-  }
-
-  public function setComments(array $comments) {
-    assert_instances_of($comments, 'DifferentialComment');
-    $this->comments = $comments;
-    return $this;
-  }
-
-  public function setInlineComments(array $inline_comments) {
-    assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface');
-    $this->inlines = $inline_comments;
-    return $this;
-  }
-
-  public function setHandles(array $handles) {
-    assert_instances_of($handles, 'PhabricatorObjectHandle');
-    $this->handles = $handles;
-    return $this;
-  }
-
-  public function setChangesets(array $changesets) {
-    assert_instances_of($changesets, 'DifferentialChangeset');
-    $this->changesets = $changesets;
-    return $this;
-  }
-
-  public function setTargetDiff(DifferentialDiff $target) {
-    $this->target = $target;
-    return $this;
-  }
-
-  public function setVersusDiffID($diff_vs) {
-    $this->versusDiffID = $diff_vs;
-    return $this;
-  }
-
-  public function getID() {
-    if (!$this->id) {
-      $this->id = celerity_generate_unique_node_id();
-    }
-    return $this->id;
-  }
-
-  public function render() {
-
-    $this->requireResource('differential-revision-comment-list-css');
-
-    $engine = new PhabricatorMarkupEngine();
-    $engine->setViewer($this->user);
-    foreach ($this->comments as $comment) {
-      $comment->giveFacebookSomeArbitraryDiff($this->target);
-
-      $engine->addObject(
-        $comment,
-        DifferentialComment::MARKUP_FIELD_BODY);
-    }
-
-    foreach ($this->inlines as $inline) {
-      $engine->addObject(
-        $inline,
-        PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY);
-    }
-
-    $engine->process();
-
-    $inlines = mgroup($this->inlines, 'getCommentID');
-
-    $num = 1;
-    $html = array();
-    foreach ($this->comments as $comment) {
-      $view = new DifferentialRevisionCommentView();
-      $view->setComment($comment);
-      $view->setUser($this->user);
-      $view->setHandles($this->handles);
-      $view->setMarkupEngine($engine);
-//      $view->setInlineComments(idx($inlines, $comment->getID(), array()));
-      $view->setChangesets($this->changesets);
-      $view->setTargetDiff($this->target);
-      $view->setRevision($this->getRevision());
-      $view->setVersusDiffID($this->versusDiffID);
-      if ($comment->getAction() == DifferentialAction::ACTION_SUMMARIZE) {
-        $view->setAnchorName('summary');
-      } else if ($comment->getAction() == DifferentialAction::ACTION_TESTPLAN) {
-        $view->setAnchorName('test-plan');
-      } else {
-        $view->setAnchorName('comment-'.$num);
-        $num++;
-      }
-
-      $html[] = $view->render();
-    }
-
-    $objs = array_reverse(array_values($this->comments));
-    $html = array_reverse(array_values($html));
-    $user = $this->user;
-
-    $last_comment = null;
-    // Find the most recent comment by the viewer.
-    foreach ($objs as $position => $comment) {
-      if ($user && ($comment->getAuthorPHID() == $user->getPHID())) {
-        if ($last_comment === null) {
-          $last_comment = $position;
-        } else if ($last_comment == $position - 1) {
-          // If the viewer made several comments in a row, show them all. This
-          // is a spaz rule for epriestley.
-          $last_comment = $position;
-        }
-      }
-    }
-
-    $header = array();
-    $hidden = array();
-    if ($last_comment !== null) {
-      foreach ($objs as $position => $comment) {
-        if (!$comment->getPHID()) {
-          // These are synthetic comments with summary/test plan information.
-          $header[] = $html[$position];
-          unset($html[$position]);
-          continue;
-        }
-        if ($position <= $last_comment) {
-          // Always show comments after the viewer's last comment.
-          continue;
-        }
-        if ($position < 3) {
-          // Always show the 3 most recent comments.
-          continue;
-        }
-        $hidden[] = $position;
-      }
-    }
-
-    if (count($hidden) <= 3) {
-      // Don't hide if there's not much to hide.
-      $hidden = array();
-    }
-
-    $header = array_reverse($header);
-
-
-    $hidden = array_select_keys($html, $hidden);
-    $visible = array_diff_key($html, $hidden);
-
-    $hidden = array_reverse($hidden);
-    $visible = array_reverse($visible);
-
-    if ($hidden) {
-      $this->initBehavior(
-        'differential-show-all-comments',
-        array(
-          'markup' => implode("\n", $hidden),
-        ));
-      $hidden = javelin_tag(
-        'div',
-        array(
-          'sigil' =>  "differential-all-comments-container",
-        ),
-        phutil_tag(
-          'div',
-          array(
-            'class' => 'differential-older-comments-are-hidden',
-          ),
-          array(
-            pht(
-              '%s older comments are hidden.',
-              new PhutilNumber(count($hidden))),
-            ' ',
-            javelin_tag(
-              'a',
-              array(
-                'href' => '#',
-                'mustcapture' => true,
-                'sigil' => 'differential-show-all-comments',
-              ),
-              pht('Show all comments.')),
-          )));
-    } else {
-      $hidden = null;
-    }
-
-    return javelin_tag(
-      'div',
-      array(
-        'class' => 'differential-comment-list',
-        'id' => $this->getID(),
-      ),
-      array_merge($header, array($hidden), $visible));
-  }
-}

src/applications/differential/view/DifferentialTransactionView.php

@@ -0,0 +1,127 @@
+<?php
+
+final class DifferentialTransactionView
+  extends PhabricatorApplicationTransactionView {
+
+  private $changesets;
+
+  public function setChangesets(array $changesets) {
+    assert_instances_of($changesets, 'DifferentialChangeset');
+    $this->changesets = $changesets;
+    return $this;
+  }
+
+  public function getChangesets() {
+    return $this->changesets;
+  }
+
+  // TODO: There's a whole lot of code duplication between this and
+  // PholioTransactionView to handle inlines. Merge this into the core? Some of
+  // it can probably be shared, while other parts are trickier.
+
+  protected function shouldGroupTransactions(
+    PhabricatorApplicationTransaction $u,
+    PhabricatorApplicationTransaction $v) {
+
+    if ($u->getAuthorPHID() != $v->getAuthorPHID()) {
+      // Don't group transactions by different authors.
+      return false;
+    }
+
+    if (($v->getDateCreated() - $u->getDateCreated()) > 60) {
+      // Don't group if transactions that happened more than 60s apart.
+      return false;
+    }
+
+    switch ($u->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_COMMENT:
+      case DifferentialTransaction::TYPE_INLINE:
+        break;
+      default:
+        return false;
+    }
+
+    switch ($v->getTransactionType()) {
+      case DifferentialTransaction::TYPE_INLINE:
+        return true;
+    }
+
+    return parent::shouldGroupTransactions($u, $v);
+  }
+
+  protected function renderTransactionContent(
+    PhabricatorApplicationTransaction $xaction) {
+
+    $out = array();
+
+    $type_inline = DifferentialTransaction::TYPE_INLINE;
+
+    $group = $xaction->getTransactionGroup();
+    if ($xaction->getTransactionType() == $type_inline) {
+      array_unshift($group, $xaction);
+    } else {
+      $out[] = parent::renderTransactionContent($xaction);
+    }
+
+    if (!$group) {
+      return $out;
+    }
+
+    $inlines = array();
+    foreach ($group as $xaction) {
+      switch ($xaction->getTransactionType()) {
+        case DifferentialTransaction::TYPE_INLINE:
+          $inlines[] = $xaction;
+          break;
+        default:
+          throw new Exception("Unknown grouped transaction type!");
+      }
+    }
+
+    if ($inlines) {
+      $inline_view = new PhabricatorInlineSummaryView();
+
+      $changesets = $this->getChangesets();
+      $changesets = mpull($changesets, null, 'getID');
+
+      // Group the changesets by file and reorder them by display order.
+      $inline_groups = array();
+      foreach ($inlines as $inline) {
+        $inline_groups[$inline->getComment()->getChangesetID()][] = $inline;
+      }
+
+      $changsets = msort($changesets, 'getFilename');
+      $inline_groups = array_select_keys(
+        $inline_groups,
+        array_keys($changesets));
+
+      foreach ($inline_groups as $changeset_id => $group) {
+        $group = msort($group, 'getLineNumber');
+
+        $changeset = $changesets[$changeset_id];
+        $items = array();
+        foreach ($group as $inline) {
+          $comment = $inline->getComment();
+          $item = array(
+            'id' => $comment->getID(),
+            'line' => $comment->getLineNumber(),
+            'length' => $comment->getLineLength(),
+            'content' => parent::renderTransactionContent($inline),
+          );
+
+          // TODO: Fix the where/href stuff for nonlocal inlines.
+
+          $items[] = $item;
+        }
+        $inline_view->addCommentGroup(
+          $changeset->getFilename(),
+          $items);
+      }
+
+      $out[] = $inline_view;
+    }
+
+    return $out;
+  }
+
+}

src/applications/pholio/storage/PholioTransaction.php

@@ -1,8 +1,5 @@
 <?php
 
-/**
- * @group pholio
- */
 final class PholioTransaction extends PhabricatorApplicationTransaction {
 
   public function getApplicationName() {

src/applications/pholio/view/PholioTransactionView.php

@@ -16,7 +16,7 @@ final class PholioTransactionView
     }
 
     if (($v->getDateCreated() - $u->getDateCreated()) > 60) {
-      // Don't group if transactions happend more than 60s apart.
+      // Don't group if transactions happened more than 60s apart.
       return false;
     }