From b2d280ff51422212c08c39c81537bbbaf8dde49f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 20 Apr 2015 15:06:20 -0700 Subject: [PATCH] Port comments through time and space in the common/best case Summary: Ref T7447. This ports comments forward and backward in the best case: - The old comment is on a changeset with the same filename. - The old and new files are pretty much the same, line-for-line. This will fail to port a lot of comments around and probably port a lot of comments into goofy places. We can see how bad it is in practice. Errata: - Design is me cobbling something together, probably worth tweaking. - "Old Comment" should, at a minimum, say "Newer Comment" sometimes, or we should come up with some better name for this stuff. Test Plan: {F377214} Reviewers: chad, btrahan Reviewed By: btrahan Subscribers: johnny-bit, yelirekim, epriestley Maniphest Tasks: T7447 Differential Revision: https://secure.phabricator.com/D12484 --- .../storage/PhabricatorAuditInlineComment.php | 10 ++ .../DifferentialChangesetViewController.php | 129 ++++++++++++++---- .../query/DifferentialInlineCommentQuery.php | 13 -- .../storage/DifferentialInlineComment.php | 15 ++ .../PhabricatorInlineCommentInterface.php | 3 + .../view/PHUIDiffInlineCommentDetailView.php | 14 ++ .../differential/phui-inline-comment.css | 28 +++- 7 files changed, 169 insertions(+), 43 deletions(-) diff --git a/src/applications/audit/storage/PhabricatorAuditInlineComment.php b/src/applications/audit/storage/PhabricatorAuditInlineComment.php index 1e1b47f73e..8c0aa78d5d 100644 --- a/src/applications/audit/storage/PhabricatorAuditInlineComment.php +++ b/src/applications/audit/storage/PhabricatorAuditInlineComment.php @@ -5,6 +5,7 @@ final class PhabricatorAuditInlineComment private $proxy; private $syntheticAuthor; + private $isGhost; public function __construct() { $this->proxy = new PhabricatorAuditTransactionComment(); @@ -308,6 +309,15 @@ final class PhabricatorAuditInlineComment return $this->proxy->getFixedState(); } + public function setIsGhost($is_ghost) { + $this->isGhost = $is_ghost; + return $this; + } + + public function getIsGhost() { + return $this->isGhost; + } + /* -( PhabricatorMarkupInterface Implementation )-------------------------- */ diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index b701bb05b1..6f8f3fe7a9 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -9,8 +9,6 @@ final class DifferentialChangesetViewController extends DifferentialController { public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); - $author_phid = $viewer->getPHID(); - $rendering_reference = $request->getStr('ref'); $parts = explode('/', $rendering_reference); if (count($parts) == 2) { @@ -161,10 +159,33 @@ final class DifferentialChangesetViewController extends DifferentialController { $parser->setOriginals($left, $right); } + $diff = $changeset->getDiff(); + $revision_id = $diff->getRevisionID(); + + $can_mark = false; + $object_owner_phid = null; + if ($revision_id) { + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withIDs(array($revision_id)) + ->executeOne(); + if ($revision) { + $can_mark = ($revision->getAuthorPHID() == $viewer->getPHID()); + $object_owner_phid = $revision->getAuthorPHID(); + } + } + // Load both left-side and right-side inline comments. - $inlines = $this->loadInlineComments( - array($left_source, $right_source), - $author_phid); + if ($revision) { + $inlines = $this->loadInlineComments( + $revision, + nonempty($left, $right), + $left_new, + $right, + $right_new); + } else { + $inlines = array(); + } if ($left_new) { $inlines = array_merge( @@ -201,22 +222,6 @@ final class DifferentialChangesetViewController extends DifferentialController { $engine->process(); - $diff = $changeset->getDiff(); - $revision_id = $diff->getRevisionID(); - - $can_mark = false; - $object_owner_phid = null; - if ($revision_id) { - $revision = id(new DifferentialRevisionQuery()) - ->setViewer($viewer) - ->withIDs(array($revision_id)) - ->executeOne(); - if ($revision) { - $can_mark = ($revision->getAuthorPHID() == $viewer->getPHID()); - $object_owner_phid = $revision->getAuthorPHID(); - } - } - $parser ->setUser($viewer) ->setMarkupEngine($engine) @@ -280,16 +285,82 @@ final class DifferentialChangesetViewController extends DifferentialController { )); } - private function loadInlineComments(array $changeset_ids, $author_phid) { - $changeset_ids = array_unique(array_filter($changeset_ids)); - if (!$changeset_ids) { - return; + private function loadInlineComments( + DifferentialRevision $revision, + DifferentialChangeset $left, + $left_new, + DifferentialChangeset $right, + $right_new) { + + $viewer = $this->getViewer(); + $all = array($left, $right); + + $inlines = id(new DifferentialInlineCommentQuery()) + ->setViewer($viewer) + ->withRevisionPHIDs(array($revision->getPHID())) + ->execute(); + + $changeset_ids = mpull($inlines, 'getChangesetID'); + $changeset_ids = array_unique($changeset_ids); + if ($changeset_ids) { + $changesets = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withIDs($changeset_ids) + ->execute(); + $changesets = mpull($changesets, null, 'getID'); + } else { + $changesets = array(); + } + $changesets += mpull($all, null, 'getID'); + + $id_map = array( + $left->getID() => $left->getID(), + $right->getID() => $right->getID(), + ); + + $name_map = array( + $left->getFilename() => $left->getID(), + $right->getFilename() => $right->getID(), + ); + + $results = array(); + foreach ($inlines as $inline) { + $changeset_id = $inline->getChangesetID(); + if (isset($id_map[$changeset_id])) { + // This inline is legitimately on one of the current changesets, so + // we can include it in the result set unmodified. + $results[] = $inline; + continue; + } + + $changeset = idx($changesets, $changeset_id); + if (!$changeset) { + // Just discard this inline with bogus data. + continue; + } + + $target_id = null; + + $filename = $changeset->getFilename(); + if (isset($name_map[$filename])) { + // This changeset is on a file with the same name as the current + // changeset, so we're going to port it forward or backward. + $target_id = $name_map[$filename]; + } + + // If we found a changeset to port this comment to, bring it forward + // or backward and mark it. + if ($target_id) { + $inline + ->makeEphemeral(true) + ->setChangesetID($target_id) + ->setIsGhost(true); + + $results[] = $inline; + } } - return id(new DifferentialInlineCommentQuery()) - ->setViewer($this->getViewer()) - ->withChangesetIDs($changeset_ids) - ->execute(); + return $results; } private function buildRawFileResponse( diff --git a/src/applications/differential/query/DifferentialInlineCommentQuery.php b/src/applications/differential/query/DifferentialInlineCommentQuery.php index dcc85b1e92..0a1a22c986 100644 --- a/src/applications/differential/query/DifferentialInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php @@ -13,7 +13,6 @@ final class DifferentialInlineCommentQuery private $phids; private $drafts; private $authorPHIDs; - private $changesetIDs; private $revisionPHIDs; private $deletedDrafts; @@ -46,11 +45,6 @@ final class DifferentialInlineCommentQuery return $this; } - public function withChangesetIDs(array $ids) { - $this->changesetIDs = $ids; - return $this; - } - public function withRevisionPHIDs(array $revision_phids) { $this->revisionPHIDs = $revision_phids; return $this; @@ -116,13 +110,6 @@ final class DifferentialInlineCommentQuery $this->revisionPHIDs); } - if ($this->changesetIDs !== null) { - $where[] = qsprintf( - $conn_r, - 'changesetID IN (%Ld)', - $this->changesetIDs); - } - if ($this->drafts === null) { if ($this->deletedDrafts) { $where[] = qsprintf( diff --git a/src/applications/differential/storage/DifferentialInlineComment.php b/src/applications/differential/storage/DifferentialInlineComment.php index 52ab4a2944..65ea64773b 100644 --- a/src/applications/differential/storage/DifferentialInlineComment.php +++ b/src/applications/differential/storage/DifferentialInlineComment.php @@ -5,6 +5,7 @@ final class DifferentialInlineComment private $proxy; private $syntheticAuthor; + private $isGhost; public function __construct() { $this->proxy = new DifferentialTransactionComment(); @@ -225,6 +226,20 @@ final class DifferentialInlineComment return $this->proxy->getFixedState(); } + public function setIsGhost($is_ghost) { + $this->isGhost = $is_ghost; + return $this; + } + + public function getIsGhost() { + return $this->isGhost; + } + + public function makeEphemeral() { + $this->proxy->makeEphemeral(); + return $this; + } + /* -( PhabricatorMarkupInterface Implementation )-------------------------- */ diff --git a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php index d123f82c51..8ba648f2b6 100644 --- a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php +++ b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php @@ -54,4 +54,7 @@ interface PhabricatorInlineCommentInterface extends PhabricatorMarkupInterface { public function save(); public function delete(); + public function setIsGhost($is_ghost); + public function getIsGhost(); + } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index 6bb57b9779..e0b147d68d 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -122,6 +122,19 @@ final class PHUIDiffInlineCommentDetailView ->addClass('mml inline-draft-text'); } + $ghost_tag = null; + if ($inline->getIsGhost()) { + // TODO: This could also be a newer comment, not necessarily an older + // comment. + $ghost_tag = id(new PHUITagView()) + ->setType(PHUITagView::TYPE_SHADE) + ->setName(pht('Old Comment')) + ->setSlimShady(true) + ->setShade(PHUITagView::COLOR_BLUE) + ->addClass('mml'); + $classes[] = 'inline-comment-ghost'; + } + // I think this is unused if ($inline->getHasReplies()) { $classes[] = 'inline-comment-has-reply'; @@ -351,6 +364,7 @@ final class PHUIDiffInlineCommentDetailView $author, $author_owner, $draft_text, + $ghost_tag, )); $group_right = phutil_tag( diff --git a/webroot/rsrc/css/application/differential/phui-inline-comment.css b/webroot/rsrc/css/application/differential/phui-inline-comment.css index 7666c1dad4..4eafdc83f2 100644 --- a/webroot/rsrc/css/application/differential/phui-inline-comment.css +++ b/webroot/rsrc/css/application/differential/phui-inline-comment.css @@ -132,6 +132,32 @@ padding-bottom: 4px; } +/* - Ghost Comment ------------------------------------------------------------ + + Comments from older or newer versions of the changeset. + +*/ + +.differential-inline-comment.inline-comment-ghost { + border: 1px solid {$lightgreyborder}; + opacity: 0.75; +} + +.differential-inline-comment.inline-comment-ghost + .differential-inline-comment-head { + border-bottom: 1px solid {$lightgreyborder}; + background-color: {$lightgreybackground}; +} + +.differential-inline-comment.inline-comment-ghost + .button.simple { + border-color: {$lightgreyborder}; +} + +.differential-inline-comment.inline-comment-ghost + .button.simple .phui-icon-view { + color: {$lightgreytext}; +} /* - New/Edit Inline Comment -------------------------------------------------- @@ -350,7 +376,7 @@ } .differential-inline-comment.inline-state-is-draft .inline-draft-text { - display: inline-block; + display: inline-block; } .inline-state-is-draft .differential-inline-done-label {