diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -7,6 +7,10 @@ return new DifferentialDiffInlineCommentQuery(); } + protected function newContainerObject() { + return $this->loadRevision(); + } + private function getRevisionID() { return $this->getRequest()->getURIData('id'); } @@ -137,28 +141,6 @@ 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 @@ $ids); } - private function syncDraft() { - $viewer = $this->getViewer(); - $revision = $this->loadRevision(); - - $revision->newDraftEngine() - ->setObject($revision) - ->setViewer($viewer) - ->synchronize(); - } - } diff --git a/src/applications/differential/engine/DifferentialRevisionDraftEngine.php b/src/applications/differential/engine/DifferentialRevisionDraftEngine.php --- a/src/applications/differential/engine/DifferentialRevisionDraftEngine.php +++ b/src/applications/differential/engine/DifferentialRevisionDraftEngine.php @@ -11,6 +11,7 @@ ->setViewer($viewer) ->withRevisionPHIDs(array($revision->getPHID())) ->withPublishableComments(true) + ->setLimit(1) ->execute(); return (bool)$inlines; diff --git a/src/applications/diffusion/controller/DiffusionInlineCommentController.php b/src/applications/diffusion/controller/DiffusionInlineCommentController.php --- a/src/applications/diffusion/controller/DiffusionInlineCommentController.php +++ b/src/applications/diffusion/controller/DiffusionInlineCommentController.php @@ -7,6 +7,10 @@ return new DiffusionDiffInlineCommentQuery(); } + protected function newContainerObject() { + return $this->loadCommit(); + } + private function getCommitPHID() { return $this->getRequest()->getURIData('phid'); } @@ -103,19 +107,6 @@ 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(); diff --git a/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php b/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php --- a/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php @@ -11,6 +11,7 @@ ->setViewer($viewer) ->withCommitPHIDs(array($commit->getPHID())) ->withPublishableComments(true) + ->setLimit(1) ->execute(); return (bool)$inlines; diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/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 @@ $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 @@ ->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 @@ $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 @@ ->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 @@ ->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 @@ 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); + } + }