Save drafts for inline comments currently being edited

Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.

This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.

Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.

Maniphest Tasks: T13513

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

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '632fb8f5',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '2d70b7b9',
-    'differential.pkg.js' => '4287e51f',
+    'differential.pkg.js' => '4d375e61',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -380,8 +380,8 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'a49dc31e',
-    'rsrc/js/application/diff/DiffChangesetList.js' => '10726e6a',
+    'rsrc/js/application/diff/DiffChangesetList.js' => '6992b85c',
-    'rsrc/js/application/diff/DiffInline.js' => '7f804f2b',
+    'rsrc/js/application/diff/DiffInline.js' => 'a39fd98e',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17',
@@ -462,7 +462,7 @@ return array(
     'rsrc/js/core/MultirowRowManager.js' => '5b54c823',
     'rsrc/js/core/Notification.js' => 'a9b91e3f',
     'rsrc/js/core/Prefab.js' => '5793d835',
-    'rsrc/js/core/ShapedRequest.js' => 'abf88db8',
+    'rsrc/js/core/ShapedRequest.js' => '995f5102',
     'rsrc/js/core/TextAreaUtils.js' => 'f340a484',
     'rsrc/js/core/Title.js' => '43bc9360',
     'rsrc/js/core/ToolTip.js' => '83754533',
@@ -777,8 +777,8 @@ return array(
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'a49dc31e',
-    'phabricator-diff-changeset-list' => '10726e6a',
+    'phabricator-diff-changeset-list' => '6992b85c',
-    'phabricator-diff-inline' => '7f804f2b',
+    'phabricator-diff-inline' => 'a39fd98e',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -800,7 +800,7 @@ return array(
     'phabricator-prefab' => '5793d835',
     'phabricator-remarkup-css' => 'c286eaef',
     'phabricator-search-results-css' => '9ea70ace',
-    'phabricator-shaped-request' => 'abf88db8',
+    'phabricator-shaped-request' => '995f5102',
     'phabricator-slowvote-css' => '1694baed',
     'phabricator-source-code-view-css' => '03d7ac28',
     'phabricator-standard-page-view' => 'a374f94c',
@@ -1022,11 +1022,6 @@ return array(
       'javelin-workflow',
       'phuix-icon-view',
     ),
-    '10726e6a' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     '111bfd2d' => array(
       'javelin-install',
     ),
@@ -1519,6 +1514,11 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
+    '6992b85c' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     '6a1583a8' => array(
       'javelin-behavior',
       'javelin-history',
@@ -1626,9 +1626,6 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
-    '7f804f2b' => array(
-      'javelin-dom',
-    ),
     '80bff3af' => array(
       'javelin-install',
       'javelin-typeahead-source',
@@ -1797,6 +1794,12 @@ return array(
       'javelin-request',
       'javelin-util',
     ),
+    '995f5102' => array(
+      'javelin-install',
+      'javelin-util',
+      'javelin-request',
+      'javelin-router',
+    ),
     '9aae2b66' => array(
       'javelin-install',
       'javelin-util',
@@ -1838,6 +1841,9 @@ return array(
       'javelin-workflow',
       'phabricator-draggable-list',
     ),
+    'a39fd98e' => array(
+      'javelin-dom',
+    ),
     'a4356cde' => array(
       'javelin-install',
       'javelin-dom',
@@ -1916,12 +1922,6 @@ return array(
       'javelin-dom',
       'phabricator-notification',
     ),
-    'abf88db8' => array(
-      'javelin-install',
-      'javelin-util',
-      'javelin-request',
-      'javelin-router',
-    ),
     'ad258e28' => array(
       'javelin-behavior',
       'javelin-dom',

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -105,7 +105,13 @@ final class PhabricatorAuditEditor
 
     switch ($xaction->getTransactionType()) {
       case PhabricatorAuditActionConstants::INLINE:
-        $xaction->getComment()->setAttribute('editing', false);
+        $comment = $xaction->getComment();
+
+        $comment->setAttribute('editing', false);
+
+        PhabricatorVersionedDraft::purgeDrafts(
+          $comment->getPHID(),
+          $this->getActingAsPHID());
         return;
       case PhabricatorAuditTransaction::TYPE_COMMIT:
         return;

src/applications/audit/storage/PhabricatorAuditInlineComment.php

@@ -111,13 +111,6 @@ final class PhabricatorAuditInlineComment
         $viewer->getPHID());
     }
 
-    foreach ($inlines as $key => $inline) {
-      $is_draft = !$inline->getTransactionPHID();
-      if ($is_draft && $inline->isEmptyInlineComment()) {
-        unset($inlines[$key]);
-      }
-    }
-
     return self::buildProxies($inlines);
   }
 

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -197,7 +197,6 @@ final class DifferentialChangesetViewController extends DifferentialController {
       $query = id(new DifferentialInlineCommentQuery())
         ->setViewer($viewer)
         ->needHidden(true)
-        ->withEmptyInlineComments(false)
         ->withRevisionPHIDs(array($revision->getPHID()));
       $inlines = $query->execute();
       $inlines = $query->adjustInlinesForChangesets(

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -112,7 +112,13 @@ final class DifferentialTransactionEditor
 
     switch ($xaction->getTransactionType()) {
       case DifferentialTransaction::TYPE_INLINE:
-        $xaction->getComment()->setAttribute('editing', false);
+        $comment = $xaction->getComment();
+
+        $comment->setAttribute('editing', false);
+
+        PhabricatorVersionedDraft::purgeDrafts(
+          $comment->getPHID(),
+          $this->getActingAsPHID());
         return;
     }
 

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -752,6 +752,8 @@ final class DifferentialChangesetParser extends Phobject {
     $range_len    = null,
     $mask_force   = array()) {
 
+    $viewer = $this->getViewer();
+
     $renderer = $this->getRenderer();
     if (!$renderer) {
       $renderer = $this->newRenderer();
@@ -853,6 +855,16 @@ final class DifferentialChangesetParser extends Phobject {
 
     $has_document_engine = ($engine_blocks !== null);
 
+    // Remove empty comments that don't have any unsaved draft data.
+    PhabricatorInlineComment::loadAndAttachVersionedDrafts(
+      $viewer,
+      $this->comments);
+    foreach ($this->comments as $key => $comment) {
+      if ($comment->isVoidComment($viewer)) {
+        unset($this->comments[$key]);
+      }
+    }
+
     // See T13515. Sometimes, we collapse file content by default: for
     // example, if the file is marked as containing generated code.
 
@@ -1050,6 +1062,7 @@ final class DifferentialChangesetParser extends Phobject {
         }
       }
     }
+
     $renderer
       ->setOldComments($old_comments)
       ->setNewComments($new_comments);

src/applications/differential/query/DifferentialInlineCommentQuery.php

@@ -16,7 +16,6 @@ final class DifferentialInlineCommentQuery
   private $revisionPHIDs;
   private $deletedDrafts;
   private $needHidden;
-  private $withEmpty;
 
   public function setViewer(PhabricatorUser $viewer) {
     $this->viewer = $viewer;
@@ -62,11 +61,6 @@ final class DifferentialInlineCommentQuery
     return $this;
   }
 
-  public function withEmptyInlineComments($empty) {
-    $this->withEmpty = $empty;
-    return $this;
-  }
-
   public function execute() {
     $table = new DifferentialTransactionComment();
     $conn_r = $table->establishConnection('r');
@@ -80,15 +74,6 @@ final class DifferentialInlineCommentQuery
 
     $comments = $table->loadAllFromArray($data);
 
-    if ($this->withEmpty !== null) {
-      $want_empty = (bool)$this->withEmpty;
-      foreach ($comments as $key => $value) {
-        if ($value->isEmptyInlineComment() !== $want_empty) {
-          unset($comments[$key]);
-        }
-      }
-    }
-
     if ($this->needHidden) {
       $viewer_phid = $this->getViewer()->getPHID();
       if ($viewer_phid && $comments) {

src/applications/differential/query/DifferentialTransactionQuery.php

@@ -20,13 +20,24 @@ final class DifferentialTransactionQuery
       ->needReplyToComments(true)
       ->execute();
 
-    // Don't count empty inlines when considering draft state.
     foreach ($inlines as $key => $inline) {
-      if ($inline->isEmptyInlineComment()) {
+      $inlines[$key] = DifferentialInlineComment::newFromModernComment(
+        $inline);
+    }
+
+    PhabricatorInlineComment::loadAndAttachVersionedDrafts(
+      $viewer,
+      $inlines);
+
+    // Don't count void inlines when considering draft state.
+    foreach ($inlines as $key => $inline) {
+      if ($inline->isVoidComment($viewer)) {
         unset($inlines[$key]);
       }
     }
 
+    $inlines = mpull($inlines, 'getStorageObject');
+
     return $inlines;
   }
 

src/applications/draft/storage/PhabricatorVersionedDraft.php

@@ -35,6 +35,23 @@ final class PhabricatorVersionedDraft extends PhabricatorDraftDAO {
     return idx($this->properties, $key, $default);
   }
 
+  public static function loadDrafts(
+    array $object_phids,
+    $viewer_phid) {
+
+    $rows = id(new self())->loadAllWhere(
+      'objectPHID IN (%Ls) AND authorPHID = %s ORDER BY version ASC',
+      $object_phids,
+      $viewer_phid);
+
+    $map = array();
+    foreach ($rows as $row) {
+      $map[$row->getObjectPHID()] = $row;
+    }
+
+    return $map;
+  }
+
   public static function loadDraft(
     $object_phid,
     $viewer_phid) {

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -192,11 +192,15 @@ abstract class PhabricatorInlineCommentController
               ->setIsEditing(false);
 
             $this->saveComment($inline);
+            $this->purgeVersionedDrafts($inline);
+
             return $this->buildRenderedCommentResponse(
               $inline,
               $this->getIsOnRight());
           } else {
             $this->deleteComment($inline);
+            $this->purgeVersionedDrafts($inline);
+
             return $this->buildEmptyResponse();
           }
         } else {
@@ -235,6 +239,23 @@ abstract class PhabricatorInlineCommentController
           $this->saveComment($inline);
         }
 
+        $this->purgeVersionedDrafts($inline);
+
+        return $this->buildEmptyResponse();
+      case 'draft':
+        $inline = $this->loadCommentForEdit($this->getCommentID());
+
+        $versioned_draft = PhabricatorVersionedDraft::loadOrCreateDraft(
+          $inline->getPHID(),
+          $viewer->getPHID(),
+          $inline->getID());
+
+        $text = $this->getCommentText();
+
+        $versioned_draft
+          ->setProperty('inline.text', $text)
+          ->save();
+
         return $this->buildEmptyResponse();
       case 'new':
       case 'reply':
@@ -405,4 +426,13 @@ abstract class PhabricatorInlineCommentController
       ->setContent($response);
   }
 
+  private function purgeVersionedDrafts(
+    PhabricatorInlineComment $inline) {
+    $viewer = $this->getViewer();
+    PhabricatorVersionedDraft::purgeDrafts(
+      $inline->getPHID(),
+      $viewer->getPHID());
+  }
+
+
 }

src/infrastructure/diff/interface/PhabricatorInlineComment.php

@@ -15,11 +15,51 @@ abstract class PhabricatorInlineComment
   private $storageObject;
   private $syntheticAuthor;
   private $isGhost;
+  private $versionedDrafts = array();
 
   public function __clone() {
     $this->storageObject = clone $this->storageObject;
   }
 
+  final public static function loadAndAttachVersionedDrafts(
+    PhabricatorUser $viewer,
+    array $inlines) {
+
+    $viewer_phid = $viewer->getPHID();
+    if (!$viewer_phid) {
+      return;
+    }
+
+    $inlines = mpull($inlines, null, 'getPHID');
+
+    $load = array();
+    foreach ($inlines as $key => $inline) {
+      if (!$inline->getIsEditing()) {
+        continue;
+      }
+
+      if ($inline->getAuthorPHID() !== $viewer_phid) {
+        continue;
+      }
+
+      $load[$key] = $inline;
+    }
+
+    if (!$load) {
+      return;
+    }
+
+    $drafts = PhabricatorVersionedDraft::loadDrafts(
+      array_keys($load),
+      $viewer_phid);
+
+    $drafts = mpull($drafts, null, 'getObjectPHID');
+    foreach ($inlines as $inline) {
+      $draft = idx($drafts, $inline->getPHID());
+      $inline->attachVersionedDraftForViewer($viewer, $draft);
+    }
+  }
+
   public function setSyntheticAuthor($synthetic_author) {
     $this->syntheticAuthor = $synthetic_author;
     return $this;
@@ -204,6 +244,57 @@ abstract class PhabricatorInlineComment
     return $this;
   }
 
+  public function attachVersionedDraftForViewer(
+    PhabricatorUser $viewer,
+    PhabricatorVersionedDraft $draft = null) {
+
+    $key = $viewer->getCacheFragment();
+    $this->versionedDrafts[$key] = $draft;
+
+    return $this;
+  }
+
+  public function hasVersionedDraftForViewer(PhabricatorUser $viewer) {
+    $key = $viewer->getCacheFragment();
+    return array_key_exists($key, $this->versionedDrafts);
+  }
+
+  public function getVersionedDraftForViewer(PhabricatorUser $viewer) {
+    $key = $viewer->getCacheFragment();
+    if (!array_key_exists($key, $this->versionedDrafts)) {
+      throw new Exception(
+        pht(
+          'Versioned draft is not attached for user with fragment "%s".',
+          $key));
+    }
+
+    return $this->versionedDrafts[$key];
+  }
+
+  public function isVoidComment(PhabricatorUser $viewer) {
+    return !strlen($this->getContentForEdit($viewer));
+  }
+
+  public function getContentForEdit(PhabricatorUser $viewer) {
+    $content = $this->getContent();
+
+    if (!$this->hasVersionedDraftForViewer($viewer)) {
+      return $content;
+    }
+
+    $versioned_draft = $this->getVersionedDraftForViewer($viewer);
+    if (!$versioned_draft) {
+      return $content;
+    }
+
+    $draft_text = $versioned_draft->getProperty('inline.text');
+    if ($draft_text === null) {
+      return $content;
+    }
+
+    return $draft_text;
+  }
+
 
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 

src/infrastructure/diff/view/PHUIDiffInlineCommentView.php

@@ -54,6 +54,7 @@ abstract class PHUIDiffInlineCommentView extends AphrontView {
   }
 
   protected function getInlineCommentMetadata() {
+    $viewer = $this->getViewer();
     $inline = $this->getInlineComment();
 
     $is_synthetic = (bool)$inline->getSyntheticAuthor();
@@ -74,6 +75,8 @@ abstract class PHUIDiffInlineCommentView extends AphrontView {
         break;
     }
 
+    $original_text = $inline->getContentForEdit($viewer);
+
     return array(
       'id' => $inline->getID(),
       'phid' => $inline->getPHID(),
@@ -81,7 +84,7 @@ abstract class PHUIDiffInlineCommentView extends AphrontView {
       'number' => $inline->getLineNumber(),
       'length' => $inline->getLineLength(),
       'isNewFile' => (bool)$inline->getIsNewFile(),
-      'original' => $inline->getContent(),
+      'original' => $original_text,
       'replyToCommentPHID' => $inline->getReplyToCommentPHID(),
       'isDraft' => $inline->isDraft(),
       'isFixed' => $is_fixed,

webroot/rsrc/js/application/diff/DiffChangesetList.js

@@ -2110,6 +2110,13 @@ JX.install('DiffChangesetList', {
         'click',
         ['differential-inline-comment', 'differential-inline-reply'],
         onreply);
+
+      var ondraft = JX.bind(this, this._onInlineEvent, 'draft');
+      JX.Stratcom.listen(
+        'keydown',
+        ['differential-inline-comment', 'tag:textarea'],
+        ondraft);
+
     },
 
     _onInlineEvent: function(action, e) {
@@ -2117,7 +2124,9 @@ JX.install('DiffChangesetList', {
         return;
       }
 
-      e.kill();
+      if (action !== 'draft') {
+        e.kill();
+      }
 
       var inline = this._getInlineForEvent(e);
       var is_ref = false;
@@ -2172,6 +2181,9 @@ JX.install('DiffChangesetList', {
         case 'reply':
           inline.reply();
           break;
+        case 'draft':
+          inline.triggerDraft();
+          break;
       }
     }
 

webroot/rsrc/js/application/diff/DiffInline.js

@@ -42,6 +42,8 @@ JX.install('DiffInline', {
     _undoType: null,
     _undoText: null,
 
+    _draftRequest: null,
+
     bindToRow: function(row) {
       this._row = row;
 
@@ -89,11 +91,17 @@ JX.install('DiffInline', {
       this._isEditing = data.isEditing;
 
       if (this._isEditing) {
-        this.edit();
+        // NOTE: The "original" shipped down in the DOM may reflect a draft
+        // which we're currently editing. This flow is a little clumsy, but
+        // reasonable until some future change moves away from "send down
+        // the inline, then immediately click edit".
+        this.edit(this._originalText);
       } else {
         this.setInvisible(false);
       }
 
+      this._startDrafts();
+
       return this;
     },
 
@@ -153,6 +161,7 @@ JX.install('DiffInline', {
       parent_row.parentNode.insertBefore(row, target_row);
 
       this.setInvisible(true);
+      this._startDrafts();
 
       return this;
     },
@@ -213,6 +222,7 @@ JX.install('DiffInline', {
       parent_row.parentNode.insertBefore(row, target_row);
 
       this.setInvisible(true);
+      this._startDrafts();
 
       return this;
     },
@@ -795,7 +805,59 @@ JX.install('DiffInline', {
       var changeset = this.getChangeset();
       var list = changeset.getChangesetList();
       return list.getInlineURI();
+    },
+
+    _startDrafts: function() {
+      if (this._draftRequest) {
+        return;
+      }
+
+      var onresponse = JX.bind(this, this._onDraftResponse);
+      var draft = JX.bind(this, this._getDraftState);
+
+      var uri = this._getInlineURI();
+      var request = new JX.PhabricatorShapedRequest(uri, onresponse, draft);
+
+      // The main transaction code uses a 500ms delay on desktop and a
+      // 10s delay on mobile. Perhaps this should be standardized.
+      request.setRateLimit(2000);
+
+      this._draftRequest = request;
+
+      request.start();
+    },
+
+    _onDraftResponse: function() {
+      // For now, do nothing.
+    },
+
+    _getDraftState: function() {
+      if (this.isDeleted()) {
+        return null;
+      }
+
+      if (!this.isEditing()) {
+        return null;
+      }
+
+      var text = this._readText(this._editRow);
+      if (text === null) {
+        return null;
+      }
+
+      return {
+        op: 'draft',
+        id: this.getID(),
+        text: text
+      };
+    },
+
+    triggerDraft: function() {
+      if (this._draftRequest) {
+        this._draftRequest.trigger();
+      }
     }
+
   }
 
 });

webroot/rsrc/js/core/ShapedRequest.js

@@ -81,6 +81,10 @@ JX.install('PhabricatorShapedRequest', {
     },
 
     shouldSendRequest : function(last, data) {
+      if (data === null) {
+        return false;
+      }
+
       if (last === null) {
         return true;
       }