diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ '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 @@ '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/DiffInline.js' => '7f804f2b', + 'rsrc/js/application/diff/DiffChangesetList.js' => '6992b85c', + '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 @@ '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 @@ 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '5a205b9d', 'phabricator-diff-changeset' => 'a49dc31e', - 'phabricator-diff-changeset-list' => '10726e6a', - 'phabricator-diff-inline' => '7f804f2b', + 'phabricator-diff-changeset-list' => '6992b85c', + '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 @@ '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 @@ 'javelin-workflow', 'phuix-icon-view', ), - '10726e6a' => array( - 'javelin-install', - 'phuix-button-view', - 'phabricator-diff-tree-view', - ), '111bfd2d' => array( 'javelin-install', ), @@ -1519,6 +1514,11 @@ '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 @@ 'javelin-install', 'javelin-dom', ), - '7f804f2b' => array( - 'javelin-dom', - ), '80bff3af' => array( 'javelin-install', 'javelin-typeahead-source', @@ -1797,6 +1794,12 @@ 'javelin-request', 'javelin-util', ), + '995f5102' => array( + 'javelin-install', + 'javelin-util', + 'javelin-request', + 'javelin-router', + ), '9aae2b66' => array( 'javelin-install', 'javelin-util', @@ -1838,6 +1841,9 @@ 'javelin-workflow', 'phabricator-draggable-list', ), + 'a39fd98e' => array( + 'javelin-dom', + ), 'a4356cde' => array( 'javelin-install', 'javelin-dom', @@ -1916,12 +1922,6 @@ 'javelin-dom', 'phabricator-notification', ), - 'abf88db8' => array( - 'javelin-install', - 'javelin-util', - 'javelin-request', - 'javelin-router', - ), 'ad258e28' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -105,7 +105,13 @@ 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; diff --git a/src/applications/audit/storage/PhabricatorAuditInlineComment.php b/src/applications/audit/storage/PhabricatorAuditInlineComment.php --- a/src/applications/audit/storage/PhabricatorAuditInlineComment.php +++ b/src/applications/audit/storage/PhabricatorAuditInlineComment.php @@ -111,13 +111,6 @@ $viewer->getPHID()); } - foreach ($inlines as $key => $inline) { - $is_draft = !$inline->getTransactionPHID(); - if ($is_draft && $inline->isEmptyInlineComment()) { - unset($inlines[$key]); - } - } - return self::buildProxies($inlines); } diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -197,7 +197,6 @@ $query = id(new DifferentialInlineCommentQuery()) ->setViewer($viewer) ->needHidden(true) - ->withEmptyInlineComments(false) ->withRevisionPHIDs(array($revision->getPHID())); $inlines = $query->execute(); $inlines = $query->adjustInlinesForChangesets( diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -112,7 +112,13 @@ 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; } diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -752,6 +752,8 @@ $range_len = null, $mask_force = array()) { + $viewer = $this->getViewer(); + $renderer = $this->getRenderer(); if (!$renderer) { $renderer = $this->newRenderer(); @@ -853,6 +855,16 @@ $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 @@ } } } + $renderer ->setOldComments($old_comments) ->setNewComments($new_comments); diff --git a/src/applications/differential/query/DifferentialInlineCommentQuery.php b/src/applications/differential/query/DifferentialInlineCommentQuery.php --- a/src/applications/differential/query/DifferentialInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php @@ -16,7 +16,6 @@ private $revisionPHIDs; private $deletedDrafts; private $needHidden; - private $withEmpty; public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; @@ -62,11 +61,6 @@ 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 @@ $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) { diff --git a/src/applications/differential/query/DifferentialTransactionQuery.php b/src/applications/differential/query/DifferentialTransactionQuery.php --- a/src/applications/differential/query/DifferentialTransactionQuery.php +++ b/src/applications/differential/query/DifferentialTransactionQuery.php @@ -20,13 +20,24 @@ ->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; } diff --git a/src/applications/draft/storage/PhabricatorVersionedDraft.php b/src/applications/draft/storage/PhabricatorVersionedDraft.php --- a/src/applications/draft/storage/PhabricatorVersionedDraft.php +++ b/src/applications/draft/storage/PhabricatorVersionedDraft.php @@ -35,6 +35,23 @@ 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) { 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 @@ -192,11 +192,15 @@ ->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 @@ $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 @@ ->setContent($response); } + private function purgeVersionedDrafts( + PhabricatorInlineComment $inline) { + $viewer = $this->getViewer(); + PhabricatorVersionedDraft::purgeDrafts( + $inline->getPHID(), + $viewer->getPHID()); + } + + } diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php --- a/src/infrastructure/diff/interface/PhabricatorInlineComment.php +++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php @@ -15,11 +15,51 @@ 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 @@ 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 )-------------------------- */ diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php @@ -54,6 +54,7 @@ } protected function getInlineCommentMetadata() { + $viewer = $this->getViewer(); $inline = $this->getInlineComment(); $is_synthetic = (bool)$inline->getSyntheticAuthor(); @@ -74,6 +75,8 @@ break; } + $original_text = $inline->getContentForEdit($viewer); + return array( 'id' => $inline->getID(), 'phid' => $inline->getPHID(), @@ -81,7 +84,7 @@ '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, diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -2110,6 +2110,13 @@ '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 @@ return; } - e.kill(); + if (action !== 'draft') { + e.kill(); + } var inline = this._getInlineForEvent(e); var is_ref = false; @@ -2172,6 +2181,9 @@ case 'reply': inline.reply(); break; + case 'draft': + inline.triggerDraft(); + break; } } diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -42,6 +42,8 @@ _undoType: null, _undoText: null, + _draftRequest: null, + bindToRow: function(row) { this._row = row; @@ -89,11 +91,17 @@ 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 @@ parent_row.parentNode.insertBefore(row, target_row); this.setInvisible(true); + this._startDrafts(); return this; }, @@ -213,6 +222,7 @@ parent_row.parentNode.insertBefore(row, target_row); this.setInvisible(true); + this._startDrafts(); return this; }, @@ -795,7 +805,59 @@ 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(); + } } + } }); diff --git a/webroot/rsrc/js/core/ShapedRequest.js b/webroot/rsrc/js/core/ShapedRequest.js --- a/webroot/rsrc/js/core/ShapedRequest.js +++ b/webroot/rsrc/js/core/ShapedRequest.js @@ -81,6 +81,10 @@ }, shouldSendRequest : function(last, data) { + if (data === null) { + return false; + } + if (last === null) { return true; }