diff --git a/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php b/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php index b19dfa961c..2f5cc7362a 100644 --- a/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php @@ -154,12 +154,8 @@ class DifferentialChangesetViewController extends DifferentialController { $parser->setLeftSideCommentMapping($left_source, $left_new); $parser->setWhitespaceMode($request->getStr('whitespace')); - // Load both left-side and right-side inline comments. - $inlines = $this->loadInlineComments( - array($left_source, $right_source), - $author_phid); - $phids = array(); + $inlines = $this->loadInlineComments($id, $author_phid); foreach ($inlines as $inline) { $parser->parseInlineComment($inline); $phids[$inline->getAuthorPHID()] = true; @@ -232,15 +228,10 @@ 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($changeset_id, $author_phid) { return id(new DifferentialInlineComment())->loadAllWhere( - 'changesetID IN (%Ld) AND (commentID IS NOT NULL OR authorPHID = %s)', - $changeset_ids, + 'changesetID = %d AND (commentID IS NOT NULL OR authorPHID = %s)', + $changeset_id, $author_phid); } diff --git a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php index 4f366bba0d..0144e22f3b 100644 --- a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php @@ -39,6 +39,7 @@ class DifferentialChangesetParser { protected $whitespaceMode = null; protected $subparser; + protected $oldChangesetID = null; protected $noHighlight; protected $renderCacheKey = null; @@ -46,12 +47,6 @@ class DifferentialChangesetParser { private $handles; private $user; - private $leftSideChangesetID; - private $leftSideAttachesToNewFile; - - private $rightSideChangesetID; - private $rightSideAttachesToNewFile; - const CACHE_VERSION = 4; const ATTR_GENERATED = 'attr:generated'; @@ -65,38 +60,12 @@ class DifferentialChangesetParser { const WHITESPACE_IGNORE_TRAILING = 'ignore-trailing'; const WHITESPACE_IGNORE_ALL = 'ignore-all'; - /** - * Configure which Changeset comments added to the right side of the visible - * diff will be attached to. The ID must be the ID of a real Differential - * Changeset. - * - * The complexity here is that we may show an arbitrary side of an arbitrary - * changeset as either the left or right part of a diff. This method allows - * the left and right halves of the displayed diff to be correctly mapped to - * storage changesets. - * - * @param id The Differential Changeset ID that comments added to the right - * side of the visible diff should be attached to. - * @param bool If true, attach new comments to the right side of the storage - * changeset. Note that this may be false, if the left side of - * some storage changeset is being shown as the right side of - * a display diff. - * @return this - */ public function setRightSideCommentMapping($id, $is_new) { - $this->rightSideChangesetID = $id; - $this->rightSideAttachesToNewFile = $is_new; - return $this; + } - /** - * See setRightSideCommentMapping(), but this sets information for the left - * side of the display diff. - */ public function setLeftSideCommentMapping($id, $is_new) { - $this->leftSideChangesetID = $id; - $this->leftSideAttachesToNewFile = $is_new; - return $this; + } /** @@ -135,6 +104,11 @@ class DifferentialChangesetParser { return $this; } + public function setOldChangesetID($old_changeset_id) { + $this->oldChangesetID = $old_changeset_id; + return $this; + } + public function setChangesetID($changeset_id) { $this->changesetID = $changeset_id; return $this; @@ -262,10 +236,7 @@ class DifferentialChangesetParser { } public function parseInlineComment(DifferentialInlineComment $comment) { - // Parse only comments which are actually visible. - if ($this->isCommentVisibleOnRenderedDiff($comment)) { - $this->comments[] = $comment; - } + $this->comments[] = $comment; return $this; } @@ -866,7 +837,7 @@ EOSYNTHETIC; $end = $comment->getLineNumber() + $comment->getLineLength() + self::LINES_CONTEXT; - $new = $this->isCommentOnRightSideWhenDisplayed($comment); + $new = $this->isCommentInNewFile($comment); for ($ii = $start; $ii <= $end; $ii++) { if ($new) { $new_mask[$ii] = true; @@ -891,7 +862,7 @@ EOSYNTHETIC; foreach ($this->comments as $comment) { $final = $comment->getLineNumber() + $comment->getLineLength(); - if ($this->isCommentOnRightSideWhenDisplayed($comment)) { + if ($this->isCommentInNewFile($comment)) { $new_comments[$final][] = $comment; } else { $old_comments[$final][] = $comment; @@ -910,58 +881,12 @@ EOSYNTHETIC; return $this->renderChangesetTable($this->changeset, $html); } - /** - * Determine if an inline comment will appear on the rendered diff, - * taking into consideration which halves of which changesets will actually - * be shown. - * - * @param DifferentialInlineComment Comment to test for visibility. - * @return bool True if the comment is visible on the rendered diff. - */ - private function isCommentVisibleOnRenderedDiff( - DifferentialInlineComment $comment) { - - $changeset_id = $comment->getChangesetID(); - $is_new = $comment->getIsNewFile(); - - if ($changeset_id == $this->rightSideChangesetID && - $is_new == $this->rightSideAttachesToNewFile) { - return true; + private function isCommentInNewFile(DifferentialInlineComment $comment) { + if ($this->oldChangesetID) { + return ($comment->getChangesetID() != $this->oldChangesetID); + } else { + return $comment->getIsNewFile(); } - - if ($changeset_id == $this->leftSideChangesetID && - $is_new == $this->leftSideAttachesToNewFile) { - return true; - } - - return false; - } - - - /** - * Determine if a comment will appear on the right side of the display diff. - * Note that the comment must appear somewhere on the rendered changeset, as - * per isCommentVisibleOnRenderedDiff(). - * - * @param DifferentialInlineComment Comment to test for display location. - * @return bool True for right, false for left. - */ - private function isCommentOnRightSideWhenDisplayed( - DifferentialInlineComment $comment) { - - if (!$this->isCommentVisibleOnRenderedDiff($comment)) { - throw new Exception("Comment is not visible on changeset!"); - } - - $changeset_id = $comment->getChangesetID(); - $is_new = $comment->getIsNewFile(); - - if ($changeset_id == $this->rightSideChangesetID && - $is_new == $this->rightSideAttachesToNewFile) { - return true; - } - - return false; } protected function renderShield($message, $more) { @@ -1036,15 +961,6 @@ EOSYNTHETIC; $range_len = min($range_len, $rows - $range_start); - // Gaps - compute gaps in the visible display diff, where we will render - // "Show more context" spacers. This builds an aggregate $mask of all the - // lines we must show (because they are near changed lines, near inline - // comments, or the request has explicitly asked for them, i.e. resulting - // from the user clicking "show more") and then finds all the gaps between - // visible lines. If a gap is smaller than the context size, we just - // display it. Otherwise, we record it into $gaps and will render a - // "show more context" element instead of diff text below. - $gaps = array(); $gap_start = 0; $in_gap = false; @@ -1073,29 +989,11 @@ EOSYNTHETIC; $gaps = array_reverse($gaps); + $changeset = $this->changesetID; $reference = $this->getChangeset()->getRenderingReference(); - $left_id = $this->leftSideChangesetID; - $right_id = $this->rightSideChangesetID; - - // "N" stands for 'new' and means the comment should attach to the new file - // when stored, i.e. DifferentialInlineComment->setIsNewFile(). - // "O" stands for 'old' and means the comment should attach to the old file. - - $left_char = $this->leftSideAttachesToNewFile - ? 'N' - : 'O'; - $right_char = $this->rightSideAttachesToNewFile - ? 'N' - : 'O'; - 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. - // Pop information about the next gap off the $gaps stack and render - // an appropriate "Show more context" element. This branch eventually - // increments $ii by the entire size of the gap and then continues - // the loop. $gap = array_pop($gaps); $top = $gap[0]; $len = $gap[1]; @@ -1202,14 +1100,14 @@ EOSYNTHETIC; $html[] = $context_not_available; } - if ($o_num && $left_id) { - $o_id = ' id="C'.$left_id.$left_char.'L'.$o_num.'"'; + if ($o_num && $changeset) { + $o_id = ' id="C'.$changeset.'OL'.$o_num.'"'; } else { $o_id = null; } - if ($n_num && $right_id) { - $n_id = ' id="C'.$right_id.$right_char.'L'.$n_num.'"'; + if ($n_num && $changeset) { + $n_id = ' id="C'.$changeset.'NL'.$n_num.'"'; } else { $n_id = null; } @@ -1258,7 +1156,7 @@ EOSYNTHETIC; ($comment->getAuthorPHID() == $user->getPHID()) && (!$comment->getCommentID()); - $on_right = $this->isCommentOnRightSideWhenDisplayed($comment); + $on_right = $this->isCommentInNewFile($comment); return id(new DifferentialInlineCommentView()) ->setInlineComment($comment)