diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ '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 @@ '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 @@ '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 @@ 'javelin-behavior', 'javelin-request', ), + '215129ef' => array( + 'phui-inline-comment-view-css', + ), '225bbb98' => array( 'javelin-install', 'javelin-reactor', @@ -1960,9 +1963,6 @@ '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 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3138,6 +3138,8 @@ '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 @@ '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 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1013,84 +1013,36 @@ ->setOldComments($old_comments) ->setNewComments($new_comments); - $engine_view = $this->newDocumentEngineChangesetView(); - if ($engine_view !== null) { - return $engine_view; - } - - 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; - } + $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; + } - $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; - } - } - } + // If we don't have an explicit "vs" changeset, it's the left side of + // the "id" changeset. + if (!$vs) { + $vs = $id; + } - $renderer->attachOldFile($old); - $renderer->attachNewFile($new); + return $renderer->renderDocumentEngineBlocks( + $engine_blocks, + (string)$id, + (string)$vs); + } - return $renderer->renderFileChange($old, $new, $id, $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_DIRECTORY: case DifferentialChangeType::FILE_BINARY: + case DifferentialChangeType::FILE_IMAGE: $output = $renderer->renderChangesetTable(null); return $output; } @@ -1699,21 +1651,39 @@ 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 @@ 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 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -608,17 +608,4 @@ 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 --- a/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php @@ -31,14 +31,6 @@ 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 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -228,34 +228,21 @@ 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 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -378,11 +378,13 @@ $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 --- a/src/applications/differential/render/DifferentialChangesetTestRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTestRenderer.php @@ -134,14 +134,4 @@ 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 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -364,26 +364,28 @@ return $this->wrapChangeInTable(phutil_implode_html('', $html)); } - public function renderFileChange( - $old_file = null, - $new_file = null, - $id = 0, - $vs = 0) { - - $old = null; - if ($old_file) { - $old = $this->renderImageStage($old_file); - } + public function renderDocumentEngineBlocks( + PhabricatorDocumentEngineBlocks $block_list, + $old_changeset_key, + $new_changeset_key) { - $new = null; - if ($new_file) { - $new = $this->renderImageStage($new_file); - } + $old_view = null; + $new_view = null; + + 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 @@ } } - 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 --- /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 --- /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 --- a/src/applications/files/document/PhabricatorImageDocumentEngine.php +++ b/src/applications/files/document/PhabricatorImageDocumentEngine.php @@ -17,6 +17,50 @@ 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 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -282,11 +282,11 @@ 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); }