From 8ff0e3ab351f5e806c1a9e072695e4f905604913 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 25 Oct 2019 11:36:43 -0700 Subject: [PATCH] Support rich diff rendering with DocumentEngine for added/removed files Summary: Ref T13425. When a file (like a Jupyter notebook) is added or removed, we can still render a useful line-by-line diff. Test Plan: - Viewed add/modify/remove of Jupyter, source code, and images in 2up/1up mode, everything looked okay. Maniphest Tasks: T13425 Differential Revision: https://secure.phabricator.com/D20865 --- .../parser/DifferentialChangesetParser.php | 104 ++++++++++++------ .../DifferentialChangesetOneUpRenderer.php | 10 ++ .../diff/PhabricatorDocumentEngineBlocks.php | 5 +- .../document/PhabricatorDocumentEngine.php | 4 +- .../PhabricatorImageDocumentEngine.php | 33 ++++-- .../PhabricatorJupyterDocumentEngine.php | 21 +++- 6 files changed, 124 insertions(+), 53 deletions(-) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 6c4ed3d14f..571405f64b 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1686,47 +1686,79 @@ final class DifferentialChangesetParser extends Phobject { break; } - $old_ref = id(new PhabricatorDocumentRef()) - ->setName($changeset->getOldFile()); - if ($old_file) { - $old_ref->setFile($old_file); + $type_delete = DifferentialChangeType::TYPE_DELETE; + $type_add = DifferentialChangeType::TYPE_ADD; + $change_type = $changeset->getChangeType(); + + $no_old = ($change_type == $type_add); + $no_new = ($change_type == $type_delete); + + if ($no_old) { + $old_ref = null; } else { - $old_data = $this->old; - $old_data = ipull($old_data, 'text'); - $old_data = implode('', $old_data); + $old_ref = id(new PhabricatorDocumentRef()) + ->setName($changeset->getOldFile()); + if ($old_file) { + $old_ref->setFile($old_file); + } else { + $old_data = $this->old; + $old_data = ipull($old_data, 'text'); + $old_data = implode('', $old_data); - $old_ref->setData($old_data); - } - - $new_ref = id(new PhabricatorDocumentRef()) - ->setName($changeset->getFilename()); - if ($new_file) { - $new_ref->setFile($new_file); - } else { - $new_data = $this->new; - $new_data = ipull($new_data, 'text'); - $new_data = implode('', $new_data); - - $new_ref->setData($new_data); - } - - $old_engines = PhabricatorDocumentEngine::getEnginesForRef( - $viewer, - $old_ref); - - $new_engines = PhabricatorDocumentEngine::getEnginesForRef( - $viewer, - $new_ref); - - $shared_engines = array_intersect_key($new_engines, $old_engines); - $default_engine = head_key($new_engines); - - foreach ($shared_engines as $key => $shared_engine) { - if (!$shared_engine->canDiffDocuments($old_ref, $new_ref)) { - unset($shared_engines[$key]); + $old_ref->setData($old_data); } } + if ($no_new) { + $new_ref = null; + } else { + $new_ref = id(new PhabricatorDocumentRef()) + ->setName($changeset->getFilename()); + if ($new_file) { + $new_ref->setFile($new_file); + } else { + $new_data = $this->new; + $new_data = ipull($new_data, 'text'); + $new_data = implode('', $new_data); + + $new_ref->setData($new_data); + } + } + + + $old_engines = null; + if ($old_ref) { + $old_engines = PhabricatorDocumentEngine::getEnginesForRef( + $viewer, + $old_ref); + } + + $new_engines = null; + if ($new_ref) { + $new_engines = PhabricatorDocumentEngine::getEnginesForRef( + $viewer, + $new_ref); + } + + if ($new_engines !== null && $old_engines !== null) { + $shared_engines = array_intersect_key($new_engines, $old_engines); + $default_engine = head_key($new_engines); + + foreach ($shared_engines as $key => $shared_engine) { + if (!$shared_engine->canDiffDocuments($old_ref, $new_ref)) { + unset($shared_engines[$key]); + } + } + } else if ($new_engines !== null) { + $shared_engines = $new_engines; + $default_engine = head_key($shared_engines); + } else if ($old_engines !== null) { + $shared_engines = $old_engines; + $default_engine = head_key($shared_engines); + } else { + return null; + } + $engine_key = $this->getDocumentEngineKey(); if (strlen($engine_key)) { if (isset($shared_engines[$engine_key])) { diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index fc50b0d605..19c939274d 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -371,17 +371,27 @@ final class DifferentialChangesetOneUpRenderer $cell_classes = $block_diff->getNewClasses(); } } else if ($row_type === 'old') { + if (!$old_ref) { + continue; + } + $cell_content = $engine->newBlockContentView( $old_ref, $old); + $cell_classes[] = 'old'; $cell_classes[] = 'old-full'; $new_key = null; } else if ($row_type === 'new') { + if (!$new_ref) { + continue; + } + $cell_content = $engine->newBlockContentView( $new_ref, $new); + $cell_classes[] = 'new'; $cell_classes[] = 'new-full'; diff --git a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php index f8f3a414b1..47ddf194ee 100644 --- a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php +++ b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php @@ -15,7 +15,10 @@ final class PhabricatorDocumentEngineBlocks return $this->messages; } - public function addBlockList(PhabricatorDocumentRef $ref, array $blocks) { + public function addBlockList( + PhabricatorDocumentRef $ref = null, + array $blocks = array()) { + assert_instances_of($blocks, 'PhabricatorDocumentEngineBlock'); $this->lists[] = array( diff --git a/src/applications/files/document/PhabricatorDocumentEngine.php b/src/applications/files/document/PhabricatorDocumentEngine.php index bc7960b4ad..9593e1d98d 100644 --- a/src/applications/files/document/PhabricatorDocumentEngine.php +++ b/src/applications/files/document/PhabricatorDocumentEngine.php @@ -32,8 +32,8 @@ abstract class PhabricatorDocumentEngine } public function canDiffDocuments( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { return false; } diff --git a/src/applications/files/document/PhabricatorImageDocumentEngine.php b/src/applications/files/document/PhabricatorImageDocumentEngine.php index fa678cc034..449d604370 100644 --- a/src/applications/files/document/PhabricatorImageDocumentEngine.php +++ b/src/applications/files/document/PhabricatorImageDocumentEngine.php @@ -18,21 +18,38 @@ final class PhabricatorImageDocumentEngine } public function canDiffDocuments( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { - // For now, we can only render a rich image diff if both documents have + // For now, we can only render a rich image diff if the documents have // their data stored in Files already. - return ($uref->getFile() && $vref->getFile()); + if ($uref && !$uref->getFile()) { + return false; + } + + if ($vref && !$vref->getFile()) { + return false; + } + + return true; } public function newEngineBlocks( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { - $u_blocks = $this->newDiffBlocks($uref); - $v_blocks = $this->newDiffBlocks($vref); + if ($uref) { + $u_blocks = $this->newDiffBlocks($uref); + } else { + $u_blocks = array(); + } + + if ($vref) { + $v_blocks = $this->newDiffBlocks($vref); + } else { + $v_blocks = array(); + } return id(new PhabricatorDocumentEngineBlocks()) ->addBlockList($uref, $u_blocks) diff --git a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php index d9d4c43abb..0c3e891c3c 100644 --- a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php +++ b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php @@ -36,20 +36,29 @@ final class PhabricatorJupyterDocumentEngine } public function canDiffDocuments( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { return true; } public function newEngineBlocks( - PhabricatorDocumentRef $uref, - PhabricatorDocumentRef $vref) { + PhabricatorDocumentRef $uref = null, + PhabricatorDocumentRef $vref = null) { $blocks = new PhabricatorDocumentEngineBlocks(); try { - $u_blocks = $this->newDiffBlocks($uref); - $v_blocks = $this->newDiffBlocks($vref); + if ($uref) { + $u_blocks = $this->newDiffBlocks($uref); + } else { + $u_blocks = array(); + } + + if ($vref) { + $v_blocks = $this->newDiffBlocks($vref); + } else { + $v_blocks = array(); + } $blocks->addBlockList($uref, $u_blocks); $blocks->addBlockList($vref, $v_blocks);