mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-10 08:52:39 +01:00
refactor pass 2 of N on DifferentialChangesetParser and bonus bug fix
Summary: pull out some more stuff from the TwoUp renderer that's generically useful. also clean up $xhp variable and add a get method for the $coverage. Finally, fix T2177 while I'm in here. Test Plan: played around with Differential. Also opened things up in Firefox to verify T2177. Reviewers: epriestley, vrana Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T2009, T2177 Differential Revision: https://secure.phabricator.com/D4161
This commit is contained in:
Bob Trahan
parent
a6aa8f746f
commit
c970f7e89c
4 changed files with 170 additions and 126 deletions
|
|
@ -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 = '<em>-</em>';
|
||||
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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[] =
|
||||
'<tr class="inline">'.
|
||||
'<th />'.
|
||||
'<td class="left">'.$xhp.'</td>'.
|
||||
'<td class="left">'.$comment_html.'</td>'.
|
||||
'<th />'.
|
||||
'<td colspan="3" class="right3">'.$new.'</td>'.
|
||||
'</tr>';
|
||||
|
|
@ -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[] =
|
||||
'<tr class="inline">'.
|
||||
'<th />'.
|
||||
'<td class="left" />'.
|
||||
'<th />'.
|
||||
'<td colspan="3" class="right3">'.$xhp.'</td>'.
|
||||
'<td colspan="3" class="right3">'.$comment_html.'</td>'.
|
||||
'</tr>';
|
||||
}
|
||||
}
|
||||
|
|
@ -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[] =
|
||||
'<tr class="inline">'.
|
||||
'<th />'.
|
||||
'<td class="left">'.$xhp.'</td>'.
|
||||
'<td class="left">'.$comment_html.'</td>'.
|
||||
'<th />'.
|
||||
'<td class="right3" colspan="3" />'.
|
||||
'</tr>';
|
||||
}
|
||||
foreach ($this->getNewComments() as $comment) {
|
||||
$xhp = $this->renderInlineComment($comment, $on_right = true);
|
||||
$comment_html = $this->renderInlineComment($comment, $on_right = true);
|
||||
$html_new[] =
|
||||
'<tr class="inline">'.
|
||||
'<th />'.
|
||||
'<td class="left" />'.
|
||||
'<th />'.
|
||||
'<td class="right3" colspan="3">'.$xhp.'</td>'.
|
||||
'<td class="right3" colspan="3">'.$comment_html.'</td>'.
|
||||
'</tr>';
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -251,9 +251,9 @@ final class DifferentialInlineCommentView extends AphrontView {
|
|||
'<table>'.
|
||||
'<tr class="inline">'.
|
||||
'<th></th>'.
|
||||
'<td>'.$left_markup.'</td>'.
|
||||
'<td class="left">'.$left_markup.'</td>'.
|
||||
'<th></th>'.
|
||||
'<td colspan="2">'.$right_markup.'</td>'.
|
||||
'<td class="right3" colspan="3">'.$right_markup.'</td>'.
|
||||
'</tr>'.
|
||||
'</table>';
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue