From e9f4a84a892ca0150e22c45356cdd559b26ab8b3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 27 May 2015 10:28:38 -0700 Subject: [PATCH] Allow inline comments to be individually hidden Summary: Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely. This is sticky per-user. My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all). Specifically, this adds a new action here: {F435621} Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you: {F435626} You can click the icon to reveal all hidden comments below the line. Test Plan: - Hid comments. - Showed comments. - Created, edited, deleted and submitted comments. - Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential). Reviewers: btrahan, chad Reviewed By: btrahan Subscribers: jparise, yelirekim, epriestley Maniphest Tasks: T7447 Differential Revision: https://secure.phabricator.com/D13009 --- resources/celerity/map.php | 46 ++++---- .../autopatches/20150525.diff.hidden.1.sql | 7 ++ src/__phutil_library_map__.php | 4 + .../storage/PhabricatorAuditInlineComment.php | 8 ++ .../DifferentialChangesetViewController.php | 1 + ...ifferentialInlineCommentEditController.php | 36 +++++++ ...erentialInlineCommentPreviewController.php | 1 + .../DifferentialRevisionViewController.php | 1 + .../query/DifferentialInlineCommentQuery.php | 26 +++++ .../DifferentialChangesetOneUpRenderer.php | 48 ++++++++- .../DifferentialChangesetTwoUpRenderer.php | 101 ++++++++++++------ .../storage/DifferentialHiddenComment.php | 24 +++++ .../storage/DifferentialInlineComment.php | 9 ++ .../storage/DifferentialRevision.php | 1 + .../DifferentialTransactionComment.php | 10 ++ .../view/DifferentialChangesetListView.php | 5 +- .../PhabricatorInlineCommentController.php | 24 +++++ .../PhabricatorInlineCommentInterface.php | 3 + .../view/PHUIDiffInlineCommentDetailView.php | 18 ++++ .../view/PHUIDiffInlineCommentRowScaffold.php | 10 ++ .../diff/view/PHUIDiffInlineCommentView.php | 4 + .../PHUIDiffOneUpInlineCommentRowScaffold.php | 2 +- .../diff/view/PHUIDiffRevealIconView.php | 27 +++++ .../PHUIDiffTwoUpInlineCommentRowScaffold.php | 2 +- .../differential/phui-inline-comment.css | 18 ++++ .../behavior-edit-inline-comments.js | 83 ++++++++++++++ 26 files changed, 454 insertions(+), 65 deletions(-) create mode 100644 resources/sql/autopatches/20150525.diff.hidden.1.sql create mode 100644 src/applications/differential/storage/DifferentialHiddenComment.php create mode 100644 src/infrastructure/diff/view/PHUIDiffRevealIconView.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index bf3f6f59fa..f9ebfb461b 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,8 +10,8 @@ return array( 'core.pkg.css' => '439658b5', 'core.pkg.js' => '328799d0', 'darkconsole.pkg.js' => 'e7393ebb', - 'differential.pkg.css' => 'bb338e4b', - 'differential.pkg.js' => '63a77807', + 'differential.pkg.css' => '30602b8c', + 'differential.pkg.js' => '8c98ce21', 'diffusion.pkg.css' => '591664fa', 'diffusion.pkg.js' => '0115b37c', 'maniphest.pkg.css' => '68d4dd3d', @@ -60,7 +60,7 @@ return array( 'rsrc/css/application/differential/add-comment.css' => 'c47f8c40', 'rsrc/css/application/differential/changeset-view.css' => 'e19cfd6e', 'rsrc/css/application/differential/core.css' => '7ac3cabc', - 'rsrc/css/application/differential/phui-inline-comment.css' => '2174771a', + 'rsrc/css/application/differential/phui-inline-comment.css' => 'aa16f165', 'rsrc/css/application/differential/results-table.css' => '181aa9d9', 'rsrc/css/application/differential/revision-comment.css' => '14b8565a', 'rsrc/css/application/differential/revision-history.css' => '0e8eb855', @@ -353,7 +353,7 @@ return array( 'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', 'rsrc/js/application/differential/behavior-dropdown-menus.js' => '2035b9cb', - 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'e723c323', + 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '037b59eb', 'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492', 'rsrc/js/application/differential/behavior-populate.js' => '8694b1df', 'rsrc/js/application/differential/behavior-show-field-details.js' => 'bba9eedf', @@ -443,7 +443,7 @@ return array( 'rsrc/js/core/behavior-device.js' => 'a205cf28', 'rsrc/js/core/behavior-drag-and-drop-textarea.js' => '6d49590e', 'rsrc/js/core/behavior-error-log.js' => '6882e80a', - 'rsrc/js/core/behavior-fancy-datepicker.js' => '2d4029a8', + 'rsrc/js/core/behavior-fancy-datepicker.js' => '5c0f680f', 'rsrc/js/core/behavior-file-tree.js' => '88236f00', 'rsrc/js/core/behavior-form.js' => '5c54cbf3', 'rsrc/js/core/behavior-gesture.js' => '3ab51e2c', @@ -560,7 +560,7 @@ return array( 'javelin-behavior-differential-comment-jump' => '4fdb476d', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-dropdown-menus' => '2035b9cb', - 'javelin-behavior-differential-edit-inline-comments' => 'e723c323', + 'javelin-behavior-differential-edit-inline-comments' => '037b59eb', 'javelin-behavior-differential-feedback-preview' => 'b064af76', 'javelin-behavior-differential-keyboard-navigation' => '2c426492', 'javelin-behavior-differential-populate' => '8694b1df', @@ -576,7 +576,7 @@ return array( 'javelin-behavior-durable-column' => '16c695bf', 'javelin-behavior-error-log' => '6882e80a', 'javelin-behavior-event-all-day' => '38dcf3c8', - 'javelin-behavior-fancy-datepicker' => '2d4029a8', + 'javelin-behavior-fancy-datepicker' => '5c0f680f', 'javelin-behavior-global-drag-and-drop' => 'c8e57404', 'javelin-behavior-herald-rule-editor' => '7ebaeed3', 'javelin-behavior-high-security-warning' => 'a464fe03', @@ -782,7 +782,7 @@ return array( 'phui-image-mask-css' => '5a8b09c8', 'phui-info-panel-css' => '27ea50a1', 'phui-info-view-css' => 'c6f0aef8', - 'phui-inline-comment-view-css' => '2174771a', + 'phui-inline-comment-view-css' => 'aa16f165', 'phui-list-view-css' => '2e25ebfb', 'phui-object-box-css' => '7d160002', 'phui-object-item-list-view-css' => 'f3a22696', @@ -830,6 +830,14 @@ return array( '029a133d' => array( 'aphront-dialog-view-css', ), + '037b59eb' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'differential-inline-comment-editor', + ), '048330fa' => array( 'javelin-behavior', 'javelin-typeahead-ondemand-source', @@ -1042,13 +1050,6 @@ return array( 'javelin-install', 'javelin-event', ), - '2d4029a8' => array( - 'javelin-behavior', - 'javelin-util', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-vector', - ), '331b1611' => array( 'javelin-install', ), @@ -1241,6 +1242,13 @@ return array( 'javelin-uri', 'javelin-routable', ), + '5c0f680f' => array( + 'javelin-behavior', + 'javelin-util', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-vector', + ), '5c54cbf3' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1925,14 +1933,6 @@ return array( 'e6e25838' => array( 'javelin-install', ), - 'e723c323' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'differential-inline-comment-editor', - ), 'e9581f08' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/resources/sql/autopatches/20150525.diff.hidden.1.sql b/resources/sql/autopatches/20150525.diff.hidden.1.sql new file mode 100644 index 0000000000..d6b3df6440 --- /dev/null +++ b/resources/sql/autopatches/20150525.diff.hidden.1.sql @@ -0,0 +1,7 @@ +CREATE TABLE {$NAMESPACE}_differential.differential_hiddencomment ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + userPHID VARBINARY(64) NOT NULL, + commentID INT UNSIGNED NOT NULL, + UNIQUE KEY `key_user` (userPHID, commentID), + KEY `key_comment` (commentID) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6a67775a9a..773867f271 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -374,6 +374,7 @@ phutil_register_library_map(array( 'DifferentialGetWorkingCopy' => 'applications/differential/DifferentialGetWorkingCopy.php', 'DifferentialGitHubLandingStrategy' => 'applications/differential/landing/DifferentialGitHubLandingStrategy.php', 'DifferentialGitSVNIDField' => 'applications/differential/customfield/DifferentialGitSVNIDField.php', + 'DifferentialHiddenComment' => 'applications/differential/storage/DifferentialHiddenComment.php', 'DifferentialHostField' => 'applications/differential/customfield/DifferentialHostField.php', 'DifferentialHostedGitLandingStrategy' => 'applications/differential/landing/DifferentialHostedGitLandingStrategy.php', 'DifferentialHostedMercurialLandingStrategy' => 'applications/differential/landing/DifferentialHostedMercurialLandingStrategy.php', @@ -1178,6 +1179,7 @@ phutil_register_library_map(array( 'PHUIDiffInlineCommentUndoView' => 'infrastructure/diff/view/PHUIDiffInlineCommentUndoView.php', 'PHUIDiffInlineCommentView' => 'infrastructure/diff/view/PHUIDiffInlineCommentView.php', 'PHUIDiffOneUpInlineCommentRowScaffold' => 'infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php', + 'PHUIDiffRevealIconView' => 'infrastructure/diff/view/PHUIDiffRevealIconView.php', 'PHUIDiffTwoUpInlineCommentRowScaffold' => 'infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php', 'PHUIDocumentExample' => 'applications/uiexample/examples/PHUIDocumentExample.php', 'PHUIDocumentView' => 'view/phui/PHUIDocumentView.php', @@ -3616,6 +3618,7 @@ phutil_register_library_map(array( 'DifferentialGetRevisionConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialGitHubLandingStrategy' => 'DifferentialLandingStrategy', 'DifferentialGitSVNIDField' => 'DifferentialCustomField', + 'DifferentialHiddenComment' => 'DifferentialDAO', 'DifferentialHostField' => 'DifferentialCustomField', 'DifferentialHostedGitLandingStrategy' => 'DifferentialLandingStrategy', 'DifferentialHostedMercurialLandingStrategy' => 'DifferentialLandingStrategy', @@ -4506,6 +4509,7 @@ phutil_register_library_map(array( 'PHUIDiffInlineCommentUndoView' => 'PHUIDiffInlineCommentView', 'PHUIDiffInlineCommentView' => 'AphrontView', 'PHUIDiffOneUpInlineCommentRowScaffold' => 'PHUIDiffInlineCommentRowScaffold', + 'PHUIDiffRevealIconView' => 'AphrontView', 'PHUIDiffTwoUpInlineCommentRowScaffold' => 'PHUIDiffInlineCommentRowScaffold', 'PHUIDocumentExample' => 'PhabricatorUIExample', 'PHUIDocumentView' => 'AphrontTagView', diff --git a/src/applications/audit/storage/PhabricatorAuditInlineComment.php b/src/applications/audit/storage/PhabricatorAuditInlineComment.php index 292e0274a5..8779b5ffdd 100644 --- a/src/applications/audit/storage/PhabricatorAuditInlineComment.php +++ b/src/applications/audit/storage/PhabricatorAuditInlineComment.php @@ -23,6 +23,14 @@ final class PhabricatorAuditInlineComment return $this->proxy; } + public function supportsHiding() { + return false; + } + + public function isHidden() { + return false; + } + public function getTransactionCommentForSave() { $content_source = PhabricatorContentSource::newForSource( PhabricatorContentSource::SOURCE_LEGACY, diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index 0638578f94..2373d3ef2c 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -191,6 +191,7 @@ final class DifferentialChangesetViewController extends DifferentialController { if ($revision) { $query = id(new DifferentialInlineCommentQuery()) ->setViewer($viewer) + ->needHidden(true) ->withRevisionPHIDs(array($revision->getPHID())); $inlines = $query->execute(); $inlines = $query->adjustInlinesForChangesets( diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 13aac1776c..deba6af01b 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -42,6 +42,7 @@ final class DifferentialInlineCommentEditController ->setViewer($this->getViewer()) ->withIDs(array($id)) ->withDeletedDrafts(true) + ->needHidden(true) ->executeOne(); } @@ -50,6 +51,7 @@ final class DifferentialInlineCommentEditController ->setViewer($this->getViewer()) ->withPHIDs(array($phid)) ->withDeletedDrafts(true) + ->needHidden(true) ->executeOne(); } @@ -152,4 +154,38 @@ final class DifferentialInlineCommentEditController return $this->loadRevision()->getAuthorPHID(); } + protected function hideComments(array $ids) { + $viewer = $this->getViewer(); + $table = new DifferentialHiddenComment(); + $conn_w = $table->establishConnection('w'); + + $sql = array(); + foreach ($ids as $id) { + $sql[] = qsprintf( + $conn_w, + '(%s, %d)', + $viewer->getPHID(), + $id); + } + + queryfx( + $conn_w, + 'INSERT IGNORE INTO %T (userPHID, commentID) VALUES %Q', + $table->getTableName(), + implode(', ', $sql)); + } + + protected function showComments(array $ids) { + $viewer = $this->getViewer(); + $table = new DifferentialHiddenComment(); + $conn_w = $table->establishConnection('w'); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE userPHID = %s AND commentID IN (%Ld)', + $table->getTableName(), + $viewer->getPHID(), + $ids); + } + } diff --git a/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php b/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php index 846cab5439..519b08bb63 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentPreviewController.php @@ -19,6 +19,7 @@ extends PhabricatorInlineCommentPreviewController { ->withDrafts(true) ->withAuthorPHIDs(array($viewer->getPHID())) ->withRevisionPHIDs(array($revision->getPHID())) + ->needHidden(true) ->execute(); } diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 312d4a7f4e..7069e5ae97 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -175,6 +175,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $query = id(new DifferentialInlineCommentQuery()) ->setViewer($user) + ->needHidden(true) ->withRevisionPHIDs(array($revision->getPHID())); $inlines = $query->execute(); $inlines = $query->adjustInlinesForChangesets( diff --git a/src/applications/differential/query/DifferentialInlineCommentQuery.php b/src/applications/differential/query/DifferentialInlineCommentQuery.php index 3fb26739c0..7e986bdc60 100644 --- a/src/applications/differential/query/DifferentialInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php @@ -15,6 +15,7 @@ final class DifferentialInlineCommentQuery private $authorPHIDs; private $revisionPHIDs; private $deletedDrafts; + private $needHidden; public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; @@ -55,6 +56,11 @@ final class DifferentialInlineCommentQuery return $this; } + public function needHidden($need) { + $this->needHidden = $need; + return $this; + } + public function execute() { $table = new DifferentialTransactionComment(); $conn_r = $table->establishConnection('r'); @@ -68,6 +74,26 @@ final class DifferentialInlineCommentQuery $comments = $table->loadAllFromArray($data); + if ($this->needHidden) { + $viewer_phid = $this->getViewer()->getPHID(); + if ($viewer_phid && $comments) { + $hidden = queryfx_all( + $conn_r, + 'SELECT commentID FROM %T WHERE userPHID = %s + AND commentID IN (%Ls)', + id(new DifferentialHiddenComment())->getTableName(), + $viewer_phid, + mpull($comments, 'getID')); + $hidden = array_fuse(ipull($hidden, 'commentID')); + } else { + $hidden = array(); + } + + foreach ($comments as $inline) { + $inline->attachIsHidden(isset($hidden[$inline->getID()])); + } + } + foreach ($comments as $key => $value) { $comments[$key] = DifferentialInlineComment::newFromModernComment( $value); diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index 2634d037d7..dfc990d4d4 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -41,8 +41,10 @@ final class DifferentialChangesetOneUpRenderer $column_width = 4; + $hidden = new PHUIDiffRevealIconView(); + $out = array(); - foreach ($primitives as $p) { + foreach ($primitives as $k => $p) { $type = $p['type']; switch ($type) { case 'old': @@ -51,6 +53,27 @@ final class DifferentialChangesetOneUpRenderer case 'new-file': $is_old = ($type == 'old' || $type == 'old-file'); + $o_hidden = array(); + $n_hidden = array(); + + for ($look = $k + 1; isset($primitives[$look]); $look++) { + $next = $primitives[$look]; + switch ($next['type']) { + case 'inline': + $comment = $next['comment']; + if ($comment->isHidden()) { + if ($next['right']) { + $n_hidden[] = $comment; + } else { + $o_hidden[] = $comment; + } + } + break; + default: + break 2; + } + } + $cells = array(); if ($is_old) { if ($p['htype']) { @@ -68,7 +91,13 @@ final class DifferentialChangesetOneUpRenderer } else { $left_id = null; } - $cells[] = phutil_tag('th', array('id' => $left_id), $p['line']); + + $line = $p['line']; + if ($o_hidden) { + $line = array($hidden, $line); + } + + $cells[] = phutil_tag('th', array('id' => $left_id), $line); $cells[] = phutil_tag('th', array()); $cells[] = $no_copy; @@ -85,7 +114,13 @@ final class DifferentialChangesetOneUpRenderer } else { $left_id = null; } - $cells[] = phutil_tag('th', array('id' => $left_id), $p['oline']); + + $oline = $p['oline']; + if ($o_hidden) { + $oline = array($hidden, $oline); + } + + $cells[] = phutil_tag('th', array('id' => $left_id), $oline); } if ($type == 'new-file') { @@ -97,8 +132,13 @@ final class DifferentialChangesetOneUpRenderer } else { $right_id = null; } - $cells[] = phutil_tag('th', array('id' => $right_id), $p['line']); + $line = $p['line']; + if ($n_hidden) { + $line = array($hidden, $line); + } + + $cells[] = phutil_tag('th', array('id' => $right_id), $line); $cells[] = $no_copy; $cells[] = phutil_tag('td', array('class' => $class), $p['render']); diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index 181e3d1306..16752b9402 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -69,6 +69,8 @@ final class DifferentialChangesetTwoUpRenderer $depths = $this->getDepths(); $mask = $this->getMask(); + $hidden = new PHUIDiffRevealIconView(); + for ($ii = $range_start; $ii < $range_start + $range_len; $ii++) { if (empty($mask[$ii])) { // If we aren't going to show this line, we've just entered a gap. @@ -235,6 +237,69 @@ final class DifferentialChangesetTwoUpRenderer $n_id = null; } + $old_comments = $this->getOldComments(); + $new_comments = $this->getNewComments(); + $scaffolds = array(); + + $o_hidden = array(); + $n_hidden = array(); + + if ($o_num && isset($old_comments[$o_num])) { + foreach ($old_comments[$o_num] as $comment) { + $inline = $this->buildInlineComment( + $comment, + $on_right = false); + $scaffold = $this->getRowScaffoldForInline($inline); + + if ($comment->isHidden()) { + $o_hidden[] = $comment; + } + + if ($n_num && isset($new_comments[$n_num])) { + foreach ($new_comments[$n_num] as $key => $new_comment) { + if ($comment->isCompatible($new_comment)) { + $companion = $this->buildInlineComment( + $new_comment, + $on_right = true); + + if ($new_comment->isHidden()) { + $n_hidden = $new_comment; + } + + $scaffold->addInlineView($companion); + unset($new_comments[$n_num][$key]); + break; + } + } + } + + + $scaffolds[] = $scaffold; + } + } + + if ($n_num && isset($new_comments[$n_num])) { + foreach ($new_comments[$n_num] as $comment) { + $inline = $this->buildInlineComment( + $comment, + $on_right = true); + + if ($comment->isHidden()) { + $n_hidden[] = $comment; + } + + $scaffolds[] = $this->getRowScaffoldForInline($inline); + } + } + + if ($o_hidden) { + $o_num = array($hidden, $o_num); + } + + if ($n_hidden) { + $n_num = array($hidden, $n_num); + } + // NOTE: This is a unicode zero-width space, which we use as a hint when // intercepting 'copy' events to make sure sensible text ends up on the // clipboard. See the 'phabricator-oncopy' behavior. @@ -259,40 +324,8 @@ final class DifferentialChangesetTwoUpRenderer $html[] = $context_not_available; } - $old_comments = $this->getOldComments(); - $new_comments = $this->getNewComments(); - - if ($o_num && isset($old_comments[$o_num])) { - foreach ($old_comments[$o_num] as $comment) { - $inline = $this->buildInlineComment( - $comment, - $on_right = false); - $scaffold = $this->getRowScaffoldForInline($inline); - - if ($n_num && isset($new_comments[$n_num])) { - foreach ($new_comments[$n_num] as $key => $new_comment) { - if ($comment->isCompatible($new_comment)) { - $companion = $this->buildInlineComment( - $new_comment, - $on_right = true); - - $scaffold->addInlineView($companion); - unset($new_comments[$n_num][$key]); - break; - } - } - } - - $html[] = $scaffold; - } - } - if ($n_num && isset($new_comments[$n_num])) { - foreach ($new_comments[$n_num] as $comment) { - $inline = $this->buildInlineComment( - $comment, - $on_right = true); - $html[] = $this->getRowScaffoldForInline($inline); - } + foreach ($scaffolds as $scaffold) { + $html[] = $scaffold; } } diff --git a/src/applications/differential/storage/DifferentialHiddenComment.php b/src/applications/differential/storage/DifferentialHiddenComment.php new file mode 100644 index 0000000000..90dbb527f2 --- /dev/null +++ b/src/applications/differential/storage/DifferentialHiddenComment.php @@ -0,0 +1,24 @@ + false, + self::CONFIG_KEY_SCHEMA => array( + 'key_user' => array( + 'columns' => array('userPHID', 'commentID'), + 'unique' => true, + ), + 'key_comment' => array( + 'columns' => array('commentID'), + ), + ), + ) + parent::getConfiguration(); + } + +} diff --git a/src/applications/differential/storage/DifferentialInlineComment.php b/src/applications/differential/storage/DifferentialInlineComment.php index 65ea64773b..979220fc02 100644 --- a/src/applications/differential/storage/DifferentialInlineComment.php +++ b/src/applications/differential/storage/DifferentialInlineComment.php @@ -24,6 +24,7 @@ final class DifferentialInlineComment ->setViewPolicy('public') ->setEditPolicy($this->getAuthorPHID()) ->setContentSource($content_source) + ->attachIsHidden(false) ->setCommentVersion(1); return $this->proxy; @@ -49,6 +50,14 @@ final class DifferentialInlineComment return $this; } + public function supportsHiding() { + return true; + } + + public function isHidden() { + return $this->proxy->getIsHidden(); + } + public function getID() { return $this->proxy->getID(); } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 7630814882..0a4d459568 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -522,6 +522,7 @@ final class DifferentialRevision extends DifferentialDAO } $query = id(new DifferentialInlineCommentQuery()) + ->needHidden(true) ->setViewer($viewer); // NOTE: This is a bit sketchy: this method adjusts the inlines as a diff --git a/src/applications/differential/storage/DifferentialTransactionComment.php b/src/applications/differential/storage/DifferentialTransactionComment.php index 0d1e6ea33a..5a9b031dad 100644 --- a/src/applications/differential/storage/DifferentialTransactionComment.php +++ b/src/applications/differential/storage/DifferentialTransactionComment.php @@ -13,6 +13,7 @@ final class DifferentialTransactionComment protected $replyToCommentPHID; private $replyToComment = self::ATTACHABLE; + private $isHidden = self::ATTACHABLE; public function getApplicationTransactionObject() { return new DifferentialTransaction(); @@ -99,4 +100,13 @@ final class DifferentialTransactionComment return $inline_groups; } + public function getIsHidden() { + return $this->assertAttached($this->isHidden); + } + + public function attachIsHidden($hidden) { + $this->isHidden = $hidden; + return $this; + } + } diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 3e7bfb677c..0dd40f39c3 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -232,8 +232,9 @@ final class DifferentialChangesetListView extends AphrontView { if ($this->inlineURI) { Javelin::initBehavior('differential-edit-inline-comments', array( - 'uri' => $this->inlineURI, - 'stage' => 'differential-review-stage', + 'uri' => $this->inlineURI, + 'stage' => 'differential-review-stage', + 'revealIcon' => hsprintf('%s', new PHUIDiffRevealIconView()), )); } diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 4608ebeffa..343319b3d8 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -15,6 +15,14 @@ abstract class PhabricatorInlineCommentController abstract protected function saveComment( PhabricatorInlineCommentInterface $inline); + protected function hideComments(array $ids) { + throw new PhutilMethodNotImplementedException(); + } + + protected function showComments(array $ids) { + throw new PhutilMethodNotImplementedException(); + } + private $changesetID; private $isNewFile; private $isOnRight; @@ -84,6 +92,22 @@ abstract class PhabricatorInlineCommentController $op = $this->getOperation(); switch ($op) { + case 'hide': + case 'show': + if (!$request->validateCSRF()) { + return new Aphront404Response(); + } + + $ids = $request->getStrList('ids'); + if ($ids) { + if ($op == 'hide') { + $this->hideComments($ids); + } else { + $this->showComments($ids); + } + } + + return id(new AphrontAjaxResponse())->setContent(array()); case 'done': if (!$request->validateCSRF()) { return new Aphront404Response(); diff --git a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php index 8ba648f2b6..13bf3ad83b 100644 --- a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php +++ b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php @@ -57,4 +57,7 @@ interface PhabricatorInlineCommentInterface extends PhabricatorMarkupInterface { public function setIsGhost($is_ghost); public function getIsGhost(); + public function supportsHiding(); + public function isHidden(); + } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index e265cafad1..de17b5dd2b 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -18,6 +18,10 @@ final class PHUIDiffInlineCommentDetailView return $this; } + public function isHidden() { + return $this->inlineComment->isHidden(); + } + public function setHandles(array $handles) { assert_instances_of($handles, 'PhabricatorObjectHandle'); $this->handles = $handles; @@ -192,6 +196,8 @@ final class PHUIDiffInlineCommentDetailView if (!$this->preview) { $nextprev = new PHUIButtonBarView(); $nextprev->addClass('mml'); + + $up = id(new PHUIButtonView()) ->setTag('a') ->setColor(PHUIButtonView::SIMPLE) @@ -208,6 +214,18 @@ final class PHUIDiffInlineCommentDetailView ->addSigil('differential-inline-next') ->setMustCapture(true); + $hide = id(new PHUIButtonView()) + ->setTag('a') + ->setColor(PHUIButtonView::SIMPLE) + ->setTooltip(pht('Hide Comment')) + ->setIconFont('fa-times') + ->addSigil('hide-inline') + ->setMustCapture(true); + + if ($viewer_phid && $inline->getID() && $inline->supportsHiding()) { + $nextprev->addButton($hide); + } + $nextprev->addButton($up); $nextprev->addButton($down); diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php index 194ff6f5cc..b0c0b4ccac 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php @@ -23,8 +23,18 @@ abstract class PHUIDiffInlineCommentRowScaffold extends AphrontView { protected function getRowAttributes() { // TODO: This is semantic information used by the JS when placing comments // and using keyboard navigation; we should move it out of class names. + + $style = null; + foreach ($this->getInlineViews() as $view) { + if ($view->isHidden()) { + $style = 'display: none'; + } + } + return array( 'class' => 'inline', + 'sigil' => 'inline-row', + 'style' => $style, ); } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php index f2f82471d6..b62160e232 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php @@ -17,4 +17,8 @@ abstract class PHUIDiffInlineCommentView extends AphrontView { return null; } + public function isHidden() { + return false; + } + } diff --git a/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php index 6b8cb0a32e..708b70b360 100644 --- a/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php +++ b/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php @@ -28,7 +28,7 @@ final class PHUIDiffOneUpInlineCommentRowScaffold phutil_tag('td', $attrs, $inline), ); - return phutil_tag('tr', $this->getRowAttributes(), $cells); + return javelin_tag('tr', $this->getRowAttributes(), $cells); } } diff --git a/src/infrastructure/diff/view/PHUIDiffRevealIconView.php b/src/infrastructure/diff/view/PHUIDiffRevealIconView.php new file mode 100644 index 0000000000..b2c879a8b5 --- /dev/null +++ b/src/infrastructure/diff/view/PHUIDiffRevealIconView.php @@ -0,0 +1,27 @@ +setIconFont('fa-comment') + ->addSigil('has-tooltip') + ->setMetadata( + array( + 'tip' => pht('Show Hidden Comments'), + 'align' => 'E', + 'size' => 275, + )); + + return javelin_tag( + 'a', + array( + 'href' => '#', + 'class' => 'reveal-inlines', + 'sigil' => 'reveal-inlines', + 'mustcapture' => true, + ), + $icon); + } + +} diff --git a/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php index e2c2183856..4fac5088d1 100644 --- a/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php +++ b/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php @@ -68,7 +68,7 @@ final class PHUIDiffTwoUpInlineCommentRowScaffold phutil_tag('td', $right_attrs, $right_side), ); - return phutil_tag('tr', $this->getRowAttributes(), $cells); + return javelin_tag('tr', $this->getRowAttributes(), $cells); } } diff --git a/webroot/rsrc/css/application/differential/phui-inline-comment.css b/webroot/rsrc/css/application/differential/phui-inline-comment.css index 5b6e2d79ee..632635ba3a 100644 --- a/webroot/rsrc/css/application/differential/phui-inline-comment.css +++ b/webroot/rsrc/css/application/differential/phui-inline-comment.css @@ -434,3 +434,21 @@ border-color: {$lightgreyborder}; color: {$lightgreytext}; } + + +/* - Hiding Inlines ------------------------------------------------------------ +*/ + +.reveal-inlines { + float: left; + margin-left: 4px; + color: {$lightbluetext}; +} + +.reveal-inlines span.phui-icon-view { + color: {$lightbluetext}; +} + +.reveal-inlines:hover span.phui-icon-view { + color: {$darkbluetext}; +} diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js index 0a572a999a..821d279f50 100644 --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js @@ -395,4 +395,87 @@ JX.behavior('differential-edit-inline-comments', function(config) { handle_inline_action(data.node, data.op); }); + // Respond to the user clicking the "Hide Inline" button on an inline + // comment. + JX.Stratcom.listen('click', 'hide-inline', function(e) { + e.kill(); + + var row = e.getNode('inline-row'); + JX.DOM.hide(row); + + var prev = row.previousSibling; + while (prev && JX.Stratcom.hasSigil(prev, 'inline-row')) { + prev = prev.previousSibling; + } + + if (!prev) { + return; + } + + var comment = e.getNodeData('differential-inline-comment'); + + var slots = []; + for (var ii = 0; ii < prev.childNodes.length; ii++) { + if (JX.DOM.isType(prev.childNodes[ii], 'th')) { + slots.push(prev.childNodes[ii]); + } + } + + // Select the right-hand side if the comment is on the right. + var slot = (comment.on_right && slots[1]) || slots[0]; + + var reveal = JX.DOM.scry(slot, 'a', 'reveal-inlines')[0]; + if (!reveal) { + reveal = JX.$N( + 'a', + { + className: 'reveal-inlines', + sigil: 'reveal-inlines' + }, + JX.$H(config.revealIcon)); + + JX.DOM.prependContent(slot, reveal); + } + + new JX.Workflow(config.uri, {op: 'hide', ids: comment.id}) + .setHandler(JX.bag) + .start(); + }); + + JX.Stratcom.listen('click', 'reveal-inlines', function(e) { + e.kill(); + + var row = e.getNode('tag:tr'); + var next = row.nextSibling; + + var ids = []; + var ii; + + // Show any hidden inline comment rows directly below this one. + while (next && JX.Stratcom.hasSigil(next, 'inline-row')) { + JX.DOM.show(next); + + var comments = JX.DOM.scry(next, 'div', 'differential-inline-comment'); + for (ii = 0; ii < comments.length; ii++) { + var id = JX.Stratcom.getData(comments[ii]).id; + if (id) { + ids.push(id); + } + } + + next = next.nextSibling; + } + + // Remove any "reveal" icons on the row. + var reveals = JX.DOM.scry(row, 'a', 'reveal-inlines'); + for (ii = 0; ii < reveals.length; ii++) { + JX.DOM.remove(reveals[ii]); + } + + new JX.Workflow(config.uri, {op: 'show', ids: ids.join(',')}) + .setHandler(JX.bag) + .start(); + }); + + });