1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-09 16:32:39 +01:00

Render indent depth changes more clearly

Summary:
Ref T13161. See PHI723. Our whitespace handling is based on whitespace flags like `diff -bw`, mostly just for historical reasons: long ago, the easiest way to minimize the visual impact of indentation changes was to literally use `diff -bw`.

However, this approach is very coarse and has a lot of problems, like detecting `"ab" -> "a b"` as "only a whitespace change" even though this is always semantic. It also causes problems in YAML, Python, etc. Over time, we've added a lot of stuff to mitigate the downsides to this approach.

We also no longer get any benefits from this approach being simple: we need faithful diffs as the authoritative source, and have to completely rebuild the diff to `diff -bw` it. In the UI, we have a "whitespace mode" flag. We have the "whitespace matters" configuration.

I think ReviewBoard generally has a better approach to indent depth changes than we do (see T13161) where it detects them and renders them in a minimal way with low visual impact. This is ultimately what we want: reduce visual clutter for depth-only changes, but preserve whitespace changes in strings, etc.

Move toward detecting and rendering indent depth changes. Followup work:

  - These should get colorblind colors and the design can probably use a little more tweaking.
  - The OneUp mode is okay, but could be improved.
  - Whitespace mode can now be removed completely.
  - I'm trying to handle tabs correctly, but since we currently mangle them into spaces today, it's hard to be sure I actually got it right.

Test Plan: {F6214084}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161

Differential Revision: https://secure.phabricator.com/D20181
This commit is contained in:
epriestley 2019-02-15 08:10:56 -08:00
parent deea2f01f5
commit 661c758ff9
12 changed files with 251 additions and 16 deletions

View file

@ -11,7 +11,7 @@ return array(
'conpherence.pkg.js' => '020aebcf',
'core.pkg.css' => '261ee8cf',
'core.pkg.js' => '5ace8a1e',
'differential.pkg.css' => 'b8df73d4',
'differential.pkg.css' => 'c3f15714',
'differential.pkg.js' => '67c9ea4c',
'diffusion.pkg.css' => '42c75c37',
'diffusion.pkg.js' => '91192d85',
@ -61,7 +61,7 @@ return array(
'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6',
'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d',
'rsrc/css/application/differential/add-comment.css' => '7e5900d9',
'rsrc/css/application/differential/changeset-view.css' => '73660575',
'rsrc/css/application/differential/changeset-view.css' => '783a9206',
'rsrc/css/application/differential/core.css' => 'bdb93065',
'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b',
'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
@ -275,6 +275,8 @@ return array(
'rsrc/image/checker_dark.png' => '7fc8fa7b',
'rsrc/image/checker_light.png' => '3157a202',
'rsrc/image/checker_lighter.png' => 'c45928c1',
'rsrc/image/chevron-in.png' => '1aa2f88f',
'rsrc/image/chevron-out.png' => 'c815e272',
'rsrc/image/controls/checkbox-checked.png' => '1770d7a0',
'rsrc/image/controls/checkbox-unchecked.png' => 'e1deba0a',
'rsrc/image/d5d8e1.png' => '6764616e',
@ -539,7 +541,7 @@ return array(
'conpherence-thread-manager' => 'aec8e38c',
'conpherence-transaction-css' => '3a3f5e7e',
'd3' => 'd67475f5',
'differential-changeset-view-css' => '73660575',
'differential-changeset-view-css' => '783a9206',
'differential-core-view-css' => 'bdb93065',
'differential-revision-add-comment-css' => '7e5900d9',
'differential-revision-comment-css' => '7dbc8d1d',
@ -1490,9 +1492,6 @@ return array(
'javelin-dom',
'javelin-uri',
),
73660575 => array(
'phui-inline-comment-view-css',
),
'73ecc1f8' => array(
'javelin-behavior',
'javelin-behavior-device',
@ -1514,6 +1513,9 @@ return array(
'javelin-uri',
'javelin-request',
),
'783a9206' => array(
'phui-inline-comment-view-css',
),
'78bc5d94' => array(
'javelin-behavior',
'javelin-uri',

View file

@ -199,8 +199,10 @@ final class CelerityDefaultPostprocessor
'diff.background' => '#fff',
'new-background' => 'rgba(151, 234, 151, .3)',
'new-bright' => 'rgba(151, 234, 151, .6)',
'new-background-strong' => 'rgba(151, 234, 151, 1)',
'old-background' => 'rgba(251, 175, 175, .3)',
'old-bright' => 'rgba(251, 175, 175, .7)',
'old-background-strong' => 'rgba(251, 175, 175, 1)',
'move-background' => '#fdf5d4',
'copy-background' => '#f1c40f',

View file

@ -1,2 +1,2 @@
O 1 - -=[-Rocket-Ship>\n~
N 1 + {( )}-=[-Rocket-Ship>\n~
N 1 + {> )}-=[-Rocket-Ship>\n~

View file

@ -1,2 +1,2 @@
O 1 - -=[-Rocket-Ship>\n~
N 1 + {( )}-=[-Rocket-Ship>\n~
N 1 + {> )}-=[-Rocket-Ship>\n~

View file

@ -8,6 +8,7 @@ final class DifferentialChangesetParser extends Phobject {
protected $new = array();
protected $old = array();
protected $intra = array();
protected $depthOnlyLines = array();
protected $newRender = null;
protected $oldRender = null;
@ -190,7 +191,7 @@ final class DifferentialChangesetParser extends Phobject {
return $this;
}
const CACHE_VERSION = 11;
const CACHE_VERSION = 12;
const CACHE_MAX_SIZE = 8e6;
const ATTR_GENERATED = 'attr:generated';
@ -224,6 +225,15 @@ final class DifferentialChangesetParser extends Phobject {
return $this;
}
public function setDepthOnlyLines(array $lines) {
$this->depthOnlyLines = $lines;
return $this;
}
public function getDepthOnlyLines() {
return $this->depthOnlyLines;
}
public function setVisibileLinesMask(array $mask) {
$this->visible = $mask;
return $this;
@ -450,6 +460,7 @@ final class DifferentialChangesetParser extends Phobject {
'new',
'old',
'intra',
'depthOnlyLines',
'newRender',
'oldRender',
'specialAttributes',
@ -754,6 +765,7 @@ final class DifferentialChangesetParser extends Phobject {
$this->setOldLines($hunk_parser->getOldLines());
$this->setNewLines($hunk_parser->getNewLines());
$this->setIntraLineDiffs($hunk_parser->getIntraLineDiffs());
$this->setDepthOnlyLines($hunk_parser->getDepthOnlyLines());
$this->setVisibileLinesMask($hunk_parser->getVisibleLinesMask());
$this->hunkStartLines = $hunk_parser->getHunkStartLines(
$changeset->getHunks());
@ -914,7 +926,8 @@ final class DifferentialChangesetParser extends Phobject {
->setShowEditAndReplyLinks($this->getShowEditAndReplyLinks())
->setCanMarkDone($this->getCanMarkDone())
->setObjectOwnerPHID($this->getObjectOwnerPHID())
->setHighlightingDisabled($this->highlightingDisabled);
->setHighlightingDisabled($this->highlightingDisabled)
->setDepthOnlyLines($this->getDepthOnlyLines());
$shield = null;
if ($this->isTopLevel && !$this->comments) {

View file

@ -5,6 +5,7 @@ final class DifferentialHunkParser extends Phobject {
private $oldLines;
private $newLines;
private $intraLineDiffs;
private $depthOnlyLines;
private $visibleLinesMask;
private $whitespaceMode;
@ -115,6 +116,14 @@ final class DifferentialHunkParser extends Phobject {
return $this;
}
public function setDepthOnlyLines(array $map) {
$this->depthOnlyLines = $map;
return $this;
}
public function getDepthOnlyLines() {
return $this->depthOnlyLines;
}
public function setWhitespaceMode($white_space_mode) {
$this->whitespaceMode = $white_space_mode;
@ -334,6 +343,7 @@ final class DifferentialHunkParser extends Phobject {
$new = $this->getNewLines();
$diffs = array();
$depth_only = array();
foreach ($old as $key => $o) {
$n = $new[$key];
@ -342,13 +352,75 @@ final class DifferentialHunkParser extends Phobject {
}
if ($o['type'] != $n['type']) {
$diffs[$key] = ArcanistDiffUtils::generateIntralineDiff(
$o['text'],
$n['text']);
$o_segments = array();
$n_segments = array();
$tab_width = 2;
$o_text = $o['text'];
$n_text = $n['text'];
if ($o_text !== $n_text) {
$o_depth = $this->getIndentDepth($o_text, $tab_width);
$n_depth = $this->getIndentDepth($n_text, $tab_width);
if ($o_depth < $n_depth) {
$segment_type = '>';
$segment_width = $this->getCharacterCountForVisualWhitespace(
$n_text,
($n_depth - $o_depth),
$tab_width);
if ($segment_width) {
$n_text = substr($n_text, $segment_width);
$n_segments[] = array(
$segment_type,
$segment_width,
);
}
} else if ($o_depth > $n_depth) {
$segment_type = '<';
$segment_width = $this->getCharacterCountForVisualWhitespace(
$o_text,
($o_depth - $n_depth),
$tab_width);
if ($segment_width) {
$o_text = substr($o_text, $segment_width);
$o_segments[] = array(
$segment_type,
$segment_width,
);
}
}
// If there are no remaining changes to this line after we've marked
// off the indent depth changes, this line was only modified by
// changing the indent depth. Mark it for later so we can change how
// it is displayed.
if ($o_text === $n_text) {
$depth_only[$key] = $segment_type;
}
}
$intraline_segments = ArcanistDiffUtils::generateIntralineDiff(
$o_text,
$n_text);
foreach ($intraline_segments[0] as $o_segment) {
$o_segments[] = $o_segment;
}
foreach ($intraline_segments[1] as $n_segment) {
$n_segments[] = $n_segment;
}
$diffs[$key] = array(
$o_segments,
$n_segments,
);
}
}
$this->setIntraLineDiffs($diffs);
$this->setDepthOnlyLines($depth_only);
return $this;
}
@ -671,4 +743,97 @@ final class DifferentialHunkParser extends Phobject {
return $offsets;
}
private function getIndentDepth($text, $tab_width) {
$len = strlen($text);
$depth = 0;
for ($ii = 0; $ii < $len; $ii++) {
$c = $text[$ii];
// If this is a space, increase the indent depth by 1.
if ($c == ' ') {
$depth++;
continue;
}
// If this is a tab, increase the indent depth to the next tabstop.
// For example, if the tab width is 4, these sequences both lead us to
// a visual width of 8, i.e. the cursor will be in the 8th column:
//
// <tab><tab>
// <space><tab><space><space><space><tab>
if ($c == "\t") {
$depth = ($depth + $tab_width);
$depth = $depth - ($depth % $tab_width);
continue;
}
break;
}
return $depth;
}
private function getCharacterCountForVisualWhitespace(
$text,
$depth,
$tab_width) {
// Here, we know the visual indent depth of a line has been increased by
// some amount (for example, 6 characters).
// We want to find the largest whitespace prefix of the string we can
// which still fits into that amount of visual space.
// In most cases, this is very easy. For example, if the string has been
// indented by two characters and the string begins with two spaces, that's
// a perfect match.
// However, if the string has been indented by 7 characters, the tab width
// is 8, and the string begins with "<space><space><tab>", we can only
// mark the two spaces as an indent change. These cases are unusual.
$character_depth = 0;
$visual_depth = 0;
$len = strlen($text);
for ($ii = 0; $ii < $len; $ii++) {
if ($visual_depth >= $depth) {
break;
}
$c = $text[$ii];
if ($c == ' ') {
$character_depth++;
$visual_depth++;
continue;
}
if ($c == "\t") {
// Figure out how many visual spaces we have until the next tabstop.
$tab_visual = ($visual_depth + $tab_width);
$tab_visual = $tab_visual - ($tab_visual % $tab_width);
$tab_visual = ($tab_visual - $visual_depth);
// If this tab would take us over the limit, we're all done.
$remaining_depth = ($depth - $visual_depth);
if ($remaining_depth < $tab_visual) {
break;
}
$character_depth++;
$visual_depth += $tab_visual;
continue;
}
break;
}
return $character_depth;
}
}

View file

@ -34,6 +34,7 @@ abstract class DifferentialChangesetRenderer extends Phobject {
private $objectOwnerPHID;
private $highlightingDisabled;
private $scopeEngine;
private $depthOnlyLines;
private $oldFile = false;
private $newFile = false;
@ -92,6 +93,15 @@ abstract class DifferentialChangesetRenderer extends Phobject {
return $this->gaps;
}
public function setDepthOnlyLines(array $lines) {
$this->depthOnlyLines = $lines;
return $this;
}
public function getDepthOnlyLines() {
return $this->depthOnlyLines;
}
public function attachOldFile(PhabricatorFile $old = null) {
$this->oldFile = $old;
return $this;

View file

@ -96,10 +96,14 @@ abstract class DifferentialChangesetTestRenderer
array(
'<span class="bright">',
'</span>',
'<span class="depth-out">',
'<span class="depth-in">',
),
array(
'{(',
')}',
'{<',
'{>',
),
$render);

View file

@ -71,8 +71,8 @@ final class DifferentialChangesetTwoUpRenderer
$mask = $this->getMask();
$scope_engine = $this->getScopeEngine();
$offset_map = null;
$depth_only = $this->getDepthOnlyLines();
for ($ii = $range_start; $ii < $range_start + $range_len; $ii++) {
if (empty($mask[$ii])) {
@ -196,11 +196,29 @@ final class DifferentialChangesetTwoUpRenderer
} else if (empty($old_lines[$ii])) {
$n_class = 'new new-full';
} else {
$n_class = 'new';
// NOTE: At least for the moment, I'm intentionally clearing the
// line highlighting only on the right side of the diff when a
// line has only depth changes. When a block depth is decreased,
// this gives us a large color block on the left (to make it easy
// to see the depth change) but a clean diff on the right (to make
// it easy to pick out actual code changes).
if (isset($depth_only[$ii])) {
$n_class = '';
} else {
$n_class = 'new';
}
}
$n_classes = $n_class;
if ($new_lines[$ii]['type'] == '\\' || !isset($copy_lines[$n_num])) {
$not_copied =
// If this line only changed depth, copy markers are pointless.
(!isset($copy_lines[$n_num])) ||
(isset($depth_only[$ii])) ||
($new_lines[$ii]['type'] == '\\');
if ($not_copied) {
$n_copy = phutil_tag('td', array('class' => "copy {$n_class}"));
} else {
list($orig_file, $orig_line, $orig_type) = $copy_lines[$n_num];

View file

@ -135,12 +135,33 @@
background: {$old-bright};
}
.differential-diff td.new span.bright,
.differential-diff td.new-full,
.prose-diff span.new {
background: {$new-bright};
}
.differential-diff td span.depth-out,
.differential-diff td span.depth-in {
padding: 2px 0;
background-size: 12px 12px;
background-repeat: no-repeat;
background-position: left center;
}
.differential-diff td span.depth-out {
background-image: url(/rsrc/image/chevron-out.png);
background-color: {$old-background-strong};
}
.differential-diff td span.depth-in {
background-position: 2px center;
background-image: url(/rsrc/image/chevron-in.png);
background-color: {$new-background-strong};
}
.differential-diff td.copy {
min-width: 0.5%;
width: 0.5%;

Binary file not shown.

After

Width:  |  Height:  |  Size: 1.4 KiB

Binary file not shown.

After

Width:  |  Height:  |  Size: 1.4 KiB