diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index be6a6546a2..fb2d32aa4c 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -239,6 +239,9 @@ final class DifferentialChangesetParser { $this->coverage = $coverage; return $this; } + private function getCoverage() { + return $this->coverage; + } public function parseHunk(DifferentialHunk $hunk) { $lines = $hunk->getChanges(); @@ -972,19 +975,15 @@ final class DifferentialChangesetParser { $renderer = id(new DifferentialChangesetTwoUpRenderer()) ->setChangeset($this->changeset) ->setRenderPropertyChangeHeader($render_pch) - ->setOldLines($this->old) - ->setNewLines($this->new) ->setOldRender($this->oldRender) ->setNewRender($this->newRender) ->setMissingOldLines($this->missingOld) ->setMissingNewLines($this->missingNew) - ->setVisibleLines($this->visible) ->setOldChangesetID($this->leftSideChangesetID) ->setNewChangesetID($this->rightSideChangesetID) ->setOldAttachesToNewFile($this->leftSideAttachesToNewFile) ->setNewAttachesToNewFile($this->rightSideAttachesToNewFile) - ->setLinesOfContext(self::LINES_CONTEXT) - ->setCodeCoverage($this->coverage) + ->setCodeCoverage($this->getCoverage()) ->setRenderingReference($this->getRenderingReference()) ->setMarkupEngine($this->markupEngine) ->setHandles($this->handles); @@ -1156,16 +1155,132 @@ final class DifferentialChangesetParser { ->setOriginalOld($this->originalLeft) ->setOriginalNew($this->originalRight); + $rows = max( + count($this->old), + count($this->new)); + if ($range_start === null) { + $range_start = 0; + } + if ($range_len === null) { + $range_len = $rows; + } + $range_len = min($range_len, $rows - $range_start); + + list($gaps, $mask, $depths) = $this->calculateGapsMaskAndDepths( + $mask_force, + $feedback_mask, + $range_start, + $range_len + ); + + $renderer + ->setOldLines($this->old) + ->setNewLines($this->new) + ->setGaps($gaps) + ->setMask($mask) + ->setDepths($depths); + $html = $renderer->renderTextChange( $range_start, $range_len, - $mask_force, - $feedback_mask + $rows ); return $renderer->renderChangesetTable($html); } + /** + * This function calculates a lot of stuff we need to know to display + * the diff: + * + * Gaps - compute gaps in the visible display diff, where we will render + * "Show more context" spacers. 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. A given $gap + * is a tuple of $gap_line_number_start and $gap_length. + * + * Mask - compute the actual lines that need to be shown (because they + * are near changes lines, near inline comments, or the request has + * explicitly asked for them, i.e. resulting from the user clicking + * "show more"). The $mask returned is a sparesely populated dictionary + * of $visible_line_number => true. + * + * Depths - compute how indented any given line is. The $depths returned + * is a sparesely populated dictionary of $visible_line_number => $depth. + * + * This function also has the side effect of modifying member variable + * new such that tabs are normalized to spaces for each line of the diff. + * + * @return array($gaps, $mask, $depths) + */ + private function calculateGapsMaskAndDepths($mask_force, + $feedback_mask, + $range_start, + $range_len) { + + // Calculate gaps and mask first + $gaps = array(); + $gap_start = 0; + $in_gap = false; + $base_mask = $this->visible + $mask_force + $feedback_mask; + $base_mask[$range_start + $range_len] = true; + for ($ii = $range_start; $ii <= $range_start + $range_len; $ii++) { + if (isset($base_mask[$ii])) { + if ($in_gap) { + $gap_length = $ii - $gap_start; + if ($gap_length <= self::LINES_CONTEXT) { + for ($jj = $gap_start; $jj <= $gap_start + $gap_length; $jj++) { + $base_mask[$jj] = true; + } + } else { + $gaps[] = array($gap_start, $gap_length); + } + $in_gap = false; + } + } else { + if (!$in_gap) { + $gap_start = $ii; + $in_gap = true; + } + } + } + $gaps = array_reverse($gaps); + $mask = $base_mask; + + // Time to calculate depth. + // We need to go backwards to properly indent whitespace in this code: + // + // 0: class C { + // 1: + // 1: function f() { + // 2: + // 2: return; + // 3: + // 3: } + // 4: + // 4: } + // + $depths = array(); + $last_depth = 0; + $range_end = $range_start + $range_len; + if (!isset($this->new[$range_end])) { + $range_end--; + } + for ($ii = $range_end; $ii >= $range_start; $ii--) { + // We need to expand tabs to process mixed indenting and to round + // correctly later. + $line = str_replace("\t", " ", $this->new[$ii]['text']); + $trimmed = ltrim($line); + if ($trimmed != '') { + // We round down to flatten "/**" and " *". + $last_depth = floor((strlen($line) - strlen($trimmed)) / 2); + } + $depths[$ii] = $last_depth; + } + + return array($gaps, $mask, $depths); + } + /** * Determine if an inline comment will appear on the rendered diff, * taking into consideration which halves of which changesets will actually @@ -1314,7 +1429,8 @@ final class DifferentialChangesetParser { public function renderModifiedCoverage() { $na = '-'; - if (!$this->coverage) { + $coverage = $this->getCoverage(); + if (!$coverage) { return $na; } @@ -1330,11 +1446,11 @@ final class DifferentialChangesetParser { continue; } - if (empty($this->coverage[$new['line'] - 1])) { + if (empty($coverage[$new['line'] - 1])) { continue; } - switch ($this->coverage[$new['line'] - 1]) { + switch ($coverage[$new['line'] - 1]) { case 'C': $covered++; break; diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index f18f980ded..acac198ae6 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -10,7 +10,6 @@ abstract class DifferentialChangesetRenderer { private $missingNewLines; private $oldLines; private $newLines; - private $visibleLines; private $oldComments; private $newComments; private $oldChangesetID; @@ -19,7 +18,6 @@ abstract class DifferentialChangesetRenderer { private $newAttachesToNewFile; private $highlightOld = array(); private $highlightNew = array(); - private $linesOfContext; private $codeCoverage; private $handles; private $markupEngine; @@ -27,6 +25,33 @@ abstract class DifferentialChangesetRenderer { private $newRender; private $originalOld; private $originalNew; + private $gaps; + private $mask; + private $depths; + + public function setDepths($depths) { + $this->depths = $depths; + return $this; + } + protected function getDepths() { + return $this->depths; + } + + public function setMask($mask) { + $this->mask = $mask; + return $this; + } + protected function getMask() { + return $this->mask; + } + + public function setGaps($gaps) { + $this->gaps = $gaps; + return $this; + } + protected function getGaps() { + return $this->gaps; + } public function setOriginalNew($original_new) { $this->originalNew = $original_new; @@ -85,14 +110,6 @@ abstract class DifferentialChangesetRenderer { return $this->codeCoverage; } - public function setLinesOfContext($lines_of_context) { - $this->linesOfContext = $lines_of_context; - return $this; - } - protected function getLinesOfContext() { - return $this->linesOfContext; - } - public function setHighlightNew($highlight_new) { $this->highlightNew = $highlight_new; return $this; @@ -163,14 +180,6 @@ abstract class DifferentialChangesetRenderer { return $this->oldComments; } - public function setVisibleLines(array $visible_lines) { - $this->visibleLines = $visible_lines; - return $this; - } - protected function getVisibleLines() { - return $this->visibleLines; - } - public function setNewLines(array $new_lines) { $this->newLines = $new_lines; return $this; @@ -239,8 +248,7 @@ abstract class DifferentialChangesetRenderer { abstract public function renderTextChange( $range_start, $range_len, - $mask_force, - $feedback_mask + $rows ); abstract public function renderFileChange( $old = null, diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index dc458f15ff..52dde106eb 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -42,8 +42,7 @@ final class DifferentialChangesetTwoUpRenderer public function renderTextChange( $range_start, $range_len, - $mask_force, - $feedback_mask) { + $rows) { $missing_old = $this->getMissingOldLines(); $missing_new = $this->getMissingNewLines(); @@ -69,61 +68,8 @@ final class DifferentialChangesetTwoUpRenderer $html = array(); $old_lines = $this->getOldLines(); $new_lines = $this->getNewLines(); - - $rows = max( - count($old_lines), - count($new_lines)); - - if ($range_start === null) { - $range_start = 0; - } - - if ($range_len === null) { - $range_len = $rows; - } - - $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; - $lines_of_context = $this->getLinesOfContext(); - $mask = $this->getVisibleLines() + $mask_force + $feedback_mask; - $mask[$range_start + $range_len] = true; - for ($ii = $range_start; $ii <= $range_start + $range_len; $ii++) { - if (isset($mask[$ii])) { - if ($in_gap) { - $gap_length = $ii - $gap_start; - if ($gap_length <= $lines_of_context) { - for ($jj = $gap_start; $jj <= $gap_start + $gap_length; $jj++) { - $mask[$jj] = true; - } - } else { - $gaps[] = array($gap_start, $gap_length); - } - $in_gap = false; - } - } else { - if (!$in_gap) { - $gap_start = $ii; - $in_gap = true; - } - } - } - - $gaps = array_reverse($gaps); - + $gaps = $this->getGaps(); $reference = $this->getRenderingReference(); - $left_id = $this->getOldChangesetID(); $right_id = $this->getNewChangesetID(); @@ -146,36 +92,8 @@ final class DifferentialChangesetTwoUpRenderer $new_render = $this->getNewRender(); $original_left = $this->getOriginalOld(); $original_right = $this->getOriginalNew(); - - // We need to go backwards to properly indent whitespace in this code: - // - // 0: class C { - // 1: - // 1: function f() { - // 2: - // 2: return; - // 3: - // 3: } - // 4: - // 4: } - // - $depths = array(); - $last_depth = 0; - $range_end = $range_start + $range_len; - if (!isset($new_lines[$range_end])) { - $range_end--; - } - for ($ii = $range_end; $ii >= $range_start; $ii--) { - // We need to expand tabs to process mixed indenting and to round - // correctly later. - $line = str_replace("\t", " ", $new_lines[$ii]['text']); - $trimmed = ltrim($line); - if ($trimmed != '') { - // We round down to flatten "/**" and " *". - $last_depth = floor((strlen($line) - strlen($trimmed)) / 2); - } - $depths[$ii] = $last_depth; - } + $depths = $this->getDepths(); + $mask = $this->getMask(); for ($ii = $range_start; $ii < $range_start + $range_len; $ii++) { if (empty($mask[$ii])) { @@ -408,7 +326,8 @@ final class DifferentialChangesetTwoUpRenderer if ($o_num && isset($old_comments[$o_num])) { foreach ($old_comments[$o_num] as $comment) { - $xhp = $this->renderInlineComment($comment, $on_right = false); + $comment_html = $this->renderInlineComment($comment, + $on_right = false); $new = ''; if ($n_num && isset($new_comments[$n_num])) { foreach ($new_comments[$n_num] as $key => $new_comment) { @@ -422,7 +341,7 @@ final class DifferentialChangesetTwoUpRenderer $html[] = ''. ''. - ''.$xhp.''. + ''.$comment_html.''. ''. ''.$new.''. ''; @@ -430,13 +349,14 @@ final class DifferentialChangesetTwoUpRenderer } if ($n_num && isset($new_comments[$n_num])) { foreach ($new_comments[$n_num] as $comment) { - $xhp = $this->renderInlineComment($comment, $on_right = true); + $comment_html = $this->renderInlineComment($comment, + $on_right = true); $html[] = ''. ''. ''. ''. - ''.$xhp.''. + ''.$comment_html.''. ''; } } @@ -484,23 +404,23 @@ final class DifferentialChangesetTwoUpRenderer $html_old = array(); $html_new = array(); foreach ($this->getOldComments() as $comment) { - $xhp = $this->renderInlineComment($comment, $on_right = false); + $comment_html = $this->renderInlineComment($comment, $on_right = false); $html_old[] = ''. ''. - ''.$xhp.''. + ''.$comment_html.''. ''. ''. ''; } foreach ($this->getNewComments() as $comment) { - $xhp = $this->renderInlineComment($comment, $on_right = true); + $comment_html = $this->renderInlineComment($comment, $on_right = true); $html_new[] = ''. ''. ''. ''. - ''.$xhp.''. + ''.$comment_html.''. ''; } diff --git a/src/applications/differential/view/DifferentialInlineCommentView.php b/src/applications/differential/view/DifferentialInlineCommentView.php index 26b123a40e..19cab3f2d8 100644 --- a/src/applications/differential/view/DifferentialInlineCommentView.php +++ b/src/applications/differential/view/DifferentialInlineCommentView.php @@ -251,9 +251,9 @@ final class DifferentialInlineCommentView extends AphrontView { ''. ''. ''. - ''. + ''. ''. - ''. + ''. ''. '
'.$left_markup.''.$left_markup.''.$right_markup.''.$right_markup.'
'; }