From b48a22bf50a32895566e9aa66c1d7aff87a492b8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Apr 2020 14:45:54 -0700 Subject: [PATCH] Make "editing" state persistent for inline comments Summary: Ref T13513. This is mostly an infrastructure cleanup change. In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change. --- Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes. On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another. Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it). To simplify this: - Make `PhabricatorInlineCommentInterface` an abstract base class instead. - Lift as much code out of the `Audit` and `Differential` subclasses as possible. - Delete methods which no longer have callers, or have only trivial callers. --- Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code. Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `
`. These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code. The `EditView` can not fully render its content. Move the content rendering code into the view. --- Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately. This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`. --- Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level. Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle. This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further. --- Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row". Test Plan: - Created comments on either side of a diff. - Edited a comment, reloaded, saw edit stick. - Saved comments, reloaded, saw save stick. - Edited a comment, typed text, cancelled, "unedited" to get state back. - Created a comment, typed text, cancelled, "unedited" to get state back. - Deleted a comment, "undeleted" to get state back. Weirdness / known issues: - Drafts don't autosave yet. - Fixed in D21187: - When you create an empty comment then reload, you get an empty editor. This is a bit silly. - "Cancel" does not save state, but should, once drafts autosave. - Mostly fixed in D21188: - "Editing" comments aren't handled specially by the overall submission flow. - "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work. Subscribers: jmeador Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21186 --- resources/celerity/map.php | 26 +- src/__phutil_library_map__.php | 17 +- .../storage/PhabricatorAuditInlineComment.php | 234 ++---------------- .../PhabricatorAuditTransactionComment.php | 9 + .../DifferentialChangesetViewController.php | 2 +- ...ifferentialInlineCommentEditController.php | 14 +- .../DifferentialRevisionInlinesController.php | 2 +- .../parser/DifferentialChangesetParser.php | 10 +- .../DifferentialChangesetHTMLRenderer.php | 12 +- .../render/DifferentialChangesetRenderer.php | 4 +- .../storage/DifferentialInlineComment.php | 230 ++--------------- .../DifferentialTransactionComment.php | 9 + .../DifferentialRevisionInlineTransaction.php | 4 +- .../controller/DiffusionDiffController.php | 2 +- .../DiffusionInlineCommentController.php | 10 +- .../constants/PhabricatorTransactions.php | 8 +- .../PhabricatorApplicationTransaction.php | 2 +- .../PhabricatorInlineCommentController.php | 180 ++++++-------- ...bricatorInlineCommentPreviewController.php | 4 +- .../interface/PhabricatorInlineComment.php | 232 +++++++++++++++++ .../PhabricatorInlineCommentInterface.php | 66 ----- .../view/PHUIDiffInlineCommentDetailView.php | 67 +---- .../view/PHUIDiffInlineCommentEditView.php | 116 +++------ .../PHUIDiffInlineCommentPreviewListView.php | 2 +- .../diff/view/PHUIDiffInlineCommentView.php | 62 +++++ .../js/application/diff/DiffChangesetList.js | 183 ++++++++------ .../rsrc/js/application/diff/DiffInline.js | 142 +++++------ 27 files changed, 696 insertions(+), 953 deletions(-) create mode 100644 src/infrastructure/diff/interface/PhabricatorInlineComment.php delete mode 100644 src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 3aa68da324..b48de26153 100644 --- a/resources/celerity/map.php +++ b/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' => 'c8f88d74', + 'differential.pkg.js' => 'b289f75d', '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' => '9a713ba5', - 'rsrc/js/application/diff/DiffChangesetList.js' => 'adf069cd', - 'rsrc/js/application/diff/DiffInline.js' => '16e97ebc', + 'rsrc/js/application/diff/DiffChangesetList.js' => '10726e6a', + 'rsrc/js/application/diff/DiffInline.js' => '7b0bdd6d', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', 'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17', @@ -777,8 +777,8 @@ return array( 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '5a205b9d', 'phabricator-diff-changeset' => '9a713ba5', - 'phabricator-diff-changeset-list' => 'adf069cd', - 'phabricator-diff-inline' => '16e97ebc', + 'phabricator-diff-changeset-list' => '10726e6a', + 'phabricator-diff-inline' => '7b0bdd6d', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', 'phabricator-drag-and-drop-file-upload' => '4370900d', @@ -1022,6 +1022,11 @@ return array( 'javelin-workflow', 'phuix-icon-view', ), + '10726e6a' => array( + 'javelin-install', + 'phuix-button-view', + 'phabricator-diff-tree-view', + ), '111bfd2d' => array( 'javelin-install', ), @@ -1036,9 +1041,6 @@ return array( 'javelin-stratcom', 'javelin-util', ), - '16e97ebc' => array( - 'javelin-dom', - ), '1a844c06' => array( 'javelin-install', 'javelin-util', @@ -1614,6 +1616,9 @@ return array( 'phabricator-drag-and-drop-file-upload', 'phabricator-textareautils', ), + '7b0bdd6d' => array( + 'javelin-dom', + ), '7b139193' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1930,11 +1935,6 @@ return array( 'javelin-typeahead-ondemand-source', 'javelin-util', ), - 'adf069cd' => array( - 'javelin-install', - 'phuix-button-view', - 'phabricator-diff-tree-view', - ), 'aec8e38c' => array( 'javelin-dom', 'javelin-util', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 765481ba45..8afbe0c5e3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3592,8 +3592,8 @@ phutil_register_library_map(array( 'PhabricatorIndexEngineExtensionModule' => 'applications/search/index/PhabricatorIndexEngineExtensionModule.php', 'PhabricatorIndexableInterface' => 'applications/search/interface/PhabricatorIndexableInterface.php', 'PhabricatorInfrastructureTestCase' => '__tests__/PhabricatorInfrastructureTestCase.php', + 'PhabricatorInlineComment' => 'infrastructure/diff/interface/PhabricatorInlineComment.php', 'PhabricatorInlineCommentController' => 'infrastructure/diff/PhabricatorInlineCommentController.php', - 'PhabricatorInlineCommentInterface' => 'infrastructure/diff/interface/PhabricatorInlineCommentInterface.php', 'PhabricatorInlineCommentPreviewController' => 'infrastructure/diff/PhabricatorInlineCommentPreviewController.php', 'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/PhabricatorInlineSummaryView.php', 'PhabricatorInstructionsEditField' => 'applications/transactions/editfield/PhabricatorInstructionsEditField.php', @@ -6623,10 +6623,7 @@ phutil_register_library_map(array( 'DifferentialHunkParserTestCase' => 'PhabricatorTestCase', 'DifferentialHunkQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialHunkTestCase' => 'PhutilTestCase', - 'DifferentialInlineComment' => array( - 'Phobject', - 'PhabricatorInlineCommentInterface', - ), + 'DifferentialInlineComment' => 'PhabricatorInlineComment', 'DifferentialInlineCommentEditController' => 'PhabricatorInlineCommentController', 'DifferentialInlineCommentMailView' => 'DifferentialMailView', 'DifferentialInlineCommentQuery' => 'PhabricatorOffsetPagedQuery', @@ -8595,10 +8592,7 @@ phutil_register_library_map(array( 'PhabricatorAuditCommentEditor' => 'PhabricatorEditor', 'PhabricatorAuditController' => 'PhabricatorController', 'PhabricatorAuditEditor' => 'PhabricatorApplicationTransactionEditor', - 'PhabricatorAuditInlineComment' => array( - 'Phobject', - 'PhabricatorInlineCommentInterface', - ), + 'PhabricatorAuditInlineComment' => 'PhabricatorInlineComment', 'PhabricatorAuditListView' => 'AphrontView', 'PhabricatorAuditMailReceiver' => 'PhabricatorObjectMailReceiver', 'PhabricatorAuditManagementDeleteWorkflow' => 'PhabricatorAuditManagementWorkflow', @@ -10115,8 +10109,11 @@ phutil_register_library_map(array( 'PhabricatorIndexEngineExtension' => 'Phobject', 'PhabricatorIndexEngineExtensionModule' => 'PhabricatorConfigModule', 'PhabricatorInfrastructureTestCase' => 'PhabricatorTestCase', + 'PhabricatorInlineComment' => array( + 'Phobject', + 'PhabricatorMarkupInterface', + ), 'PhabricatorInlineCommentController' => 'PhabricatorController', - 'PhabricatorInlineCommentInterface' => 'PhabricatorMarkupInterface', 'PhabricatorInlineCommentPreviewController' => 'PhabricatorController', 'PhabricatorInlineSummaryView' => 'AphrontView', 'PhabricatorInstructionsEditField' => 'PhabricatorEditField', diff --git a/src/applications/audit/storage/PhabricatorAuditInlineComment.php b/src/applications/audit/storage/PhabricatorAuditInlineComment.php index 2534384f61..0c2bbd3cca 100644 --- a/src/applications/audit/storage/PhabricatorAuditInlineComment.php +++ b/src/applications/audit/storage/PhabricatorAuditInlineComment.php @@ -1,27 +1,16 @@ proxy = new PhabricatorAuditTransactionComment(); + protected function newStorageObject() { + return new PhabricatorAuditTransactionComment(); } - public function __clone() { - $this->proxy = clone $this->proxy; - } - - public function getTransactionPHID() { - return $this->proxy->getTransactionPHID(); - } - - public function getTransactionComment() { - return $this->proxy; + public function getControllerURI() { + return urisprintf( + '/diffusion/inline/edit/%s/', + $this->getCommitPHID()); } public function supportsHiding() { @@ -36,13 +25,13 @@ final class PhabricatorAuditInlineComment $content_source = PhabricatorContentSource::newForSource( PhabricatorOldWorldContentSource::SOURCECONST); - $this->proxy + $this->getStorageObject() ->setViewPolicy('public') ->setEditPolicy($this->getAuthorPHID()) ->setContentSource($content_source) ->setCommentVersion(1); - return $this->proxy; + return $this->getStorageObject(); } public static function loadID($id) { @@ -134,148 +123,31 @@ final class PhabricatorAuditInlineComment return $results; } - public function setSyntheticAuthor($synthetic_author) { - $this->syntheticAuthor = $synthetic_author; - return $this; - } - - public function getSyntheticAuthor() { - return $this->syntheticAuthor; - } - - public function openTransaction() { - $this->proxy->openTransaction(); - } - - public function saveTransaction() { - $this->proxy->saveTransaction(); - } - - public function save() { - $this->getTransactionCommentForSave()->save(); - - return $this; - } - - public function delete() { - $this->proxy->delete(); - - return $this; - } - - public function getID() { - return $this->proxy->getID(); - } - - public function getPHID() { - return $this->proxy->getPHID(); - } - public static function newFromModernComment( PhabricatorAuditTransactionComment $comment) { $obj = new PhabricatorAuditInlineComment(); - $obj->proxy = $comment; + $obj->setStorageObject($comment); return $obj; } - public function isCompatible(PhabricatorInlineCommentInterface $comment) { - return - ($this->getAuthorPHID() === $comment->getAuthorPHID()) && - ($this->getSyntheticAuthor() === $comment->getSyntheticAuthor()) && - ($this->getContent() === $comment->getContent()); - } - - public function setContent($content) { - $this->proxy->setContent($content); - return $this; - } - - public function getContent() { - return $this->proxy->getContent(); - } - - public function isDraft() { - return !$this->proxy->getTransactionPHID(); - } - public function setPathID($id) { - $this->proxy->setPathID($id); + $this->getStorageObject()->setPathID($id); return $this; } public function getPathID() { - return $this->proxy->getPathID(); - } - - public function setIsNewFile($is_new) { - $this->proxy->setIsNewFile($is_new); - return $this; - } - - public function getIsNewFile() { - return $this->proxy->getIsNewFile(); - } - - public function setLineNumber($number) { - $this->proxy->setLineNumber($number); - return $this; - } - - public function getLineNumber() { - return $this->proxy->getLineNumber(); - } - - public function setLineLength($length) { - $this->proxy->setLineLength($length); - return $this; - } - - public function getLineLength() { - return $this->proxy->getLineLength(); - } - - public function setCache($cache) { - return $this; - } - - public function getCache() { - return null; - } - - public function setAuthorPHID($phid) { - $this->proxy->setAuthorPHID($phid); - return $this; - } - - public function getAuthorPHID() { - return $this->proxy->getAuthorPHID(); + return $this->getStorageObject()->getPathID(); } public function setCommitPHID($commit_phid) { - $this->proxy->setCommitPHID($commit_phid); + $this->getStorageObject()->setCommitPHID($commit_phid); return $this; } public function getCommitPHID() { - return $this->proxy->getCommitPHID(); - } - - // When setting a comment ID, we also generate a phantom transaction PHID for - // the future transaction. - - public function setAuditCommentID($id) { - $this->proxy->setLegacyCommentID($id); - $this->proxy->setTransactionPHID( - PhabricatorPHID::generateNewPHID( - PhabricatorApplicationTransactionTransactionPHIDType::TYPECONST, - PhabricatorRepositoryCommitPHIDType::TYPECONST)); - return $this; - } - - public function getAuditCommentID() { - return $this->proxy->getLegacyCommentID(); + return $this->getStorageObject()->getCommitPHID(); } public function setChangesetID($id) { @@ -286,82 +158,4 @@ final class PhabricatorAuditInlineComment return $this->getPathID(); } - public function setReplyToCommentPHID($phid) { - $this->proxy->setReplyToCommentPHID($phid); - return $this; - } - - public function getReplyToCommentPHID() { - return $this->proxy->getReplyToCommentPHID(); - } - - public function setHasReplies($has_replies) { - $this->proxy->setHasReplies($has_replies); - return $this; - } - - public function getHasReplies() { - return $this->proxy->getHasReplies(); - } - - public function setIsDeleted($is_deleted) { - $this->proxy->setIsDeleted($is_deleted); - return $this; - } - - public function getIsDeleted() { - return $this->proxy->getIsDeleted(); - } - - public function setFixedState($state) { - $this->proxy->setFixedState($state); - return $this; - } - - public function getFixedState() { - return $this->proxy->getFixedState(); - } - - public function setIsGhost($is_ghost) { - $this->isGhost = $is_ghost; - return $this; - } - - public function getIsGhost() { - return $this->isGhost; - } - - public function getDateModified() { - return $this->proxy->getDateModified(); - } - - public function getDateCreated() { - return $this->proxy->getDateCreated(); - } - - -/* -( PhabricatorMarkupInterface Implementation )-------------------------- */ - - - public function getMarkupFieldKey($field) { - return 'AI:'.$this->getID(); - } - - public function newMarkupEngine($field) { - return PhabricatorMarkupEngine::newDifferentialMarkupEngine(); - } - - public function getMarkupText($field) { - return $this->getContent(); - } - - public function didMarkupText($field, $output, PhutilMarkupEngine $engine) { - return $output; - } - - public function shouldUseMarkupCache($field) { - // Only cache submitted comments. - return ($this->getID() && $this->getAuditCommentID()); - } - } diff --git a/src/applications/audit/storage/PhabricatorAuditTransactionComment.php b/src/applications/audit/storage/PhabricatorAuditTransactionComment.php index bc4b1e675b..2a5c2b1ee8 100644 --- a/src/applications/audit/storage/PhabricatorAuditTransactionComment.php +++ b/src/applications/audit/storage/PhabricatorAuditTransactionComment.php @@ -72,4 +72,13 @@ final class PhabricatorAuditTransactionComment return $this->assertAttached($this->replyToComment); } + public function getAttribute($key, $default = null) { + return idx($this->attributes, $key, $default); + } + + public function setAttribute($key, $value) { + $this->attributes[$key] = $value; + return $this; + } + } diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index 576e0acb2b..8f432a6e98 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -238,7 +238,7 @@ final class DifferentialChangesetViewController extends DifferentialController { foreach ($inlines as $inline) { $engine->addObject( $inline, - PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY); + PhabricatorInlineComment::MARKUP_FIELD_BODY); } $engine->process(); diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 1de156a9b7..e2ab1c61ec 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -80,9 +80,15 @@ final class DifferentialInlineCommentEditController $viewer = $this->getViewer(); $inline = $this->loadComment($id); + if (!$inline) { + throw new Exception( + pht('Unable to load inline "%s".', $id)); + } + if (!$this->canEditInlineComment($viewer, $inline)) { throw new Exception(pht('That comment is not editable!')); } + return $inline; } @@ -161,7 +167,7 @@ final class DifferentialInlineCommentEditController return true; } - protected function deleteComment(PhabricatorInlineCommentInterface $inline) { + protected function deleteComment(PhabricatorInlineComment $inline) { $inline->openTransaction(); $inline->setIsDeleted(1)->save(); $this->syncDraft(); @@ -169,14 +175,14 @@ final class DifferentialInlineCommentEditController } protected function undeleteComment( - PhabricatorInlineCommentInterface $inline) { + PhabricatorInlineComment $inline) { $inline->openTransaction(); $inline->setIsDeleted(0)->save(); $this->syncDraft(); $inline->saveTransaction(); } - protected function saveComment(PhabricatorInlineCommentInterface $inline) { + protected function saveComment(PhabricatorInlineComment $inline) { $inline->openTransaction(); $inline->save(); $this->syncDraft(); @@ -184,7 +190,7 @@ final class DifferentialInlineCommentEditController } protected function loadObjectOwnerPHID( - PhabricatorInlineCommentInterface $inline) { + PhabricatorInlineComment $inline) { return $this->loadRevision()->getAuthorPHID(); } diff --git a/src/applications/differential/controller/DifferentialRevisionInlinesController.php b/src/applications/differential/controller/DifferentialRevisionInlinesController.php index c5eb063e99..735b9e8af9 100644 --- a/src/applications/differential/controller/DifferentialRevisionInlinesController.php +++ b/src/applications/differential/controller/DifferentialRevisionInlinesController.php @@ -124,7 +124,7 @@ final class DifferentialRevisionInlinesController $inline->getContent()); $state = $inline->getFixedState(); - if ($state == PhabricatorInlineCommentInterface::STATE_DONE) { + if ($state == PhabricatorInlineComment::STATE_DONE) { $status_icons[] = id(new PHUIIconView()) ->setIcon('fa-check green') ->addClass('mmr'); diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index fb7c9394af..eba596a2af 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -354,7 +354,7 @@ final class DifferentialChangesetParser extends Phobject { } public function parseInlineComment( - PhabricatorInlineCommentInterface $comment) { + PhabricatorInlineComment $comment) { // Parse only comments which are actually visible. if ($this->isCommentVisibleOnRenderedDiff($comment)) { @@ -1191,11 +1191,11 @@ final class DifferentialChangesetParser extends Phobject { * taking into consideration which halves of which changesets will actually * be shown. * - * @param PhabricatorInlineCommentInterface Comment to test for visibility. + * @param PhabricatorInlineComment Comment to test for visibility. * @return bool True if the comment is visible on the rendered diff. */ private function isCommentVisibleOnRenderedDiff( - PhabricatorInlineCommentInterface $comment) { + PhabricatorInlineComment $comment) { $changeset_id = $comment->getChangesetID(); $is_new = $comment->getIsNewFile(); @@ -1219,12 +1219,12 @@ final class DifferentialChangesetParser extends Phobject { * Note that the comment must appear somewhere on the rendered changeset, as * per isCommentVisibleOnRenderedDiff(). * - * @param PhabricatorInlineCommentInterface Comment to test for display + * @param PhabricatorInlineComment Comment to test for display * location. * @return bool True for right, false for left. */ private function isCommentOnRightSideWhenDisplayed( - PhabricatorInlineCommentInterface $comment) { + PhabricatorInlineComment $comment) { if (!$this->isCommentVisibleOnRenderedDiff($comment)) { throw new Exception(pht('Comment is not visible on changeset!')); diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index c856b9b14d..56fc3135e5 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -464,19 +464,19 @@ abstract class DifferentialChangesetHTMLRenderer } protected function buildInlineComment( - PhabricatorInlineCommentInterface $comment, + PhabricatorInlineComment $comment, $on_right = false) { - $user = $this->getUser(); - $edit = $user && - ($comment->getAuthorPHID() == $user->getPHID()) && + $viewer = $this->getUser(); + $edit = $viewer && + ($comment->getAuthorPHID() == $viewer->getPHID()) && ($comment->isDraft()) && $this->getShowEditAndReplyLinks(); - $allow_reply = (bool)$user && $this->getShowEditAndReplyLinks(); + $allow_reply = (bool)$viewer && $this->getShowEditAndReplyLinks(); $allow_done = !$comment->isDraft() && $this->getCanMarkDone(); return id(new PHUIDiffInlineCommentDetailView()) - ->setUser($user) + ->setViewer($viewer) ->setInlineComment($comment) ->setIsOnRight($on_right) ->setHandles($this->getHandles()) diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index a81a04cefb..d493c0e886 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -263,7 +263,7 @@ abstract class DifferentialChangesetRenderer extends Phobject { public function setNewComments(array $new_comments) { foreach ($new_comments as $line_number => $comments) { - assert_instances_of($comments, 'PhabricatorInlineCommentInterface'); + assert_instances_of($comments, 'PhabricatorInlineComment'); } $this->newComments = $new_comments; return $this; @@ -274,7 +274,7 @@ abstract class DifferentialChangesetRenderer extends Phobject { public function setOldComments(array $old_comments) { foreach ($old_comments as $line_number => $comments) { - assert_instances_of($comments, 'PhabricatorInlineCommentInterface'); + assert_instances_of($comments, 'PhabricatorInlineComment'); } $this->oldComments = $old_comments; return $this; diff --git a/src/applications/differential/storage/DifferentialInlineComment.php b/src/applications/differential/storage/DifferentialInlineComment.php index cbe05663db..b8a3edde32 100644 --- a/src/applications/differential/storage/DifferentialInlineComment.php +++ b/src/applications/differential/storage/DifferentialInlineComment.php @@ -1,53 +1,30 @@ proxy = new DifferentialTransactionComment(); + protected function newStorageObject() { + return new DifferentialTransactionComment(); } - public function __clone() { - $this->proxy = clone $this->proxy; + public function getControllerURI() { + return urisprintf( + '/differential/comment/inline/edit/%s/', + $this->getRevisionID()); } public function getTransactionCommentForSave() { $content_source = PhabricatorContentSource::newForSource( PhabricatorOldWorldContentSource::SOURCECONST); - $this->proxy + $this->getStorageObject() ->setViewPolicy('public') ->setEditPolicy($this->getAuthorPHID()) ->setContentSource($content_source) ->attachIsHidden(false) ->setCommentVersion(1); - return $this->proxy; - } - - public function openTransaction() { - $this->proxy->openTransaction(); - } - - public function saveTransaction() { - $this->proxy->saveTransaction(); - } - - public function save() { - $this->getTransactionCommentForSave()->save(); - - return $this; - } - - public function delete() { - $this->proxy->delete(); - - return $this; + return $this->getStorageObject(); } public function supportsHiding() { @@ -61,115 +38,34 @@ final class DifferentialInlineComment if (!$this->supportsHiding()) { return false; } - return $this->proxy->getIsHidden(); - } - - public function getID() { - return $this->proxy->getID(); - } - - public function getPHID() { - return $this->proxy->getPHID(); + return $this->getStorageObject()->getIsHidden(); } public static function newFromModernComment( DifferentialTransactionComment $comment) { $obj = new DifferentialInlineComment(); - $obj->proxy = $comment; + $obj->setStorageObject($comment); return $obj; } - public function setSyntheticAuthor($synthetic_author) { - $this->syntheticAuthor = $synthetic_author; - return $this; - } - - public function getSyntheticAuthor() { - return $this->syntheticAuthor; - } - - public function isCompatible(PhabricatorInlineCommentInterface $comment) { - return - ($this->getAuthorPHID() === $comment->getAuthorPHID()) && - ($this->getSyntheticAuthor() === $comment->getSyntheticAuthor()) && - ($this->getContent() === $comment->getContent()); - } - - public function setContent($content) { - $this->proxy->setContent($content); - return $this; - } - - public function getContent() { - return $this->proxy->getContent(); - } - - public function isDraft() { - return !$this->proxy->getTransactionPHID(); - } - public function setChangesetID($id) { - $this->proxy->setChangesetID($id); + $this->getStorageObject()->setChangesetID($id); return $this; } public function getChangesetID() { - return $this->proxy->getChangesetID(); - } - - public function setIsNewFile($is_new) { - $this->proxy->setIsNewFile($is_new); - return $this; - } - - public function getIsNewFile() { - return $this->proxy->getIsNewFile(); - } - - public function setLineNumber($number) { - $this->proxy->setLineNumber($number); - return $this; - } - - public function getLineNumber() { - return $this->proxy->getLineNumber(); - } - - public function setLineLength($length) { - $this->proxy->setLineLength($length); - return $this; - } - - public function getLineLength() { - return $this->proxy->getLineLength(); - } - - public function setCache($cache) { - return $this; - } - - public function getCache() { - return null; - } - - public function setAuthorPHID($phid) { - $this->proxy->setAuthorPHID($phid); - return $this; - } - - public function getAuthorPHID() { - return $this->proxy->getAuthorPHID(); + return $this->getStorageObject()->getChangesetID(); } public function setRevision(DifferentialRevision $revision) { - $this->proxy->setRevisionPHID($revision->getPHID()); + $this->getStorageObject()->setRevisionPHID($revision->getPHID()); return $this; } public function getRevisionPHID() { - return $this->proxy->getRevisionPHID(); + return $this->getStorageObject()->getRevisionPHID(); } // Although these are purely transitional, they're also *extra* dumb. @@ -180,7 +76,7 @@ final class DifferentialInlineComment } public function getRevisionID() { - $phid = $this->proxy->getRevisionPHID(); + $phid = $this->getStorageObject()->getRevisionPHID(); if (!$phid) { return null; } @@ -194,98 +90,4 @@ final class DifferentialInlineComment return $revision->getID(); } - // When setting a comment ID, we also generate a phantom transaction PHID for - // the future transaction. - - public function setCommentID($id) { - $this->proxy->setTransactionPHID( - PhabricatorPHID::generateNewPHID( - PhabricatorApplicationTransactionTransactionPHIDType::TYPECONST, - DifferentialRevisionPHIDType::TYPECONST)); - return $this; - } - - public function setReplyToCommentPHID($phid) { - $this->proxy->setReplyToCommentPHID($phid); - return $this; - } - - public function getReplyToCommentPHID() { - return $this->proxy->getReplyToCommentPHID(); - } - - public function setHasReplies($has_replies) { - $this->proxy->setHasReplies($has_replies); - return $this; - } - - public function getHasReplies() { - return $this->proxy->getHasReplies(); - } - - public function setIsDeleted($is_deleted) { - $this->proxy->setIsDeleted($is_deleted); - return $this; - } - - public function getIsDeleted() { - return $this->proxy->getIsDeleted(); - } - - public function setFixedState($state) { - $this->proxy->setFixedState($state); - return $this; - } - - public function getFixedState() { - return $this->proxy->getFixedState(); - } - - public function setIsGhost($is_ghost) { - $this->isGhost = $is_ghost; - return $this; - } - - public function getIsGhost() { - return $this->isGhost; - } - - public function makeEphemeral() { - $this->proxy->makeEphemeral(); - return $this; - } - - public function getDateModified() { - return $this->proxy->getDateModified(); - } - - public function getDateCreated() { - return $this->proxy->getDateCreated(); - } - -/* -( PhabricatorMarkupInterface Implementation )-------------------------- */ - - - public function getMarkupFieldKey($field) { - $content = $this->getMarkupText($field); - return PhabricatorMarkupEngine::digestRemarkupContent($this, $content); - } - - public function newMarkupEngine($field) { - return PhabricatorMarkupEngine::newDifferentialMarkupEngine(); - } - - public function getMarkupText($field) { - return $this->getContent(); - } - - public function didMarkupText($field, $output, PhutilMarkupEngine $engine) { - return $output; - } - - public function shouldUseMarkupCache($field) { - // Only cache submitted comments. - return ($this->getID() && !$this->isDraft()); - } - } diff --git a/src/applications/differential/storage/DifferentialTransactionComment.php b/src/applications/differential/storage/DifferentialTransactionComment.php index b2c24c340c..257b205a58 100644 --- a/src/applications/differential/storage/DifferentialTransactionComment.php +++ b/src/applications/differential/storage/DifferentialTransactionComment.php @@ -118,4 +118,13 @@ final class DifferentialTransactionComment return $this; } + public function getAttribute($key, $default = null) { + return idx($this->attributes, $key, $default); + } + + public function setAttribute($key, $value) { + $this->attributes[$key] = $value; + return $this; + } + } diff --git a/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php b/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php index 8ad4e00762..486793b51e 100644 --- a/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php @@ -40,8 +40,8 @@ final class DifferentialRevisionInlineTransaction $is_done = false; switch ($comment->getFixedState()) { - case PhabricatorInlineCommentInterface::STATE_DONE: - case PhabricatorInlineCommentInterface::STATE_UNDRAFT: + case PhabricatorInlineComment::STATE_DONE: + case PhabricatorInlineComment::STATE_UNDRAFT: $is_done = true; break; } diff --git a/src/applications/diffusion/controller/DiffusionDiffController.php b/src/applications/diffusion/controller/DiffusionDiffController.php index 17d6b85af1..fc505ab245 100644 --- a/src/applications/diffusion/controller/DiffusionDiffController.php +++ b/src/applications/diffusion/controller/DiffusionDiffController.php @@ -120,7 +120,7 @@ final class DiffusionDiffController extends DiffusionController { foreach ($inlines as $inline) { $engine->addObject( $inline, - PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY); + PhabricatorInlineComment::MARKUP_FIELD_BODY); } $engine->process(); diff --git a/src/applications/diffusion/controller/DiffusionInlineCommentController.php b/src/applications/diffusion/controller/DiffusionInlineCommentController.php index ce69511dde..33e2903799 100644 --- a/src/applications/diffusion/controller/DiffusionInlineCommentController.php +++ b/src/applications/diffusion/controller/DiffusionInlineCommentController.php @@ -105,7 +105,7 @@ final class DiffusionInlineCommentController } // Saved comments may not be edited. - if ($inline->getAuditCommentID()) { + if ($inline->getTransactionPHID()) { return false; } @@ -117,21 +117,21 @@ final class DiffusionInlineCommentController return true; } - protected function deleteComment(PhabricatorInlineCommentInterface $inline) { + protected function deleteComment(PhabricatorInlineComment $inline) { $inline->setIsDeleted(1)->save(); } protected function undeleteComment( - PhabricatorInlineCommentInterface $inline) { + PhabricatorInlineComment $inline) { $inline->setIsDeleted(0)->save(); } - protected function saveComment(PhabricatorInlineCommentInterface $inline) { + protected function saveComment(PhabricatorInlineComment $inline) { return $inline->save(); } protected function loadObjectOwnerPHID( - PhabricatorInlineCommentInterface $inline) { + PhabricatorInlineComment $inline) { return $this->loadCommit()->getAuthorPHID(); } diff --git a/src/applications/transactions/constants/PhabricatorTransactions.php b/src/applications/transactions/constants/PhabricatorTransactions.php index 3b523417dd..aa83227625 100644 --- a/src/applications/transactions/constants/PhabricatorTransactions.php +++ b/src/applications/transactions/constants/PhabricatorTransactions.php @@ -32,10 +32,10 @@ final class PhabricatorTransactions extends Phobject { public static function getInlineStateMap() { return array( - PhabricatorInlineCommentInterface::STATE_DRAFT => - PhabricatorInlineCommentInterface::STATE_DONE, - PhabricatorInlineCommentInterface::STATE_UNDRAFT => - PhabricatorInlineCommentInterface::STATE_UNDONE, + PhabricatorInlineComment::STATE_DRAFT => + PhabricatorInlineComment::STATE_DONE, + PhabricatorInlineComment::STATE_UNDRAFT => + PhabricatorInlineComment::STATE_UNDONE, ); } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 1ec29557da..b815354d11 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -1690,7 +1690,7 @@ abstract class PhabricatorApplicationTransaction $done = 0; $undone = 0; foreach ($new as $phid => $state) { - $is_done = ($state == PhabricatorInlineCommentInterface::STATE_DONE); + $is_done = ($state == PhabricatorInlineComment::STATE_DONE); // See PHI995. If you're marking your own inline comments as "Done", // don't count them when rendering a timeline story. In the case where diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index e6572672ce..bba4666bb9 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -9,13 +9,13 @@ abstract class PhabricatorInlineCommentController abstract protected function loadCommentForDone($id); abstract protected function loadCommentByPHID($phid); abstract protected function loadObjectOwnerPHID( - PhabricatorInlineCommentInterface $inline); + PhabricatorInlineComment $inline); abstract protected function deleteComment( - PhabricatorInlineCommentInterface $inline); + PhabricatorInlineComment $inline); abstract protected function undeleteComment( - PhabricatorInlineCommentInterface $inline); + PhabricatorInlineComment $inline); abstract protected function saveComment( - PhabricatorInlineCommentInterface $inline); + PhabricatorInlineComment $inline); protected function hideComments(array $ids) { throw new PhutilMethodNotImplementedException(); @@ -119,20 +119,20 @@ abstract class PhabricatorInlineCommentController $is_draft_state = false; $is_checked = false; switch ($inline->getFixedState()) { - case PhabricatorInlineCommentInterface::STATE_DRAFT: - $next_state = PhabricatorInlineCommentInterface::STATE_UNDONE; + case PhabricatorInlineComment::STATE_DRAFT: + $next_state = PhabricatorInlineComment::STATE_UNDONE; break; - case PhabricatorInlineCommentInterface::STATE_UNDRAFT: - $next_state = PhabricatorInlineCommentInterface::STATE_DONE; + case PhabricatorInlineComment::STATE_UNDRAFT: + $next_state = PhabricatorInlineComment::STATE_DONE; $is_checked = true; break; - case PhabricatorInlineCommentInterface::STATE_DONE: - $next_state = PhabricatorInlineCommentInterface::STATE_UNDRAFT; + case PhabricatorInlineComment::STATE_DONE: + $next_state = PhabricatorInlineComment::STATE_UNDRAFT; $is_draft_state = true; break; default: - case PhabricatorInlineCommentInterface::STATE_UNDONE: - $next_state = PhabricatorInlineCommentInterface::STATE_DRAFT; + case PhabricatorInlineComment::STATE_UNDONE: + $next_state = PhabricatorInlineComment::STATE_DRAFT; $is_draft_state = true; $is_checked = true; break; @@ -187,7 +187,10 @@ abstract class PhabricatorInlineCommentController if ($request->isFormPost()) { if (strlen($text)) { - $inline->setContent($text); + $inline + ->setContent($text) + ->setIsEditing(false); + $this->saveComment($inline); return $this->buildRenderedCommentResponse( $inline, @@ -196,65 +199,25 @@ abstract class PhabricatorInlineCommentController $this->deleteComment($inline); return $this->buildEmptyResponse(); } + } else { + $inline->setIsEditing(true); + + if (strlen($text)) { + $inline->setContent($text); + } + + $this->saveComment($inline); } - $edit_dialog = $this->buildEditDialog(); - $edit_dialog->setTitle(pht('Edit Inline Comment')); - - $edit_dialog->addHiddenInput('id', $this->getCommentID()); - $edit_dialog->addHiddenInput('op', 'edit'); - - $edit_dialog->appendChild( - $this->renderTextArea( - nonempty($text, $inline->getContent()))); + $edit_dialog = $this->buildEditDialog($inline) + ->setTitle(pht('Edit Inline Comment')); $view = $this->buildScaffoldForView($edit_dialog); - return id(new AphrontAjaxResponse()) - ->setContent($view->render()); - case 'create': - $text = $this->getCommentText(); - - if (!$request->isFormPost() || !strlen($text)) { - return $this->buildEmptyResponse(); - } - - $inline = $this->createComment() - ->setChangesetID($this->getChangesetID()) - ->setAuthorPHID($viewer->getPHID()) - ->setLineNumber($this->getLineNumber()) - ->setLineLength($this->getLineLength()) - ->setIsNewFile($this->getIsNewFile()) - ->setContent($text); - - if ($this->getReplyToCommentPHID()) { - $inline->setReplyToCommentPHID($this->getReplyToCommentPHID()); - } - - // If you own this object, mark your own inlines as "Done" by default. - $owner_phid = $this->loadObjectOwnerPHID($inline); - if ($owner_phid) { - if ($viewer->getPHID() == $owner_phid) { - $fixed_state = PhabricatorInlineCommentInterface::STATE_DRAFT; - $inline->setFixedState($fixed_state); - } - } - - $this->saveComment($inline); - - return $this->buildRenderedCommentResponse( - $inline, - $this->getIsOnRight()); + return $this->newInlineResponse($inline, $view); + case 'new': case 'reply': default: - $edit_dialog = $this->buildEditDialog(); - - if ($this->getOperation() == 'reply') { - $edit_dialog->setTitle(pht('Reply to Inline Comment')); - } else { - $edit_dialog->setTitle(pht('New Inline Comment')); - } - // NOTE: We read the values from the client (the display values), not // the values from the database (the original values) when replying. // In particular, when replying to a ghost comment which was moved @@ -265,18 +228,38 @@ abstract class PhabricatorInlineCommentController $number = $this->getLineNumber(); $length = $this->getLineLength(); - $edit_dialog->addHiddenInput('op', 'create'); - $edit_dialog->addHiddenInput('is_new', $is_new); - $edit_dialog->addHiddenInput('number', $number); - $edit_dialog->addHiddenInput('length', $length); + $inline = $this->createComment() + ->setChangesetID($this->getChangesetID()) + ->setAuthorPHID($viewer->getPHID()) + ->setIsNewFile($is_new) + ->setLineNumber($number) + ->setLineLength($length) + ->setContent($this->getCommentText()) + ->setReplyToCommentPHID($this->getReplyToCommentPHID()) + ->setIsEditing(true); - $text_area = $this->renderTextArea($this->getCommentText()); - $edit_dialog->appendChild($text_area); + // If you own this object, mark your own inlines as "Done" by default. + $owner_phid = $this->loadObjectOwnerPHID($inline); + if ($owner_phid) { + if ($viewer->getPHID() == $owner_phid) { + $fixed_state = PhabricatorInlineComment::STATE_DRAFT; + $inline->setFixedState($fixed_state); + } + } + + $this->saveComment($inline); + + $edit_dialog = $this->buildEditDialog($inline); + + if ($this->getOperation() == 'reply') { + $edit_dialog->setTitle(pht('Reply to Inline Comment')); + } else { + $edit_dialog->setTitle(pht('New Inline Comment')); + } $view = $this->buildScaffoldForView($edit_dialog); - return id(new AphrontAjaxResponse()) - ->setContent($view->render()); + return $this->newInlineResponse($inline, $view); } } @@ -320,20 +303,15 @@ abstract class PhabricatorInlineCommentController } } - private function buildEditDialog() { + private function buildEditDialog(PhabricatorInlineComment $inline) { $request = $this->getRequest(); $viewer = $this->getViewer(); $edit_dialog = id(new PHUIDiffInlineCommentEditView()) - ->setUser($viewer) - ->setSubmitURI($request->getRequestURI()) + ->setViewer($viewer) + ->setInlineComment($inline) ->setIsOnRight($this->getIsOnRight()) - ->setIsNewFile($this->getIsNewFile()) - ->setNumber($this->getLineNumber()) - ->setLength($this->getLineLength()) - ->setRenderer($this->getRenderer()) - ->setReplyToCommentPHID($this->getReplyToCommentPHID()) - ->setChangesetID($this->getChangesetID()); + ->setRenderer($this->getRenderer()); return $edit_dialog; } @@ -342,12 +320,13 @@ abstract class PhabricatorInlineCommentController return id(new AphrontAjaxResponse()) ->setContent( array( - 'markup' => '', + 'inline' => array(), + 'view' => null, )); } private function buildRenderedCommentResponse( - PhabricatorInlineCommentInterface $inline, + PhabricatorInlineComment $inline, $on_right) { $request = $this->getRequest(); @@ -357,7 +336,7 @@ abstract class PhabricatorInlineCommentController $engine->setViewer($viewer); $engine->addObject( $inline, - PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY); + PhabricatorInlineComment::MARKUP_FIELD_BODY); $engine->process(); $phids = array($viewer->getPHID()); @@ -377,21 +356,7 @@ abstract class PhabricatorInlineCommentController $view = $this->buildScaffoldForView($view); - return id(new AphrontAjaxResponse()) - ->setContent( - array( - 'inlineCommentID' => $inline->getID(), - 'markup' => $view->render(), - )); - } - - private function renderTextArea($text) { - return id(new PhabricatorRemarkupControl()) - ->setViewer($this->getViewer()) - ->setSigil('differential-inline-comment-edit-textarea') - ->setName('text') - ->setValue($text) - ->setDisableFullScreen(true); + return $this->newInlineResponse($inline, $view); } private function buildScaffoldForView(PHUIDiffInlineCommentView $view) { @@ -404,4 +369,19 @@ abstract class PhabricatorInlineCommentController ->addRowScaffold($view); } + private function newInlineResponse( + PhabricatorInlineComment $inline, + $view) { + + $response = array( + 'inline' => array( + 'id' => $inline->getID(), + ), + 'view' => hsprintf('%s', $view), + ); + + return id(new AphrontAjaxResponse()) + ->setContent($response); + } + } diff --git a/src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php b/src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php index 9ecb9b35c7..1d46e85e6d 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php @@ -11,14 +11,14 @@ abstract class PhabricatorInlineCommentPreviewController $viewer = $request->getUser(); $inlines = $this->loadInlineComments(); - assert_instances_of($inlines, 'PhabricatorInlineCommentInterface'); + assert_instances_of($inlines, 'PhabricatorInlineComment'); $engine = new PhabricatorMarkupEngine(); $engine->setViewer($viewer); foreach ($inlines as $inline) { $engine->addObject( $inline, - PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY); + PhabricatorInlineComment::MARKUP_FIELD_BODY); } $engine->process(); diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php new file mode 100644 index 0000000000..8440765525 --- /dev/null +++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php @@ -0,0 +1,232 @@ +storageObject = clone $this->storageObject; + } + + public function setSyntheticAuthor($synthetic_author) { + $this->syntheticAuthor = $synthetic_author; + return $this; + } + + public function getSyntheticAuthor() { + return $this->syntheticAuthor; + } + + public function setStorageObject($storage_object) { + $this->storageObject = $storage_object; + return $this; + } + + public function getStorageObject() { + if (!$this->storageObject) { + $this->storageObject = $this->newStorageObject(); + } + + return $this->storageObject; + } + + abstract protected function newStorageObject(); + abstract public function getControllerURI(); + + abstract public function setChangesetID($id); + abstract public function getChangesetID(); + + abstract public function supportsHiding(); + abstract public function isHidden(); + + public function isDraft() { + return !$this->getTransactionPHID(); + } + + public function getTransactionPHID() { + return $this->getStorageObject()->getTransactionPHID(); + } + + public function isCompatible(PhabricatorInlineComment $comment) { + return + ($this->getAuthorPHID() === $comment->getAuthorPHID()) && + ($this->getSyntheticAuthor() === $comment->getSyntheticAuthor()) && + ($this->getContent() === $comment->getContent()); + } + + public function setIsGhost($is_ghost) { + $this->isGhost = $is_ghost; + return $this; + } + + public function getIsGhost() { + return $this->isGhost; + } + + public function setContent($content) { + $this->getStorageObject()->setContent($content); + return $this; + } + + public function getContent() { + return $this->getStorageObject()->getContent(); + } + + public function getID() { + return $this->getStorageObject()->getID(); + } + + public function getPHID() { + return $this->getStorageObject()->getPHID(); + } + + public function setIsNewFile($is_new) { + $this->getStorageObject()->setIsNewFile($is_new); + return $this; + } + + public function getIsNewFile() { + return $this->getStorageObject()->getIsNewFile(); + } + + public function setFixedState($state) { + $this->getStorageObject()->setFixedState($state); + return $this; + } + + public function setHasReplies($has_replies) { + $this->getStorageObject()->setHasReplies($has_replies); + return $this; + } + + public function getHasReplies() { + return $this->getStorageObject()->getHasReplies(); + } + + public function getFixedState() { + return $this->getStorageObject()->getFixedState(); + } + + public function setLineNumber($number) { + $this->getStorageObject()->setLineNumber($number); + return $this; + } + + public function getLineNumber() { + return $this->getStorageObject()->getLineNumber(); + } + + public function setLineLength($length) { + $this->getStorageObject()->setLineLength($length); + return $this; + } + + public function getLineLength() { + return $this->getStorageObject()->getLineLength(); + } + + public function setAuthorPHID($phid) { + $this->getStorageObject()->setAuthorPHID($phid); + return $this; + } + + public function getAuthorPHID() { + return $this->getStorageObject()->getAuthorPHID(); + } + + public function setReplyToCommentPHID($phid) { + $this->getStorageObject()->setReplyToCommentPHID($phid); + return $this; + } + + public function getReplyToCommentPHID() { + return $this->getStorageObject()->getReplyToCommentPHID(); + } + + public function setIsDeleted($is_deleted) { + $this->getStorageObject()->setIsDeleted($is_deleted); + return $this; + } + + public function getIsDeleted() { + return $this->getStorageObject()->getIsDeleted(); + } + + public function setIsEditing($is_editing) { + $this->getStorageObject()->setAttribute('editing', (bool)$is_editing); + return $this; + } + + public function getIsEditing() { + return (bool)$this->getStorageObject()->getAttribute('editing', false); + } + + public function getDateModified() { + return $this->getStorageObject()->getDateModified(); + } + + public function getDateCreated() { + return $this->getStorageObject()->getDateCreated(); + } + + public function openTransaction() { + $this->getStorageObject()->openTransaction(); + } + + public function saveTransaction() { + $this->getStorageObject()->saveTransaction(); + } + + public function save() { + $this->getTransactionCommentForSave()->save(); + return $this; + } + + public function delete() { + $this->getStorageObject()->delete(); + return $this; + } + + public function makeEphemeral() { + $this->getStorageObject()->makeEphemeral(); + return $this; + } + + +/* -( PhabricatorMarkupInterface Implementation )-------------------------- */ + + + public function getMarkupFieldKey($field) { + $content = $this->getMarkupText($field); + return PhabricatorMarkupEngine::digestRemarkupContent($this, $content); + } + + public function newMarkupEngine($field) { + return PhabricatorMarkupEngine::newDifferentialMarkupEngine(); + } + + public function getMarkupText($field) { + return $this->getContent(); + } + + public function didMarkupText($field, $output, PhutilMarkupEngine $engine) { + return $output; + } + + public function shouldUseMarkupCache($field) { + return !$this->isDraft(); + } + +} diff --git a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php deleted file mode 100644 index 5c2bafdc86..0000000000 --- a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php +++ /dev/null @@ -1,66 +0,0 @@ -inlineComment = $comment; - return $this; - } - public function isHidden() { - return $this->inlineComment->isHidden(); + return $this->getInlineComment()->isHidden(); } public function setHandles(array $handles) { @@ -48,15 +41,6 @@ final class PHUIDiffInlineCommentDetailView return $this; } - public function setRenderer($renderer) { - $this->renderer = $renderer; - return $this; - } - - public function getRenderer() { - return $this->renderer; - } - public function setCanMarkDone($can_mark_done) { $this->canMarkDone = $can_mark_done; return $this; @@ -76,7 +60,7 @@ final class PHUIDiffInlineCommentDetailView } public function getAnchorName() { - $inline = $this->inlineComment; + $inline = $this->getInlineComment(); if ($inline->getID()) { return 'inline-'.$inline->getID(); } @@ -93,49 +77,18 @@ final class PHUIDiffInlineCommentDetailView public function render() { require_celerity_resource('phui-inline-comment-view-css'); - $inline = $this->inlineComment; + $inline = $this->getInlineComment(); $classes = array( 'differential-inline-comment', ); - $is_fixed = false; - switch ($inline->getFixedState()) { - case PhabricatorInlineCommentInterface::STATE_DONE: - case PhabricatorInlineCommentInterface::STATE_DRAFT: - $is_fixed = true; - break; - } - - $is_draft_done = false; - switch ($inline->getFixedState()) { - case PhabricatorInlineCommentInterface::STATE_DRAFT: - case PhabricatorInlineCommentInterface::STATE_UNDRAFT: - $is_draft_done = true; - break; - } - $is_synthetic = false; if ($inline->getSyntheticAuthor()) { $is_synthetic = true; } - $metadata = array( - 'id' => $inline->getID(), - 'phid' => $inline->getPHID(), - 'changesetID' => $inline->getChangesetID(), - 'number' => $inline->getLineNumber(), - 'length' => $inline->getLineLength(), - 'isNewFile' => (bool)$inline->getIsNewFile(), - 'on_right' => $this->getIsOnRight(), - 'original' => $inline->getContent(), - 'replyToCommentPHID' => $inline->getReplyToCommentPHID(), - 'isDraft' => $inline->isDraft(), - 'isFixed' => $is_fixed, - 'isGhost' => $inline->getIsGhost(), - 'isSynthetic' => $is_synthetic, - 'isDraftDone' => $is_draft_done, - ); + $metadata = $this->getInlineCommentMetadata(); $sigil = 'differential-inline-comment'; if ($this->preview) { @@ -299,19 +252,19 @@ final class PHUIDiffInlineCommentDetailView if (!$is_synthetic) { $draft_state = false; switch ($inline->getFixedState()) { - case PhabricatorInlineCommentInterface::STATE_DRAFT: + case PhabricatorInlineComment::STATE_DRAFT: $is_done = $mark_done; $draft_state = true; break; - case PhabricatorInlineCommentInterface::STATE_UNDRAFT: + case PhabricatorInlineComment::STATE_UNDRAFT: $is_done = !$mark_done; $draft_state = true; break; - case PhabricatorInlineCommentInterface::STATE_DONE: + case PhabricatorInlineComment::STATE_DONE: $is_done = true; break; default: - case PhabricatorInlineCommentInterface::STATE_UNDONE: + case PhabricatorInlineComment::STATE_UNDONE: $is_done = false; break; } @@ -372,7 +325,7 @@ final class PHUIDiffInlineCommentDetailView $content = $this->markupEngine->getOutput( $inline, - PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY); + PhabricatorInlineComment::MARKUP_FIELD_BODY); if ($this->preview) { $anchor = null; @@ -490,7 +443,7 @@ final class PHUIDiffInlineCommentDetailView } private function canHide() { - $inline = $this->inlineComment; + $inline = $this->getInlineComment(); if ($inline->isDraft()) { return false; diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php index 7ad3b1f1a6..c1449b77ba 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php @@ -3,90 +3,23 @@ final class PHUIDiffInlineCommentEditView extends PHUIDiffInlineCommentView { - private $inputs = array(); - private $uri; private $title; - private $number; - private $length; - private $renderer; - private $isNewFile; - private $replyToCommentPHID; - private $changesetID; - - public function setIsNewFile($is_new_file) { - $this->isNewFile = $is_new_file; - return $this; - } - - public function getIsNewFile() { - return $this->isNewFile; - } - - public function setRenderer($renderer) { - $this->renderer = $renderer; - return $this; - } - - public function getRenderer() { - return $this->renderer; - } - - public function addHiddenInput($key, $value) { - $this->inputs[] = array($key, $value); - return $this; - } - - public function setSubmitURI($uri) { - $this->uri = $uri; - return $this; - } public function setTitle($title) { $this->title = $title; return $this; } - public function setReplyToCommentPHID($reply_to_phid) { - $this->replyToCommentPHID = $reply_to_phid; - return $this; - } - - public function getReplyToCommentPHID() { - return $this->replyToCommentPHID; - } - - public function setChangesetID($changeset_id) { - $this->changesetID = $changeset_id; - return $this; - } - - public function getChangesetID() { - return $this->changesetID; - } - - public function setNumber($number) { - $this->number = $number; - return $this; - } - - public function setLength($length) { - $this->length = $length; - return $this; - } - public function render() { - if (!$this->uri) { - throw new PhutilInvalidStateException('setSubmitURI'); - } - $viewer = $this->getViewer(); + $inline = $this->getInlineComment(); $content = phabricator_form( $viewer, array( - 'action' => $this->uri, - 'method' => 'POST', - 'sigil' => 'inline-edit-form', + 'action' => $inline->getControllerURI(), + 'method' => 'POST', + 'sigil' => 'inline-edit-form', ), array( $this->renderInputs(), @@ -97,13 +30,16 @@ final class PHUIDiffInlineCommentEditView } private function renderInputs() { - $inputs = $this->inputs; - $out = array(); + $inputs = array(); + $inline = $this->getInlineComment(); - $inputs[] = array('on_right', (bool)$this->getIsOnRight()); - $inputs[] = array('replyToCommentPHID', $this->getReplyToCommentPHID()); + $inputs[] = array('op', 'edit'); + $inputs[] = array('id', $inline->getID()); + + $inputs[] = array('on_right', $this->getIsOnRight()); $inputs[] = array('renderer', $this->getRenderer()); - $inputs[] = array('changesetID', $this->getChangesetID()); + + $out = array(); foreach ($inputs as $input) { list($name, $value) = $input; @@ -115,6 +51,7 @@ final class PHUIDiffInlineCommentEditView 'value' => $value, )); } + return $out; } @@ -141,7 +78,7 @@ final class PHUIDiffInlineCommentEditView array( 'class' => 'differential-inline-comment-edit-body', ), - $this->renderChildren()); + $this->newTextarea()); $edit = phutil_tag( 'div', @@ -152,19 +89,14 @@ final class PHUIDiffInlineCommentEditView $buttons, )); + $inline = $this->getInlineComment(); + return javelin_tag( 'div', array( 'class' => 'differential-inline-comment-edit', 'sigil' => 'differential-inline-comment', - 'meta' => array( - 'changesetID' => $this->getChangesetID(), - 'on_right' => $this->getIsOnRight(), - 'isNewFile' => (bool)$this->getIsNewFile(), - 'number' => $this->number, - 'length' => $this->length, - 'replyToCommentPHID' => $this->getReplyToCommentPHID(), - ), + 'meta' => $this->getInlineCommentMetadata(), ), array( $title, @@ -173,4 +105,18 @@ final class PHUIDiffInlineCommentEditView )); } + private function newTextarea() { + $viewer = $this->getViewer(); + $inline = $this->getInlineComment(); + + $text = $inline->getContent(); + + return id(new PhabricatorRemarkupControl()) + ->setViewer($viewer) + ->setSigil('differential-inline-comment-edit-textarea') + ->setName('text') + ->setValue($text) + ->setDisableFullScreen(true); + } + } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentPreviewListView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentPreviewListView.php index 4bede1603b..dcf9cb319e 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentPreviewListView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentPreviewListView.php @@ -54,7 +54,7 @@ final class PHUIDiffInlineCommentPreviewListView foreach ($inlines as $inline) { $engine->addObject( $inline, - PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY); + PhabricatorInlineComment::MARKUP_FIELD_BODY); } $engine->process(); diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php index e2c89e238c..7e468ed607 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php @@ -3,6 +3,17 @@ abstract class PHUIDiffInlineCommentView extends AphrontView { private $isOnRight; + private $renderer; + private $inlineComment; + + public function setInlineComment(PhabricatorInlineComment $comment) { + $this->inlineComment = $comment; + return $this; + } + + public function getInlineComment() { + return $this->inlineComment; + } public function getIsOnRight() { return $this->isOnRight; @@ -13,6 +24,15 @@ abstract class PHUIDiffInlineCommentView extends AphrontView { return $this; } + public function setRenderer($renderer) { + $this->renderer = $renderer; + return $this; + } + + public function getRenderer() { + return $this->renderer; + } + public function getScaffoldCellID() { return null; } @@ -33,4 +53,46 @@ abstract class PHUIDiffInlineCommentView extends AphrontView { } } + protected function getInlineCommentMetadata() { + $inline = $this->getInlineComment(); + + $is_synthetic = (bool)$inline->getSyntheticAuthor(); + + $is_fixed = false; + switch ($inline->getFixedState()) { + case PhabricatorInlineComment::STATE_DONE: + case PhabricatorInlineComment::STATE_DRAFT: + $is_fixed = true; + break; + } + + $is_draft_done = false; + switch ($inline->getFixedState()) { + case PhabricatorInlineComment::STATE_DRAFT: + case PhabricatorInlineComment::STATE_UNDRAFT: + $is_draft_done = true; + break; + } + + return array( + 'id' => $inline->getID(), + 'phid' => $inline->getPHID(), + 'changesetID' => $inline->getChangesetID(), + 'number' => $inline->getLineNumber(), + 'length' => $inline->getLineLength(), + 'isNewFile' => (bool)$inline->getIsNewFile(), + 'original' => $inline->getContent(), + 'replyToCommentPHID' => $inline->getReplyToCommentPHID(), + 'isDraft' => $inline->isDraft(), + 'isFixed' => $is_fixed, + 'isGhost' => $inline->getIsGhost(), + 'isSynthetic' => $is_synthetic, + 'isDraftDone' => $is_draft_done, + 'isEditing' => $inline->getIsEditing(), + + 'on_right' => $this->getIsOnRight(), + ); + } + + } diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index e18991eafb..d977d3d1e6 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -26,30 +26,6 @@ JX.install('DiffChangesetList', { var onexpand = JX.bind(this, this._ifawake, this._oncollapse, false); JX.Stratcom.listen('click', 'reveal-inline', onexpand); - var onedit = JX.bind(this, this._ifawake, this._onaction, 'edit'); - JX.Stratcom.listen( - 'click', - ['differential-inline-comment', 'differential-inline-edit'], - onedit); - - var ondone = JX.bind(this, this._ifawake, this._onaction, 'done'); - JX.Stratcom.listen( - 'click', - ['differential-inline-comment', 'differential-inline-done'], - ondone); - - var ondelete = JX.bind(this, this._ifawake, this._onaction, 'delete'); - JX.Stratcom.listen( - 'click', - ['differential-inline-comment', 'differential-inline-delete'], - ondelete); - - var onreply = JX.bind(this, this._ifawake, this._onaction, 'reply'); - JX.Stratcom.listen( - 'click', - ['differential-inline-comment', 'differential-inline-reply'], - onreply); - var onresize = JX.bind(this, this._ifawake, this._onresize); JX.Stratcom.listen('resize', null, onresize); @@ -85,6 +61,8 @@ JX.install('DiffChangesetList', { 'mouseup', null, onrangeup); + + this._setupInlineCommentListeners(); }, properties: { @@ -1163,56 +1141,6 @@ JX.install('DiffChangesetList', { } }, - _onaction: function(action, e) { - e.kill(); - - var inline = this._getInlineForEvent(e); - var is_ref = false; - - // If we don't have a natural inline object, the user may have clicked - // an action (like "Delete") inside a preview element at the bottom of - // the page. - - // If they did, try to find an associated normal inline to act on, and - // pretend they clicked that instead. This makes the overall state of - // the page more consistent. - - // However, there may be no normal inline (for example, because it is - // on a version of the diff which is not visible). In this case, we - // act by reference. - - if (inline === null) { - var data = e.getNodeData('differential-inline-comment'); - inline = this.getInlineByID(data.id); - if (inline) { - is_ref = true; - } else { - switch (action) { - case 'delete': - this._deleteInlineByID(data.id); - return; - } - } - } - - // TODO: For normal operations, highlight the inline range here. - - switch (action) { - case 'edit': - inline.edit(); - break; - case 'done': - inline.toggleDone(); - break; - case 'delete': - inline.delete(is_ref); - break; - case 'reply': - inline.reply(); - break; - } - }, - redrawPreview: function() { // TODO: This isn't the cleanest way to find the preview form, but // rendering no longer has direct access to it. @@ -2138,6 +2066,113 @@ JX.install('DiffChangesetList', { var tree = this._getTreeView(); JX.DOM.setContent(flank_body, tree.getNode()); + }, + + _setupInlineCommentListeners: function() { + var onsave = JX.bind(this, this._onInlineEvent, 'save'); + JX.Stratcom.listen( + ['submit', 'didSyntheticSubmit'], + 'inline-edit-form', + onsave); + + var oncancel = JX.bind(this, this._onInlineEvent, 'cancel'); + JX.Stratcom.listen( + 'click', + 'inline-edit-cancel', + oncancel); + + var onundo = JX.bind(this, this._onInlineEvent, 'undo'); + JX.Stratcom.listen( + 'click', + 'differential-inline-comment-undo', + onundo); + + var onedit = JX.bind(this, this._onInlineEvent, 'edit'); + JX.Stratcom.listen( + 'click', + ['differential-inline-comment', 'differential-inline-edit'], + onedit); + + var ondone = JX.bind(this, this._onInlineEvent, 'done'); + JX.Stratcom.listen( + 'click', + ['differential-inline-comment', 'differential-inline-done'], + ondone); + + var ondelete = JX.bind(this, this._onInlineEvent, 'delete'); + JX.Stratcom.listen( + 'click', + ['differential-inline-comment', 'differential-inline-delete'], + ondelete); + + var onreply = JX.bind(this, this._onInlineEvent, 'reply'); + JX.Stratcom.listen( + 'click', + ['differential-inline-comment', 'differential-inline-reply'], + onreply); + }, + + _onInlineEvent: function(action, e) { + if (this.isAsleep()) { + return; + } + + e.kill(); + + var inline = this._getInlineForEvent(e); + var is_ref = false; + + // If we don't have a natural inline object, the user may have clicked + // an action (like "Delete") inside a preview element at the bottom of + // the page. + + // If they did, try to find an associated normal inline to act on, and + // pretend they clicked that instead. This makes the overall state of + // the page more consistent. + + // However, there may be no normal inline (for example, because it is + // on a version of the diff which is not visible). In this case, we + // act by reference. + + if (inline === null) { + var data = e.getNodeData('differential-inline-comment'); + inline = this.getInlineByID(data.id); + if (inline) { + is_ref = true; + } else { + switch (action) { + case 'delete': + this._deleteInlineByID(data.id); + return; + } + } + } + + // TODO: For normal operations, highlight the inline range here. + + switch (action) { + case 'save': + inline.save(e.getTarget()); + break; + case 'cancel': + inline.cancel(); + break; + case 'undo': + inline.undo(); + break; + case 'edit': + inline.edit(); + break; + case 'done': + inline.toggleDone(); + break; + case 'delete': + inline.delete(is_ref); + break; + case 'reply': + inline.reply(); + break; + } } } diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index 3cc32a9357..ae56e28e59 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -18,7 +18,6 @@ JX.install('DiffInline', { _length: null, _displaySide: null, _isNewFile: null, - _undoRow: null, _replyToCommentPHID: null, _originalText: null, _snippet: null, @@ -38,6 +37,11 @@ JX.install('DiffInline', { _isSynthetic: false, _isHidden: false, + _editRow: null, + _undoRow: null, + _undoType: null, + _undoText: null, + bindToRow: function(row) { this._row = row; @@ -50,13 +54,10 @@ JX.install('DiffInline', { var comment = JX.DOM.find(row, 'div', 'differential-inline-comment'); var data = JX.Stratcom.getData(comment); - this._id = data.id; + this._readInlineState(data); this._phid = data.phid; - // TODO: This is very, very, very, very, very, very, very hacky. - var td = comment.parentNode; - var th = td.previousSibling; - if (th.parentNode.firstChild != th) { + if (data.on_right) { this._displaySide = 'right'; } else { this._displaySide = 'left'; @@ -65,9 +66,7 @@ JX.install('DiffInline', { this._number = parseInt(data.number, 10); this._length = parseInt(data.length, 10); this._originalText = data.original; - this._isNewFile = - (this.getDisplaySide() == 'right') || - (data.left != data.right); + this._isNewFile = data.isNewFile; this._replyToCommentPHID = data.replyToCommentPHID; @@ -81,7 +80,13 @@ JX.install('DiffInline', { this._isNew = false; this._snippet = data.snippet; - this.setInvisible(false); + this._isEditing = data.isEditing; + + if (this._isEditing) { + this.edit(); + } else { + this.setInvisible(false); + } return this; }, @@ -397,6 +402,12 @@ JX.install('DiffInline', { op = 'delete'; } + // If there's an existing "unedit" undo element, remove it. + if (this._undoRow) { + JX.DOM.remove(this._undoRow); + this._undoRow = null; + } + var data = this._newRequestData(op); this.setLoading(true); @@ -472,8 +483,9 @@ JX.install('DiffInline', { }, _oneditresponse: function(response) { - var rows = JX.$H(response).getNode(); + var rows = JX.$H(response.view).getNode(); + this._readInlineState(response.inline); this._drawEditRows(rows); this.setLoading(false); @@ -481,11 +493,16 @@ JX.install('DiffInline', { }, _oncreateresponse: function(response) { - var rows = JX.$H(response).getNode(); + var rows = JX.$H(response.view).getNode(); + this._readInlineState(response.inline); this._drawEditRows(rows); }, + _readInlineState: function(state) { + this._id = state.id; + }, + _ondeleteresponse: function() { this._drawUndeleteRows(); @@ -496,10 +513,16 @@ JX.install('DiffInline', { }, _drawUndeleteRows: function() { + this._undoType = 'undelete'; + this._undoText = null; + return this._drawUndoRows('undelete', this._row); }, _drawUneditRows: function(text) { + this._undoType = 'unedit'; + this._undoText = text; + return this._drawUndoRows('unedit', null, text); }, @@ -523,16 +546,17 @@ JX.install('DiffInline', { _drawEditRows: function(rows) { this.setEditing(true); - return this._drawRows(rows, null, 'edit'); + this._editRow = this._drawRows(rows, null, 'edit'); }, _drawRows: function(rows, cursor, type, text) { var first_row = JX.DOM.scry(rows, 'tr')[0]; - var first_meta; var row = first_row; var anchor = cursor || this._row; cursor = cursor || this._row.nextSibling; + + var result_row; var next_row; while (row) { // Grab this first, since it's going to change once we insert the row @@ -546,40 +570,8 @@ JX.install('DiffInline', { anchor.parentNode.insertBefore(row, cursor); cursor = row; - var row_meta = { - node: row, - type: type, - text: text || null, - listeners: [] - }; - - if (!first_meta) { - first_meta = row_meta; - } - - if (type == 'edit') { - row_meta.listeners.push( - JX.DOM.listen( - row, - ['submit', 'didSyntheticSubmit'], - 'inline-edit-form', - JX.bind(this, this._onsubmit, row_meta))); - - row_meta.listeners.push( - JX.DOM.listen( - row, - 'click', - 'inline-edit-cancel', - JX.bind(this, this._oncancel, row_meta))); - } else if (type == 'content') { - // No special listeners for these rows. - } else { - row_meta.listeners.push( - JX.DOM.listen( - row, - 'click', - 'differential-inline-comment-undo', - JX.bind(this, this._onundo, row_meta))); + if (!result_row) { + result_row = row; } // If the row has a textarea, focus it. This allows the user to start @@ -602,27 +594,25 @@ JX.install('DiffInline', { JX.Stratcom.invoke('resize'); - return first_meta; + return result_row; }, - _onsubmit: function(row, e) { - e.kill(); - - var handler = JX.bind(this, this._onsubmitresponse, row); + save: function(form) { + var handler = JX.bind(this, this._onsubmitresponse); this.setLoading(true); - JX.Workflow.newFromForm(e.getTarget()) + JX.Workflow.newFromForm(form) .setHandler(handler) .start(); }, - _onundo: function(row, e) { - e.kill(); + undo: function() { - this._removeRow(row); + JX.DOM.remove(this._undoRow); + this._undoRow = null; - if (row.type == 'undelete') { + if (this._undoType === 'undelete') { var uri = this._getInlineURI(); var data = this._newRequestData('undelete'); var handler = JX.bind(this, this._onundelete); @@ -635,13 +625,10 @@ JX.install('DiffInline', { .send(); } - if (row.type == 'unedit') { - if (this.getID()) { - this.edit(row.text); - } else { - this.create(row.text); - } + if (this._undoType === 'unedit') { + this.edit(this._undoText); } + }, _onundelete: function() { @@ -649,15 +636,16 @@ JX.install('DiffInline', { this._didUpdate(); }, - _oncancel: function(row, e) { - e.kill(); + cancel: function() { + var text = this._readText(this._editRow); + + JX.DOM.remove(this._editRow); + this._editRow = null; - var text = this._readText(row.node); if (text && text.length && (text != this._originalText)) { this._drawUneditRows(text); } - this._removeRow(row); this.setEditing(false); this.setInvisible(false); @@ -679,8 +667,11 @@ JX.install('DiffInline', { return textarea.value; }, - _onsubmitresponse: function(row, response) { - this._removeRow(row); + _onsubmitresponse: function(response) { + if (this._editRow) { + JX.DOM.remove(this._editRow); + this._editRow = null; + } this.setLoading(false); this.setInvisible(false); @@ -691,8 +682,8 @@ JX.install('DiffInline', { _onupdate: function(response) { var new_row; - if (response.markup) { - new_row = this._drawContentRows(JX.$H(response.markup).getNode()).node; + if (response.view) { + new_row = this._drawContentRows(JX.$H(response.view).getNode()); } // TODO: Save the old row so the action it's undo-able if it was a @@ -741,13 +732,6 @@ JX.install('DiffInline', { JX.DOM.alterClass(row, 'inline-hidden', is_collapsed); }, - _removeRow: function(row) { - JX.DOM.remove(row.node); - for (var ii = 0; ii < row.listeners.length; ii++) { - row.listeners[ii].remove(); - } - }, - _getInlineURI: function() { var changeset = this.getChangeset(); var list = changeset.getChangesetList();