Separate the inline comment summary element into a separate view

Summary:
  - Affects the "Inline Comments" summary table which appears in comments that have attached inlines in the discussion threads in Differential.
  - Prepares for inclusion in Diffusion.
  - No application changes (minor CSS), just factors code better.
  - Simplify/separate CSS.

Test Plan: Looked at on-diff and off-diff comment summaries in Differential, display looked correct.

Reviewers: davidreuss, nh, btrahan

Reviewed By: davidreuss

CC: aran, epriestley

Maniphest Tasks: T904

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

src/__celerity_resource_map__.php

@@ -223,7 +223,7 @@ celerity_register_resource_map(array(
   ),
   'differential-revision-comment-css' =>
   array(
-    'uri' => '/res/7a0002f1/rsrc/css/application/differential/revision-comment.css',
+    'uri' => '/res/cac35316/rsrc/css/application/differential/revision-comment.css',
     'type' => 'css',
     'requires' =>
     array(
@@ -330,6 +330,15 @@ celerity_register_resource_map(array(
     ),
     'disk' => '/rsrc/css/application/herald/herald-test.css',
   ),
+  'inline-comment-summary-css' =>
+  array(
+    'uri' => '/res/338704f7/rsrc/css/application/diff/inline-comment-summary.css',
+    'type' => 'css',
+    'requires' =>
+    array(
+    ),
+    'disk' => '/rsrc/css/application/diff/inline-comment-summary.css',
+  ),
   'javelin-aphlict' =>
   array(
     'uri' => '/res/50cae715/rsrc/js/application/aphlict/Aphlict.js',
@@ -2003,7 +2012,7 @@ celerity_register_resource_map(array(
       'uri' => '/res/pkg/21d01ed8/core.pkg.js',
       'type' => 'js',
     ),
-    '1fd457be' =>
+    '551249fc' =>
     array(
       'name' => 'differential.pkg.css',
       'symbols' =>
@@ -2021,7 +2030,7 @@ celerity_register_resource_map(array(
         10 => 'phabricator-content-source-view-css',
         11 => 'differential-local-commits-view-css',
       ),
-      'uri' => '/res/pkg/1fd457be/differential.pkg.css',
+      'uri' => '/res/pkg/551249fc/differential.pkg.css',
       'type' => 'css',
     ),
     '0d08d81b' =>
@@ -2128,7 +2137,7 @@ celerity_register_resource_map(array(
     'aphront-crumbs-view-css' => '78e8854e',
     'aphront-dialog-view-css' => '78e8854e',
     'aphront-form-view-css' => '78e8854e',
-    'aphront-headsup-action-list-view-css' => '1fd457be',
+    'aphront-headsup-action-list-view-css' => '551249fc',
     'aphront-list-filter-view-css' => '78e8854e',
     'aphront-pager-view-css' => '78e8854e',
     'aphront-panel-view-css' => '78e8854e',
@@ -2136,16 +2145,16 @@ celerity_register_resource_map(array(
     'aphront-table-view-css' => '78e8854e',
     'aphront-tokenizer-control-css' => '78e8854e',
     'aphront-typeahead-control-css' => '78e8854e',
-    'differential-changeset-view-css' => '1fd457be',
+    'differential-changeset-view-css' => '551249fc',
-    'differential-core-view-css' => '1fd457be',
+    'differential-core-view-css' => '551249fc',
     'differential-inline-comment-editor' => '0d08d81b',
-    'differential-local-commits-view-css' => '1fd457be',
+    'differential-local-commits-view-css' => '551249fc',
-    'differential-revision-add-comment-css' => '1fd457be',
+    'differential-revision-add-comment-css' => '551249fc',
-    'differential-revision-comment-css' => '1fd457be',
+    'differential-revision-comment-css' => '551249fc',
-    'differential-revision-comment-list-css' => '1fd457be',
+    'differential-revision-comment-list-css' => '551249fc',
-    'differential-revision-detail-css' => '1fd457be',
+    'differential-revision-detail-css' => '551249fc',
-    'differential-revision-history-css' => '1fd457be',
+    'differential-revision-history-css' => '551249fc',
-    'differential-table-of-contents-css' => '1fd457be',
+    'differential-table-of-contents-css' => '551249fc',
     'diffusion-commit-view-css' => '61f9d480',
     'javelin-behavior' => '4fbae2af',
     'javelin-behavior-aphront-basic-tokenizer' => '2af849fb',
@@ -2194,7 +2203,7 @@ celerity_register_resource_map(array(
     'maniphest-task-summary-css' => '31583232',
     'maniphest-transaction-detail-css' => '31583232',
     'phabricator-app-buttons-css' => '78e8854e',
-    'phabricator-content-source-view-css' => '1fd457be',
+    'phabricator-content-source-view-css' => '551249fc',
     'phabricator-core-buttons-css' => '78e8854e',
     'phabricator-core-css' => '78e8854e',
     'phabricator-directory-css' => '78e8854e',
@@ -2204,7 +2213,7 @@ celerity_register_resource_map(array(
     'phabricator-keyboard-shortcut' => '21d01ed8',
     'phabricator-keyboard-shortcut-manager' => '21d01ed8',
     'phabricator-menu-item' => '21d01ed8',
-    'phabricator-object-selector-css' => '1fd457be',
+    'phabricator-object-selector-css' => '551249fc',
     'phabricator-paste-file-upload' => '21d01ed8',
     'phabricator-remarkup-css' => '78e8854e',
     'phabricator-shaped-request' => '0d08d81b',

src/__phutil_library_map__.php

@@ -595,6 +595,7 @@ phutil_register_library_map(array(
     'PhabricatorInfrastructureTestCase' => 'infrastructure/__tests__',
     'PhabricatorInlineCommentController' => 'infrastructure/diff/controller',
     'PhabricatorInlineCommentInterface' => 'infrastructure/diff/interface/inline',
+    'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/inline',
     'PhabricatorJavelinLinter' => 'infrastructure/lint/linter/javelin',
     'PhabricatorJumpNavHandler' => 'applications/search/engine/jumpnav',
     'PhabricatorLintEngine' => 'infrastructure/lint/engine',
@@ -1381,6 +1382,7 @@ phutil_register_library_map(array(
     'PhabricatorIRCWhatsNewHandler' => 'PhabricatorIRCHandler',
     'PhabricatorInfrastructureTestCase' => 'PhabricatorTestCase',
     'PhabricatorInlineCommentController' => 'PhabricatorController',
+    'PhabricatorInlineSummaryView' => 'AphrontView',
     'PhabricatorJavelinLinter' => 'ArcanistLinter',
     'PhabricatorLintEngine' => 'PhutilLintEngine',
     'PhabricatorLiskDAO' => 'LiskDAO',

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

@@ -120,127 +120,9 @@ final class DifferentialRevisionCommentView extends AphrontView {
         '</div>';
     }
 
-    if ($this->inlines) {
+    $inline_render = $this->renderInlineComments();
+    if ($inline_render) {
       $hide_comments = false;
-      $inline_render = array();
-      $inlines = $this->inlines;
-      $changesets = $this->changesets;
-      $inlines_by_changeset = mgroup($inlines, 'getChangesetID');
-      $inlines_by_changeset = array_select_keys(
-        $inlines_by_changeset,
-        array_keys($this->changesets));
-      $inline_render[] = '<table class="differential-inline-summary">';
-      foreach ($inlines_by_changeset as $changeset_id => $inlines) {
-        $changeset = $changesets[$changeset_id];
-        $inlines = msort($inlines, 'getLineNumber');
-        $inline_render[] =
-          '<tr>'.
-            '<th colspan="3">'.
-              phutil_escape_html($changeset->getFilename()).
-            '</th>'.
-          '</tr>';
-        foreach ($inlines as $inline) {
-          if (!$inline->getLineLength()) {
-            $lines = $inline->getLineNumber();
-          } else {
-            $lines = $inline->getLineNumber()."\xE2\x80\x93".
-                     ($inline->getLineNumber() + $inline->getLineLength());
-          }
-
-          $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',
-              ),
-              $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();
-          if (strlen($inline_content)) {
-            $inline_cache = $inline->getCache();
-            if ($inline_cache) {
-              $inline_content = $inline_cache;
-            } else {
-              $inline_content = $this->markupEngine->markupText(
-                $inline_content);
-              if ($inline->getID()) {
-                $inline->setCache($inline_content);
-                $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-                $inline->save();
-                unset($unguarded);
-              }
-            }
-          }
-
-          $inline_render[] =
-            '<tr>'.
-              '<td class="inline-line-number">'.$lines.'</td>'.
-              '<td class="inline-which-diff">'.$where.'</td>'.
-              '<td>'.
-                '<div class="phabricator-remarkup">'.
-                  $inline_content.
-                '</div>'.
-              '</td>'.
-            '</tr>';
-        }
-      }
-      $inline_render[] = '</table>';
-      $inline_render = implode("\n", $inline_render);
-      $inline_render =
-        '<div class="differential-inline-summary-section">'.
-          'Inline Comments'.
-        '</div>'.
-        $inline_render;
-    } else {
-      $inline_render = null;
     }
 
     $author = $this->handles[$comment->getAuthorPHID()];
@@ -326,7 +208,7 @@ final class DifferentialRevisionCommentView extends AphrontView {
         '<div class="differential-comment-core">'.
           $content.
         '</div>'.
-        $inline_render);
+        $this->renderSingleView($inline_render));
     }
 
     return $xaction_view->render();
@@ -340,4 +222,80 @@ final class DifferentialRevisionCommentView extends AphrontView {
     return implode(', ', $result);
   }
 
+  private function renderInlineComments() {
+    if (!$this->inlines) {
+      return null;
+    }
+
+    $inlines = $this->inlines;
+    $changesets = $this->changesets;
+    $inlines_by_changeset = mgroup($inlines, 'getChangesetID');
+    $inlines_by_changeset = array_select_keys(
+      $inlines_by_changeset,
+      array_keys($this->changesets));
+
+    $view = new PhabricatorInlineSummaryView();
+    foreach ($inlines_by_changeset as $changeset_id => $inlines) {
+      $changeset = $changesets[$changeset_id];
+      $items = array();
+      foreach ($inlines as $inline) {
+
+        $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.
+        }
+
+        $item = array(
+          'id'      => $inline->getID(),
+          'line'    => $inline->getLineNumber(),
+          'length'  => $inline->getLineLength(),
+          'content' => PhabricatorInlineSummaryView::renderCommentContent(
+            $inline,
+            $this->markupEngine),
+        );
+
+        if (!$is_visible) {
+          $diff_id = $changeset->getDiffID();
+          $item['where'] = '(On Diff #'.$diff_id.')';
+          $item['href']  = '?id='.$diff_id.'#inline-'.$inline->getID();
+        }
+
+        $items[] = $item;
+      }
+      $view->addCommentGroup($changeset->getFilename(), $items);
+    }
+
+    return $view;
+  }
+
+
 }

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

@@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'aphront/writeguard');
 phutil_require_module('phabricator', 'applications/differential/constants/action');
 phutil_require_module('phabricator', 'applications/differential/storage/comment');
 phutil_require_module('phabricator', 'infrastructure/celerity/api');
+phutil_require_module('phabricator', 'infrastructure/diff/view/inline');
 phutil_require_module('phabricator', 'view/base');
 phutil_require_module('phabricator', 'view/layout/transaction');
 

src/infrastructure/diff/view/inline/PhabricatorInlineSummaryView.php

@@ -0,0 +1,147 @@
+<?php
+
+/*
+ * 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.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class PhabricatorInlineSummaryView extends AphrontView {
+
+  private $groups = array();
+
+  public function addCommentGroup($name, array $items) {
+    $this->groups[$name] = $items;
+    return $this;
+  }
+
+  public static function renderCommentContent(
+    PhabricatorInlineCommentInterface $inline,
+    PhutilMarkupEngine $engine) {
+
+    $inline_content = $inline->getContent();
+    if (strlen($inline_content)) {
+      $inline_cache = $inline->getCache();
+      if ($inline_cache) {
+        $inline_content = $inline_cache;
+      } else {
+        $inline_content = $engine->markupText($inline_content);
+        if ($inline->getID()) {
+          $inline->setCache($inline_content);
+          $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+          $inline->save();
+          unset($unguarded);
+        }
+      }
+    }
+
+    return $inline_content;
+  }
+
+  public function render() {
+    require_celerity_resource('inline-comment-summary-css');
+    return $this->renderHeader().$this->renderTable();
+  }
+
+  private function renderHeader() {
+    return phutil_render_tag(
+      'div',
+      array(
+        'class' => 'phabricator-inline-summary',
+      ),
+      'Inline Comments');
+  }
+
+  private function renderTable() {
+    $rows = array();
+    foreach ($this->groups as $group => $items) {
+
+      $has_where = false;
+      foreach ($items as $item) {
+        if (!empty($item['where'])) {
+          $has_where = true;
+          break;
+        }
+      }
+
+      $cols = $has_where ? 3 : 2;
+
+      $rows[] =
+        '<tr>'.
+          '<th colspan="'.$cols.'">'.
+            phutil_escape_html($group).
+          '</th>'.
+        '</tr>';
+
+      foreach ($items as $item) {
+
+        $items = isort($items, 'line');
+
+        $line = $item['line'];
+        $length = $item['length'];
+        if ($length) {
+          $lines = $line."\xE2\x80\x93".($line + $length);
+        } else {
+          $lines = $line;
+        }
+
+        if (isset($item['href'])) {
+          $href = $item['href'];
+          $target = '_blank';
+          $tail = " \xE2\x86\x97";
+        } else {
+          $href = '#inline-'.$item['id'];
+          $target = null;
+          $tail = null;
+        }
+
+        $lines = phutil_escape_html($lines);
+        if ($href) {
+          $lines = phutil_render_tag(
+            'a',
+            array(
+              'href'    => $href,
+              'target'  => $target,
+              'class'   => 'num',
+            ),
+            $lines.$tail);
+        }
+
+        $where = idx($item, 'where');
+
+        $rows[] =
+          '<tr>'.
+            '<td class="inline-line-number">'.$lines.'</td>'.
+            ($has_where ?
+              '<td class="inline-which-diff">'.
+                phutil_escape_html($where).
+              '</td>'
+              : null).
+            '<td class="inline-summary-content">'.
+              '<div class="phabricator-remarkup">'.
+                $item['content'].
+              '</div>'.
+            '</td>'.
+          '</tr>';
+      }
+    }
+
+    return phutil_render_tag(
+      'table',
+      array(
+        'class' => 'phabricator-inline-summary-table',
+      ),
+      implode("\n", $rows));
+  }
+
+}

src/infrastructure/diff/view/inline/__init__.php

@@ -0,0 +1,17 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'aphront/writeguard');
+phutil_require_module('phabricator', 'infrastructure/celerity/api');
+phutil_require_module('phabricator', 'view/base');
+
+phutil_require_module('phutil', 'markup');
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('PhabricatorInlineSummaryView.php');

webroot/rsrc/css/application/diff/inline-comment-summary.css

@@ -0,0 +1,55 @@
+/**
+ * @provides inline-comment-summary-css
+ */
+
+.phabricator-inline-summary {
+  margin: .75em 0 .5em;
+  font-size: 11px;
+  border-bottom: 1px solid #dddddd;
+  color: #666666;
+}
+
+.phabricator-inline-summary-table {
+  width: 100%;
+}
+
+.phabricator-inline-summary-table th {
+  padding: 6px 0px;
+  color: #444444;
+}
+
+.phabricator-inline-summary-table td  {
+  padding: 0px 4px 6px;
+  white-space: nowrap;
+}
+
+.phabricator-inline-summary-table td.inline-line-number {
+  padding-left: 0px;
+  width: 100px;
+}
+
+.phabricator-inline-summary-table td.inline-which-diff {
+  color: #666666;
+  width: 120px;
+}
+
+.phabricator-inline-summary-table td.inline-summary-content {
+  vertical-align: top;
+  white-space: normal;
+}
+
+/* NOTE: These two rules provide a larger hit target for clicking line numbers
+   (by letting you click the entire cell) and a visual indicator that you're on
+   target (by highlighting the entire cell). */
+
+.phabricator-inline-summary-table td.inline-line-number a.num {
+  padding-left: 16px;
+  display: block;
+  font-weight: bold;
+}
+
+.phabricator-inline-summary-table td.inline-line-number a.num:hover {
+  background: #3b5998;
+  color: white;
+  text-decoration: none;
+}

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

@@ -93,56 +93,3 @@
 .phabricator-transaction-view .differential-comment-action-request_review {
   border-color:     #cc9966;
 }
-
-.differential-inline-summary th,
-.differential-inline-summary td {
-  vertical-align: top;
-  padding: 0 0 6px;
-}
-
-.differential-inline-summary th {
-  padding-top: 16px;
-  color: #666666;
-  font-weight: bold;
-}
-
-.differential-inline-summary tr > th:first-child {
-  padding-top: 2px;
-}
-
-
-.differential-inline-summary td.inline-line-number {
-  color: #444444;
-  white-space: nowrap;
-  text-align: left;
-  font-weight: bold;
-  padding: 0 4px;
-  width: 90px;
-}
-
-.differential-inline-summary td.inline-line-number .num {
-  display: block;
-  position: relative;
-  padding-left:   15px;
-  padding-right:  5px;
-  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;
-  text-decoration: none;
-}
-
-.differential-inline-summary-section {
-  margin: .75em 0 .5em;
-  font-size: 11px;
-  border-bottom: 1px solid #dddddd;
-  color: #666666;
-}
-