Page MenuHomePhabricator
Files F15466652

D12495.id30002.diff
No OneTemporary
Actions

D12495.id30002.diff
View Options

diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php
--- a/src/applications/differential/controller/DifferentialChangesetViewController.php
+++ b/src/applications/differential/controller/DifferentialChangesetViewController.php
@@ -65,6 +65,8 @@
}
}
+ $old = array();
+ $new = array();
if (!$vs) {
$right = $changeset;
$left = null;
@@ -75,6 +77,9 @@
$left_new = false;
$render_cache_key = $right->getID();
+
+ $old[] = $changeset;
+ $new[] = $changeset;
} else if ($vs == -1) {
$right = null;
$left = $changeset;
@@ -85,6 +90,9 @@
$left_new = true;
$render_cache_key = null;
+
+ $old[] = $changeset;
+ $new[] = $changeset;
} else {
$right = $changeset;
$left = $vs_changeset;
@@ -95,6 +103,9 @@
$left_new = true;
$render_cache_key = null;
+
+ $new[] = $left;
+ $new[] = $right;
}
if ($left) {
@@ -177,12 +188,11 @@
// 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 @@
));
}
- 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
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -95,9 +95,6 @@
$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 @@
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 @@
$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 @@
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
--- a/src/applications/differential/query/DifferentialInlineCommentQuery.php
+++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php
@@ -137,4 +137,192 @@
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;
+ }
+
}

File Metadata

Mime Type
text/plain
Expires
Fri, Apr 4, 7:57 AM (1 d, 11 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7387379
Default Alt Text
D12495.id30002.diff (17 KB)

Event Timeline

Phabricator · Built by Phacility