Turn thumbs into a history grid thing

Summary:
This could probably use some refinement (and, like, explanatory text, and stronger cues about what rows and columns mean) but feels fairly good to me, at least on test data.

I didn't do any scrolling for now since we have to do full height on mobile anyway I think. I did swap it so the newer ones are on top.

Left/right navigate you among current images only, but you can click any thumb to review history.

Removed history view since it's no longer useful.

Some things that would probably help:

  - Some kind of header explaining what this is ("Mock History" or something).
  - Stronger visual cue that columns are related by being the same image.
  - Clearer cues about obsolete/deleted images (e.g., on the stage itself?)
  - Maybe general tweaks.
  - Maybe a placeholder (like a grey "X") for images which have been deleted.

(I'm planning to add comment counts too, which I think will be pretty useful, but that felt good to put in another diff.)

Test Plan: See screenshots.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

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

src/__phutil_library_map__.php

@@ -2410,7 +2410,6 @@ phutil_register_library_map(array(
     'PholioController' => 'applications/pholio/controller/PholioController.php',
     'PholioDAO' => 'applications/pholio/storage/PholioDAO.php',
     'PholioImage' => 'applications/pholio/storage/PholioImage.php',
-    'PholioImageHistoryController' => 'applications/pholio/controller/PholioImageHistoryController.php',
     'PholioImageQuery' => 'applications/pholio/query/PholioImageQuery.php',
     'PholioImageUploadController' => 'applications/pholio/controller/PholioImageUploadController.php',
     'PholioInlineController' => 'applications/pholio/controller/PholioInlineController.php',
@@ -2426,6 +2425,7 @@ phutil_register_library_map(array(
     'PholioMockMailReceiver' => 'applications/pholio/mail/PholioMockMailReceiver.php',
     'PholioMockQuery' => 'applications/pholio/query/PholioMockQuery.php',
     'PholioMockSearchEngine' => 'applications/pholio/query/PholioMockSearchEngine.php',
+    'PholioMockThumbGridView' => 'applications/pholio/view/PholioMockThumbGridView.php',
     'PholioMockViewController' => 'applications/pholio/controller/PholioMockViewController.php',
     'PholioPHIDTypeImage' => 'applications/pholio/phid/PholioPHIDTypeImage.php',
     'PholioPHIDTypeMock' => 'applications/pholio/phid/PholioPHIDTypeMock.php',
@@ -5312,7 +5312,6 @@ phutil_register_library_map(array(
       1 => 'PhabricatorMarkupInterface',
       2 => 'PhabricatorPolicyInterface',
     ),
-    'PholioImageHistoryController' => 'PholioController',
     'PholioImageQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PholioImageUploadController' => 'PholioController',
     'PholioInlineController' => 'PholioController',
@@ -5338,6 +5337,7 @@ phutil_register_library_map(array(
     'PholioMockMailReceiver' => 'PhabricatorObjectMailReceiver',
     'PholioMockQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PholioMockSearchEngine' => 'PhabricatorApplicationSearchEngine',
+    'PholioMockThumbGridView' => 'AphrontView',
     'PholioMockViewController' => 'PholioController',
     'PholioPHIDTypeImage' => 'PhabricatorPHIDType',
     'PholioPHIDTypeMock' => 'PhabricatorPHIDType',

src/applications/files/controller/PhabricatorFileTransformController.php

@@ -65,6 +65,9 @@ final class PhabricatorFileTransformController
       case 'thumb-220x165':
         $xformed_file = $this->executeThumbTransform($file, 220, 165);
         break;
+      case 'preview-100':
+        $xformed_file = $this->executePreviewTransform($file, 100);
+        break;
       case 'preview-140':
         $xformed_file = $this->executePreviewTransform($file, 140);
         break;

src/applications/files/storage/PhabricatorFile.php

@@ -538,6 +538,12 @@ final class PhabricatorFile extends PhabricatorFileDAO
     return PhabricatorEnv::getCDNURI($path);
   }
 
+  public function getPreview100URI() {
+    $path = '/file/xform/preview-100/'.$this->getPHID().'/'
+      .$this->getSecretKey().'/';
+    return PhabricatorEnv::getCDNURI($path);
+  }
+
   public function getPreview140URI() {
     $path = '/file/xform/preview-140/'.$this->getPHID().'/'
       .$this->getSecretKey().'/';

src/applications/pholio/application/PhabricatorApplicationPholio.php

@@ -53,7 +53,6 @@ final class PhabricatorApplicationPholio extends PhabricatorApplication {
         ),
         'image/' => array(
           'upload/' => 'PholioImageUploadController',
-          'history/(?P<id>\d+)/' => 'PholioImageHistoryController',
         ),
       ),
     );

src/applications/pholio/controller/PholioImageHistoryController.php

@@ -1,90 +0,0 @@
-<?php
-
-/**
- * @group pholio
- */
-final class PholioImageHistoryController extends PholioController {
-
-  private $imageID;
-
-  public function willProcessRequest(array $data) {
-    $this->imageID = $data['id'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $user = $request->getUser();
-
-    $image = id(new PholioImageQuery())
-      ->setViewer($user)
-      ->withIDs(array($this->imageID))
-      ->executeOne();
-
-    if (!$image) {
-      return new Aphront404Response();
-    }
-
-    // note while we have a mock object, its missing images we need to show
-    // the history of what's happened here.
-    // fetch the real deal
-
-    $mock = id(new PholioMockQuery())
-      ->setViewer($user)
-      ->needImages(true)
-      ->withIDs(array($image->getMockID()))
-      ->executeOne();
-
-    $phids = array($mock->getAuthorPHID());
-    $this->loadHandles($phids);
-
-    $engine = id(new PhabricatorMarkupEngine())
-      ->setViewer($user);
-    $engine->addObject($mock, PholioMock::MARKUP_FIELD_DESCRIPTION);
-    $engine->process();
-
-
-    $images = $mock->getImageHistorySet($this->imageID);
-    $mock->attachImages($images);
-    $latest_image = last($images);
-
-    $title = pht(
-      'Image history for "%s" from the mock "%s."',
-      $latest_image->getName(),
-      $mock->getName());
-
-    $header = id(new PHUIHeaderView())
-      ->setHeader($title);
-
-    require_celerity_resource('pholio-css');
-    require_celerity_resource('pholio-inline-comments-css');
-
-    $comment_form_id = null;
-    $output = id(new PholioMockImagesView())
-      ->setRequestURI($request->getRequestURI())
-      ->setCommentFormID($comment_form_id)
-      ->setUser($user)
-      ->setMock($mock)
-      ->setImageID($this->imageID)
-      ->setViewMode('history');
-
-    $crumbs = $this->buildApplicationCrumbs();
-    $crumbs
-      ->addTextCrumb('M'.$mock->getID(), '/M'.$mock->getID())
-      ->addTextCrumb('Image History', $request->getRequestURI());
-
-    $content = array(
-      $crumbs,
-      $header,
-      $output->render(),
-    );
-
-    return $this->buildApplicationPage(
-      $content,
-      array(
-        'title' => 'M'.$mock->getID().' '.$title,
-        'device' => true,
-        'pageObjects' => array($mock->getPHID()),
-      ));
-  }
-
-}

src/applications/pholio/controller/PholioMockViewController.php

@@ -1,8 +1,5 @@
 <?php
 
-/**
- * @group pholio
- */
 final class PholioMockViewController extends PholioController {
 
   private $id;
@@ -89,8 +86,6 @@ final class PholioMockViewController extends PholioController {
     require_celerity_resource('pholio-css');
     require_celerity_resource('pholio-inline-comments-css');
 
-    $image_status = $this->getImageStatus($mock, $this->imageID);
-
     $comment_form_id = celerity_generate_unique_node_id();
     $output = id(new PholioMockImagesView())
       ->setRequestURI($request->getRequestURI())
@@ -115,11 +110,16 @@ final class PholioMockViewController extends PholioController {
       ->setHeader($header)
       ->addPropertyList($properties);
 
+    $thumb_grid = id(new PholioMockThumbGridView())
+      ->setUser($user)
+      ->setMock($mock);
+
     $content = array(
       $crumbs,
-      $image_status,
       $object_box,
-      $output->render(),
+      $output,
+      phutil_tag('br'),
+      $thumb_grid,
       $xaction_view,
       $add_comment,
     );
@@ -133,43 +133,6 @@ final class PholioMockViewController extends PholioController {
       ));
   }
 
-  private function getImageStatus(PholioMock $mock, $image_id) {
-    $status = null;
-    $images = $mock->getImages();
-    foreach ($images as $image) {
-      if ($image->getID() == $image_id) {
-        return $status;
-      }
-    }
-
-    $images = $mock->getAllImages();
-    $images = mpull($images, null, 'getID');
-    $image = idx($images, $image_id);
-
-    if ($image) {
-      $history = $mock->getImageHistorySet($image_id);
-      $latest_image = last($history);
-      $href = $this->getApplicationURI(
-        'image/history/'.$latest_image->getID().'/');
-      $status = id(new AphrontErrorView())
-        ->setSeverity(AphrontErrorView::SEVERITY_NOTICE)
-        ->setTitle(pht('The requested image is obsolete.'))
-        ->appendChild(phutil_tag(
-          'p',
-          array(),
-          array(
-            pht('You are viewing this mock with the latest image set.'),
-            ' ',
-            phutil_tag(
-              'a',
-              array('href' => $href),
-              pht(
-                'Click here to see the history of the now obsolete image.')))));
-    }
-
-    return $status;
-  }
-
   private function buildActionView(PholioMock $mock) {
     $user = $this->getRequest()->getUser();
 

src/applications/pholio/view/PholioMockImagesView.php

@@ -9,25 +9,12 @@ final class PholioMockImagesView extends AphrontView {
   private $imageID;
   private $requestURI;
   private $commentFormID;
-  private $viewMode = 'normal';
-
-  /**
-   * Supports normal (/MX, /MX/Y/) and history (/pholio/image/history/Y/)
-   * modes. The former has handy dandy commenting functionality and the
-   * latter does not.
-   */
-  public function setViewMode($view_mode) {
-    $this->viewMode = $view_mode;
-    return $this;
-  }
-  public function getViewMode() {
-    return $this->viewMode;
-  }
 
   public function setCommentFormID($comment_form_id) {
     $this->commentFormID = $comment_form_id;
     return $this;
   }
+
   public function getCommentFormID() {
     return $this->commentFormID;
   }
@@ -36,6 +23,7 @@ final class PholioMockImagesView extends AphrontView {
     $this->requestURI = $request_uri;
     return $this;
   }
+
   public function getRequestURI() {
     return $this->requestURI;
   }
@@ -74,7 +62,7 @@ final class PholioMockImagesView extends AphrontView {
       $selected_id = head_key($ids);
     }
 
-    foreach ($mock->getImages() as $image) {
+    foreach ($mock->getAllImages() as $image) {
       $file = $image->getFile();
       $metadata = $file->getMetadata();
       $x = idx($metadata, PhabricatorFile::METADATA_IMAGE_WIDTH);
@@ -90,10 +78,15 @@ final class PholioMockImagesView extends AphrontView {
         'height' => $y,
         'title' => $image->getName(),
         'desc' => $image->getDescription(),
-        'isObsolete' => (bool) $image->getIsObsolete(),
+        'isObsolete' => (bool)$image->getIsObsolete(),
       );
     }
 
+    $navsequence = array();
+    foreach ($mock->getImages() as $image) {
+      $navsequence[] = $image->getID();
+    }
+
     $login_uri = id(new PhutilURI('/login/'))
       ->setQueryParam('next', (string) $this->getRequestURI());
     $config = array(
@@ -105,7 +98,7 @@ final class PholioMockImagesView extends AphrontView {
       'selectedID' => $selected_id,
       'loggedIn' => $this->getUser()->isLoggedIn(),
       'logInLink' => (string) $login_uri,
-      'viewMode' => $this->getViewMode()
+      'navsequence' => $navsequence,
     );
     Javelin::initBehavior('pholio-mock-view', $config);
 
@@ -149,65 +142,19 @@ final class PholioMockImagesView extends AphrontView {
       ),
       '');
 
-    $carousel_holder = '';
-    if (count($mock->getImages()) > 0) {
-      $thumbnails = array();
-      foreach ($mock->getImages() as $image) {
-        $thumbfile = $image->getFile();
-
-        $dimensions = PhabricatorImageTransformer::getPreviewDimensions(
-          $thumbfile,
-          140);
-
-        $tag = phutil_tag(
-          'img',
-          array(
-            'width' => $dimensions['sdx'],
-            'height' => $dimensions['sdy'],
-            'src' => $thumbfile->getPreview140URI(),
-            'class' => 'pholio-mock-carousel-thumbnail',
-            'style' => 'top: '.floor((140 - $dimensions['sdy'] ) / 2).'px',
-        ));
-
-        $thumbnails[] = javelin_tag(
-          'a',
-          array(
-            'sigil' => 'mock-thumbnail',
-            'class' => 'pholio-mock-carousel-thumb-item',
-            'href' => $this->getImagePageURI($image, $mock),
-            'meta' => array(
-              'imageID' => $image->getID(),
-            ),
-          ),
-          $tag);
-      }
-
-      $carousel_holder = phutil_tag(
-        'div',
-        array(
-          'id' => 'pholio-mock-carousel',
-          'class' => 'pholio-mock-carousel',
-        ),
-        $thumbnails);
-    }
-
     $mockview[] = phutil_tag(
       'div',
-      array(
+        array(
-        'class' => 'pholio-mock-image-container',
+          'class' => 'pholio-mock-image-container',
-        'id' => 'pholio-mock-image-container'
+          'id' => 'pholio-mock-image-container'
-      ),
+        ),
-      array($mock_wrapper, $inline_comments_holder, $carousel_holder));
+      array($mock_wrapper, $inline_comments_holder));
 
     return $mockview;
   }
 
   private function getImagePageURI(PholioImage $image, PholioMock $mock) {
-    if ($this->getViewMode() == 'normal') {
+    $uri = '/M'.$mock->getID().'/'.$image->getID().'/';
-      $uri = '/M'.$mock->getID().'/'.$image->getID().'/';
-    } else {
-      $uri = '/pholio/image/history/'.$image->getID().'/';
-    }
     return $uri;
   }
 }

src/applications/pholio/view/PholioMockThumbGridView.php

@@ -0,0 +1,135 @@
+<?php
+
+final class PholioMockThumbGridView extends AphrontView {
+
+  private $mock;
+
+  public function setMock(PholioMock $mock) {
+    $this->mock = $mock;
+    return $this;
+  }
+
+  public function render() {
+    $mock = $this->mock;
+
+    $all_images = $mock->getAllImages();
+    $all_images = mpull($all_images, null, 'getPHID');
+
+    $history = mpull($all_images, 'getReplacesImagePHID', 'getPHID');
+
+    $replaced = array();
+    foreach ($history as $phid => $replaces_phid) {
+      if ($replaces_phid) {
+        $replaced[$replaces_phid] = true;
+      }
+    }
+
+    // Figure out the columns. Start with all the active images.
+    $images = mpull($mock->getImages(), null, 'getPHID');
+
+    // Now, find deleted images: obsolete images which were not replaced.
+    foreach ($mock->getAllImages() as $image) {
+      if (!$image->getIsObsolete()) {
+        // Image is current.
+        continue;
+      }
+
+      if (isset($replaced[$image->getPHID()])) {
+        // Image was replaced.
+        continue;
+      }
+
+      // This is an obsolete image which was not replaced, so it must be
+      // a deleted image.
+      $images[$image->getPHID()] = $image;
+    }
+
+    $cols = array();
+    $depth = 0;
+    foreach ($images as $image) {
+      $phid = $image->getPHID();
+
+      $col = array();
+
+      // If this is a deleted image, null out the final column.
+      if ($image->getIsObsolete()) {
+        $col[] = null;
+      }
+
+      $col[] = $phid;
+      while ($phid && isset($history[$phid])) {
+        $col[] = $history[$phid];
+        $phid = $history[$phid];
+      }
+
+      $cols[] = $col;
+      $depth = max($depth, count($col));
+    }
+
+    $grid = array();
+    for ($ii = 0; $ii < $depth; $ii++) {
+      $row = array();
+      foreach ($cols as $col) {
+        if (empty($col[$ii])) {
+          $row[] = phutil_tag('td', array(), null);
+        } else {
+          $thumb = $this->renderThumbnail($all_images[$col[$ii]]);
+          $row[] = phutil_tag('td', array(), $thumb);
+        }
+      }
+      $grid[] = phutil_tag('tr', array(), $row);
+    }
+
+    $grid = phutil_tag(
+      'table',
+      array(
+        'id' => 'pholio-mock-thumb-grid',
+        'class' => 'pholio-mock-thumb-grid',
+      ),
+      $grid);
+
+    return phutil_tag(
+      'div',
+      array(
+        'class' => 'pholio-mock-thumb-grid-container',
+      ),
+      $grid);
+  }
+
+
+  private function renderThumbnail(PholioImage $image) {
+    $thumbfile = $image->getFile();
+
+    $dimensions = PhabricatorImageTransformer::getPreviewDimensions(
+      $thumbfile,
+      100);
+
+    $tag = phutil_tag(
+      'img',
+      array(
+        'width' => $dimensions['sdx'],
+        'height' => $dimensions['sdy'],
+        'src' => $thumbfile->getPreview100URI(),
+        'class' => 'pholio-mock-thumb-grid-image',
+        'style' => 'top: '.floor((100 - $dimensions['sdy'] ) / 2).'px',
+    ));
+
+    $classes = array('pholio-mock-thumb-grid-item');
+    if ($image->getIsObsolete()) {
+      $classes[] = 'pholio-mock-thumb-grid-item-obsolete';
+    }
+
+    return javelin_tag(
+      'a',
+      array(
+        'sigil' => 'mock-thumbnail',
+        'class' => implode(' ', $classes),
+        'href' => '#',
+        'meta' => array(
+          'imageID' => $image->getID(),
+        ),
+      ),
+      $tag);
+  }
+
+}

webroot/rsrc/css/application/pholio/pholio.css

@@ -10,50 +10,41 @@
   background: url('/rsrc/image/texture/pholio-background.gif');
 }
 
-.pholio-mock-carousel {
+.pholio-mock-thumb-grid-container {
   background-color: #282828;
-  text-align: center;
+  padding: 12px;
+  overflow-x: auto;
+  overflow-y: hidden;
 }
 
-.pholio-mock-carousel-thumb-item {
+.pholio-mock-thumb-grid {
+  margin: 0 auto;
+}
+
+.pholio-mock-thumb-grid-item {
   display: inline-block;
   cursor: pointer;
-  width: 140px;
+  width: 100px;
-  height: 140px;
+  height: 100px;
   padding: 5px;
   margin: 3px;
   background: #181818;
   vertical-align: middle;
-  border: 1px solid #383838;
+  border: 1px solid {$greyborder};
   position: relative;
 }
 
-.device-desktop .pholio-mock-carousel-thumb-item:hover,
+.device-desktop .pholio-mock-thumb-grid-item:hover,
-.pholio-mock-carousel-thumb-current {
+.pholio-mock-thumb-grid-current {
   background: #383838;
-  border-color: #686868;
+  border-color: {$sky};
 }
 
-.device .pholio-mock-carousel-thumb-item {
+.pholio-mock-thumb-grid-item-obsolete {
-  width: 5px;
+  opacity: 0.5;
-  height: 5px;
-  padding: 0px;
-  border-radius: 5px;
-  margin: 5px 2px;
-  background: #383838;
-  border-color: #686868;
 }
 
-.device .pholio-mock-carousel-thumb-current {
+.pholio-mock-thumb-grid-image {
-  background: #dfdfdf;
-  border-color: #ffffff;
-}
-
-.device .pholio-mock-carousel-thumb-item img {
-  display: none;
-}
-
-.pholio-mock-carousel-thumbnail {
   margin: auto;
   position: relative;
 }
@@ -78,10 +69,6 @@
   opacity: 0.50;
 }
 
-.pholio-image-info {
-  border-bottom: 1px solid #101010;
-  margin-bottom: 5px;
-}
 
 .pholio-image-info-item {
   padding: 0 8px;

webroot/rsrc/js/application/pholio/behavior-pholio-mock-view.js

@@ -110,6 +110,15 @@ JX.behavior('pholio-mock-view', function(config) {
     return null;
   }
 
+  function get_image_navindex(id) {
+    for (var ii = 0; ii < config.navsequence.length; ii++) {
+      if (config.navsequence[ii] == id) {
+        return ii;
+      }
+    }
+    return null;
+  }
+
   function get_image(id) {
     var idx = get_image_index(id);
     if (idx === null) {
@@ -133,9 +142,12 @@ JX.behavior('pholio-mock-view', function(config) {
     if (!active_image) {
       return;
     }
-    var idx = get_image_index(active_image.id);
+    var idx = get_image_navindex(active_image.id);
-    idx = (idx + delta + config.images.length) % config.images.length;
+    if (idx === null) {
-    select_image(config.images[idx].id);
+      return;
+    }
+    idx = (idx + delta + config.navsequence.length) % config.navsequence.length;
+    select_image(config.navsequence[idx]);
   }
 
   function redraw_image() {
@@ -197,7 +209,7 @@ JX.behavior('pholio-mock-view', function(config) {
     img.src = active_image.fullURI;
 
     var thumbs = JX.DOM.scry(
-      JX.$('pholio-mock-carousel'),
+      JX.$('pholio-mock-thumb-grid'),
       'a',
       'mock-thumbnail');
 
@@ -206,7 +218,7 @@ JX.behavior('pholio-mock-view', function(config) {
 
       JX.DOM.alterClass(
         thumbs[k],
-        'pholio-mock-carousel-thumb-current',
+        'pholio-mock-thumb-grid-current',
         (active_image.id == thumb_meta.imageID));
     }
 
@@ -217,7 +229,7 @@ JX.behavior('pholio-mock-view', function(config) {
   }
 
   JX.Stratcom.listen(
-    ['mousedown', 'click'],
+    'click',
     'mock-thumbnail',
     function(e) {
       if (!e.isNormalMouseEvent()) {
@@ -238,10 +250,6 @@ JX.behavior('pholio-mock-view', function(config) {
       return;
     }
 
-    if (config.viewMode == 'history') {
-      return;
-    }
-
     if (JX.Stratcom.pass()) {
       return;
     }
@@ -602,15 +610,6 @@ JX.behavior('pholio-mock-view', function(config) {
       'View Full Image');
     info.push(full_link);
 
-    if (config.viewMode != 'history') {
-      var history_link = JX.$N(
-        'a',
-        { href: image.historyURI },
-        'View Image History');
-      info.push(history_link);
-    }
-
-
     for (var ii = 0; ii < info.length; ii++) {
       info[ii] = JX.$N('div', {className: 'pholio-image-info-item'}, info[ii]);
     }