Pholio - support editing images - fixes T3489

Summary:
Nice title. We add three new transactions - IMAGE_FILE, IMAGE_NAME, and IMAGE_DESCRIPTION. The first is a bit like subscribers as it is a list of file phids. The latter have values of the form ($file_phid => $data), where $data is $name or $description respectively. This is because we need to collate transactions based on $file_phid...

Overall, this uses the _underyling files_ and not the "PholioImage" to determine if things are unique or not. That said, simply mark PholioImages as obsolete so inline comments about no-longer applicable PholioImages don't break.

Does a reasonable job implementing the mock. Note you can't "update" an image at this time, though you can delete and add at will.

Test Plan: played with pholio a ton.

Reviewers: epriestley

Reviewed By: epriestley

CC: chad, aran, Korvin

Maniphest Tasks: T3489

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

resources/sql/patches/20130711.pholioimageobsolete.php

@@ -0,0 +1,23 @@
+<?php
+
+echo "Giving pholio images PHIDs";
+$table = new PholioImage();
+$table->openTransaction();
+
+foreach (new LiskMigrationIterator($table) as $image) {
+  if ($image->getPHID()) {
+    continue;
+  }
+
+  echo ".";
+
+  queryfx(
+    $image->establishConnection('w'),
+    'UPDATE %T SET phid = %s WHERE id = %d',
+    $image->getTableName(),
+    $image->generatePHID(),
+    $image->getID());
+}
+
+$table->saveTransaction();
+echo "\nDone.\n";

resources/sql/patches/20130711.pholioimageobsolete.sql

@@ -0,0 +1,14 @@
+ALTER TABLE {$NAMESPACE}_pholio.pholio_image
+  ADD `isObsolete` TINYINT(1) NOT NULL DEFAULT '0';
+
+ALTER TABLE {$NAMESPACE}_pholio.pholio_image
+  DROP KEY `mockID`;
+
+ALTER TABLE {$NAMESPACE}_pholio.pholio_image
+  ADD KEY `mockID` (`mockID`, `isObsolete`, `sequence`);
+
+ALTER TABLE {$NAMESPACE}_pholio.pholio_image
+  ADD `phid` VARCHAR(64) NOT NULL COLLATE utf8_bin AFTER `id`;
+
+ALTER TABLE {$NAMESPACE}_pholio.pholio_image
+  CHANGE `mockID` `mockID` INT(10) UNSIGNED;

resources/sql/patches/20130711.pholioimageobsolete2.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_pholio.pholio_image
+  ADD UNIQUE KEY `keyPHID` (`phid`);

src/__celerity_resource_map__.php

@@ -1270,7 +1270,7 @@ celerity_register_resource_map(array(
   ),
   'javelin-behavior-aphront-drag-and-drop' =>
   array(
-    'uri' => '/res/36335362/rsrc/js/core/behavior-drag-and-drop.js',
+    'uri' => '/res/fde3763f/rsrc/js/core/behavior-drag-and-drop.js',
     'type' => 'js',
     'requires' =>
     array(
@@ -3727,6 +3727,15 @@ celerity_register_resource_map(array(
     ),
     'disk' => '/rsrc/css/application/pholio/pholio.css',
   ),
+  'pholio-edit-css' =>
+  array(
+    'uri' => '/res/c4a1f392/rsrc/css/application/pholio/pholio-edit.css',
+    'type' => 'css',
+    'requires' =>
+    array(
+    ),
+    'disk' => '/rsrc/css/application/pholio/pholio-edit.css',
+  ),
   'pholio-inline-comments-css' =>
   array(
     'uri' => '/res/006fc575/rsrc/css/application/pholio/pholio-inline-comments.css',
@@ -4279,7 +4288,7 @@ celerity_register_resource_map(array(
       'uri' => '/res/pkg/dd27a69b/differential.pkg.css',
       'type' => 'css',
     ),
-    '504ca7d2' =>
+    'bb59a901' =>
     array(
       'name' => 'differential.pkg.js',
       'symbols' =>
@@ -4305,7 +4314,7 @@ celerity_register_resource_map(array(
         18 => 'javelin-behavior-differential-toggle-files',
         19 => 'javelin-behavior-differential-user-select',
       ),
-      'uri' => '/res/pkg/504ca7d2/differential.pkg.js',
+      'uri' => '/res/pkg/bb59a901/differential.pkg.js',
       'type' => 'js',
     ),
     'c8ce2d88' =>
@@ -4403,7 +4412,7 @@ celerity_register_resource_map(array(
     'aphront-typeahead-control-css' => 'f32a863a',
     'differential-changeset-view-css' => 'dd27a69b',
     'differential-core-view-css' => 'dd27a69b',
-    'differential-inline-comment-editor' => '504ca7d2',
+    'differential-inline-comment-editor' => 'bb59a901',
     'differential-local-commits-view-css' => 'dd27a69b',
     'differential-results-table-css' => 'dd27a69b',
     'differential-revision-add-comment-css' => 'dd27a69b',
@@ -4421,24 +4430,24 @@ celerity_register_resource_map(array(
     'javelin-behavior-aphlict-dropdown' => '75ccea43',
     'javelin-behavior-aphlict-listen' => '75ccea43',
     'javelin-behavior-aphront-basic-tokenizer' => '75ccea43',
-    'javelin-behavior-aphront-drag-and-drop' => '504ca7d2',
+    'javelin-behavior-aphront-drag-and-drop' => 'bb59a901',
-    'javelin-behavior-aphront-drag-and-drop-textarea' => '504ca7d2',
+    'javelin-behavior-aphront-drag-and-drop-textarea' => 'bb59a901',
     'javelin-behavior-aphront-form-disable-on-submit' => '75ccea43',
     'javelin-behavior-audit-preview' => '96909266',
     'javelin-behavior-dark-console' => '4ccfeb47',
     'javelin-behavior-device' => '75ccea43',
-    'javelin-behavior-differential-accept-with-errors' => '504ca7d2',
+    'javelin-behavior-differential-accept-with-errors' => 'bb59a901',
-    'javelin-behavior-differential-add-reviewers-and-ccs' => '504ca7d2',
+    'javelin-behavior-differential-add-reviewers-and-ccs' => 'bb59a901',
-    'javelin-behavior-differential-comment-jump' => '504ca7d2',
+    'javelin-behavior-differential-comment-jump' => 'bb59a901',
-    'javelin-behavior-differential-diff-radios' => '504ca7d2',
+    'javelin-behavior-differential-diff-radios' => 'bb59a901',
-    'javelin-behavior-differential-dropdown-menus' => '504ca7d2',
+    'javelin-behavior-differential-dropdown-menus' => 'bb59a901',
-    'javelin-behavior-differential-edit-inline-comments' => '504ca7d2',
+    'javelin-behavior-differential-edit-inline-comments' => 'bb59a901',
-    'javelin-behavior-differential-feedback-preview' => '504ca7d2',
+    'javelin-behavior-differential-feedback-preview' => 'bb59a901',
-    'javelin-behavior-differential-keyboard-navigation' => '504ca7d2',
+    'javelin-behavior-differential-keyboard-navigation' => 'bb59a901',
-    'javelin-behavior-differential-populate' => '504ca7d2',
+    'javelin-behavior-differential-populate' => 'bb59a901',
-    'javelin-behavior-differential-show-more' => '504ca7d2',
+    'javelin-behavior-differential-show-more' => 'bb59a901',
-    'javelin-behavior-differential-toggle-files' => '504ca7d2',
+    'javelin-behavior-differential-toggle-files' => 'bb59a901',
-    'javelin-behavior-differential-user-select' => '504ca7d2',
+    'javelin-behavior-differential-user-select' => 'bb59a901',
     'javelin-behavior-diffusion-commit-graph' => '96909266',
     'javelin-behavior-diffusion-pull-lastmodified' => '96909266',
     'javelin-behavior-error-log' => '4ccfeb47',
@@ -4446,7 +4455,7 @@ celerity_register_resource_map(array(
     'javelin-behavior-history-install' => '75ccea43',
     'javelin-behavior-konami' => '75ccea43',
     'javelin-behavior-lightbox-attachments' => '75ccea43',
-    'javelin-behavior-load-blame' => '504ca7d2',
+    'javelin-behavior-load-blame' => 'bb59a901',
     'javelin-behavior-maniphest-batch-selector' => '98f64f07',
     'javelin-behavior-maniphest-subpriority-editor' => '98f64f07',
     'javelin-behavior-maniphest-transaction-controls' => '98f64f07',
@@ -4458,7 +4467,7 @@ celerity_register_resource_map(array(
     'javelin-behavior-phabricator-hovercards' => '75ccea43',
     'javelin-behavior-phabricator-keyboard-shortcuts' => '75ccea43',
     'javelin-behavior-phabricator-nav' => '75ccea43',
-    'javelin-behavior-phabricator-object-selector' => '504ca7d2',
+    'javelin-behavior-phabricator-object-selector' => 'bb59a901',
     'javelin-behavior-phabricator-oncopy' => '75ccea43',
     'javelin-behavior-phabricator-remarkup-assist' => '75ccea43',
     'javelin-behavior-phabricator-reveal-content' => '75ccea43',
@@ -4466,7 +4475,7 @@ celerity_register_resource_map(array(
     'javelin-behavior-phabricator-tooltips' => '75ccea43',
     'javelin-behavior-phabricator-watch-anchor' => '75ccea43',
     'javelin-behavior-refresh-csrf' => '75ccea43',
-    'javelin-behavior-repository-crossreference' => '504ca7d2',
+    'javelin-behavior-repository-crossreference' => 'bb59a901',
     'javelin-behavior-toggle-class' => '75ccea43',
     'javelin-behavior-workflow' => '75ccea43',
     'javelin-dom' => 'a9f14d76',
@@ -4497,7 +4506,7 @@ celerity_register_resource_map(array(
     'phabricator-content-source-view-css' => 'dd27a69b',
     'phabricator-core-css' => 'f32a863a',
     'phabricator-crumbs-view-css' => 'f32a863a',
-    'phabricator-drag-and-drop-file-upload' => '504ca7d2',
+    'phabricator-drag-and-drop-file-upload' => 'bb59a901',
     'phabricator-dropdown-menu' => '75ccea43',
     'phabricator-file-upload' => '75ccea43',
     'phabricator-filetree-view-css' => 'f32a863a',
@@ -4521,7 +4530,7 @@ celerity_register_resource_map(array(
     'phabricator-project-tag-css' => 'adc3c36d',
     'phabricator-property-list-view-css' => 'f32a863a',
     'phabricator-remarkup-css' => 'f32a863a',
-    'phabricator-shaped-request' => '504ca7d2',
+    'phabricator-shaped-request' => 'bb59a901',
     'phabricator-side-menu-view-css' => 'f32a863a',
     'phabricator-standard-page-view' => 'f32a863a',
     'phabricator-tag-view-css' => 'f32a863a',

src/__phutil_library_map__.php

@@ -14,6 +14,8 @@ phutil_register_library_map(array(
     'Aphront400Response' => 'aphront/response/Aphront400Response.php',
     'Aphront403Response' => 'aphront/response/Aphront403Response.php',
     'Aphront404Response' => 'aphront/response/Aphront404Response.php',
+    'AphrontAbstractAttachedFileView' => 'view/control/AphrontAbstractAttachedFileView.php',
+    'AphrontAbstractFormDragAndDropUploadControl' => 'view/form/control/AphrontAbstractFormDragAndDropUploadControl.php',
     'AphrontAjaxResponse' => 'aphront/response/AphrontAjaxResponse.php',
     'AphrontApplicationConfiguration' => 'aphront/configuration/AphrontApplicationConfiguration.php',
     'AphrontAttachedFileView' => 'view/control/AphrontAttachedFileView.php',
@@ -1713,6 +1715,8 @@ phutil_register_library_map(array(
     'PholioConstants' => 'applications/pholio/constants/PholioConstants.php',
     'PholioController' => 'applications/pholio/controller/PholioController.php',
     'PholioDAO' => 'applications/pholio/storage/PholioDAO.php',
+    'PholioDragAndDropUploadControl' => 'applications/pholio/view/PholioDragAndDropUploadControl.php',
+    'PholioDropUploadController' => 'applications/pholio/controller/PholioDropUploadController.php',
     'PholioImage' => 'applications/pholio/storage/PholioImage.php',
     'PholioInlineCommentEditView' => 'applications/pholio/view/PholioInlineCommentEditView.php',
     'PholioInlineCommentSaveView' => 'applications/pholio/view/PholioInlineCommentSaveView.php',
@@ -1741,6 +1745,7 @@ phutil_register_library_map(array(
     'PholioTransactionQuery' => 'applications/pholio/query/PholioTransactionQuery.php',
     'PholioTransactionType' => 'applications/pholio/constants/PholioTransactionType.php',
     'PholioTransactionView' => 'applications/pholio/view/PholioTransactionView.php',
+    'PholioUploadedImageView' => 'applications/pholio/view/PholioUploadedImageView.php',
     'PhortuneAccount' => 'applications/phortune/storage/PhortuneAccount.php',
     'PhortuneAccountBuyController' => 'applications/phortune/controller/PhortuneAccountBuyController.php',
     'PhortuneAccountEditor' => 'applications/phortune/editor/PhortuneAccountEditor.php',
@@ -1967,8 +1972,10 @@ phutil_register_library_map(array(
     'Aphront400Response' => 'AphrontResponse',
     'Aphront403Response' => 'AphrontHTMLResponse',
     'Aphront404Response' => 'AphrontHTMLResponse',
+    'AphrontAbstractAttachedFileView' => 'AphrontView',
+    'AphrontAbstractFormDragAndDropUploadControl' => 'AphrontFormControl',
     'AphrontAjaxResponse' => 'AphrontResponse',
-    'AphrontAttachedFileView' => 'AphrontView',
+    'AphrontAttachedFileView' => 'AphrontAbstractAttachedFileView',
     'AphrontBarView' => 'AphrontView',
     'AphrontCSRFException' => 'AphrontException',
     'AphrontCalendarEventView' => 'AphrontView',
@@ -1988,7 +1995,7 @@ phutil_register_library_map(array(
     'AphrontFormCropControl' => 'AphrontFormControl',
     'AphrontFormDateControl' => 'AphrontFormControl',
     'AphrontFormDividerControl' => 'AphrontFormControl',
-    'AphrontFormDragAndDropUploadControl' => 'AphrontFormControl',
+    'AphrontFormDragAndDropUploadControl' => 'AphrontAbstractFormDragAndDropUploadControl',
     'AphrontFormFileControl' => 'AphrontFormControl',
     'AphrontFormImageControl' => 'AphrontFormControl',
     'AphrontFormInsetView' => 'AphrontView',
@@ -3699,6 +3706,8 @@ phutil_register_library_map(array(
     'PhluxViewController' => 'PhluxController',
     'PholioController' => 'PhabricatorController',
     'PholioDAO' => 'PhabricatorLiskDAO',
+    'PholioDragAndDropUploadControl' => 'AphrontAbstractFormDragAndDropUploadControl',
+    'PholioDropUploadController' => 'PholioController',
     'PholioImage' =>
     array(
       0 => 'PholioDAO',
@@ -3739,6 +3748,7 @@ phutil_register_library_map(array(
     'PholioTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PholioTransactionType' => 'PholioConstants',
     'PholioTransactionView' => 'PhabricatorApplicationTransactionView',
+    'PholioUploadedImageView' => 'AphrontAbstractAttachedFileView',
     'PhortuneAccount' =>
     array(
       0 => 'PhortuneDAO',

src/applications/files/controller/PhabricatorFileDropUploadController.php

@@ -3,6 +3,20 @@
 final class PhabricatorFileDropUploadController
   extends PhabricatorFileController {
 
+  private $viewObject;
+
+  public function setViewObject(AphrontAbstractAttachedFileView $view) {
+    $this->viewObject = $view;
+    return $this;
+  }
+
+  public function getViewObject() {
+    if (!$this->viewObject) {
+      $this->viewObject = new AphrontAttachedFileView();
+    }
+    return $this->viewObject;
+  }
+
   /**
    * @phutil-external-symbol class PhabricatorStartup
    */
@@ -24,7 +38,7 @@ final class PhabricatorFileDropUploadController
         'isExplicitUpload' => true,
       ));
 
-    $view = new AphrontAttachedFileView();
+    $view = $this->getViewObject();
     $view->setFile($file);
 
     return id(new AphrontAjaxResponse())->setContent(

src/applications/phid/PhabricatorObjectHandle.php

@@ -107,6 +107,7 @@ final class PhabricatorObjectHandle {
       PhabricatorPHIDConstants::PHID_TYPE_WIKI => 'Phriction Document',
       PhabricatorPHIDConstants::PHID_TYPE_MCRO => 'Image Macro',
       PhabricatorPHIDConstants::PHID_TYPE_MOCK => 'Pholio Mock',
+      PhabricatorPHIDConstants::PHID_TYPE_PIMG => 'Pholio Image',
       PhabricatorPHIDConstants::PHID_TYPE_FILE => 'File',
       PhabricatorPHIDConstants::PHID_TYPE_BLOG => 'Blog',
       PhabricatorPHIDConstants::PHID_TYPE_POST => 'Post',

src/applications/phid/PhabricatorPHIDConstants.php

@@ -29,6 +29,7 @@ final class PhabricatorPHIDConstants {
   const PHID_TYPE_QUES    = 'QUES';
   const PHID_TYPE_ANSW    = 'ANSW';
   const PHID_TYPE_MOCK    = 'MOCK';
+  const PHID_TYPE_PIMG    = 'PIMG';
   const PHID_TYPE_MCRO    = 'MCRO';
   const PHID_TYPE_CONF    = 'CONF';
   const PHID_TYPE_CONP    = 'CONP';

src/applications/phid/handle/PhabricatorObjectHandleData.php

@@ -137,6 +137,11 @@ final class PhabricatorObjectHandleData {
           ->execute();
         return mpull($mocks, null, 'getPHID');
 
+      case PhabricatorPHIDConstants::PHID_TYPE_PIMG:
+        $images = id(new PholioImage())
+          ->loadAllWhere('phid IN (%Ls)', $phids);
+        return mpull($images, null, 'getPHID');
+
       case PhabricatorPHIDConstants::PHID_TYPE_POLL:
         $polls = id(new PhabricatorSlowvotePoll())
           ->loadAllWhere('phid IN (%Ls)', $phids);
@@ -649,6 +654,26 @@ final class PhabricatorObjectHandleData {
           }
           break;
 
+        case PhabricatorPHIDConstants::PHID_TYPE_PIMG:
+          foreach ($phids as $phid) {
+            $handle = new PhabricatorObjectHandle();
+            $handle->setPHID($phid);
+            $handle->setType($type);
+            if (empty($objects[$phid])) {
+              $handle->setName('Unknown Image');
+            } else {
+              $image = $objects[$phid];
+              $handle->setName($image->getName());
+              $handle->setFullName($image->getName());
+              $handle->setURI(
+                '/M'.$image->getMockID().'/'.$image->getID().'/');
+              $handle->setComplete(true);
+            }
+            $handles[$phid] = $handle;
+          }
+          break;
+
+
         case PhabricatorPHIDConstants::PHID_TYPE_POLL:
           foreach ($phids as $phid) {
             $handle = new PhabricatorObjectHandle();

src/applications/pholio/application/PhabricatorApplicationPholio.php

@@ -57,6 +57,7 @@ final class PhabricatorApplicationPholio extends PhabricatorApplication {
           'edit/(?P<id>\d+)/' => 'PholioInlineEditController',
           'thumb/(?P<imageid>\d+)/' => 'PholioInlineThumbController'
         ),
+        'image/upload/' => 'PholioDropUploadController',
       ),
     );
   }

src/applications/pholio/config/PhabricatorPholioConfigOptions.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group pholio
+ */
 final class PhabricatorPholioConfigOptions
   extends PhabricatorApplicationConfigOptions {
 

src/applications/pholio/constants/PholioConstants.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group pholio
+ */
 abstract class PholioConstants {
 
 }

src/applications/pholio/constants/PholioTransactionType.php

@@ -1,9 +1,19 @@
 <?php
 
+/**
+ * @group pholio
+ */
 final class PholioTransactionType extends PholioConstants {
 
+  /* edits to the high level mock */
   const TYPE_NAME         = 'name';
   const TYPE_DESCRIPTION  = 'description';
-  const TYPE_INLINE  = 'inline';
 
+  /* edits to images within the mock */
+  const TYPE_IMAGE_FILE = 'image-file';
+  const TYPE_IMAGE_NAME= 'image-name';
+  const TYPE_IMAGE_DESCRIPTION = 'image-description';
+
+  /* your witty commentary at the mock : image : x,y level */
+  const TYPE_INLINE  = 'inline';
 }

src/applications/pholio/controller/PholioDropUploadController.php

@@ -0,0 +1,14 @@
+<?php
+
+/**
+ * @group pholio
+ */
+final class PholioDropUploadController extends PholioController {
+
+  public function processRequest() {
+    return $this->delegateToController(
+      id(new PhabricatorFileDropUploadController($this->getRequest()))
+      ->setViewObject(new PholioUploadedImageView()));
+  }
+
+}

src/applications/pholio/controller/PholioMockEditController.php

@@ -15,10 +15,10 @@ final class PholioMockEditController extends PholioController {
     $request = $this->getRequest();
     $user = $request->getUser();
 
-
     if ($this->id) {
       $mock = id(new PholioMockQuery())
         ->setViewer($user)
+        ->needImages(true)
         ->requireCapabilities(
           array(
             PhabricatorPolicyCapability::CAN_VIEW,
@@ -34,14 +34,20 @@ final class PholioMockEditController extends PholioController {
       $title = pht('Edit Mock');
 
       $is_new = false;
+      $mock_images = $mock->getImages();
+      $files = mpull($mock_images, 'getFile');
+      $mock_images = mpull($mock_images, null, 'getFilePHID');
     } else {
-      $mock = new PholioMock();
+      $mock = id(new PholioMock())
-      $mock->setAuthorPHID($user->getPHID());
+        ->setAuthorPHID($user->getPHID())
-      $mock->setViewPolicy(PhabricatorPolicies::POLICY_USER);
+        ->attachImages(array())
+        ->setViewPolicy(PhabricatorPolicies::POLICY_USER);
 
       $title = pht('Create Mock');
 
       $is_new = true;
+      $files = array();
+      $mock_images = array();
     }
 
     $e_name = true;
@@ -53,7 +59,6 @@ final class PholioMockEditController extends PholioController {
     $v_view = $mock->getViewPolicy();
     $v_cc = PhabricatorSubscribersQuery::loadSubscribersForPHID(
       $mock->getPHID());
-    $files = array();
 
     if ($request->isFormPost()) {
       $xactions = array();
@@ -68,54 +73,80 @@ final class PholioMockEditController extends PholioController {
       $v_view = $request->getStr('can_view');
       $v_cc   = $request->getArr('cc');
 
-      $xactions[$type_name] = $v_name;
+      $mock_xactions = array();
-      $xactions[$type_desc] = $v_desc;
+      $mock_xactions[$type_name] = $v_name;
-      $xactions[$type_view] = $v_view;
+      $mock_xactions[$type_desc] = $v_desc;
-      $xactions[$type_cc]   = array('=' => $v_cc);
+      $mock_xactions[$type_view] = $v_view;
+      $mock_xactions[$type_cc]   = array('=' => $v_cc);
 
       if (!strlen($request->getStr('name'))) {
         $e_name = 'Required';
-        $errors[] = pht('You must name the mock.');
       }
 
-      $images = array();
+      $file_phids = $request->getArr('file_phids');
-      if ($is_new) {
+      if ($file_phids) {
-        // TODO: Make this transactional and allow edits?
+        $files = id(new PhabricatorFileQuery())
+          ->setViewer($user)
+          ->withPHIDs($file_phids)
+          ->execute();
+        $files = mpull($files, null, 'getPHID');
+        $files = array_select_keys($files, $file_phids);
+      }
 
-
+      if (!$files) {
-        $file_phids = $request->getArr('file_phids');
+        $e_images = pht('Required');
-        if ($file_phids) {
+        $errors[] = pht('You must add at least one image to the mock.');
-          $files = id(new PhabricatorFileQuery())
+      } else {
-            ->setViewer($user)
+        $mock->setCoverPHID(head($files)->getPHID());
-            ->withPHIDs($file_phids)
-            ->execute();
-        }
-
-        if (!$files) {
-          $e_images = pht('Required');
-          $errors[] = pht('You must add at least one image to the mock.');
-        } else {
-          $mock->setCoverPHID(head($files)->getPHID());
-        }
-
-        $sequence = 0;
-
-        foreach ($files as $file) {
-          $image = new PholioImage();
-          $image->setFilePHID($file->getPHID());
-          $image->setSequence($sequence++);
-
-          $images[] = $image;
-        }
       }
 
       if (!$errors) {
-        foreach ($xactions as $type => $value) {
+        foreach ($mock_xactions as $type => $value) {
           $xactions[$type] = id(new PholioTransaction())
             ->setTransactionType($type)
             ->setNewValue($value);
         }
 
+        $sequence = 0;
+        foreach ($files as $file_phid => $file) {
+          $mock_image = idx($mock_images, $file_phid);
+          $title = $request->getStr('title_'.$file_phid);
+          $description = $request->getStr('description_'.$file_phid);
+          if (!$mock_image) {
+            // this is an add
+            $add_image = id(new PholioImage())
+              ->setFilePhid($file_phid)
+              ->setName(strlen($title) ? $title : $file->getName())
+              ->setDescription($description)
+              ->setSequence($sequence);
+            $xactions[] = id(new PholioTransaction())
+              ->setTransactionType(PholioTransactionType::TYPE_IMAGE_FILE)
+              ->setNewValue(
+                array('+' => array($add_image)));
+          } else {
+            // update (maybe)
+            $xactions[] = id(new PholioTransaction())
+              ->setTransactionType(PholioTransactionType::TYPE_IMAGE_NAME)
+              ->setNewValue(
+                array($mock_image->getPHID() => $title));
+            $xactions[] = id(new PholioTransaction())
+              ->setTransactionType(
+                PholioTransactionType::TYPE_IMAGE_DESCRIPTION)
+              ->setNewValue(array($mock_image->getPHID() => $description));
+            $mock_image->setSequence($sequence);
+          }
+          $sequence++;
+        }
+        foreach ($mock_images as $file_phid => $mock_image) {
+          if (!isset($files[$file_phid])) {
+            // this is a delete
+            $xactions[] = id(new PholioTransaction())
+              ->setTransactionType(PholioTransactionType::TYPE_IMAGE_FILE)
+              ->setNewValue(
+                array('-' => array($mock_image)));
+          }
+        }
+
         $mock->openTransaction();
           $editor = id(new PholioMockEditor())
             ->setContentSourceFromRequest($request)
@@ -124,13 +155,6 @@ final class PholioMockEditController extends PholioController {
 
           $xactions = $editor->applyTransactions($mock, $xactions);
 
-          if ($images) {
-            foreach ($images as $image) {
-              // TODO: Move into editor?
-              $image->setMockID($mock->getID());
-              $image->save();
-            }
-          }
         $mock->saveTransaction();
 
         return id(new AphrontRedirectResponse())
@@ -170,17 +194,17 @@ final class PholioMockEditController extends PholioController {
 
     $cc_tokens = mpull($handles, 'getFullName', 'getPHID');
 
-    $images_controller = '';
+    $images_controller =
-    if ($is_new) {
+      id(new PholioDragAndDropUploadControl($request))
-      $images_controller =
+      ->setUploadURI($this->getApplicationURI('image/upload/'))
-          id(new AphrontFormDragAndDropUploadControl($request))
+      ->setValue($files)
-            ->setValue($files)
+      ->setImages($mock_images)
-            ->setName('file_phids')
+      ->setName('file_phids')
-            ->setLabel(pht('Images'))
+      ->setLabel(pht('Images'))
-            ->setActivatedClass('aphront-textarea-drag-and-drop')
+      ->setActivatedClass('aphront-textarea-drag-and-drop')
-            ->setError($e_images);
+      ->setError($e_images);
-    }
 
+    require_celerity_resource('pholio-edit-css');
     $form = id(new AphrontFormView())
       ->setUser($user)
       ->setFlexible(true)
@@ -196,7 +220,6 @@ final class PholioMockEditController extends PholioController {
           ->setValue($v_desc)
           ->setLabel(pht('Description'))
           ->setUser($user))
-      ->appendChild($images_controller)
       ->appendChild(
         id(new AphrontFormTokenizerControl())
           ->setLabel(pht('CC'))
@@ -211,6 +234,7 @@ final class PholioMockEditController extends PholioController {
           ->setPolicyObject($mock)
           ->setPolicies($policies)
           ->setName('can_view'))
+      ->appendChild($images_controller)
       ->appendChild($submit);
 
     $crumbs = $this->buildApplicationCrumbs($this->buildSideNav());

src/applications/pholio/editor/PholioMockEditor.php

@@ -5,6 +5,16 @@
  */
 final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
 
+  private $newImages = array();
+  private function setNewImages(array $new_images) {
+    assert_instances_of($new_images, 'PholioImage');
+    $this->newImages = $new_images;
+    return $this;
+  }
+  private function getNewImages() {
+    return $this->newImages;
+  }
+
   public function getTransactionTypes() {
     $types = parent::getTransactionTypes();
 
@@ -15,6 +25,11 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     $types[] = PholioTransactionType::TYPE_NAME;
     $types[] = PholioTransactionType::TYPE_DESCRIPTION;
     $types[] = PholioTransactionType::TYPE_INLINE;
+
+    $types[] = PholioTransactionType::TYPE_IMAGE_FILE;
+    $types[] = PholioTransactionType::TYPE_IMAGE_NAME;
+    $types[] = PholioTransactionType::TYPE_IMAGE_DESCRIPTION;
+
     return $types;
   }
 
@@ -27,6 +42,27 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
         return $object->getName();
       case PholioTransactionType::TYPE_DESCRIPTION:
         return $object->getDescription();
+      case PholioTransactionType::TYPE_IMAGE_FILE:
+        $images = $object->getImages();
+        return mpull($images, 'getPHID');
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+        $name = null;
+        $phid = null;
+        $image = $this->getImageForXaction($object, $xaction);
+        if ($image && $image->getName()) {
+          $name = $image->getName();
+          $phid = $image->getPHID();
+        }
+        return array ($phid => $name);
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
+        $description = null;
+        $phid = null;
+        $image = $this->getImageForXaction($object, $xaction);
+        if ($image && $image->getDescription()) {
+          $description = $image->getDescription();
+          $phid = $image->getPHID();
+        }
+        return array($phid => $description);
     }
   }
 
@@ -37,7 +73,17 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     switch ($xaction->getTransactionType()) {
       case PholioTransactionType::TYPE_NAME:
       case PholioTransactionType::TYPE_DESCRIPTION:
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
         return $xaction->getNewValue();
+      case PholioTransactionType::TYPE_IMAGE_FILE:
+        $raw_new_value = $xaction->getNewValue();
+        $new_value = array();
+        foreach ($raw_new_value as $key => $images) {
+          $new_value[$key] = mpull($images, 'getPHID');
+        }
+        $xaction->setNewValue($new_value);
+        return $this->getPHIDTransactionNewValue($xaction);
     }
   }
 
@@ -53,6 +99,44 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     return parent::transactionHasEffect($object, $xaction);
   }
 
+  protected function shouldApplyInitialEffects(
+    PhabricatorLiskDAO $object,
+    array $xactions) {
+
+    foreach ($xactions as $xaction) {
+      switch ($xaction->getTransactionType()) {
+        case PholioTransactionType::TYPE_IMAGE_FILE:
+          return true;
+          break;
+      }
+    }
+    return false;
+  }
+
+  protected function applyInitialEffects(
+    PhabricatorLiskDAO $object,
+    array $xactions) {
+
+    $new_images = array();
+    foreach ($xactions as $xaction) {
+      switch ($xaction->getTransactionType()) {
+        case PholioTransactionType::TYPE_IMAGE_FILE:
+          $new_value = $xaction->getNewValue();
+          foreach ($new_value as $key => $txn_images) {
+            if ($key != '+') {
+              continue;
+            }
+            foreach ($txn_images as $image) {
+              $image->save();
+              $new_images[] = $image;
+            }
+          }
+          break;
+      }
+    }
+    $this->setNewImages($new_images);
+  }
+
   protected function applyCustomInternalTransaction(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
@@ -70,10 +154,64 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     }
   }
 
+  private function getImageForXaction(
+    PholioMock $mock,
+    PhabricatorApplicationTransaction $xaction) {
+    $raw_new_value = $xaction->getNewValue();
+    $image_phid = key($raw_new_value);
+    $images = $mock->getImages();
+    foreach ($images as $image) {
+      if ($image->getPHID() == $image_phid) {
+        return $image;
+      }
+    }
+    return null;
+  }
+
   protected function applyCustomExternalTransaction(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
-    return;
+
+    switch ($xaction->getTransactionType()) {
+      case PholioTransactionType::TYPE_IMAGE_FILE:
+        $old_map = array_fuse($xaction->getOldValue());
+        $new_map = array_fuse($xaction->getNewValue());
+
+        $obsolete_map = array_diff_key($old_map, $new_map);
+        $images = $object->getImages();
+        foreach ($images as $seq => $image) {
+          if (isset($obsolete_map[$image->getPHID()])) {
+            $image->setIsObsolete(1);
+            $image->save();
+            unset($images[$seq]);
+          }
+        }
+        $object->attachImages($images);
+        break;
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+        $image = $this->getImageForXaction($object, $xaction);
+        $value = (string) head($xaction->getNewValue());
+        $image->setName($value);
+        $image->save();
+        break;
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
+        $image = $this->getImageForXaction($object, $xaction);
+        $value = (string) head($xaction->getNewValue());
+        $image->setDescription($value);
+        $image->save();
+        break;
+    }
+  }
+
+  protected function applyFinalEffects(
+    PhabricatorLiskDAO $object,
+    array $xactions) {
+
+    $images = $this->getNewImages();
+    foreach ($images as $image) {
+      $image->setMockID($object->getID());
+      $image->save();
+    }
   }
 
   protected function mergeTransactions(
@@ -85,6 +223,18 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
       case PholioTransactionType::TYPE_NAME:
       case PholioTransactionType::TYPE_DESCRIPTION:
         return $v;
+      case PholioTransactionType::TYPE_IMAGE_FILE:
+        return $this->mergePHIDOrEdgeTransactions($u, $v);
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
+        $raw_new_value_u = $u->getNewValue();
+        $raw_new_value_v = $v->getNewValue();
+        $phid_u = key($raw_new_value_u);
+        $phid_v = key($raw_new_value_v);
+        if ($phid_u == $phid_v) {
+          return $v;
+        }
+        break;
     }
 
     return parent::mergeTransactions($u, $v);
@@ -126,6 +276,9 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     $inline_comments = array();
 
     foreach ($xactions as $xaction) {
+      if ($xaction->shouldHide()) {
+        continue;
+      }
       $comment = $xaction->getComment();
       switch ($xaction->getTransactionType()) {
         case PholioTransactionType::TYPE_INLINE:
@@ -199,7 +352,6 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     return array_values(array_merge($head, $tail));
   }
 
-
   protected function shouldImplyCC(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {

src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group pholio
+ */
 final class PhabricatorPholioMockTestDataGenerator
   extends PhabricatorTestDataGenerator {
 

src/applications/pholio/mail/PholioMockMailReceiver.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group pholio
+ */
 final class PholioMockMailReceiver extends PhabricatorObjectMailReceiver {
 
   public function isEnabled() {

src/applications/pholio/query/PholioMockQuery.php

@@ -113,8 +113,9 @@ final class PholioMockQuery
 
     $mock_ids = mpull($mocks, 'getID');
     $all_images = id(new PholioImage())->loadAllWhere(
-      'mockID IN (%Ld)',
+      'mockID IN (%Ld) AND isObsolete = %d',
-      $mock_ids);
+      $mock_ids,
+      0);
 
     $file_phids = mpull($all_images, 'getFilePHID');
     $all_files = mpull(id(new PhabricatorFile())->loadAllWhere(
@@ -143,7 +144,8 @@ final class PholioMockQuery
     $image_groups = mgroup($all_images, 'getMockID');
 
     foreach ($mocks as $mock) {
-      $mock->attachImages($image_groups[$mock->getID()]);
+      $mock_images = $image_groups[$mock->getID()];
+      $mock->attachImages($mock_images);
     }
   }
 

src/applications/pholio/remarkup/PholioRemarkupRule.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group pholio
+ */
 final class PholioRemarkupRule
   extends PhabricatorRemarkupRuleObject {
 

src/applications/pholio/storage/PholioImage.php

@@ -13,10 +13,21 @@ final class PholioImage extends PholioDAO
   protected $name = '';
   protected $description = '';
   protected $sequence;
+  protected $isObsolete = 0;
 
   private $inlineComments;
   private $file;
 
+  public function getConfiguration() {
+    return array(
+      self::CONFIG_AUX_PHID => true,
+    ) + parent::getConfiguration();
+  }
+
+  public function generatePHID() {
+    return PhabricatorPHID::generateNewPHID('PIMG');
+  }
+
   public function attachInlineComments(array $inline_comments) {
     assert_instances_of($inline_comments, 'PholioTransactionComment');
     $this->inlineComments = $inline_comments;

src/applications/pholio/storage/PholioTransaction.php

@@ -25,6 +25,26 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
     return pht('mock');
   }
 
+  public function getRequiredHandlePHIDs() {
+    $phids = parent::getRequiredHandlePHIDs();
+    $phids[] = $this->getObjectPHID();
+
+    $new = $this->getNewValue();
+    $old = $this->getOldValue();
+
+    switch ($this->getTransactionType()) {
+      case PholioTransactionType::TYPE_IMAGE_FILE:
+        $phids = array_merge($phids, $new, $old);
+        break;
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+        $phids[] = key($new);
+        break;
+    }
+
+    return $phids;
+  }
+
   public function shouldHide() {
     $old = $this->getOldValue();
 
@@ -32,6 +52,10 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
       case PholioTransactionType::TYPE_NAME:
       case PholioTransactionType::TYPE_DESCRIPTION:
         return ($old === null);
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
+        $old_value = reset($old);
+        return ($old_value === null);
     }
 
     return parent::shouldHide();
@@ -41,7 +65,15 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
     switch ($this->getTransactionType()) {
       case PholioTransactionType::TYPE_INLINE:
         return 'comment';
+      case PholioTransactionType::TYPE_NAME:
+      case PholioTransactionType::TYPE_DESCRIPTION:
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
+        return 'edit';
+      case PholioTransactionType::TYPE_IMAGE_FILE:
+        return 'attach';
     }
+
     return parent::getIcon();
   }
 
@@ -77,6 +109,48 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
           '%s added %d inline comment(s).',
           $this->renderHandleLink($author_phid),
           $count);
+        break;
+      case PholioTransactionType::TYPE_IMAGE_FILE:
+        $add = array_diff($new, $old);
+        $rem = array_diff($old, $new);
+
+        if ($add && $rem) {
+          return pht(
+            '%s edited image(s), added %d: %s; removed %d: %s.',
+            $this->renderHandleLink($author_phid),
+            count($add),
+            $this->renderHandleList($add),
+            count($rem),
+            $this->renderHandleList($rem));
+        } else if ($add) {
+          return pht(
+            '%s added %d image(s): %s.',
+            $this->renderHandleLink($author_phid),
+            count($add),
+            $this->renderHandleList($add));
+        } else {
+          return pht(
+            '%s removed %d image(s): %s.',
+            $this->renderHandleLink($author_phid),
+            count($rem),
+            $this->renderHandleList($rem));
+        }
+        break;
+
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+        return pht(
+          '%s renamed an image (%s) from "%s" to "%s".',
+          $this->renderHandleLink($author_phid),
+          $this->renderHandleLink(key($new)),
+          reset($old),
+          reset($new));
+        break;
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
+        return pht(
+          '%s updated an image\'s (%s) description.',
+          $this->renderHandleLink($author_phid),
+          $this->renderHandleLink(key($new)));
+        break;
     }
 
     return parent::getTitle();
@@ -118,6 +192,24 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
           $this->renderHandleLink($author_phid),
           $this->renderHandleLink($object_phid));
         break;
+      case PholioTransactionType::TYPE_IMAGE_FILE:
+        return pht(
+          '%s updated images of %s.',
+          $this->renderHandleLink($author_phid),
+          $this->renderHandleLink($object_phid));
+        break;
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+        return pht(
+          '%s updated the image names of %s.',
+          $this->renderHandleLink($author_phid),
+          $this->renderHandleLink($object_phid));
+        break;
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
+        return pht(
+          '%s updated image descriptions of %s.',
+          $this->renderHandleLink($author_phid),
+          $this->renderHandleLink($object_phid));
+        break;
     }
 
     return parent::getTitleForFeed();
@@ -126,6 +218,7 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
   public function hasChangeDetails() {
     switch ($this->getTransactionType()) {
       case PholioTransactionType::TYPE_DESCRIPTION:
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
         return true;
     }
     return parent::hasChangeDetails();
@@ -134,6 +227,11 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
   public function renderChangeDetails(PhabricatorUser $viewer) {
     $old = $this->getOldValue();
     $new = $this->getNewValue();
+    if ($this->getTransactionType() ==
+        PholioTransactionType::TYPE_IMAGE_DESCRIPTION) {
+      $old = reset($old);
+      $new = reset($new);
+    }
 
     $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
       ->setUser($viewer)
@@ -143,5 +241,28 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
     return $view->render();
   }
 
+  public function getColor() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
 
+    switch ($this->getTransactionType()) {
+      case PholioTransactionType::TYPE_NAME:
+      case PholioTransactionType::TYPE_DESCRIPTION:
+      case PholioTransactionType::TYPE_IMAGE_NAME:
+      case PholioTransactionType::TYPE_IMAGE_DESCRIPTION:
+        return PhabricatorTransactions::COLOR_BLUE;
+      case PholioTransactionType::TYPE_IMAGE_FILE:
+        $add = array_diff($new, $old);
+        $rem = array_diff($old, $new);
+        if ($add && $rem) {
+          return PhabricatorTransactions::COLOR_YELLOW;
+        } else if ($add) {
+          return PhabricatorTransactions::COLOR_GREEN;
+        } else {
+          return PhabricatorTransactions::COLOR_RED;
+        }
+    }
+
+    return parent::getColor();
+  }
 }

src/applications/pholio/view/PholioDragAndDropUploadControl.php

@@ -0,0 +1,25 @@
+<?php
+
+/**
+ * @group pholio
+ */
+final class PholioDragAndDropUploadControl
+  extends AphrontAbstractFormDragAndDropUploadControl {
+
+  private $images;
+
+  public function setImages(array $images) {
+    assert_instances_of($images, 'PholioImage');
+    $this->images = $images;
+    return $this;
+  }
+  public function getImages() {
+    return $this->images;
+  }
+
+  protected function getFileView() {
+    return id(new PholioUploadedImageView())
+      ->setImages($this->getImages());
+  }
+
+}

src/applications/pholio/view/PholioInlineCommentView.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group pholio
+ */
 final class PholioInlineCommentView extends AphrontView {
 
   private $engine;

src/applications/pholio/view/PholioMockEmbedView.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group pholio
+ */
 final class PholioMockEmbedView extends AphrontView {
 
   private $mock;

src/applications/pholio/view/PholioMockImagesView.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group pholio
+ */
 final class PholioMockImagesView extends AphrontView {
 
   private $mock;

src/applications/pholio/view/PholioTransactionView.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @group pholio
+ */
 final class PholioTransactionView
   extends PhabricatorApplicationTransactionView {
 

src/applications/pholio/view/PholioUploadedImageView.php

@@ -0,0 +1,75 @@
+<?php
+
+/**
+ * @group pholio
+ */
+final class PholioUploadedImageView extends AphrontAbstractAttachedFileView {
+
+  private $images = array();
+
+  public function setImages(array $images) {
+    assert_instances_of($images, 'PholioImage');
+    $this->images = $images;
+    return $this;
+  }
+  public function getImages() {
+    return $this->images;
+  }
+  public function getImage($phid) {
+    $images = $this->getImages();
+    return idx($images, $phid, new PholioImage());
+  }
+
+  public function render() {
+    require_celerity_resource('pholio-edit-css');
+
+    $file = $this->getFile();
+    $phid = $file->getPHID();
+    $image = $this->getImage($phid);
+
+    $thumb = phutil_tag(
+      'img',
+      array(
+        'src'     => $file->getThumb280x210URI(),
+        'width'   => 280,
+        'height'  => 210,
+      ));
+
+    $file_link = $this->getName();
+    if (!$image->getName()) {
+      $image->setName($this->getFile()->getName());
+    }
+    $remove = $this->getRemoveElement();
+
+    $title = id(new AphrontFormTextControl())
+      ->setName('title_'.$phid)
+      ->setValue($image->getName())
+      ->setLabel(pht('Title'));
+
+    $description = id(new AphrontFormTextAreaControl())
+      ->setName('description_'.$phid)
+      ->setValue($image->getDescription())
+      ->setLabel(pht('Description'));
+
+    return hsprintf(
+      '<div class="pholio-uploaded-image">
+        <div class="thumb-box">
+          <div class="title">
+            <div class="text">%s</div>
+            <div class="remove">%s</div>
+          </div>
+          <div class="thumb">%s</div>
+        </div>
+        <div class="image-data">
+          <div class="title">%s</title>
+          <div class="description">%s</description>
+        </div>',
+      $file_link,
+      $remove,
+      $thumb,
+      $title,
+      $description);
+
+  }
+
+}

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -179,6 +179,19 @@ abstract class PhabricatorApplicationTransactionEditor
     return ($xaction->getOldValue() !== $xaction->getNewValue());
   }
 
+  protected function shouldApplyInitialEffects(
+    PhabricatorLiskDAO $object,
+    array $xactions) {
+    return false;
+
+  }
+
+  protected function applyInitialEffects(
+    PhabricatorLiskDAO $object,
+    array $xactions) {
+    throw new Exception('Not implemented.');
+  }
+
   private function applyInternalEffects(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
@@ -338,24 +351,39 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $is_preview = $this->getIsPreview();
     $read_locking = false;
+    $transaction_open = false;
 
-    if (!$is_preview && $object->getID()) {
+    if (!$is_preview) {
-      foreach ($xactions as $xaction) {
+      if ($object->getID()) {
+        foreach ($xactions as $xaction) {
 
-        // If any of the transactions require a read lock, hold one and reload
+          // If any of the transactions require a read lock, hold one and
-        // the object. We need to do this fairly early so that the call to
+          // reload the object. We need to do this fairly early so that the
-        // `adjustTransactionValues()` (which populates old values) is based
+          // call to `adjustTransactionValues()` (which populates old values)
-        // on the synchronized state of the object, which may differ from the
+          // is based on the synchronized state of the object, which may differ
-        // state when it was originally loaded.
+          // from the state when it was originally loaded.
 
-        if ($this->shouldReadLock($object, $xaction)) {
+          if ($this->shouldReadLock($object, $xaction)) {
-          $object->openTransaction();
+            $object->openTransaction();
-          $object->beginReadLocking();
+            $object->beginReadLocking();
-          $read_locking = true;
+            $transaction_open = true;
-          $object->reload();
+            $read_locking = true;
-          break;
+            $object->reload();
+            break;
+          }
         }
       }
+
+      if ($this->shouldApplyInitialEffects($object, $xactions)) {
+        if (!$transaction_open) {
+          $object->openTransaction();
+          $transaction_open = true;
+        }
+      }
+    }
+
+    if ($this->shouldApplyInitialEffects($object, $xactions)) {
+      $this->applyInitialEffects($object, $xactions);
     }
 
     foreach ($xactions as $xaction) {
@@ -368,7 +396,10 @@ abstract class PhabricatorApplicationTransactionEditor
       if ($read_locking) {
         $object->endReadLocking();
         $read_locking = false;
+      }
+      if ($transaction_open) {
         $object->killTransaction();
+        $transaction_open = false;
       }
       return array();
     }
@@ -384,7 +415,7 @@ abstract class PhabricatorApplicationTransactionEditor
       ->setActor($actor)
       ->setContentSource($this->getContentSource());
 
-    if (!$read_locking) {
+    if (!$transaction_open) {
       $object->openTransaction();
     }
 

src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php

@@ -222,6 +222,20 @@ abstract class PhabricatorBaseEnglishTranslation
         ),
       ),
 
+      '%s added %d image(s): %s.' => array(
+        array(
+          '%s added an image: %3$s.',
+          '%s added images: %3$s.',
+        ),
+      ),
+
+      '%s removed %d image(s): %s.' => array(
+        array(
+          '%s removed an image: %3$s.',
+          '%s removed images: %3$s.',
+        ),
+      ),
+
       '%d people(s)' => array(
         array(
           '%d person',

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

@@ -1454,6 +1454,18 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type' => 'sql',
         'name' => $this->getPatchPath('20130715.voteedges.sql'),
       ),
+      '20130711.pholioimageobsolete.sql' => array(
+        'type' => 'sql',
+        'name' => $this->getPatchPath('20130711.pholioimageobsolete.sql'),
+      ),
+      '20130711.pholioimageobsolete.php' => array(
+        'type' => 'php',
+        'name' => $this->getPatchPath('20130711.pholioimageobsolete.php'),
+      ),
+      '20130711.pholioimageobsolete2.sql' => array(
+        'type' => 'sql',
+        'name' => $this->getPatchPath('20130711.pholioimageobsolete2.sql'),
+      ),
     );
   }
 }

src/view/control/AphrontAbstractAttachedFileView.php

@@ -0,0 +1,41 @@
+<?php
+
+abstract class AphrontAbstractAttachedFileView extends AphrontView {
+
+  private $file;
+
+  final public function setFile(PhabricatorFile $file) {
+    $this->file = $file;
+    return $this;
+  }
+
+  final protected function getFile() {
+    return $this->file;
+  }
+
+  final protected function getName() {
+    $file = $this->getFile();
+    return phutil_tag(
+      'a',
+      array(
+        'href'    => $file->getViewURI(),
+        'target'  => '_blank',
+      ),
+      $file->getName());
+  }
+
+  final protected function getRemoveElement() {
+    $file = $this->getFile();
+    return javelin_tag(
+      'a',
+      array(
+        'class' => 'button grey',
+        'sigil' => 'aphront-attached-file-view-remove',
+        // NOTE: Using 'ref' here instead of 'meta' because the file upload
+        // endpoint doesn't receive request metadata and thus can't generate
+        // a valid response with node metadata.
+        'ref'   => $file->getPHID(),
+      ),
+      "\xE2\x9C\x96"); // "Heavy Multiplication X"
+  }
+}

src/view/control/AphrontAttachedFileView.php

@@ -1,18 +1,11 @@
 <?php
 
-final class AphrontAttachedFileView extends AphrontView {
+final class AphrontAttachedFileView extends AphrontAbstractAttachedFileView {
-
-  private $file;
-
-  public function setFile(PhabricatorFile $file) {
-    $this->file = $file;
-    return $this;
-  }
 
   public function render() {
     require_celerity_resource('aphront-attached-file-view-css');
 
-    $file = $this->file;
+    $file = $this->getFile();
     $phid = $file->getPHID();
 
     $thumb = phutil_tag(
@@ -23,26 +16,10 @@ final class AphrontAttachedFileView extends AphrontView {
         'height'  => 45,
       ));
 
-    $name = phutil_tag(
+    $name = $this->getName();
-      'a',
-      array(
-        'href'    => $file->getViewURI(),
-        'target'  => '_blank',
-      ),
-      $file->getName());
     $size = number_format($file->getByteSize()).' ' .pht('bytes');
 
-    $remove = javelin_tag(
+    $remove = $this->getRemoveElement();
-      'a',
-      array(
-        'class' => 'button grey',
-        'sigil' => 'aphront-attached-file-view-remove',
-        // NOTE: Using 'ref' here instead of 'meta' because the file upload
-        // endpoint doesn't receive request metadata and thus can't generate
-        // a valid response with node metadata.
-        'ref'   => $file->getPHID(),
-      ),
-      "\xE2\x9C\x96"); // "Heavy Multiplication X"
 
     return hsprintf(
       '<table class="aphront-attached-file-view">

src/view/form/control/AphrontAbstractFormDragAndDropUploadControl.php

@@ -0,0 +1,73 @@
+<?php
+
+abstract class AphrontAbstractFormDragAndDropUploadControl
+  extends AphrontFormControl {
+
+  private $activatedClass;
+  private $uploadURI = '/file/dropupload/';
+
+  public function __construct() {
+    $this->setControlID(celerity_generate_unique_node_id());
+    $this->setControlStyle('display: none;');
+  }
+
+  protected function getCustomControlClass() {
+    return 'aphront-form-drag-and-drop-upload';
+  }
+
+  public function setUploadURI($upload_uri) {
+    $this->uploadURI = $upload_uri;
+    return $this;
+  }
+
+  public function getUploadURI() {
+    return $this->uploadURI;
+  }
+
+  public function setActivatedClass($class) {
+    $this->activatedClass = $class;
+    return $this;
+  }
+
+  protected function getFileView() {
+    return new AphrontAttachedFileView();
+  }
+
+  protected function renderInput() {
+    require_celerity_resource('aphront-attached-file-view-css');
+    $list_id = celerity_generate_unique_node_id();
+
+    $files = $this->getValue();
+    $value = array();
+    if ($files) {
+      foreach ($files as $file) {
+        $view = $this->getFileView();
+        $view->setFile($file);
+        $value[$file->getPHID()] = array(
+          'phid' => $file->getPHID(),
+          'html' => $view->render(),
+        );
+      }
+    }
+
+    Javelin::initBehavior(
+      'aphront-drag-and-drop',
+      array(
+        'control'         => $this->getControlID(),
+        'name'            => $this->getName(),
+        'value'           => nonempty($value, null),
+        'list'            => $list_id,
+        'uri'             => $this->getUploadURI(),
+        'activatedClass'  => $this->activatedClass,
+      ));
+
+    return phutil_tag(
+      'div',
+      array(
+        'id'    => $list_id,
+        'class' => 'aphront-form-drag-and-drop-file-list',
+      ),
+      '');
+  }
+
+}

src/view/form/control/AphrontFormDragAndDropUploadControl.php

@@ -1,58 +1,6 @@
 <?php
 
-final class AphrontFormDragAndDropUploadControl extends AphrontFormControl {
+final class AphrontFormDragAndDropUploadControl
-
+  extends AphrontAbstractFormDragAndDropUploadControl {
-  private $activatedClass;
-
-  public function __construct() {
-    $this->setControlID(celerity_generate_unique_node_id());
-    $this->setControlStyle('display: none;');
-  }
-
-  protected function getCustomControlClass() {
-    return 'aphront-form-drag-and-drop-upload';
-  }
-
-  public function setActivatedClass($class) {
-    $this->activatedClass = $class;
-    return $this;
-  }
-
-  protected function renderInput() {
-    require_celerity_resource('aphront-attached-file-view-css');
-    $list_id = celerity_generate_unique_node_id();
-
-    $files = $this->getValue();
-    $value = array();
-    if ($files) {
-      foreach ($files as $file) {
-        $view = new AphrontAttachedFileView();
-        $view->setFile($file);
-        $value[$file->getPHID()] = array(
-          'phid' => $file->getPHID(),
-          'html' => $view->render(),
-        );
-      }
-    }
-
-    Javelin::initBehavior(
-      'aphront-drag-and-drop',
-      array(
-        'control'         => $this->getControlID(),
-        'name'            => $this->getName(),
-        'value'           => nonempty($value, null),
-        'list'            => $list_id,
-        'uri'             => '/file/dropupload/',
-        'activatedClass'  => $this->activatedClass,
-      ));
-
-    return phutil_tag(
-      'div',
-      array(
-        'id'    => $list_id,
-        'class' => 'aphront-form-drag-and-drop-file-list',
-      ),
-      '');
-  }
 
 }

webroot/rsrc/css/application/pholio/pholio-edit.css

@@ -0,0 +1,81 @@
+/**
+ * @provides pholio-edit-css
+ */
+
+.pholio-uploaded-image {
+  float: left;
+  margin: 0px 0px 12px 0px;
+}
+
+.pholio-uploaded-image .thumb-box {
+  border: 1px solid #D5D9DF;
+  border-radius: 3px;
+  min-width: 280px;
+  width: 48%;
+  float: left;
+}
+
+.pholio-uploaded-image .image-data {
+  width: 48%;
+  float: right;
+}
+
+.pholio-uploaded-image .thumb-box .title {
+  width: 100%;
+  float: left;
+  background: #EDF0F4;
+  border-top-left-radius: 3px;
+  border-top-right-radius: 3px;
+  border-bottom: 1px solid #D5D9DF;
+}
+
+.pholio-uploaded-image .thumb-box .thumb {
+  float: left;
+  background: white;
+  padding: 12px 0px 0px 0px;
+  width: 100%;
+}
+
+.pholio-uploaded-image .thumb-box .title .text {
+  width: 220px;
+  float: left;
+  text-overflow: ellipsis;
+  overflow: hidden;
+  white-space: nowrap;
+  padding: 6px 0px 0px 8px;
+}
+
+.pholio-uploaded-image .thumb-box .title .remove {
+  float: right;
+}
+
+.pholio-uploaded-image .thumb-box .title .remove .button {
+  background: #EDF0F4;
+  border: none;
+}
+
+.pholio-uploaded-image .thumb-box .title .remove .button:hover {
+  box-shadow: inset 0 0 2px rgba(0,0,0,.2);
+}
+
+.pholio-uploaded-image .image-data .aphront-form-control {
+  padding: 0px 0px 4px 4px;
+}
+.pholio-uploaded-image .image-data .aphront-form-label {
+  text-align: left;
+  margin: 0px 0px 5px 0px;
+}
+.pholio-uploaded-image .image-data .aphront-form-input {
+  margin: 0;
+  width: 100%;
+}
+
+.aphront-form-input .aphront-form-drag-and-drop-file-list {
+  float: left;
+  width: 100%;
+}
+.aphront-form-input .aphront-form-drag-and-drop-file-list
+.drag-and-drop-file-target {
+  padding: 20px;
+  clear: both;
+}

webroot/rsrc/js/core/behavior-drag-and-drop.js

@@ -53,7 +53,7 @@ JX.behavior('aphront-drag-and-drop', function(config) {
     pending--;
     redraw(true);
 
-    // This redraws the instructions.
+    // This redraws the instructions and clears "Upload complete!"
     setTimeout(redraw, 1000);
   });