Show image dimensions and some other metadata in Differential

Summary: Fixes T2101. When viewing an image change, show image dimensions, MIME type, and filesize.

Test Plan:
{F190189}

{F190190}

very utility

such wow

Reviewers: mailson, btrahan, chad

Reviewed By: chad

Subscribers: epriestley, Korvin, aran

Maniphest Tasks: T2101

Differential Revision: https://secure.phabricator.com/D5206

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -690,22 +690,6 @@ final class DifferentialChangesetParser {
       return false;
     }
 
-    $old = $changeset->getOldProperties();
-    $new = $changeset->getNewProperties();
-
-    if ($old === $new) {
-      return false;
-    }
-
-    if ($changeset->getChangeType() == DifferentialChangeType::TYPE_ADD &&
-        $new == array('unix:filemode' => '100644')) {
-      return false;
-    }
-
-    if ($changeset->getChangeType() == DifferentialChangeType::TYPE_DELETE &&
-        $old == array('unix:filemode' => '100644')) {
-      return false;
-    }
     return true;
   }
 
@@ -927,6 +911,9 @@ final class DifferentialChangesetParser {
           }
         }
 
+        $renderer->attachOldFile($old);
+        $renderer->attachNewFile($new);
+
         return $renderer->renderFileChange($old, $new, $id, $vs);
       case DifferentialChangeType::FILE_DIRECTORY:
       case DifferentialChangeType::FILE_BINARY:

src/applications/differential/render/DifferentialChangesetHTMLRenderer.php

@@ -289,13 +289,23 @@ abstract class DifferentialChangesetHTMLRenderer
 
   protected function renderPropertyChangeHeader() {
     $changeset = $this->getChangeset();
+    list($old, $new) = $this->getChangesetProperties($changeset);
 
-    $old = $changeset->getOldProperties();
-    $new = $changeset->getNewProperties();
+    // If we don't have any property changes, don't render this table.
+    if ($old === $new) {
+      return null;
+    }
 
     $keys = array_keys($old + $new);
     sort($keys);
 
+    $key_map = array(
+      'unix:filemode' => pht('File Mode'),
+      'file:dimensions' => pht('Image Dimensions'),
+      'file:mimetype' => pht('MIME Type'),
+      'file:size' => pht('File Size'),
+    );
+
     $rows = array();
     foreach ($keys as $key) {
       $oval = idx($old, $key);
@@ -313,8 +323,10 @@ abstract class DifferentialChangesetHTMLRenderer
           $nval = phutil_escape_html_newlines($nval);
         }
 
+        $readable_key = idx($key_map, $key, $key);
+
         $rows[] = phutil_tag('tr', array(), array(
-          phutil_tag('th', array(), $key),
+          phutil_tag('th', array(), $readable_key),
           phutil_tag('td', array('class' => 'oval'), $oval),
           phutil_tag('td', array('class' => 'nval'), $nval),
         ));

src/applications/differential/render/DifferentialChangesetRenderer.php

@@ -30,6 +30,9 @@ abstract class DifferentialChangesetRenderer {
   private $depths;
   private $originalCharacterEncoding;
 
+  private $oldFile = false;
+  private $newFile = false;
+
   public function setOriginalCharacterEncoding($original_character_encoding) {
     $this->originalCharacterEncoding = $original_character_encoding;
     return $this;
@@ -63,6 +66,38 @@ abstract class DifferentialChangesetRenderer {
     return $this->gaps;
   }
 
+  public function attachOldFile(PhabricatorFile $old = null) {
+    $this->oldFile = $old;
+    return $this;
+  }
+
+  public function getOldFile() {
+    if ($this->oldFile === false) {
+      throw new PhabricatorDataNotAttachedException($this);
+    }
+    return $this->oldFile;
+  }
+
+  public function hasOldFile() {
+    return (bool)$this->oldFile;
+  }
+
+  public function attachNewFile(PhabricatorFile $new = null) {
+    $this->newFile = $new;
+    return $this;
+  }
+
+  public function getNewFile() {
+    if ($this->newFile === false) {
+      throw new PhabricatorDataNotAttachedException($this);
+    }
+    return $this->newFile;
+  }
+
+  public function hasNewFile() {
+    return (bool)$this->newFile;
+  }
+
   public function setOriginalNew($original_new) {
     $this->originalNew = $original_new;
     return $this;
@@ -498,4 +533,43 @@ abstract class DifferentialChangesetRenderer {
     return $out;
   }
 
+  protected function getChangesetProperties($changeset) {
+    $old = $changeset->getOldProperties();
+    $new = $changeset->getNewProperties();
+
+    // When adding files, don't show the uninteresting 644 filemode change.
+    if ($changeset->getChangeType() == DifferentialChangeType::TYPE_ADD &&
+        $new == array('unix:filemode' => '100644')) {
+      unset($new['unix:filemode']);
+    }
+
+    // Likewise when removing files.
+    if ($changeset->getChangeType() == DifferentialChangeType::TYPE_DELETE &&
+        $old == array('unix:filemode' => '100644')) {
+      unset($old['unix:filemode']);
+    }
+
+    if ($this->hasOldFile()) {
+      $file = $this->getOldFile();
+      if ($file->getImageWidth()) {
+        $dimensions = $file->getImageWidth().'x'.$file->getImageHeight();
+        $old['file:dimensions'] = $dimensions;
+      }
+      $old['file:mimetype'] = $file->getMimeType();
+      $old['file:size'] = phutil_format_bytes($file->getByteSize());
+    }
+
+    if ($this->hasNewFile()) {
+      $file = $this->getNewFile();
+      if ($file->getImageWidth()) {
+        $dimensions = $file->getImageWidth().'x'.$file->getImageHeight();
+        $new['file:dimensions'] = $dimensions;
+      }
+      $new['file:mimetype'] = $file->getMimeType();
+      $new['file:size'] = phutil_format_bytes($file->getByteSize());
+    }
+
+    return array($old, $new);
+  }
+
 }

src/applications/differential/render/DifferentialChangesetTestRenderer.php

@@ -23,8 +23,7 @@ abstract class DifferentialChangesetTestRenderer
 
   protected function renderPropertyChangeHeader() {
     $changeset = $this->getChangeset();
-    $old = $changeset->getOldProperties();
-    $new = $changeset->getNewProperties();
+    list($old, $new) = $this->getChangesetProperties($changeset);
 
     if (!$old && !$new) {
       return null;

src/infrastructure/storage/lisk/PhabricatorDataNotAttachedException.php

@@ -2,7 +2,7 @@
 
 final class PhabricatorDataNotAttachedException extends Exception {
 
-  public function __construct(PhabricatorLiskDAO $dao) {
+  public function __construct($object) {
     $stack = debug_backtrace();
 
     // Shift off `PhabricatorDataNotAttachedException::__construct()`.
@@ -19,7 +19,7 @@ final class PhabricatorDataNotAttachedException extends Exception {
       }
     }
 
-    $class = get_class($dao);
+    $class = get_class($object);
 
     $message =
       "Attempting to access attached data on {$class}{$via}, but the data is ".