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
2024-11-22 06:42:42 +01:00 · 2019-09-23 11:09:29 -07:00 · 2019-09-23 11:09:29 -07:00 · bb71ef6ad6
commit bb71ef6ad6
parent a73f592d7d
13 changed files with 349 additions and 176 deletions
--- 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',
    ),
--- 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',
--- 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);
+  }
+
 }
--- 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(),
-        )));
-  }
-
 }
--- 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,
--- 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(),
      );
    }

--- 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();
--- 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();
-  }
-
 }
--- 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(
      '<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));

--- a/src/applications/files/diff/PhabricatorDocumentEngineBlock.php
+++ 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();
+  }
+
+}
--- a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php
+++ 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;
+  }
+
+
+}
--- 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) {
--- 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);
 }