From 1858d2aa6679ece69e5d7d1339530c2ec515046b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 21 Apr 2015 15:32:23 -0700 Subject: [PATCH] Make Differential timeline aware of ghostly inlines Summary: Ref T7447. Ref T7870. When a ghostly inline appears on the page, the timeline isn't currently aware that it's present, so it links elsewhere. Instead, apply adjustments before rendering the timeline. Ref T5030. This makes the behaviors in T5030 irrelevant most of the time. Test Plan: Before: {F378106} After: - Inlines visible on page are linked directly. - Inlines which we can't port (e.g., on files not present on current page) are still linked off-page. - Used "Show Older" to page through and verify consistent rendering. Reviewers: btrahan Reviewed By: btrahan Subscribers: yelirekim, epriestley Maniphest Tasks: T5030, T7870, T7447 Differential Revision: https://secure.phabricator.com/D12497 --- .../DifferentialRevisionViewController.php | 47 +++++++++++-------- .../query/DifferentialInlineCommentQuery.php | 14 +++++- .../storage/DifferentialRevision.php | 26 +++++++++- 3 files changed, 65 insertions(+), 22 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 215a455ef7..2a645c4d05 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -90,6 +90,23 @@ final class DifferentialRevisionViewController extends DifferentialController { $repository); } + $map = $vs_map; + if (!$map) { + $map = array_fill_keys(array_keys($changesets), 0); + } + + $old_ids = array(); + $new_ids = array(); + foreach ($map as $id => $vs) { + if ($vs <= 0) { + $old_ids[] = $id; + $new_ids[] = $id; + } else { + $new_ids[] = $id; + $new_ids[] = $vs; + } + } + $props = id(new DifferentialDiffProperty())->loadAllWhere( 'diffID = %d', $target_manual->getID()); @@ -154,24 +171,8 @@ final class DifferentialRevisionViewController extends DifferentialController { pht('Show All Files Inline')))); $warning = $warning->render(); - $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); + $old = array_select_keys($changesets, $old_ids); + $new = array_select_keys($changesets, $new_ids); $query = id(new DifferentialInlineCommentQuery()) ->setViewer($user) @@ -285,7 +286,9 @@ final class DifferentialRevisionViewController extends DifferentialController { $comment_view = $this->buildTransactions( $revision, $diff_vs ? $diffs[$diff_vs] : $target, - $target); + $target, + $old_ids, + $new_ids); if (!$viewer_is_anonymous) { $comment_view->setQuoteRef('D'.$revision->getID()); @@ -931,7 +934,9 @@ final class DifferentialRevisionViewController extends DifferentialController { private function buildTransactions( DifferentialRevision $revision, DifferentialDiff $left_diff, - DifferentialDiff $right_diff) { + DifferentialDiff $right_diff, + array $old_ids, + array $new_ids) { $timeline = $this->buildTransactionTimeline( $revision, @@ -940,6 +945,8 @@ final class DifferentialRevisionViewController extends DifferentialController { array( 'left' => $left_diff->getID(), 'right' => $right_diff->getID(), + 'old' => implode(',', $old_ids), + 'new' => implode(',', $new_ids), )); return $timeline; diff --git a/src/applications/differential/query/DifferentialInlineCommentQuery.php b/src/applications/differential/query/DifferentialInlineCommentQuery.php index 7ef6f0a1ca..a7b2e2f444 100644 --- a/src/applications/differential/query/DifferentialInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php @@ -151,6 +151,18 @@ final class DifferentialInlineCommentQuery $changeset_ids = mpull($inlines, 'getChangesetID'); $changeset_ids = array_unique($changeset_ids); + + $all_map = mpull($all, null, 'getID'); + + // We already have at least some changesets, and we might not need to do + // any more data fetching. Remove everything we already have so we can + // tell if we need new stuff. + foreach ($changeset_ids as $key => $id) { + if (isset($all_map[$id])) { + unset($changeset_ids[$key]); + } + } + if ($changeset_ids) { $changesets = id(new DifferentialChangesetQuery()) ->setViewer($viewer) @@ -160,7 +172,7 @@ final class DifferentialInlineCommentQuery } else { $changesets = array(); } - $changesets += mpull($all, null, 'getID'); + $changesets += $all_map; $id_map = array(); foreach ($all as $changeset) { diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 291799891a..de8e08dcdc 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -478,6 +478,7 @@ final class DifferentialRevision extends DifferentialDAO public function willRenderTimeline( PhabricatorApplicationTransactionView $timeline, AphrontRequest $request) { + $viewer = $request->getViewer(); $render_data = $timeline->getRenderData(); $left = $request->getInt('left', idx($render_data, 'left')); @@ -491,10 +492,17 @@ final class DifferentialRevision extends DifferentialDAO $left_diff = $diffs[$left]; $right_diff = $diffs[$right]; + $old_ids = $request->getStr('old', idx($render_data, 'old')); + $new_ids = $request->getStr('new', idx($render_data, 'new')); + $old_ids = explode(',', $old_ids); + $new_ids = explode(',', $new_ids); + $type_inline = DifferentialTransaction::TYPE_INLINE; - $changeset_ids = array(); + $changeset_ids = array_merge($old_ids, $new_ids); + $inlines = array(); foreach ($timeline->getTransactions() as $xaction) { if ($xaction->getTransactionType() == $type_inline) { + $inlines[] = $xaction->getComment(); $changeset_ids[] = $xaction->getComment()->getChangesetID(); } } @@ -509,6 +517,22 @@ final class DifferentialRevision extends DifferentialDAO $changesets = array(); } + foreach ($inlines as $key => $inline) { + $inlines[$key] = DifferentialInlineComment::newFromModernComment( + $inline); + } + + $query = id(new DifferentialInlineCommentQuery()) + ->setViewer($viewer); + + // NOTE: This is a bit sketchy: this method adjusts the inlines as a + // side effect, which means it will ultimately adjust the transaction + // comments and affect timeline rendering. + $query->adjustInlinesForChangesets( + $inlines, + array_select_keys($changesets, $old_ids), + array_select_keys($changesets, $new_ids)); + return $timeline ->setChangesets($changesets) ->setRevision($this)