From bb71ef6ad6cd6a0cd8f7c24fecf52d86f288f630 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 23 Sep 2019 11:09:29 -0700 Subject: [PATCH] Render image diffs as abstract blocks diffs via DocumentEngine Summary: Depends on D20830. Ref T13425. Have the image engine elect into block rendering, then emit blocks. This is rough (the blocks aren't actually diffed yet) but image diffs were already pretty rough so this is approximately a net improvement. Test Plan: Viewed image diffs, saw nothing worse than before. Maniphest Tasks: T13425 Differential Revision: https://secure.phabricator.com/D20831 --- resources/celerity/map.php | 12 +- src/__phutil_library_map__.php | 4 + .../parser/DifferentialChangesetParser.php | 206 +++++++++++------- .../DifferentialChangesetHTMLRenderer.php | 13 -- ...DifferentialChangesetOneUpMailRenderer.php | 8 - .../DifferentialChangesetOneUpRenderer.php | 27 +-- .../render/DifferentialChangesetRenderer.php | 12 +- .../DifferentialChangesetTestRenderer.php | 10 - .../DifferentialChangesetTwoUpRenderer.php | 81 ++++--- .../diff/PhabricatorDocumentEngineBlock.php | 21 ++ .../diff/PhabricatorDocumentEngineBlocks.php | 83 +++++++ .../PhabricatorImageDocumentEngine.php | 44 ++++ .../differential/changeset-view.css | 4 +- 13 files changed, 349 insertions(+), 176 deletions(-) create mode 100644 src/applications/files/diff/PhabricatorDocumentEngineBlock.php create mode 100644 src/applications/files/diff/PhabricatorDocumentEngineBlocks.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 13a2619854..ff7b2f300f 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => 'c69171e6', 'core.pkg.js' => '6e5c894f', - 'differential.pkg.css' => '8d8360fb', + 'differential.pkg.css' => 'eef74643', 'differential.pkg.js' => '0b037a4f', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '5a205b9d', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => 'bde53589', + 'rsrc/css/application/differential/changeset-view.css' => '215129ef', 'rsrc/css/application/differential/core.css' => '7300a73e', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -554,7 +554,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => '9d068042', - 'differential-changeset-view-css' => 'bde53589', + 'differential-changeset-view-css' => '215129ef', 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -1069,6 +1069,9 @@ return array( 'javelin-behavior', 'javelin-request', ), + '215129ef' => array( + 'phui-inline-comment-view-css', + ), '225bbb98' => array( 'javelin-install', 'javelin-reactor', @@ -1960,9 +1963,6 @@ return array( 'phabricator-drag-and-drop-file-upload', 'javelin-workboard-board', ), - 'bde53589' => array( - 'phui-inline-comment-view-css', - ), 'c03f2fb4' => array( 'javelin-install', ), diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f1ba222cc2..1ea9d9f67d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3138,6 +3138,8 @@ phutil_register_library_map(array( 'PhabricatorDividerProfileMenuItem' => 'applications/search/menuitem/PhabricatorDividerProfileMenuItem.php', 'PhabricatorDivinerApplication' => 'applications/diviner/application/PhabricatorDivinerApplication.php', 'PhabricatorDocumentEngine' => 'applications/files/document/PhabricatorDocumentEngine.php', + 'PhabricatorDocumentEngineBlock' => 'applications/files/diff/PhabricatorDocumentEngineBlock.php', + 'PhabricatorDocumentEngineBlocks' => 'applications/files/diff/PhabricatorDocumentEngineBlocks.php', 'PhabricatorDocumentRef' => 'applications/files/document/PhabricatorDocumentRef.php', 'PhabricatorDocumentRenderingEngine' => 'applications/files/document/render/PhabricatorDocumentRenderingEngine.php', 'PhabricatorDoorkeeperApplication' => 'applications/doorkeeper/application/PhabricatorDoorkeeperApplication.php', @@ -9471,6 +9473,8 @@ phutil_register_library_map(array( 'PhabricatorDividerProfileMenuItem' => 'PhabricatorProfileMenuItem', 'PhabricatorDivinerApplication' => 'PhabricatorApplication', 'PhabricatorDocumentEngine' => 'Phobject', + 'PhabricatorDocumentEngineBlock' => 'Phobject', + 'PhabricatorDocumentEngineBlocks' => 'Phobject', 'PhabricatorDocumentRef' => 'Phobject', 'PhabricatorDocumentRenderingEngine' => 'Phobject', 'PhabricatorDoorkeeperApplication' => 'PhabricatorApplication', diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 058fc9c766..88ab8c7bd6 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1013,84 +1013,36 @@ final class DifferentialChangesetParser extends Phobject { ->setOldComments($old_comments) ->setNewComments($new_comments); - $engine_view = $this->newDocumentEngineChangesetView(); - if ($engine_view !== null) { - return $engine_view; + $engine_blocks = $this->newDocumentEngineChangesetView(); + if ($engine_blocks !== null) { + $reference = $this->getRenderingReference(); + $parts = explode('/', $reference); + if (count($parts) == 2) { + list($id, $vs) = $parts; + } else { + $id = $parts[0]; + $vs = 0; + } + + // If we don't have an explicit "vs" changeset, it's the left side of + // the "id" changeset. + if (!$vs) { + $vs = $id; + } + + return $renderer->renderDocumentEngineBlocks( + $engine_blocks, + (string)$id, + (string)$vs); } + // If we've made it here with a type of file we don't know how to render, + // bail out with a default empty rendering. Normally, we'd expect a + // document engine to catch these changes before we make it this far. switch ($this->changeset->getFileType()) { - case DifferentialChangeType::FILE_IMAGE: - $old = null; - $new = null; - // TODO: Improve the architectural issue as discussed in D955 - // https://secure.phabricator.com/D955 - $reference = $this->getRenderingReference(); - $parts = explode('/', $reference); - if (count($parts) == 2) { - list($id, $vs) = $parts; - } else { - $id = $parts[0]; - $vs = 0; - } - $id = (int)$id; - $vs = (int)$vs; - - if (!$vs) { - $metadata = $this->changeset->getMetadata(); - $data = idx($metadata, 'attachment-data'); - - $old_phid = idx($metadata, 'old:binary-phid'); - $new_phid = idx($metadata, 'new:binary-phid'); - } else { - $vs_changeset = id(new DifferentialChangeset())->load($vs); - $old_phid = null; - $new_phid = null; - - // TODO: This is spooky, see D6851 - if ($vs_changeset) { - $vs_metadata = $vs_changeset->getMetadata(); - $old_phid = idx($vs_metadata, 'new:binary-phid'); - } - - $changeset = id(new DifferentialChangeset())->load($id); - if ($changeset) { - $metadata = $changeset->getMetadata(); - $new_phid = idx($metadata, 'new:binary-phid'); - } - } - - if ($old_phid || $new_phid) { - // grab the files, (micro) optimization for 1 query not 2 - $file_phids = array(); - if ($old_phid) { - $file_phids[] = $old_phid; - } - if ($new_phid) { - $file_phids[] = $new_phid; - } - - $files = id(new PhabricatorFileQuery()) - ->setViewer($this->getUser()) - ->withPHIDs($file_phids) - ->execute(); - foreach ($files as $file) { - if (empty($file)) { - continue; - } - if ($file->getPHID() == $old_phid) { - $old = $file; - } else if ($file->getPHID() == $new_phid) { - $new = $file; - } - } - } - - $renderer->attachOldFile($old); - $renderer->attachNewFile($new); - - return $renderer->renderFileChange($old, $new, $id, $vs); case DifferentialChangeType::FILE_DIRECTORY: case DifferentialChangeType::FILE_BINARY: + case DifferentialChangeType::FILE_IMAGE: $output = $renderer->renderChangesetTable(null); return $output; } @@ -1699,21 +1651,39 @@ final class DifferentialChangesetParser extends Phobject { return null; } - $old_data = $this->old; - $old_data = ipull($old_data, 'text'); - $old_data = implode('', $old_data); + $old_file = null; + $new_file = null; - $new_data = $this->new; - $new_data = ipull($new_data, 'text'); - $new_data = implode('', $new_data); + switch ($changeset->getFileType()) { + case DifferentialChangeType::FILE_IMAGE: + case DifferentialChangeType::FILE_BINARY: + list($old_file, $new_file) = $this->loadFileObjectsForChangeset(); + break; + } $old_ref = id(new PhabricatorDocumentRef()) - ->setName($changeset->getOldFile()) - ->setData($old_data); + ->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()) - ->setData($new_data); + ->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($old_data); + } $old_engines = PhabricatorDocumentEngine::getEnginesForRef( $viewer, @@ -1743,4 +1713,74 @@ final class DifferentialChangesetParser extends Phobject { return null; } + private function loadFileObjectsForChangeset() { + $changeset = $this->changeset; + $viewer = $this->getViewer(); + + $old_file = null; + $new_file = null; + + // TODO: Improve the architectural issue as discussed in D955 + // https://secure.phabricator.com/D955 + $reference = $this->getRenderingReference(); + $parts = explode('/', $reference); + if (count($parts) == 2) { + list($id, $vs) = $parts; + } else { + $id = $parts[0]; + $vs = 0; + } + $id = (int)$id; + $vs = (int)$vs; + + if (!$vs) { + $metadata = $this->changeset->getMetadata(); + $data = idx($metadata, 'attachment-data'); + + $old_phid = idx($metadata, 'old:binary-phid'); + $new_phid = idx($metadata, 'new:binary-phid'); + } else { + $vs_changeset = id(new DifferentialChangeset())->load($vs); + $old_phid = null; + $new_phid = null; + + // TODO: This is spooky, see D6851 + if ($vs_changeset) { + $vs_metadata = $vs_changeset->getMetadata(); + $old_phid = idx($vs_metadata, 'new:binary-phid'); + } + + $changeset = id(new DifferentialChangeset())->load($id); + if ($changeset) { + $metadata = $changeset->getMetadata(); + $new_phid = idx($metadata, 'new:binary-phid'); + } + } + + if ($old_phid || $new_phid) { + $file_phids = array(); + if ($old_phid) { + $file_phids[] = $old_phid; + } + if ($new_phid) { + $file_phids[] = $new_phid; + } + + $files = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs($file_phids) + ->execute(); + + foreach ($files as $file) { + if ($file->getPHID() == $old_phid) { + $old_file = $file; + } else if ($file->getPHID() == $new_phid) { + $new_file = $file; + } + } + } + + return array($old_file, $new_file); + } + } diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index df59db9228..ae8db861b6 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -608,17 +608,4 @@ abstract class DifferentialChangesetHTMLRenderer return array($left_prefix, $right_prefix); } - protected function renderImageStage(PhabricatorFile $file) { - return phutil_tag( - 'div', - array( - 'class' => 'differential-image-stage', - ), - phutil_tag( - 'img', - array( - 'src' => $file->getBestURI(), - ))); - } - } diff --git a/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php index ad8939d275..c149808f12 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php @@ -31,14 +31,6 @@ final class DifferentialChangesetOneUpMailRenderer return null; } - public function renderFileChange( - $old_file = null, - $new_file = null, - $id = 0, - $vs = 0) { - return null; - } - public function renderTextChange( $range_start, $range_len, diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index 289b802485..1b7c75b082 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -228,34 +228,21 @@ final class DifferentialChangesetOneUpRenderer return null; } - public function renderFileChange( - $old_file = null, - $new_file = null, - $id = 0, - $vs = 0) { + public function renderDocumentEngineBlocks( + PhabricatorDocumentEngineBlocks $block_list, + $old_changeset_key, + $new_changeset_key) { // TODO: This should eventually merge into the normal primitives pathway, // but fake it for now and just share as much code as possible. $primitives = array(); - if ($old_file) { + foreach ($block_list->newOneUpLayout() as $block) { $primitives[] = array( 'type' => 'old-file', - 'htype' => ($new_file ? 'new-file' : null), - 'file' => $old_file, + 'htype' => '', 'line' => 1, - 'render' => $this->renderImageStage($old_file), - ); - } - - if ($new_file) { - $primitives[] = array( - 'type' => 'new-file', - 'htype' => ($old_file ? 'old-file' : null), - 'file' => $new_file, - 'line' => 1, - 'oline' => ($old_file ? 1 : null), - 'render' => $this->renderImageStage($new_file), + 'render' => $block->newContentView(), ); } diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index 4ed77bf041..e6c6f01fb7 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -378,11 +378,13 @@ abstract class DifferentialChangesetRenderer extends Phobject { $range_start, $range_len, $rows); - abstract public function renderFileChange( - $old = null, - $new = null, - $id = 0, - $vs = 0); + + public function renderDocumentEngineBlocks( + PhabricatorDocumentEngineBlocks $blocks, + $old_changeset_key, + $new_changeset_key) { + return null; + } abstract protected function renderChangeTypeHeader($force); abstract protected function renderUndershieldHeader(); diff --git a/src/applications/differential/render/DifferentialChangesetTestRenderer.php b/src/applications/differential/render/DifferentialChangesetTestRenderer.php index e2bd3f53ed..56b96dcbca 100644 --- a/src/applications/differential/render/DifferentialChangesetTestRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTestRenderer.php @@ -134,14 +134,4 @@ abstract class DifferentialChangesetTestRenderer return phutil_safe_html($out); } - - public function renderFileChange( - $old_file = null, - $new_file = null, - $id = 0, - $vs = 0) { - - throw new PhutilMethodNotImplementedException(); - } - } diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index d803e92c6c..160bdbbb90 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -364,26 +364,28 @@ final class DifferentialChangesetTwoUpRenderer return $this->wrapChangeInTable(phutil_implode_html('', $html)); } - public function renderFileChange( - $old_file = null, - $new_file = null, - $id = 0, - $vs = 0) { + public function renderDocumentEngineBlocks( + PhabricatorDocumentEngineBlocks $block_list, + $old_changeset_key, + $new_changeset_key) { - $old = null; - if ($old_file) { - $old = $this->renderImageStage($old_file); - } + $old_view = null; + $new_view = null; - $new = null; - if ($new_file) { - $new = $this->renderImageStage($new_file); - } + foreach ($block_list->newTwoUpLayout() as $row) { + list($old, $new) = $row; - // If we don't have an explicit "vs" changeset, it's the left side of the - // "id" changeset. - if (!$vs) { - $vs = $id; + if ($old) { + $old_view = $old->newContentView(); + } else { + $old_view = null; + } + + if ($new) { + $new_view = $new->newContentView(); + } else { + $new_view = null; + } } $html_old = array(); @@ -405,31 +407,52 @@ final class DifferentialChangesetTwoUpRenderer } } - if (!$old) { - $th_old = phutil_tag('th', array()); + if ($old_view === null) { + $old_id = null; + $old_label = null; } else { - $th_old = phutil_tag('th', array('id' => "C{$vs}OL1"), 1); + $old_id = "C{$old_changeset_key}OL1"; + $old_label = '1'; } - if (!$new) { - $th_new = phutil_tag('th', array()); + $old_cell = phutil_tag( + 'td', + array( + 'id' => $old_id, + 'class' => 'n', + ), + $old_label); + + if ($new_view === null) { + $new_id = null; + $new_label = null; } else { - $th_new = phutil_tag('th', array('id' => "C{$id}NL1"), 1); + $new_id = "C{$new_changeset_key}NL1"; + $new_label = '1'; } + $new_cell = phutil_tag( + 'td', + array( + 'id' => $new_id, + 'class' => 'n', + ), + $new_label); + $output = hsprintf( ''. '%s'. - '%s'. + '%s'. '%s'. - '%s'. + '%s'. ''. '%s'. '%s', - $th_old, - $old, - $th_new, - $new, + $old_cell, + $old_view, + $new_cell, + $new_view, phutil_implode_html('', $html_old), phutil_implode_html('', $html_new)); diff --git a/src/applications/files/diff/PhabricatorDocumentEngineBlock.php b/src/applications/files/diff/PhabricatorDocumentEngineBlock.php new file mode 100644 index 0000000000..90fe9676df --- /dev/null +++ b/src/applications/files/diff/PhabricatorDocumentEngineBlock.php @@ -0,0 +1,21 @@ +content = $content; + return $this; + } + + public function getContent() { + return $this->content; + } + + public function newContentView() { + return $this->getContent(); + } + +} diff --git a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php new file mode 100644 index 0000000000..fc06135063 --- /dev/null +++ b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php @@ -0,0 +1,83 @@ +lists[] = array( + 'ref' => $ref, + 'blocks' => array_values($blocks), + ); + + return $this; + } + + public function newTwoUpLayout() { + $rows = array(); + $lists = $this->lists; + + $idx = 0; + while (true) { + $found_any = false; + + $row = array(); + foreach ($lists as $list) { + $blocks = $list['blocks']; + $cell = idx($blocks, $idx); + + if ($cell !== null) { + $found_any = true; + } + + $row[] = $cell; + } + + if (!$found_any) { + break; + } + + $rows[] = $row; + $idx++; + } + + return $rows; + } + + public function newOneUpLayout() { + $rows = array(); + $lists = $this->lists; + + $idx = 0; + while (true) { + $found_any = false; + + $row = array(); + foreach ($lists as $list) { + $blocks = $list['blocks']; + $cell = idx($blocks, $idx); + + if ($cell !== null) { + $found_any = true; + } + + if ($cell) { + $rows[] = $cell; + } + } + + if (!$found_any) { + break; + } + + $idx++; + } + + return $rows; + } + + +} diff --git a/src/applications/files/document/PhabricatorImageDocumentEngine.php b/src/applications/files/document/PhabricatorImageDocumentEngine.php index d0b2099dd0..2d397eae96 100644 --- a/src/applications/files/document/PhabricatorImageDocumentEngine.php +++ b/src/applications/files/document/PhabricatorImageDocumentEngine.php @@ -17,6 +17,50 @@ final class PhabricatorImageDocumentEngine return (1024 * 1024 * 64); } + public function canDiffDocuments( + PhabricatorDocumentRef $uref, + PhabricatorDocumentRef $vref) { + + // For now, we can only render a rich image diff if both documents have + // their data stored in Files already. + + return ($uref->getFile() && $vref->getFile()); + } + + public function newDiffView( + PhabricatorDocumentRef $uref, + PhabricatorDocumentRef $vref) { + + $u_blocks = $this->newDiffBlocks($uref); + $v_blocks = $this->newDiffBlocks($vref); + + return id(new PhabricatorDocumentEngineBlocks()) + ->addBlockList($uref, $u_blocks) + ->addBlockList($vref, $v_blocks); + } + + private function newDiffBlocks(PhabricatorDocumentRef $ref) { + $blocks = array(); + + $file = $ref->getFile(); + + $image_view = phutil_tag( + 'div', + array( + 'class' => 'differential-image-stage', + ), + phutil_tag( + 'img', + array( + 'src' => $file->getBestURI(), + ))); + + $blocks[] = id(new PhabricatorDocumentEngineBlock()) + ->setContent($image_view); + + return $blocks; + } + protected function canRenderDocumentType(PhabricatorDocumentRef $ref) { $file = $ref->getFile(); if ($file) { diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 233ac4cca5..28f004a16a 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -282,11 +282,11 @@ td.cov-I { font-weight: bold; } -.differential-diff .differential-image-diff { +.differential-diff .diff-image-cell { background-image: url(/rsrc/image/checker_light.png); } -.differential-diff .differential-image-diff:hover { +.device-desktop .differential-diff .diff-image-cell:hover { background-image: url(/rsrc/image/checker_dark.png); }