From 6a13b3ea7e8244a645a2235187bf5bab332783a4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Mar 2012 19:45:16 -0700 Subject: [PATCH] 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 | 39 ++-- src/__phutil_library_map__.php | 2 + .../DifferentialRevisionCommentView.php | 200 +++++++----------- .../view/revisioncomment/__init__.php | 1 + .../inline/PhabricatorInlineSummaryView.php | 147 +++++++++++++ .../diff/view/inline/__init__.php | 17 ++ .../diff/inline-comment-summary.css | 55 +++++ .../differential/revision-comment.css | 53 ----- 8 files changed, 325 insertions(+), 189 deletions(-) create mode 100644 src/infrastructure/diff/view/inline/PhabricatorInlineSummaryView.php create mode 100644 src/infrastructure/diff/view/inline/__init__.php create mode 100644 webroot/rsrc/css/application/diff/inline-comment-summary.css diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 1075e0d3d0..6cf8ea88c8 100644 --- a/src/__celerity_resource_map__.php +++ b/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-core-view-css' => '1fd457be', + 'differential-changeset-view-css' => '551249fc', + 'differential-core-view-css' => '551249fc', 'differential-inline-comment-editor' => '0d08d81b', - 'differential-local-commits-view-css' => '1fd457be', - 'differential-revision-add-comment-css' => '1fd457be', - 'differential-revision-comment-css' => '1fd457be', - 'differential-revision-comment-list-css' => '1fd457be', - 'differential-revision-detail-css' => '1fd457be', - 'differential-revision-history-css' => '1fd457be', - 'differential-table-of-contents-css' => '1fd457be', + 'differential-local-commits-view-css' => '551249fc', + 'differential-revision-add-comment-css' => '551249fc', + 'differential-revision-comment-css' => '551249fc', + 'differential-revision-comment-list-css' => '551249fc', + 'differential-revision-detail-css' => '551249fc', + 'differential-revision-history-css' => '551249fc', + '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', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 411c8c2f80..6fff0395ea 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php b/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php index a38b944f8b..a08df37894 100644 --- a/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php +++ b/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php @@ -120,127 +120,9 @@ final class DifferentialRevisionCommentView extends AphrontView { ''; } - 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[] = ''; - foreach ($inlines_by_changeset as $changeset_id => $inlines) { - $changeset = $changesets[$changeset_id]; - $inlines = msort($inlines, 'getLineNumber'); - $inline_render[] = - ''. - ''. - ''; - 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[] = - ''. - ''. - ''. - ''. - ''; - } - } - $inline_render[] = '
'. - phutil_escape_html($changeset->getFilename()). - '
'.$lines.''.$where.''. - '
'. - $inline_content. - '
'. - '
'; - $inline_render = implode("\n", $inline_render); - $inline_render = - '
'. - 'Inline Comments'. - '
'. - $inline_render; - } else { - $inline_render = null; } $author = $this->handles[$comment->getAuthorPHID()]; @@ -326,7 +208,7 @@ final class DifferentialRevisionCommentView extends AphrontView { '
'. $content. '
'. - $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; + } + + } diff --git a/src/applications/differential/view/revisioncomment/__init__.php b/src/applications/differential/view/revisioncomment/__init__.php index 2996c1085e..9884e63639 100644 --- a/src/applications/differential/view/revisioncomment/__init__.php +++ b/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'); diff --git a/src/infrastructure/diff/view/inline/PhabricatorInlineSummaryView.php b/src/infrastructure/diff/view/inline/PhabricatorInlineSummaryView.php new file mode 100644 index 0000000000..0688a18469 --- /dev/null +++ b/src/infrastructure/diff/view/inline/PhabricatorInlineSummaryView.php @@ -0,0 +1,147 @@ +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[] = + ''. + ''. + phutil_escape_html($group). + ''. + ''; + + 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[] = + ''. + ''.$lines.''. + ($has_where ? + ''. + phutil_escape_html($where). + '' + : null). + ''. + '
'. + $item['content']. + '
'. + ''. + ''; + } + } + + return phutil_render_tag( + 'table', + array( + 'class' => 'phabricator-inline-summary-table', + ), + implode("\n", $rows)); + } + +} diff --git a/src/infrastructure/diff/view/inline/__init__.php b/src/infrastructure/diff/view/inline/__init__.php new file mode 100644 index 0000000000..0a33d3bedf --- /dev/null +++ b/src/infrastructure/diff/view/inline/__init__.php @@ -0,0 +1,17 @@ + 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; -} -