Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F15397177
D20831.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
22 KB
Referenced Files
None
Subscribers
None
D20831.diff
View Options
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(
'<tr class="differential-image-diff">'.
'%s'.
- '<td class="differential-old-image">%s</td>'.
+ '<td class="differential-old-image diff-image-cell">%s</td>'.
'%s'.
- '<td class="differential-new-image" colspan="3">%s</td>'.
+ '<td class="differential-new-image diff-image-cell" '.
+ 'colspan="3">%s</td>'.
'</tr>'.
'%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 @@
+<?php
+
+final class PhabricatorDocumentEngineBlock
+ extends Phobject {
+
+ private $content;
+
+ public function setContent($content) {
+ $this->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 @@
+<?php
+
+final class PhabricatorDocumentEngineBlocks
+ extends Phobject {
+
+ private $lists = array();
+
+ public function addBlockList(PhabricatorDocumentRef $ref, array $blocks) {
+ assert_instances_of($blocks, 'PhabricatorDocumentEngineBlock');
+
+ $this->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);
}
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Mon, Mar 17, 6:38 PM (3 w, 1 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7708725
Default Alt Text
D20831.diff (22 KB)
Attached To
Mode
D20831: Render image diffs as abstract blocks diffs via DocumentEngine
Attached
Detach File
Event Timeline
Log In to Comment