mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-23 05:50:55 +01:00
Fix an issue where the last line of block-based diffs could be incorrectly hidden
Summary: Fixes T13468. See that task for discussion. The older source-rendering code mixes "line number" / "1-based" lists with "block number" / "0-based" lists and then has other bugs which cancel this out. For block-based diffs, build an explicit block-based mask with only block numbers. This sort of sidesteps the whole issue. Test Plan: Viewed the diff with the original reproduction case, plus various other block-based diffs, including one-block image diffs, in unified and side-by-side mode. Didn't spot any oddities. Maniphest Tasks: T13468 Differential Revision: https://secure.phabricator.com/D20959
This commit is contained in:
parent
12c3370988
commit
ccf28a8112
2 changed files with 66 additions and 9 deletions
|
@ -354,6 +354,69 @@ final class DifferentialHunkParser extends Phobject {
|
|||
return $this;
|
||||
}
|
||||
|
||||
public function generateVisibleBlocksMask($lines_context) {
|
||||
|
||||
// See T13468. This is similar to "generateVisibleLinesMask()", but
|
||||
// attempts to work around a series of bugs which cancel each other
|
||||
// out but make a mess of the intermediate steps.
|
||||
|
||||
$old = $this->getOldLines();
|
||||
$new = $this->getNewLines();
|
||||
|
||||
$length = max(count($old), count($new));
|
||||
|
||||
$visible_lines = array();
|
||||
for ($ii = 0; $ii < $length; $ii++) {
|
||||
$old_visible = (isset($old[$ii]) && $old[$ii]['type']);
|
||||
$new_visible = (isset($new[$ii]) && $new[$ii]['type']);
|
||||
|
||||
$visible_lines[$ii] = ($old_visible || $new_visible);
|
||||
}
|
||||
|
||||
$mask = array();
|
||||
$reveal_cursor = -1;
|
||||
for ($ii = 0; $ii < $length; $ii++) {
|
||||
|
||||
// If this line isn't visible, it isn't going to reveal anything.
|
||||
if (!$visible_lines[$ii]) {
|
||||
|
||||
// If it hasn't been revealed by a nearby line, mark it as masked.
|
||||
if (empty($mask[$ii])) {
|
||||
$mask[$ii] = false;
|
||||
}
|
||||
|
||||
continue;
|
||||
}
|
||||
|
||||
// If this line is visible, reveal all the lines nearby.
|
||||
|
||||
// First, compute the minimum and maximum offsets we want to reveal.
|
||||
$min_reveal = max($ii - $lines_context, 0);
|
||||
$max_reveal = min($ii + $lines_context, $length - 1);
|
||||
|
||||
// Naively, we'd do more work than necessary when revealing context for
|
||||
// several adjacent visible lines: we would mark all the overlapping
|
||||
// lines as revealed several times.
|
||||
|
||||
// To avoid duplicating work, keep track of the largest line we've
|
||||
// revealed to. Since we reveal context by marking every consecutive
|
||||
// line, we don't need to touch any line above it.
|
||||
$min_reveal = max($min_reveal, $reveal_cursor);
|
||||
|
||||
// Reveal the remaining unrevealed lines.
|
||||
for ($jj = $min_reveal; $jj <= $max_reveal; $jj++) {
|
||||
$mask[$jj] = true;
|
||||
}
|
||||
|
||||
// Move the cursor to the next line which may still need to be revealed.
|
||||
$reveal_cursor = $max_reveal + 1;
|
||||
}
|
||||
|
||||
$this->setVisibleLinesMask($mask);
|
||||
|
||||
return $mask;
|
||||
}
|
||||
|
||||
public function generateVisibleLinesMask($lines_context) {
|
||||
$old = $this->getOldLines();
|
||||
$new = $this->getNewLines();
|
||||
|
@ -361,6 +424,7 @@ final class DifferentialHunkParser extends Phobject {
|
|||
$visible = false;
|
||||
$last = 0;
|
||||
$mask = array();
|
||||
|
||||
for ($cursor = -$lines_context; $cursor < $max_length; $cursor++) {
|
||||
$offset = $cursor + $lines_context;
|
||||
if ((isset($old[$offset]) && $old[$offset]['type']) ||
|
||||
|
|
|
@ -59,7 +59,7 @@ final class PhabricatorDocumentEngineBlocks
|
|||
->parseHunksForLineData($changeset->getHunks())
|
||||
->reparseHunksForSpecialAttributes();
|
||||
|
||||
$hunk_parser->generateVisibleLinesMask(2);
|
||||
$hunk_parser->generateVisibleBlocksMask(2);
|
||||
$mask = $hunk_parser->getVisibleLinesMask();
|
||||
|
||||
$old_lines = $hunk_parser->getOldLines();
|
||||
|
@ -72,14 +72,7 @@ final class PhabricatorDocumentEngineBlocks
|
|||
$old_line = idx($old_lines, $ii);
|
||||
$new_line = idx($new_lines, $ii);
|
||||
|
||||
$is_visible = !empty($mask[$ii + 1]);
|
||||
|
||||
// TODO: There's currently a bug where one-line files get incorrectly
|
||||
// masked. This causes images to completely fail to render. Just ignore
|
||||
// the mask if it came back empty.
|
||||
if (!$mask) {
|
||||
$is_visible = true;
|
||||
}
|
||||
$is_visible = !empty($mask[$ii]);
|
||||
|
||||
if ($old_line) {
|
||||
$old_hash = rtrim($old_line['text'], "\n");
|
||||
|
|
Loading…
Reference in a new issue