Show lint issues inline in Differential

Summary: Take lint information and use it to render synthetic inline comments in Differential. Test Plan: https://secure.phabricator.com/file/view/PHID-FILE-2uapwmf5tqhgscbuty3b/ Reviewers: btrahan, jungejason Reviewed By: btrahan CC: aran, btrahan, epriestley, pad Maniphest Tasks: T24 Differential Revision: https://secure.phabricator.com/D1309
2012-01-04 09:10:37 -08:00
parent c289ec304a
commit 4077ef2555
6 changed files with 187 additions and 98 deletions
--- a/src/__celerity_resource_map__.php
+++ b/src/__celerity_resource_map__.php
@@ -163,7 +163,7 @@ celerity_register_resource_map(array(
  ),
  'differential-changeset-view-css' =>
  array(
-    'uri' => '/res/be4f9b2a/rsrc/css/application/differential/changeset-view.css',
+    'uri' => '/res/bc78a228/rsrc/css/application/differential/changeset-view.css',
    'type' => 'css',
    'requires' =>
    array(
@@ -330,6 +330,17 @@ celerity_register_resource_map(array(
    ),
    'disk' => '/rsrc/js/javelin/lib/behavior.js',
  ),
+  0 =>
+  array(
+    'uri' => '/res/b6096fdd/rsrc/js/javelin/lib/__tests__/URI.js',
+    'type' => 'js',
+    'requires' =>
+    array(
+      0 => 'javelin-uri',
+      1 => 'javelin-php-serializer',
+    ),
+    'disk' => '/rsrc/js/javelin/lib/__tests__/URI.js',
+  ),
  'javelin-behavior-aphront-basic-tokenizer' =>
  array(
    'uri' => '/res/9be30797/rsrc/js/application/core/behavior-tokenizer.js',
@@ -944,15 +955,6 @@ celerity_register_resource_map(array(
    ),
    'disk' => '/rsrc/js/javelin/ext/reactor/dom/RDOM.js',
  ),
-  0 =>
-  array(
-    'uri' => '/res/936e8e81/rsrc/js/javelin/docs/onload.js',
-    'type' => 'js',
-    'requires' =>
-    array(
-    ),
-    'disk' => '/rsrc/js/javelin/docs/onload.js',
-  ),
  'javelin-reactor-node-calmer' =>
  array(
    'uri' => '/res/5a35920a/rsrc/js/javelin/ext/reactor/core/ReactorNodeCalmer.js',
@@ -1464,7 +1466,7 @@ celerity_register_resource_map(array(
  ),
  'phabricator-standard-page-view' =>
  array(
-    'uri' => '/res/70d968ee/rsrc/css/application/base/standard-page-view.css',
+    'uri' => '/res/86e63cc7/rsrc/css/application/base/standard-page-view.css',
    'type' => 'css',
    'requires' =>
    array(
@@ -1680,6 +1682,30 @@ celerity_register_resource_map(array(
      'uri' => '/res/pkg/4e7acf1a/core.pkg.js',
      'type' => 'js',
    ),
+    '4f6e449d' =>
+    array(
+      'name' => 'core.pkg.css',
+      'symbols' =>
+      array(
+        0 => 'phabricator-core-css',
+        1 => 'phabricator-core-buttons-css',
+        2 => 'phabricator-standard-page-view',
+        3 => 'aphront-dialog-view-css',
+        4 => 'aphront-form-view-css',
+        5 => 'aphront-panel-view-css',
+        6 => 'aphront-side-nav-view-css',
+        7 => 'aphront-table-view-css',
+        8 => 'aphront-crumbs-view-css',
+        9 => 'aphront-tokenizer-control-css',
+        10 => 'aphront-typeahead-control-css',
+        11 => 'aphront-list-filter-view-css',
+        12 => 'phabricator-directory-css',
+        13 => 'phabricator-remarkup-css',
+        14 => 'syntax-highlighting-css',
+      ),
+      'uri' => '/res/pkg/4f6e449d/core.pkg.css',
+      'type' => 'css',
+    ),
    '6d89c54c' =>
    array(
      'name' => 'differential.pkg.js',
@@ -1704,6 +1730,27 @@ celerity_register_resource_map(array(
      'uri' => '/res/pkg/6d89c54c/differential.pkg.js',
      'type' => 'js',
    ),
+    '8b139246' =>
+    array(
+      'name' => 'differential.pkg.css',
+      'symbols' =>
+      array(
+        0 => 'differential-core-view-css',
+        1 => 'differential-changeset-view-css',
+        2 => 'differential-revision-detail-css',
+        3 => 'differential-revision-history-css',
+        4 => 'differential-table-of-contents-css',
+        5 => 'differential-revision-comment-css',
+        6 => 'differential-revision-add-comment-css',
+        7 => 'differential-revision-comment-list-css',
+        8 => 'phabricator-object-selector-css',
+        9 => 'aphront-headsup-action-list-view-css',
+        10 => 'phabricator-content-source-view-css',
+        11 => 'differential-local-commits-view-css',
+      ),
+      'uri' => '/res/pkg/8b139246/differential.pkg.css',
+      'type' => 'css',
+    ),
    'b164acea' =>
    array(
      'name' => 'javelin.pkg.js',
@@ -1739,74 +1786,29 @@ celerity_register_resource_map(array(
      'uri' => '/res/pkg/bbe7e6f7/typeahead.pkg.js',
      'type' => 'js',
    ),
-    'e4f8b52c' =>
-    array(
-      'name' => 'differential.pkg.css',
-      'symbols' =>
-      array(
-        0 => 'differential-core-view-css',
-        1 => 'differential-changeset-view-css',
-        2 => 'differential-revision-detail-css',
-        3 => 'differential-revision-history-css',
-        4 => 'differential-table-of-contents-css',
-        5 => 'differential-revision-comment-css',
-        6 => 'differential-revision-add-comment-css',
-        7 => 'differential-revision-comment-list-css',
-        8 => 'phabricator-object-selector-css',
-        9 => 'aphront-headsup-action-list-view-css',
-        10 => 'phabricator-content-source-view-css',
-        11 => 'differential-local-commits-view-css',
-      ),
-      'uri' => '/res/pkg/e4f8b52c/differential.pkg.css',
-      'type' => 'css',
-    ),
-    'f3884b93' =>
-    array(
-      'name' => 'core.pkg.css',
-      'symbols' =>
-      array(
-        0 => 'phabricator-core-css',
-        1 => 'phabricator-core-buttons-css',
-        2 => 'phabricator-standard-page-view',
-        3 => 'aphront-dialog-view-css',
-        4 => 'aphront-form-view-css',
-        5 => 'aphront-panel-view-css',
-        6 => 'aphront-side-nav-view-css',
-        7 => 'aphront-table-view-css',
-        8 => 'aphront-crumbs-view-css',
-        9 => 'aphront-tokenizer-control-css',
-        10 => 'aphront-typeahead-control-css',
-        11 => 'aphront-list-filter-view-css',
-        12 => 'phabricator-directory-css',
-        13 => 'phabricator-remarkup-css',
-        14 => 'syntax-highlighting-css',
-      ),
-      'uri' => '/res/pkg/f3884b93/core.pkg.css',
-      'type' => 'css',
-    ),
  ),
  'reverse' =>
  array(
-    'aphront-crumbs-view-css' => 'f3884b93',
-    'aphront-dialog-view-css' => 'f3884b93',
-    'aphront-form-view-css' => 'f3884b93',
-    'aphront-headsup-action-list-view-css' => 'e4f8b52c',
-    'aphront-list-filter-view-css' => 'f3884b93',
-    'aphront-panel-view-css' => 'f3884b93',
-    'aphront-side-nav-view-css' => 'f3884b93',
-    'aphront-table-view-css' => 'f3884b93',
-    'aphront-tokenizer-control-css' => 'f3884b93',
-    'aphront-typeahead-control-css' => 'f3884b93',
-    'differential-changeset-view-css' => 'e4f8b52c',
-    'differential-core-view-css' => 'e4f8b52c',
+    'aphront-crumbs-view-css' => '4f6e449d',
+    'aphront-dialog-view-css' => '4f6e449d',
+    'aphront-form-view-css' => '4f6e449d',
+    'aphront-headsup-action-list-view-css' => '8b139246',
+    'aphront-list-filter-view-css' => '4f6e449d',
+    'aphront-panel-view-css' => '4f6e449d',
+    'aphront-side-nav-view-css' => '4f6e449d',
+    'aphront-table-view-css' => '4f6e449d',
+    'aphront-tokenizer-control-css' => '4f6e449d',
+    'aphront-typeahead-control-css' => '4f6e449d',
+    'differential-changeset-view-css' => '8b139246',
+    'differential-core-view-css' => '8b139246',
    'differential-inline-comment-editor' => '6d89c54c',
-    'differential-local-commits-view-css' => 'e4f8b52c',
-    'differential-revision-add-comment-css' => 'e4f8b52c',
-    'differential-revision-comment-css' => 'e4f8b52c',
-    'differential-revision-comment-list-css' => 'e4f8b52c',
-    'differential-revision-detail-css' => 'e4f8b52c',
-    'differential-revision-history-css' => 'e4f8b52c',
-    'differential-table-of-contents-css' => 'e4f8b52c',
+    'differential-local-commits-view-css' => '8b139246',
+    'differential-revision-add-comment-css' => '8b139246',
+    'differential-revision-comment-css' => '8b139246',
+    'differential-revision-comment-list-css' => '8b139246',
+    'differential-revision-detail-css' => '8b139246',
+    'differential-revision-history-css' => '8b139246',
+    'differential-table-of-contents-css' => '8b139246',
    'diffusion-commit-view-css' => '03ef179e',
    'javelin-behavior' => 'b164acea',
    'javelin-behavior-aphront-basic-tokenizer' => 'bbe7e6f7',
@@ -1844,17 +1846,17 @@ celerity_register_resource_map(array(
    'javelin-util' => 'b164acea',
    'javelin-vector' => 'b164acea',
    'javelin-workflow' => '4e7acf1a',
-    'phabricator-content-source-view-css' => 'e4f8b52c',
-    'phabricator-core-buttons-css' => 'f3884b93',
-    'phabricator-core-css' => 'f3884b93',
-    'phabricator-directory-css' => 'f3884b93',
+    'phabricator-content-source-view-css' => '8b139246',
+    'phabricator-core-buttons-css' => '4f6e449d',
+    'phabricator-core-css' => '4f6e449d',
+    'phabricator-directory-css' => '4f6e449d',
    'phabricator-drag-and-drop-file-upload' => '6d89c54c',
    'phabricator-keyboard-shortcut' => '4e7acf1a',
    'phabricator-keyboard-shortcut-manager' => '4e7acf1a',
-    'phabricator-object-selector-css' => 'e4f8b52c',
-    'phabricator-remarkup-css' => 'f3884b93',
+    'phabricator-object-selector-css' => '8b139246',
+    'phabricator-remarkup-css' => '4f6e449d',
    'phabricator-shaped-request' => '6d89c54c',
-    'phabricator-standard-page-view' => 'f3884b93',
-    'syntax-highlighting-css' => 'f3884b93',
+    'phabricator-standard-page-view' => '4f6e449d',
+    'syntax-highlighting-css' => '4f6e449d',
  ),
 ));
--- a/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php
+++ b/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.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.
@@ -123,7 +123,6 @@ class DifferentialChangesetViewController extends DifferentialController {
      $choice->attachHunks($synthetic->getHunks());

      $changeset = $choice;
-      $changeset->setID(null);
    }

    $spec = $request->getStr('range');
@@ -143,10 +142,24 @@ class DifferentialChangesetViewController extends DifferentialController {
      array($left_source, $right_source),
      $author_phid);

+    if ($left_new) {
+      $inlines = array_merge(
+        $inlines,
+        $this->buildLintInlineComments($left));
+    }
+
+    if ($right_new) {
+      $inlines = array_merge(
+        $inlines,
+        $this->buildLintInlineComments($right));
+    }
+
    $phids = array();
    foreach ($inlines as $inline) {
      $parser->parseInlineComment($inline);
-      $phids[$inline->getAuthorPHID()] = true;
+      if ($inline->getAuthorPHID()) {
+        $phids[$inline->getAuthorPHID()] = true;
+      }
    }
    $phids = array_keys($phids);

@@ -238,4 +251,34 @@ class DifferentialChangesetViewController extends DifferentialController {
      ->setContent($text);
  }

+  private function buildLintInlineComments($changeset) {
+    $lint = id(new DifferentialDiffProperty())->loadOneWhere(
+      'diffID = %d AND name = %s',
+      $changeset->getDiffID(),
+      'arc:lint');
+    if (!$lint) {
+      return;
+    }
+    $lint = $lint->getData();
+
+    $inlines = array();
+    foreach ($lint as $msg) {
+      if ($msg['path'] != $changeset->getFileName()) {
+        continue;
+      }
+      $inline = new DifferentialInlineComment();
+      $inline->setChangesetID($changeset->getID());
+      $inline->setIsNewFile(true);
+      $inline->setSyntheticAuthor('Lint: '.$msg['name']);
+      $inline->setLineNumber($msg['line']);
+      $inline->setLineLength(0);
+
+      $inline->setContent('%%%'.$msg['description'].'%%%');
+
+      $inlines[] = $inline;
+    }
+
+    return $inlines;
+  }
+
 }
--- a/src/applications/differential/controller/changesetview/init.php
+++ b/src/applications/differential/controller/changesetview/init.php
@@ -13,6 +13,7 @@ phutil_require_module('phabricator', 'aphront/response/file');
 phutil_require_module('phabricator', 'applications/differential/controller/base');
 phutil_require_module('phabricator', 'applications/differential/parser/changeset');
 phutil_require_module('phabricator', 'applications/differential/storage/changeset');
+phutil_require_module('phabricator', 'applications/differential/storage/diffproperty');
 phutil_require_module('phabricator', 'applications/differential/storage/inlinecomment');
 phutil_require_module('phabricator', 'applications/differential/view/changesetdetailview');
 phutil_require_module('phabricator', 'applications/differential/view/primarypane');
--- a/src/applications/differential/storage/inlinecomment/DifferentialInlineComment.php
+++ b/src/applications/differential/storage/inlinecomment/DifferentialInlineComment.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.
@@ -31,4 +31,15 @@ class DifferentialInlineComment extends DifferentialDAO {
  protected $content;
  protected $cache;

+  private $syntheticAuthor;
+
+  public function setSyntheticAuthor($synthetic_author) {
+    $this->syntheticAuthor = $synthetic_author;
+    return $this;
+  }
+
+  public function getSyntheticAuthor() {
+    return $this->syntheticAuthor;
+  }
+
 }
--- a/src/applications/differential/view/inlinecomment/DifferentialInlineCommentView.php
+++ b/src/applications/differential/view/inlinecomment/DifferentialInlineCommentView.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.
@@ -88,8 +88,13 @@ final class DifferentialInlineCommentView extends AphrontView {

    $links = array();

+    $is_synthetic = false;
+    if ($inline->getSyntheticAuthor()) {
+      $is_synthetic = true;
+    }
+
    $is_draft = false;
-    if (!$inline->getCommentID()) {
+    if (!$inline->getCommentID() && !$is_synthetic) {
      $links[] = 'Not Submitted Yet';
      $is_draft = true;
    }
@@ -113,14 +118,22 @@ final class DifferentialInlineCommentView extends AphrontView {
        ),
        'Next');

-      $links[] = javelin_render_tag(
-        'a',
-        array(
-          'href'        => '#',
-          'mustcapture' => true,
-          'sigil'       => 'differential-inline-reply',
-        ),
-        'Reply');
+      if (!$is_synthetic) {
+
+        // NOTE: No product reason why you can't reply to these, but the reply
+        // mechanism currently sends the inline comment ID to the server, not
+        // file/line information, and synthetic comments don't have an inline
+        // comment ID.
+
+        $links[] = javelin_render_tag(
+          'a',
+          array(
+            'href'        => '#',
+            'mustcapture' => true,
+            'sigil'       => 'differential-inline-reply',
+          ),
+          'Reply');
+      }
    }

    if ($this->editable && !$this->preview) {
@@ -178,8 +191,17 @@ final class DifferentialInlineCommentView extends AphrontView {
    if ($is_draft) {
      $classes[] = 'differential-inline-comment-unsaved-draft';
    }
+    if ($is_synthetic) {
+      $classes[] = 'differential-inline-comment-synthetic';
+    }
    $classes = implode(' ', $classes);

+    if ($is_synthetic) {
+      $author = $inline->getSyntheticAuthor();
+    } else {
+      $author = $handles[$inline->getAuthorPHID()]->getName();
+    }
+
    $markup = javelin_render_tag(
      'div',
      array(
@@ -191,7 +213,7 @@ final class DifferentialInlineCommentView extends AphrontView {
        $anchor.
        $links.
        '<span class="differential-inline-comment-line">'.$line.'</span>'.
-        phutil_escape_html($handles[$inline->getAuthorPHID()]->getName()).
+        phutil_escape_html($author).
      '</div>'.
      '<div class="phabricator-remarkup">'.
        $content.
--- a/webroot/rsrc/css/application/differential/changeset-view.css
+++ b/webroot/rsrc/css/application/differential/changeset-view.css
@@ -161,6 +161,16 @@
  border-bottom: 1px solid #aaaaaa;
 }

+.differential-inline-comment-synthetic {
+  background: #efffff;
+  border: 1px solid #20dfdf;
+}
+
+.differential-inline-comment-synthetic .differential-inline-comment-head {
+  border-bottom: 1px solid #20dfdf;
+}
+
+
 .differential-inline-comment-links,
 .differential-inline-comment-line {
  font-weight:  normal;