1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-22 14:52:41 +01:00

Update the diff table of contents to use hierarchical views and edit distance renames

Summary:
Ref T13520. Generally, make the table of contents look and more like the paths panel:

  - Show a hierarchy, with compression for single-sibling children.
  - Use the same icons, instead of "M D" and "(img)" stuff.
  - Use EditDistanceMatrix to do a piece-by-piece diff of paths changes.
  - Show path changes within the path list.

I'm not entirely sold on this, but it was complicated to write and I've never heard the term "sunk cost fallacy". I think this is mostly a net improvement, but may need some adjustments and followup.

Test Plan: Viewed various changes in Differential and Diffusion, saw a more usable table of contents.

Maniphest Tasks: T13520

Differential Revision: https://secure.phabricator.com/D21183
This commit is contained in:
epriestley 2020-04-28 08:58:17 -07:00
parent a7b2327c34
commit f21f1d8ab9
6 changed files with 474 additions and 210 deletions

View file

@ -12,7 +12,7 @@ return array(
'core.pkg.css' => '589cd2fe', 'core.pkg.css' => '589cd2fe',
'core.pkg.js' => '49814bac', 'core.pkg.js' => '49814bac',
'dark-console.pkg.js' => '187792c2', 'dark-console.pkg.js' => '187792c2',
'differential.pkg.css' => '8e1a7922', 'differential.pkg.css' => '73cf6fb1',
'differential.pkg.js' => 'c8f88d74', 'differential.pkg.js' => 'c8f88d74',
'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.css' => '42c75c37',
'diffusion.pkg.js' => 'a98c0bf7', 'diffusion.pkg.js' => 'a98c0bf7',
@ -69,7 +69,7 @@ return array(
'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
'rsrc/css/application/differential/revision-history.css' => '8aa3eac5', 'rsrc/css/application/differential/revision-history.css' => '8aa3eac5',
'rsrc/css/application/differential/revision-list.css' => '93d2df7d', 'rsrc/css/application/differential/revision-list.css' => '93d2df7d',
'rsrc/css/application/differential/table-of-contents.css' => '0e3364c7', 'rsrc/css/application/differential/table-of-contents.css' => 'e7f8c50e',
'rsrc/css/application/diffusion/diffusion-icons.css' => '23b31a1b', 'rsrc/css/application/diffusion/diffusion-icons.css' => '23b31a1b',
'rsrc/css/application/diffusion/diffusion-readme.css' => 'b68a76e4', 'rsrc/css/application/diffusion/diffusion-readme.css' => 'b68a76e4',
'rsrc/css/application/diffusion/diffusion-repository.css' => 'b89e8c6c', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'b89e8c6c',
@ -566,7 +566,7 @@ return array(
'differential-revision-comment-css' => '7dbc8d1d', 'differential-revision-comment-css' => '7dbc8d1d',
'differential-revision-history-css' => '8aa3eac5', 'differential-revision-history-css' => '8aa3eac5',
'differential-revision-list-css' => '93d2df7d', 'differential-revision-list-css' => '93d2df7d',
'differential-table-of-contents-css' => '0e3364c7', 'differential-table-of-contents-css' => 'e7f8c50e',
'diffusion-css' => 'b54c77b0', 'diffusion-css' => 'b54c77b0',
'diffusion-icons-css' => '23b31a1b', 'diffusion-icons-css' => '23b31a1b',
'diffusion-readme-css' => 'b68a76e4', 'diffusion-readme-css' => 'b68a76e4',

View file

@ -22,50 +22,6 @@ final class DifferentialChangeType extends Phobject {
const FILE_NORMAL = 7; const FILE_NORMAL = 7;
const FILE_SUBMODULE = 8; const FILE_SUBMODULE = 8;
public static function getSummaryCharacterForChangeType($type) {
static $types = array(
self::TYPE_ADD => 'A',
self::TYPE_CHANGE => 'M',
self::TYPE_DELETE => 'D',
self::TYPE_MOVE_AWAY => 'V',
self::TYPE_COPY_AWAY => 'P',
self::TYPE_MOVE_HERE => 'V',
self::TYPE_COPY_HERE => 'P',
self::TYPE_MULTICOPY => 'P',
self::TYPE_MESSAGE => 'Q',
self::TYPE_CHILD => '@',
);
return idx($types, coalesce($type, '?'), '~');
}
public static function getSummaryColorForChangeType($type) {
static $types = array(
self::TYPE_ADD => 'green',
self::TYPE_CHANGE => 'black',
self::TYPE_DELETE => 'red',
self::TYPE_MOVE_AWAY => 'orange',
self::TYPE_COPY_AWAY => 'black',
self::TYPE_MOVE_HERE => 'green',
self::TYPE_COPY_HERE => 'green',
self::TYPE_MULTICOPY => 'orange',
self::TYPE_MESSAGE => 'black',
self::TYPE_CHILD => 'black',
);
return idx($types, coalesce($type, '?'), 'black');
}
public static function getShortNameForFileType($type) {
static $names = array(
self::FILE_TEXT => null,
self::FILE_DIRECTORY => 'dir',
self::FILE_IMAGE => 'img',
self::FILE_BINARY => 'bin',
self::FILE_SYMLINK => 'sym',
self::FILE_SUBMODULE => 'sub',
);
return idx($names, coalesce($type, '?'), '???');
}
public static function getIconForFileType($type) { public static function getIconForFileType($type) {
static $icons = array( static $icons = array(
self::FILE_TEXT => 'fa-file-text-o', self::FILE_TEXT => 'fa-file-text-o',

View file

@ -315,6 +315,30 @@ final class DifferentialChangeset
return $this->assertAttached($this->diff); return $this->assertAttached($this->diff);
} }
public function getOldStatePathVector() {
$path = $this->getOldFile();
if (!strlen($path)) {
$path = $this->getFilename();
}
$path = trim($path, '/');
$path = explode('/', $path);
return $path;
}
public function getNewStatePathVector() {
if (!$this->hasNewState()) {
return null;
}
$path = $this->getFilename();
$path = trim($path, '/');
$path = explode('/', $path);
return $path;
}
public function newFileTreeIcon() { public function newFileTreeIcon() {
$icon = $this->getPathIconIcon(); $icon = $this->getPathIconIcon();
$color = $this->getPathIconColor(); $color = $this->getPathIconColor();
@ -335,16 +359,8 @@ final class DifferentialChangeset
} }
public function getIsLowImportanceChangeset() { public function getIsLowImportanceChangeset() {
$change_type = $this->getChangeType(); if (!$this->hasNewState()) {
return true;
$change_map = array(
DifferentialChangeType::TYPE_DELETE => true,
DifferentialChangeType::TYPE_MOVE_AWAY => true,
DifferentialChangeType::TYPE_MULTICOPY => true,
);
if (isset($change_map[$change_type])) {
return $change_map[$change_type];
} }
if ($this->isGeneratedChangeset()) { if ($this->isGeneratedChangeset()) {

View file

@ -81,9 +81,7 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView {
$cells[] = $this->getContext(); $cells[] = $this->getContext();
$cells[] = $this->renderPathChangeCharacter(); $cells[] = $changeset->newFileTreeIcon();
$cells[] = $this->renderPropertyChangeCharacter();
$cells[] = $this->renderPropertyChangeDescription();
$link = $this->renderChangesetLink(); $link = $this->renderChangesetLink();
$lines = $this->renderChangesetLines(); $lines = $this->renderChangesetLines();
@ -103,82 +101,12 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView {
return $cells; return $cells;
} }
private function renderPathChangeCharacter() { public function newLink() {
$changeset = $this->getChangeset();
$type = $changeset->getChangeType();
$color = DifferentialChangeType::getSummaryColorForChangeType($type);
$char = DifferentialChangeType::getSummaryCharacterForChangeType($type);
$title = DifferentialChangeType::getFullNameForChangeType($type);
return javelin_tag(
'span',
array(
'sigil' => 'has-tooltip',
'meta' => array(
'tip' => $title,
'align' => 'E',
),
'class' => 'phui-text-'.$color,
),
$char);
}
private function renderPropertyChangeCharacter() {
$changeset = $this->getChangeset();
$old = $changeset->getOldProperties();
$new = $changeset->getNewProperties();
if ($old === $new) {
return null;
}
return javelin_tag(
'span',
array(
'sigil' => 'has-tooltip',
'meta' => array(
'tip' => pht('Properties Modified'),
'align' => 'E',
'size' => 200,
),
),
'M');
}
private function renderPropertyChangeDescription() {
$changeset = $this->getChangeset();
$file_type = $changeset->getFileType();
$desc = DifferentialChangeType::getShortNameForFileType($file_type);
if ($desc === null) {
return null;
}
return pht('(%s)', $desc);
}
private function renderChangesetLink() {
$anchor = $this->getAnchor(); $anchor = $this->getAnchor();
$changeset = $this->getChangeset(); $changeset = $this->getChangeset();
$name = $changeset->getDisplayFilename(); $name = $changeset->getDisplayFilename();
$name = basename($name);
$change_type = $changeset->getChangeType();
if (DifferentialChangeType::isOldLocationChangeType($change_type)) {
$away = $changeset->getAwayPaths();
if (count($away) == 1) {
if ($change_type == DifferentialChangeType::TYPE_MOVE_AWAY) {
$right_arrow = "\xE2\x86\x92";
$name = $this->renderRename($name, head($away), $right_arrow);
}
}
} else if ($change_type == DifferentialChangeType::TYPE_MOVE_HERE) {
$left_arrow = "\xE2\x86\x90";
$name = $this->renderRename($name, $changeset->getOldFile(), $left_arrow);
}
return javelin_tag( return javelin_tag(
'a', 'a',
@ -192,18 +120,22 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView {
$name); $name);
} }
private function renderChangesetLines() { public function renderChangesetLines() {
$changeset = $this->getChangeset(); $changeset = $this->getChangeset();
if ($changeset->getIsLowImportanceChangeset()) {
return null;
}
$line_count = $changeset->getAffectedLineCount(); $line_count = $changeset->getAffectedLineCount();
if (!$line_count) { if (!$line_count) {
return null; return null;
} }
return ' '.pht('(%d line(s))', $line_count); return pht('%d line(s)', $line_count);
} }
private function renderCoverage() { public function renderCoverage() {
$not_applicable = '-'; $not_applicable = '-';
$coverage = $this->getCoverage(); $coverage = $this->getCoverage();
@ -221,7 +153,7 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView {
return sprintf('%d%%', 100 * ($covered / ($covered + $not_covered))); return sprintf('%d%%', 100 * ($covered / ($covered + $not_covered)));
} }
private function renderModifiedCoverage() { public function renderModifiedCoverage() {
$not_applicable = '-'; $not_applicable = '-';
$coverage = $this->getCoverage(); $coverage = $this->getCoverage();
@ -285,50 +217,18 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView {
$meta); $meta);
} }
private function renderPackages() { public function renderPackages() {
$packages = $this->getPackages(); $packages = $this->getPackages();
if (!$packages) { if (!$packages) {
return null; return null;
} }
$viewer = $this->getUser(); $viewer = $this->getViewer();
$package_phids = mpull($packages, 'getPHID'); $package_phids = mpull($packages, 'getPHID');
return $viewer->renderHandleList($package_phids) return $viewer->renderHandleList($package_phids)
->setGlyphLimit(48); ->setGlyphLimit(48);
} }
private function renderRename($self, $other, $arrow) {
$old = explode('/', $self);
$new = explode('/', $other);
$start = count($old);
foreach ($old as $index => $part) {
if (!isset($new[$index]) || $part != $new[$index]) {
$start = $index;
break;
}
}
$end = count($old);
foreach (array_reverse($old) as $from_end => $part) {
$index = count($new) - $from_end - 1;
if (!isset($new[$index]) || $part != $new[$index]) {
$end = $from_end;
break;
}
}
$rename =
'{'.
implode('/', array_slice($old, $start, count($old) - $end - $start)).
' '.$arrow.' '.
implode('/', array_slice($new, $start, count($new) - $end - $start)).
'}';
array_splice($new, $start, count($new) - $end - $start, $rename);
return implode('/', $new);
}
} }

View file

@ -9,6 +9,8 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
private $background; private $background;
private $bare; private $bare;
private $components = array();
public function addItem(PHUIDiffTableOfContentsItemView $item) { public function addItem(PHUIDiffTableOfContentsItemView $item) {
$this->items[] = $item; $this->items[] = $item;
return $this; return $this;
@ -61,57 +63,179 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
} }
$items = $this->items; $items = $this->items;
$viewer = $this->getViewer();
$item_map = array();
$vector_tree = new ArcanistDiffVectorTree();
foreach ($items as $item) {
$item->setViewer($viewer);
$changeset = $item->getChangeset();
$old_vector = $changeset->getOldStatePathVector();
$new_vector = $changeset->getNewStatePathVector();
$tree_vector = $this->newTreeVector($old_vector, $new_vector);
$item_map[implode("\n", $tree_vector)] = $item;
$vector_tree->addVector($tree_vector);
}
$node_list = $vector_tree->newDisplayList();
$node_map = array();
foreach ($node_list as $node) {
$path_vector = $node->getVector();
$path_vector = implode("\n", $path_vector);
$node_map[$path_vector] = $node;
}
// Mark all nodes which contain at least one path which exists in the new
// state. Nodes we don't mark contain only deleted or moved files, so they
// can be rendered with a less-prominent style.
foreach ($node_map as $node_key => $node) {
$item = idx($item_map, $node_key);
if (!$item) {
continue;
}
$changeset = $item->getChangeset();
if (!$changeset->getIsLowImportanceChangeset()) {
$node->setAncestralAttribute('important', true);
}
}
$any_packages = false;
$any_coverage = false;
$any_context = false;
$rows = array(); $rows = array();
$rowc = array(); $rowc = array();
foreach ($items as $item) { foreach ($node_map as $node_key => $node) {
$item->setUser($this->getUser()); $display_vector = $node->getDisplayVector();
$rows[] = $item->render(); $item = idx($item_map, $node_key);
if ($item) {
$changeset = $item->getChangeset();
$icon = $changeset->newFileTreeIcon();
} else {
$changeset = null;
$icon = id(new PHUIIconView())
->setIcon('fa-folder-open-o grey');
}
if ($node->getChildren()) {
$old_dir = true;
$new_dir = true;
} else {
// TODO: When properties are set on a directory in SVN directly, this
// might be incorrect.
$old_dir = false;
$new_dir = false;
}
$display_view = $this->newComponentView(
$icon,
$display_vector,
$old_dir,
$new_dir,
$item);
$depth = $node->getDisplayDepth();
$style = sprintf('padding-left: %dpx;', $depth * 16);
if ($item) {
$packages = $item->renderPackages();
} else {
$packages = null;
}
if ($packages) {
$any_packages = true;
}
if ($item) {
if ($item->getCoverage()) {
$any_coverage = true;
}
$coverage = $item->renderCoverage();
$modified_coverage = $item->renderModifiedCoverage();
} else {
$coverage = null;
$modified_coverage = null;
}
if ($item) {
$context = $item->getContext();
if ($context) {
$any_context = true;
}
} else {
$context = null;
}
if ($item) {
$lines = $item->renderChangesetLines();
} else {
$lines = null;
}
$rows[] = array(
$context,
phutil_tag(
'div',
array(
'style' => $style,
),
$display_view),
$lines,
$coverage,
$modified_coverage,
$packages,
);
$classes = array();
$have_authority = false; $have_authority = false;
$packages = $item->getPackages(); if ($item) {
if ($packages) { $packages = $item->getPackages();
if (array_intersect_key($packages, $authority)) { if ($packages) {
$have_authority = true; if (array_intersect_key($packages, $authority)) {
$have_authority = true;
}
} }
} }
if ($have_authority) { if ($have_authority) {
$rowc[] = 'highlighted'; $classes[] = 'highlighted';
}
if (!$node->getAttribute('important')) {
$classes[] = 'diff-toc-low-importance-row';
}
if ($changeset) {
$classes[] = 'diff-toc-changeset-row';
} else { } else {
$rowc[] = null; $classes[] = 'diff-toc-no-changeset-row';
}
}
// Check if any item has content in these columns. If no item does, we'll
// just hide them.
$any_coverage = false;
$any_context = false;
$any_packages = false;
foreach ($items as $item) {
if ($item->getContext() !== null) {
$any_context = true;
} }
if (strlen($item->getCoverage())) { $rowc[] = implode(' ', $classes);
$any_coverage = true;
}
if ($item->getPackages() !== null) {
$any_packages = true;
}
} }
$table = id(new AphrontTableView($rows)) $table = id(new AphrontTableView($rows))
->setRowClasses($rowc) ->setRowClasses($rowc)
->setClassName('aphront-table-view-compact')
->setHeaders( ->setHeaders(
array( array(
null,
null,
null,
null, null,
pht('Path'), pht('Path'),
pht('Size'),
pht('Coverage (All)'), pht('Coverage (All)'),
pht('Coverage (Touched)'), pht('Coverage (Touched)'),
pht('Packages'), pht('Packages'),
@ -119,10 +243,8 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
->setColumnClasses( ->setColumnClasses(
array( array(
null, null,
'differential-toc-char center', 'diff-toc-path wide',
'differential-toc-prop center', 'right',
'differential-toc-ftype center',
'differential-toc-file wide',
'differential-toc-cov', 'differential-toc-cov',
'differential-toc-cov', 'differential-toc-cov',
null, null,
@ -132,8 +254,6 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
$any_context, $any_context,
true, true,
true, true,
true,
true,
$any_coverage, $any_coverage,
$any_coverage, $any_coverage,
$any_packages, $any_packages,
@ -142,9 +262,7 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
array( array(
true, true,
true, true,
true, false,
true,
true,
false, false,
false, false,
true, true,
@ -174,7 +292,240 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
if ($this->infoView) { if ($this->infoView) {
$box->setInfoView($this->infoView); $box->setInfoView($this->infoView);
} }
return $box; return $box;
} }
private function newTreeVector($old, $new) {
if ($old === null && $new === null) {
throw new Exception(pht('Changeset has no path vectors!'));
}
$vector = null;
if ($old === null) {
$vector = $new;
} else if ($new === null) {
$vector = $old;
} else if ($old === $new) {
$vector = $new;
}
if ($vector) {
foreach ($vector as $k => $v) {
$vector[$k] = $this->newScalarComponent($v);
}
return $vector;
}
$matrix = id(new PhutilEditDistanceMatrix())
->setSequences($old, $new)
->setComputeString(true);
$edits = $matrix->getEditString();
// If the edit sequence contains deletions followed by edits, move
// the deletions to the end to left-align the new path.
$edits = preg_replace('/(d+)(x+)/', '\2\1', $edits);
$vector = array();
$length = strlen($edits);
$old_cursor = 0;
$new_cursor = 0;
for ($ii = 0; $ii < strlen($edits); $ii++) {
$c = $edits[$ii];
switch ($c) {
case 'i':
$vector[] = $this->newPairComponent(null, $new[$new_cursor]);
$new_cursor++;
break;
case 'd':
$vector[] = $this->newPairComponent($old[$old_cursor], null);
$old_cursor++;
break;
case 's':
case 'x':
case 't':
$vector[] = $this->newPairComponent(
$old[$old_cursor],
$new[$new_cursor]);
$old_cursor++;
$new_cursor++;
break;
default:
throw new Exception(pht('Unknown edit string "%s"!', $c));
}
}
return $vector;
}
private function newScalarComponent($v) {
$key = sprintf('path(%s)', $v);
if (!isset($this->components[$key])) {
$this->components[$key] = $v;
}
return $key;
}
private function newPairComponent($u, $v) {
if ($u === $v) {
return $this->newScalarComponent($u);
}
$key = sprintf('pair(%s > %s)', $u, $v);
if (!isset($this->components[$key])) {
$this->components[$key] = array($u, $v);
}
return $key;
}
private function newComponentView(
$icon,
array $keys,
$old_dir,
$new_dir,
$item) {
$is_simple = true;
$items = array();
foreach ($keys as $key) {
$component = $this->components[$key];
if (is_array($component)) {
$is_simple = false;
} else {
$component = array(
$component,
$component,
);
}
$items[] = $component;
}
$move_icon = id(new PHUIIconView())
->setIcon('fa-angle-double-right pink');
$old_row = array(
phutil_tag('td', array(), $move_icon),
);
$new_row = array(
phutil_tag('td', array(), $icon),
);
$last_old_key = null;
$last_new_key = null;
foreach ($items as $key => $component) {
if (!is_array($component)) {
$last_old_key = $key;
$last_new_key = $key;
} else {
if ($component[0] !== null) {
$last_old_key = $key;
}
if ($component[1] !== null) {
$last_new_key = $key;
}
}
}
foreach ($items as $key => $component) {
if (!is_array($component)) {
$old = $component;
$new = $component;
} else {
$old = $component[0];
$new = $component[1];
}
$old_classes = array();
$new_classes = array();
if ($old === $new) {
// Do nothing.
} else if ($old === null) {
$new_classes[] = 'diff-path-component-new';
} else if ($new === null) {
$old_classes[] = 'diff-path-component-old';
} else {
$old_classes[] = 'diff-path-component-old';
$new_classes[] = 'diff-path-component-new';
}
if ($old !== null) {
if (($key === $last_old_key) && !$old_dir) {
// Do nothing.
} else {
$old = $old.'/';
}
}
if ($new !== null) {
if (($key === $last_new_key) && $item) {
$new = $item->newLink();
} else if (($key === $last_new_key) && !$new_dir) {
// Do nothing.
} else {
$new = $new.'/';
}
}
$old_row[] = phutil_tag(
'td',
array(),
phutil_tag(
'div',
array(
'class' => implode(' ', $old_classes),
),
$old));
$new_row[] = phutil_tag(
'td',
array(),
phutil_tag(
'div',
array(
'class' => implode(' ', $new_classes),
),
$new));
}
$old_row = phutil_tag(
'tr',
array(
'class' => 'diff-path-old',
),
$old_row);
$new_row = phutil_tag(
'tr',
array(
'class' => 'diff-path-new',
),
$new_row);
$rows = array();
$rows[] = $new_row;
if (!$is_simple) {
$rows[] = $old_row;
}
$body = phutil_tag('tbody', array(), $rows);
$table = phutil_tag(
'table',
array(
),
$body);
return $table;
}
} }

View file

@ -88,3 +88,44 @@ table.aphront-table-view td.differential-toc-ftype {
margin: 4px 0; margin: 4px 0;
border: 1px solid {$thinblueborder}; border: 1px solid {$thinblueborder};
} }
.diff-toc-path .phui-icon-view {
margin-right: 2px;
text-align: center;
width: 20px;
display: inline-block;
}
table.aphront-table-view-compact td {
padding: 3px 8px;
}
td.diff-toc-path td {
padding: 0;
color: {$greytext};
}
.diff-toc-path div.diff-path-component-new {
padding: 0 4px;
background: {$new-bright};
margin: 0 2px;
}
.diff-toc-path div.diff-path-component-old {
padding: 0 4px;
background: {$old-bright};
margin: 0 2px;
}
.diff-toc-path a {
color: {$darkbluetext};
}
td.diff-toc-path .diff-path-old td {
color: {$lightgreytext};
}
.diff-toc-low-importance-row,
.alt-diff-toc-low-importance-row {
opacity: 0.5;
}