Lift inline comment draft behaviors to "InlineController"

Summary:
Ref T13513. Currently, if you:

  - click a line to create an inline;
  - type some text;
  - wait a moment; and
  - close the page.

...you don't get an "Unsubmitted Draft" marker in the revision list.

Lift all the draft behavior to "InlineController" and make saving a draft dirty the overall container draft state.

Test Plan:
  - Took the steps described above, got a draft state marker.
  - Created, edited, submitted, etc., inlines in Diffusion and Differential.

Maniphest Tasks: T13513

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

Author: epriestley

src/applications/differential/controller/DifferentialInlineCommentEditController.php

@@ -7,6 +7,10 @@ protected function newInlineCommentQuery() {
     return new DifferentialDiffInlineCommentQuery();
   }
 
+  protected function newContainerObject() {
+    return $this->loadRevision();
+  }
+
   private function getRevisionID() {
     return $this->getRequest()->getURIData('id');
   }
@@ -137,28 +141,6 @@ protected function canEditInlineComment(
     return true;
   }
 
-  protected function deleteComment(PhabricatorInlineComment $inline) {
-    $inline->openTransaction();
-      $inline->setIsDeleted(1)->save();
-      $this->syncDraft();
-    $inline->saveTransaction();
-  }
-
-  protected function undeleteComment(
-    PhabricatorInlineComment $inline) {
-    $inline->openTransaction();
-      $inline->setIsDeleted(0)->save();
-      $this->syncDraft();
-    $inline->saveTransaction();
-  }
-
-  protected function saveComment(PhabricatorInlineComment $inline) {
-    $inline->openTransaction();
-      $inline->save();
-      $this->syncDraft();
-    $inline->saveTransaction();
-  }
-
   protected function loadObjectOwnerPHID(
     PhabricatorInlineComment $inline) {
     return $this->loadRevision()->getAuthorPHID();
@@ -198,14 +180,4 @@ protected function showComments(array $ids) {
       $ids);
   }
 
-  private function syncDraft() {
-    $viewer = $this->getViewer();
-    $revision = $this->loadRevision();
-
-    $revision->newDraftEngine()
-      ->setObject($revision)
-      ->setViewer($viewer)
-      ->synchronize();
-  }
-
 }

src/applications/differential/engine/DifferentialRevisionDraftEngine.php

@@ -11,6 +11,7 @@ protected function hasCustomDraftContent() {
       ->setViewer($viewer)
       ->withRevisionPHIDs(array($revision->getPHID()))
       ->withPublishableComments(true)
+      ->setLimit(1)
       ->execute();
 
     return (bool)$inlines;

src/applications/diffusion/controller/DiffusionInlineCommentController.php

@@ -7,6 +7,10 @@ protected function newInlineCommentQuery() {
     return new DiffusionDiffInlineCommentQuery();
   }
 
+  protected function newContainerObject() {
+    return $this->loadCommit();
+  }
+
   private function getCommitPHID() {
     return $this->getRequest()->getURIData('phid');
   }
@@ -103,19 +107,6 @@ protected function canEditInlineComment(
     return true;
   }
 
-  protected function deleteComment(PhabricatorInlineComment $inline) {
-    $inline->setIsDeleted(1)->save();
-  }
-
-  protected function undeleteComment(
-    PhabricatorInlineComment $inline) {
-    $inline->setIsDeleted(0)->save();
-  }
-
-  protected function saveComment(PhabricatorInlineComment $inline) {
-    return $inline->save();
-  }
-
   protected function loadObjectOwnerPHID(
     PhabricatorInlineComment $inline) {
     return $this->loadCommit()->getAuthorPHID();

src/applications/diffusion/engine/DiffusionCommitDraftEngine.php

@@ -11,6 +11,7 @@ protected function hasCustomDraftContent() {
       ->setViewer($viewer)
       ->withCommitPHIDs(array($commit->getPHID()))
       ->withPublishableComments(true)
+      ->setLimit(1)
       ->execute();
 
     return (bool)$inlines;

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -3,17 +3,28 @@
 abstract class PhabricatorInlineCommentController
   extends PhabricatorController {
 
+  private $containerObject;
+
   abstract protected function createComment();
   abstract protected function newInlineCommentQuery();
   abstract protected function loadCommentForDone($id);
   abstract protected function loadObjectOwnerPHID(
     PhabricatorInlineComment $inline);
-  abstract protected function deleteComment(
-    PhabricatorInlineComment $inline);
-  abstract protected function undeleteComment(
-    PhabricatorInlineComment $inline);
-  abstract protected function saveComment(
-    PhabricatorInlineComment $inline);
+  abstract protected function newContainerObject();
+
+  final protected function getContainerObject() {
+    if ($this->containerObject === null) {
+      $object = $this->newContainerObject();
+      if (!$object) {
+        throw new Exception(
+          pht(
+            'Failed to load container object for inline comment.'));
+      }
+      $this->containerObject = $object;
+    }
+
+    return $this->containerObject;
+  }
 
   protected function hideComments(array $ids) {
     throw new PhutilMethodNotImplementedException();
@@ -173,11 +184,13 @@ public function processRequest() {
         $inline = $this->loadCommentByIDForEdit($this->getCommentID());
 
         if ($is_delete) {
-          $this->deleteComment($inline);
+          $inline->setIsDeleted(1);
         } else {
-          $this->undeleteComment($inline);
+          $inline->setIsDeleted(0);
         }
 
+        $this->saveComment($inline);
+
         return $this->buildEmptyResponse();
       case 'edit':
         $inline = $this->loadCommentByIDForEdit($this->getCommentID());
@@ -190,33 +203,49 @@ public function processRequest() {
               ->setIsEditing(false);
 
             $this->saveComment($inline);
-            $this->purgeVersionedDrafts($inline);
 
             return $this->buildRenderedCommentResponse(
               $inline,
               $this->getIsOnRight());
           } else {
-            $this->deleteComment($inline);
-            $this->purgeVersionedDrafts($inline);
+            $inline->setIsDeleted(1);
+
+            $this->saveComment($inline);
 
             return $this->buildEmptyResponse();
           }
         } else {
-          $inline->setIsEditing(true);
+          // NOTE: At time of writing, the "editing" state of inlines is
+          // preserved by simluating a click on "Edit" when the inline loads.
 
-          if (strlen($text)) {
-            $inline->setContent($text);
-          }
+          // In this case, we don't want to "saveComment()", because it
+          // recalculates object drafts and purges versioned drafts.
+
+          // The recalculation is merely unnecessary (state doesn't change)
+          // but purging drafts means that loading a page and then closing it
+          // discards your drafts.
 
-          $this->saveComment($inline);
+          // To avoid the purge, only invoke "saveComment()" if we actually
+          // have changes to apply.
+
+          $is_dirty = false;
+          if (!$inline->getIsEditing()) {
+            $inline->setIsEditing(true);
+            $is_dirty = true;
+          }
 
           if (strlen($text)) {
-            $this->purgeVersionedDrafts($inline);
+            $inline->setContent($text);
+            $is_dirty = true;
+          } else {
+            PhabricatorInlineComment::loadAndAttachVersionedDrafts(
+              $viewer,
+              array($inline));
           }
 
-          PhabricatorInlineComment::loadAndAttachVersionedDrafts(
-            $viewer,
-            array($inline));
+          if ($is_dirty) {
+            $this->saveComment($inline);
+          }
         }
 
         $edit_dialog = $this->buildEditDialog($inline)
@@ -240,12 +269,10 @@ public function processRequest() {
 
         $content = $inline->getContent();
         if (!strlen($content)) {
-          $this->deleteComment($inline);
-        } else {
-          $this->saveComment($inline);
+          $inline->setIsDeleted(1);
         }
 
-        $this->purgeVersionedDrafts($inline);
+        $this->saveComment($inline);
 
         return $this->buildEmptyResponse();
       case 'draft':
@@ -262,6 +289,17 @@ public function processRequest() {
           ->setProperty('inline.text', $text)
           ->save();
 
+        // We have to synchronize the draft engine after saving a versioned
+        // draft, because taking an inline comment from "no text, no draft"
+        // to "no text, text in a draft" marks the container object as having
+        // a draft.
+        $draft_engine = $this->newDraftEngine();
+        if ($draft_engine) {
+          $draft_engine->synchronize();
+        } else {
+          phlog('no draft engine');
+        }
+
         return $this->buildEmptyResponse();
       case 'new':
       case 'reply':
@@ -432,14 +470,6 @@ private function newInlineResponse(
       ->setContent($response);
   }
 
-  private function purgeVersionedDrafts(
-    PhabricatorInlineComment $inline) {
-    $viewer = $this->getViewer();
-    PhabricatorVersionedDraft::purgeDrafts(
-      $inline->getPHID(),
-      $viewer->getPHID());
-  }
-
   final protected function loadCommentByID($id) {
     $query = $this->newInlineCommentQuery()
       ->withIDs(array($id));
@@ -494,4 +524,35 @@ private function loadCommentByQuery(
     return $inline;
   }
 
+  private function saveComment(PhabricatorInlineComment $inline) {
+    $viewer = $this->getViewer();
+    $draft_engine = $this->newDraftEngine();
+
+    $inline->openTransaction();
+      $inline->save();
+
+      PhabricatorVersionedDraft::purgeDrafts(
+        $inline->getPHID(),
+        $viewer->getPHID());
+
+      if ($draft_engine) {
+        $draft_engine->synchronize();
+      }
+
+    $inline->saveTransaction();
+  }
+
+  private function newDraftEngine() {
+    $viewer = $this->getViewer();
+    $object = $this->getContainerObject();
+
+    if (!($object instanceof PhabricatorDraftInterface)) {
+      return null;
+    }
+
+    return $object->newDraftEngine()
+      ->setObject($object)
+      ->setViewer($viewer);
+  }
+
 }