diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index af9eeea505..fc0536902c 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -65,6 +65,8 @@ final class DifferentialChangesetViewController extends DifferentialController { } } + $old = array(); + $new = array(); if (!$vs) { $right = $changeset; $left = null; @@ -75,6 +77,9 @@ final class DifferentialChangesetViewController extends DifferentialController { $left_new = false; $render_cache_key = $right->getID(); + + $old[] = $changeset; + $new[] = $changeset; } else if ($vs == -1) { $right = null; $left = $changeset; @@ -85,6 +90,9 @@ final class DifferentialChangesetViewController extends DifferentialController { $left_new = true; $render_cache_key = null; + + $old[] = $changeset; + $new[] = $changeset; } else { $right = $changeset; $left = $vs_changeset; @@ -95,6 +103,9 @@ final class DifferentialChangesetViewController extends DifferentialController { $left_new = true; $render_cache_key = null; + + $new[] = $left; + $new[] = $right; } if ($left) { @@ -177,12 +188,11 @@ final class DifferentialChangesetViewController extends DifferentialController { // Load both left-side and right-side inline comments. if ($revision) { - $inlines = $this->loadInlineComments( - $revision, - array(nonempty($left, $right)), - $left_new, - array($right), - $right_new); + $query = id(new DifferentialInlineCommentQuery()) + ->setViewer($viewer) + ->withRevisionPHIDs(array($revision->getPHID())); + $inlines = $query->execute(); + $inlines = $query->adjustInlinesForChangesets($inlines, $old, $new); } else { $inlines = array(); } @@ -285,199 +295,6 @@ final class DifferentialChangesetViewController extends DifferentialController { )); } - private function loadInlineComments( - DifferentialRevision $revision, - array $left, - $left_new, - array $right, - $right_new) { - - assert_instances_of($left, 'DifferentialChangeset'); - assert_instances_of($right, 'DifferentialChangeset'); - - $viewer = $this->getViewer(); - $all = array_merge($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(); - foreach ($all as $changeset) { - $id_map[$changeset->getID()] = $changeset->getID(); - } - - // Generate filename maps for older and newer comments. If we're bringing - // an older comment forward in a diff-of-diffs, we want to put it on the - // left side of the screen, not the right side. Both sides are "new" files - // with the same name, so they're both appropriate targets, but the left - // is a better target conceptually for users because it's more consistent - // with the rest of the UI, which shows old information on the left and - // new information on the right. - $move_here = DifferentialChangeType::TYPE_MOVE_HERE; - - $name_map_old = array(); - $name_map_new = array(); - $move_map = array(); - foreach ($all as $changeset) { - $changeset_id = $changeset->getID(); - - $filenames = array(); - $filenames[] = $changeset->getFilename(); - - // If this is the target of a move, also map comments on the old filename - // to this changeset. - if ($changeset->getChangeType() == $move_here) { - $old_file = $changeset->getOldFile(); - $filenames[] = $old_file; - $move_map[$changeset_id][$old_file] = true; - } - - foreach ($filenames as $filename) { - // We update the old map only if we don't already have an entry (oldest - // changeset persists). - if (empty($name_map_old[$filename])) { - $name_map_old[$filename] = $changeset_id; - } - - // We always update the new map (newest changeset overwrites). - $name_map_new[$changeset->getFilename()] = $changeset_id; - } - } - - // Find the smallest "new" changeset ID. We'll consider everything - // larger than this to be "newer", and everything smaller to be "older". - $first_new_id = min(mpull($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, as it has bogus data. - continue; - } - - $target_id = null; - - if ($changeset_id >= $first_new_id) { - $name_map = $name_map_new; - $is_new = true; - } else { - $name_map = $name_map_old; - $is_new = false; - } - - $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]; - - $is_move = isset($move_map[$target_id][$filename]); - if ($is_new) { - if ($is_move) { - $reason = pht( - 'This comment was made on a file with the same name as the '. - 'file this file was moved from, but in a newer diff.'); - } else { - $reason = pht( - 'This comment was made on a file with the same name, but '. - 'in a newer diff.'); - } - } else { - if ($is_move) { - $reason = pht( - 'This comment was made on a file with the same name as the '. - 'file this file was moved from, but in an older diff.'); - } else { - $reason = pht( - 'This comment was made on a file with the same name, but '. - 'in an older diff.'); - } - } - } - - - // If we didn't find a target and this change is the target of a move, - // look for a match against the old filename. - if (!$target_id) { - if ($changeset->getChangeType() == $move_here) { - $filename = $changeset->getOldFile(); - if (isset($name_map[$filename])) { - $target_id = $name_map[$filename]; - if ($is_new) { - $reason = pht( - 'This comment was made on a file which this file was moved '. - 'to, but in a newer diff.'); - } else { - $reason = pht( - 'This comment was made on a file which this file was moved '. - 'to, but in an older diff.'); - } - } - } - } - - - // 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( - array( - 'new' => $is_new, - 'reason' => $reason, - )); - - $results[] = $inline; - } - } - - // Filter out the inlines we ported forward which won't be visible because - // they appear on the wrong side of a file. - $keep_map = array(); - foreach ($left as $changeset) { - $keep_map[$changeset->getID()][(int)$left_new] = true; - } - foreach ($right as $changeset) { - $keep_map[$changeset->getID()][(int)$right_new] = true; - } - foreach ($results as $key => $inline) { - $is_new = (int)$inline->getIsNewFile(); - $changeset_id = $inline->getChangesetID(); - if (!isset($keep_map[$changeset_id][$is_new])) { - unset($results[$key]); - continue; - } - } - - return $results; - } - private function buildRawFileResponse( DifferentialChangeset $changeset, $is_new) { diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 6905d03cea..215a455ef7 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -95,9 +95,6 @@ final class DifferentialRevisionViewController extends DifferentialController { $target_manual->getID()); $props = mpull($props, 'getData', 'getName'); - $all_changesets = $changesets; - $inlines = $revision->loadInlineComments($all_changesets, $user); - $object_phids = array_merge( $revision->getReviewers(), $revision->getCCPHIDs(), @@ -157,15 +154,33 @@ final class DifferentialRevisionViewController extends DifferentialController { pht('Show All Files Inline')))); $warning = $warning->render(); - $my_inlines = id(new DifferentialInlineCommentQuery()) + $map = $vs_map; + if (!$map) { + $map = array_fill_keys(array_keys($changesets), 0); + } + + $old = array(); + $new = array(); + foreach ($map as $id => $vs) { + if ($vs <= 0) { + $old[] = $id; + $new[] = $id; + } else { + $new[] = $id; + $new[] = $vs; + } + } + $old = array_select_keys($changesets, $old); + $new = array_select_keys($changesets, $new); + + $query = id(new DifferentialInlineCommentQuery()) ->setViewer($user) - ->withDrafts(true) - ->withAuthorPHIDs(array($user->getPHID())) - ->withRevisionPHIDs(array($revision->getPHID())) - ->execute(); + ->withRevisionPHIDs(array($revision->getPHID())); + $inlines = $query->execute(); + $inlines = $query->adjustInlinesForChangesets($inlines, $old, $new); $visible_changesets = array(); - foreach ($inlines + $my_inlines as $inline) { + foreach ($inlines as $inline) { $changeset_id = $inline->getChangesetID(); if (isset($changesets[$changeset_id])) { $visible_changesets[$changeset_id] = $changesets[$changeset_id]; @@ -270,8 +285,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $comment_view = $this->buildTransactions( $revision, $diff_vs ? $diffs[$diff_vs] : $target, - $target, - $all_changesets); + $target); if (!$viewer_is_anonymous) { $comment_view->setQuoteRef('D'.$revision->getID()); @@ -917,8 +931,7 @@ final class DifferentialRevisionViewController extends DifferentialController { private function buildTransactions( DifferentialRevision $revision, DifferentialDiff $left_diff, - DifferentialDiff $right_diff, - array $changesets) { + DifferentialDiff $right_diff) { $timeline = $this->buildTransactionTimeline( $revision, diff --git a/src/applications/differential/query/DifferentialInlineCommentQuery.php b/src/applications/differential/query/DifferentialInlineCommentQuery.php index 0a1a22c986..7ef6f0a1ca 100644 --- a/src/applications/differential/query/DifferentialInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php @@ -137,4 +137,192 @@ final class DifferentialInlineCommentQuery return $this->formatWhereClause($where); } + public function adjustInlinesForChangesets( + array $inlines, + array $old, + array $new) { + + assert_instances_of($inlines, 'DifferentialInlineComment'); + assert_instances_of($old, 'DifferentialChangeset'); + assert_instances_of($new, 'DifferentialChangeset'); + + $viewer = $this->getViewer(); + $all = array_merge($old, $new); + + $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(); + foreach ($all as $changeset) { + $id_map[$changeset->getID()] = $changeset->getID(); + } + + // Generate filename maps for older and newer comments. If we're bringing + // an older comment forward in a diff-of-diffs, we want to put it on the + // left side of the screen, not the right side. Both sides are "new" files + // with the same name, so they're both appropriate targets, but the left + // is a better target conceptually for users because it's more consistent + // with the rest of the UI, which shows old information on the left and + // new information on the right. + $move_here = DifferentialChangeType::TYPE_MOVE_HERE; + + $name_map_old = array(); + $name_map_new = array(); + $move_map = array(); + foreach ($all as $changeset) { + $changeset_id = $changeset->getID(); + + $filenames = array(); + $filenames[] = $changeset->getFilename(); + + // If this is the target of a move, also map comments on the old filename + // to this changeset. + if ($changeset->getChangeType() == $move_here) { + $old_file = $changeset->getOldFile(); + $filenames[] = $old_file; + $move_map[$changeset_id][$old_file] = true; + } + + foreach ($filenames as $filename) { + // We update the old map only if we don't already have an entry (oldest + // changeset persists). + if (empty($name_map_old[$filename])) { + $name_map_old[$filename] = $changeset_id; + } + + // We always update the new map (newest changeset overwrites). + $name_map_new[$changeset->getFilename()] = $changeset_id; + } + } + + // Find the smallest "new" changeset ID. We'll consider everything + // larger than this to be "newer", and everything smaller to be "older". + $first_new_id = min(mpull($new, '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, as it has bogus data. + continue; + } + + $target_id = null; + + if ($changeset_id >= $first_new_id) { + $name_map = $name_map_new; + $is_new = true; + } else { + $name_map = $name_map_old; + $is_new = false; + } + + $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]; + + $is_move = isset($move_map[$target_id][$filename]); + if ($is_new) { + if ($is_move) { + $reason = pht( + 'This comment was made on a file with the same name as the '. + 'file this file was moved from, but in a newer diff.'); + } else { + $reason = pht( + 'This comment was made on a file with the same name, but '. + 'in a newer diff.'); + } + } else { + if ($is_move) { + $reason = pht( + 'This comment was made on a file with the same name as the '. + 'file this file was moved from, but in an older diff.'); + } else { + $reason = pht( + 'This comment was made on a file with the same name, but '. + 'in an older diff.'); + } + } + } + + + // If we didn't find a target and this change is the target of a move, + // look for a match against the old filename. + if (!$target_id) { + if ($changeset->getChangeType() == $move_here) { + $filename = $changeset->getOldFile(); + if (isset($name_map[$filename])) { + $target_id = $name_map[$filename]; + if ($is_new) { + $reason = pht( + 'This comment was made on a file which this file was moved '. + 'to, but in a newer diff.'); + } else { + $reason = pht( + 'This comment was made on a file which this file was moved '. + 'to, but in an older diff.'); + } + } + } + } + + + // 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( + array( + 'new' => $is_new, + 'reason' => $reason, + )); + + $results[] = $inline; + } + } + + // Filter out the inlines we ported forward which won't be visible because + // they appear on the wrong side of a file. + $keep_map = array(); + foreach ($old as $changeset) { + $keep_map[$changeset->getID()][0] = true; + } + foreach ($new as $changeset) { + $keep_map[$changeset->getID()][1] = true; + } + + foreach ($results as $key => $inline) { + $is_new = (int)$inline->getIsNewFile(); + $changeset_id = $inline->getChangesetID(); + if (!isset($keep_map[$changeset_id][$is_new])) { + unset($results[$key]); + continue; + } + } + + return $results; + } + }