diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ 'core.pkg.js' => '5a1c336d', 'darkconsole.pkg.js' => '8ab24e01', 'differential.pkg.css' => '1940be3f', - 'differential.pkg.js' => '2b14c4a1', + 'differential.pkg.js' => 'e62fe1cf', 'diffusion.pkg.css' => '591664fa', 'diffusion.pkg.js' => 'bfc0737b', 'maniphest.pkg.css' => '68d4dd3d', @@ -360,14 +360,14 @@ 'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '82439934', 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', - 'rsrc/js/application/differential/ChangesetViewManager.js' => 'a9af1212', - 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => 'd3aa4b40', + 'rsrc/js/application/differential/ChangesetViewManager.js' => '88be0133', + 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '1b772f31', 'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => 'e10f8e18', 'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d', - 'rsrc/js/application/differential/behavior-comment-preview.js' => '6932def3', + 'rsrc/js/application/differential/behavior-comment-preview.js' => '8e1389b5', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', 'rsrc/js/application/differential/behavior-dropdown-menus.js' => '2035b9cb', - 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '7378d48a', + 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'a48aa699', 'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492', 'rsrc/js/application/differential/behavior-populate.js' => '8694b1df', 'rsrc/js/application/differential/behavior-show-field-details.js' => 'bba9eedf', @@ -509,7 +509,7 @@ 'aphront-two-column-view-css' => '16ab3ad2', 'aphront-typeahead-control-css' => '0e403212', 'auth-css' => '1e655982', - 'changeset-view-manager' => 'a9af1212', + 'changeset-view-manager' => '88be0133', 'config-options-css' => '7fedf08b', 'config-welcome-css' => '6abd79be', 'conpherence-durable-column-view' => '9207426d', @@ -520,7 +520,7 @@ 'conpherence-widget-pane-css' => '3d575438', 'differential-changeset-view-css' => '6a8b172a', 'differential-core-view-css' => '7ac3cabc', - 'differential-inline-comment-editor' => 'd3aa4b40', + 'differential-inline-comment-editor' => '1b772f31', 'differential-results-table-css' => '181aa9d9', 'differential-revision-add-comment-css' => 'c478bcaa', 'differential-revision-comment-css' => '48186045', @@ -569,8 +569,8 @@ 'javelin-behavior-differential-comment-jump' => '4fdb476d', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-dropdown-menus' => '2035b9cb', - 'javelin-behavior-differential-edit-inline-comments' => '7378d48a', - 'javelin-behavior-differential-feedback-preview' => '6932def3', + 'javelin-behavior-differential-edit-inline-comments' => 'a48aa699', + 'javelin-behavior-differential-feedback-preview' => '8e1389b5', 'javelin-behavior-differential-keyboard-navigation' => '2c426492', 'javelin-behavior-differential-populate' => '8694b1df', 'javelin-behavior-differential-show-field-details' => 'bba9eedf', @@ -931,6 +931,14 @@ 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), + '1b772f31' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-request', + 'javelin-workflow', + ), '1d298e3a' => array( 'javelin-install', 'javelin-util', @@ -1227,14 +1235,6 @@ '6882e80a' => array( 'javelin-dom', ), - '6932def3' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-request', - 'javelin-util', - 'phabricator-shaped-request', - ), '69adf288' => array( 'javelin-install', ), @@ -1316,14 +1316,6 @@ 'javelin-behavior', 'javelin-dom', ), - '7378d48a' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'differential-inline-comment-editor', - ), '73d09eef' => array( 'javelin-behavior', 'javelin-vector', @@ -1500,6 +1492,16 @@ 'javelin-stratcom', 'javelin-dom', ), + '88be0133' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + ), '88f0c5b3' => array( 'javelin-behavior', 'javelin-dom', @@ -1528,6 +1530,14 @@ 'javelin-stratcom', 'javelin-behavior', ), + '8e1389b5' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-request', + 'javelin-util', + 'phabricator-shaped-request', + ), '8ef9ab58' => array( 'javelin-behavior', 'javelin-dom', @@ -1609,6 +1619,14 @@ 'javelin-vector', 'javelin-magical-init', ), + 'a48aa699' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'differential-inline-comment-editor', + ), 'a4ae61bf' => array( 'javelin-install', 'javelin-dom', @@ -1629,16 +1647,6 @@ 'javelin-uri', 'phabricator-keyboard-shortcut', ), - 'a9af1212' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - ), 'a9f88de2' => array( 'javelin-behavior', 'javelin-dom', @@ -1756,14 +1764,6 @@ 'd254d646' => array( 'javelin-util', ), - 'd3aa4b40' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-request', - 'javelin-workflow', - ), 'd4a14807' => array( 'javelin-install', 'javelin-dom', 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 @@ -290,6 +290,15 @@ return $this->proxy->getHasReplies(); } + public function setIsDeleted($is_deleted) { + $this->proxy->setIsDeleted($is_deleted); + return $this; + } + + public function getIsDeleted() { + return $this->proxy->getIsDeleted(); + } + /* -( PhabricatorMarkupInterface Implementation )-------------------------- */ 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 @@ -129,7 +129,7 @@ $where[] = qsprintf( $conn_r, 'changesetID IN (%Ld) AND - (authorPHID = %s OR transactionPHID IS NOT NULL)', + ((authorPHID = %s AND isDeleted = 0) OR transactionPHID IS NOT NULL)', $ids, $phid); } @@ -151,7 +151,8 @@ $where[] = qsprintf( $conn_r, - 'authorPHID = %s AND revisionPHID = %s AND transactionPHID IS NULL', + 'authorPHID = %s AND revisionPHID = %s AND transactionPHID IS NULL + AND isDeleted = 0', $phid, $rev_phid); } @@ -159,7 +160,7 @@ if ($this->draftsByAuthors) { $where[] = qsprintf( $conn_r, - 'authorPHID IN (%Ls) AND transactionPHID IS NULL', + 'authorPHID IN (%Ls) AND isDeleted = 0 AND transactionPHID IS NULL', $this->draftsByAuthors); } diff --git a/src/applications/differential/storage/DifferentialInlineComment.php b/src/applications/differential/storage/DifferentialInlineComment.php --- a/src/applications/differential/storage/DifferentialInlineComment.php +++ b/src/applications/differential/storage/DifferentialInlineComment.php @@ -207,6 +207,15 @@ return $this->proxy->getHasReplies(); } + public function setIsDeleted($is_deleted) { + $this->proxy->setIsDeleted($is_deleted); + return $this; + } + + public function getIsDeleted() { + return $this->proxy->getIsDeleted(); + } + /* -( PhabricatorMarkupInterface Implementation )-------------------------- */ 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 @@ -79,32 +79,40 @@ $this->readRequestParameters(); - switch ($this->getOperation()) { + $op = $this->getOperation(); + switch ($op) { case 'delete': - $inline = $this->loadCommentForEdit($this->getCommentID()); - - if ($request->isFormPost()) { - $this->deleteComment($inline); - return $this->buildEmptyResponse(); + case 'undelete': + case 'refdelete': + if (!$request->validateCSRF()) { + return new Aphront404Response(); } - $dialog = new AphrontDialogView(); - $dialog->setUser($user); - $dialog->setSubmitURI($request->getRequestURI()); + // NOTE: For normal deletes, we just process the delete immediately + // and show an "Undo" action. For deletes by reference from the + // preview ("refdelete"), we prompt first (because the "Undo" may + // not draw, or may not be easy to locate). + + if ($op == 'refdelete') { + if (!$request->isFormPost()) { + return $this->newDialog() + ->setTitle(pht('Really delete comment?')) + ->addHiddenInput('id', $this->getCommentID()) + ->addHiddenInput('op', $op) + ->appendParagraph(pht('Delete this inline comment?')) + ->addCancelButton('#') + ->addSubmitButton(pht('Delete')); + } + } - $dialog->setTitle(pht('Really delete this comment?')); - $dialog->addHiddenInput('id', $this->getCommentID()); - $dialog->addHiddenInput('op', 'delete'); - $dialog->appendChild( - phutil_tag('p', array(), pht('Delete this inline comment?'))); + $is_delete = ($op == 'delete' || $op == 'refdelete'); - $dialog->addCancelButton('#'); - $dialog->addSubmitButton(pht('Delete')); + $inline = $this->loadCommentForEdit($this->getCommentID()); + $inline->setIsDeleted((int)$is_delete)->save(); - return id(new AphrontDialogResponse())->setDialog($dialog); + return $this->buildEmptyResponse(); case 'edit': $inline = $this->loadCommentForEdit($this->getCommentID()); - $text = $this->getCommentText(); if ($request->isFormPost()) { diff --git a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php --- a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php +++ b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php @@ -25,6 +25,9 @@ public function setHasReplies($has_replies); public function getHasReplies(); + public function setIsDeleted($deleted); + public function getIsDeleted(); + public function setContent($content); public function getContent(); diff --git a/webroot/rsrc/js/application/differential/ChangesetViewManager.js b/webroot/rsrc/js/application/differential/ChangesetViewManager.js --- a/webroot/rsrc/js/application/differential/ChangesetViewManager.js +++ b/webroot/rsrc/js/application/differential/ChangesetViewManager.js @@ -359,6 +359,8 @@ if (response.undoTemplates) { this._undoTemplates = response.undoTemplates; } + + JX.Stratcom.invoke('differential-inline-comment-refresh'); }, _getContentFrame: function() { diff --git a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js --- a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js +++ b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js @@ -19,6 +19,7 @@ members : { _uri : null, _undoText : null, + _completed: false, _skipOverInlineCommentRows : function(node) { // TODO: Move this semantic information out of class names. while (node && node.className.indexOf('inline') !== -1) { @@ -73,11 +74,17 @@ JX.DOM.remove(rows[ii]); } } + JX.DifferentialInlineCommentEditor._undoRows = []; }, _undo : function() { this._removeUndoLink(); - this.setText(this._undoText); + if (this._undoText) { + this.setText(this._undoText); + } else { + this.setOperation('undelete'); + } + this.start(); }, _registerUndoListener : function() { @@ -146,9 +153,18 @@ 'inline-edit-form', onsubmit); }, + + _didCompleteWorkflow : function(response) { var op = this.getOperation(); + if (op == 'delete' || op == 'refdelete') { + this._undoText = null; + this._drawUndo(); + } else { + this._removeUndoLink(); + } + // We don't get any markup back if the user deletes a comment, or saves // an empty comment (which effects a delete). if (response.markup) { @@ -156,31 +172,34 @@ } // These operations remove the old row (edit adds a new row first). - var remove_old = (op == 'edit' || op == 'delete'); + var remove_old = (op == 'edit' || op == 'delete' || op == 'refdelete'); if (remove_old) { - JX.DOM.remove(this.getRow()); - var other_rows = this.getOtherRows(); - for(var i = 0; i < other_rows.length; ++i) { - JX.DOM.remove(other_rows[i]); - } + this._setRowState('hidden'); } - // Once the user saves something, get rid of the 'undo' option. A - // particular case where we need this is saving a delete, when we might - // otherwise leave around an 'undo' for an earlier edit to the same - // comment. - this._removeUndoLink(); + if (op == 'undelete') { + this._setRowState('visible'); + } + + this._completed = true; JX.Stratcom.invoke('differential-inline-comment-update'); this.invoke('done'); }, + + _didCancelWorkflow : function() { this.invoke('done'); - var op = this.getOperation(); - if (op == 'delete') { - // No undo for delete, we prompt the user explicitly. - return; + switch (this.getOperation()) { + case 'delete': + case 'refdelete': + if (!this._completed) { + this._setRowState('visible'); + } + return; + case 'undelete': + return; } var textarea; @@ -209,6 +228,10 @@ // Save the text so we can 'undo' back to it. this._undoText = text; + this._drawUndo(); + }, + + _drawUndo: function() { var templates = this.getTemplates(); var template = this.getOnRight() ? templates.r : templates.l; template = JX.$H(template).getNode(); @@ -231,21 +254,18 @@ this._registerUndoListener(); var data = this._buildRequestData(); - var op = this.getOperation(); - if (op == 'delete') { + if (op == 'delete' || op == 'refdelete' || op == 'undelete') { this._setRowState('loading'); + var oncomplete = JX.bind(this, this._didCompleteWorkflow); - var onclose = JX.bind(this, function() { - this._setRowState('visible'); - this._didCancelWorkflow(); - }); + var oncancel = JX.bind(this, this._didCancelWorkflow); new JX.Workflow(this._uri, data) .setHandler(oncomplete) - .setCloseHandler(onclose) + .setCloseHandler(oncancel) .start(); } else { var handler = JX.bind(this, this._didContinueWorkflow); @@ -260,7 +280,21 @@ } return this; + }, + + deleteByID: function(id) { + var data = { + op: 'refdelete', + changesetID: id + }; + + new JX.Workflow(this._uri, data) + .setHandler(function() { + JX.Stratcom.invoke('differential-inline-comment-update'); + }) + .start(); } + }, statics : { @@ -280,7 +314,6 @@ properties : { operation : null, row : null, - otherRows: [], table : null, onRight : null, ID : null, diff --git a/webroot/rsrc/js/application/differential/behavior-comment-preview.js b/webroot/rsrc/js/application/differential/behavior-comment-preview.js --- a/webroot/rsrc/js/application/differential/behavior-comment-preview.js +++ b/webroot/rsrc/js/application/differential/behavior-comment-preview.js @@ -49,42 +49,51 @@ request.start(); - function refreshInlinePreview() { new JX.Request(config.inlineuri, function(r) { - var inline = JX.$(config.inline); - - JX.DOM.setContent(inline, JX.$H(r)); - JX.Stratcom.invoke('differential-preview-update', null, { - container: inline - }); - - // Go through the previews and activate any "View" links where the - // actual comment appears in the document. - - var links = JX.DOM.scry( - inline, - 'a', - 'differential-inline-preview-jump'); - for (var ii = 0; ii < links.length; ii++) { - var data = JX.Stratcom.getData(links[ii]); - try { - JX.$(data.anchor); - links[ii].href = '#' + data.anchor; - JX.DOM.setContent(links[ii], 'View'); - } catch (ignored) { - // This inline comment isn't visible, e.g. on some other diff. - } - } - }) - .setTimeout(5000) - .send(); + var inline = JX.$(config.inline); + + JX.DOM.setContent(inline, JX.$H(r)); + JX.Stratcom.invoke('differential-preview-update', null, { + container: inline + }); + + updateLinks(); + }) + .setTimeout(5000) + .send(); + } + + function updateLinks() { + var inline = JX.$(config.inline); + + var links = JX.DOM.scry( + inline, + 'a', + 'differential-inline-preview-jump'); + + for (var ii = 0; ii < links.length; ii++) { + var data = JX.Stratcom.getData(links[ii]); + try { + JX.$(data.anchor); + links[ii].href = '#' + data.anchor; + JX.DOM.setContent(links[ii], 'View'); + } catch (ignored) { + // This inline comment isn't visible, e.g. on some other diff. + } + } } + JX.Stratcom.listen( 'differential-inline-comment-update', null, refreshInlinePreview); + JX.Stratcom.listen( + 'differential-inline-comment-refresh', + null, + updateLinks); + refreshInlinePreview(); }); diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js @@ -298,25 +298,40 @@ var handle_inline_action = function(node, op) { var data = JX.Stratcom.getData(node); - var row = node.parentNode.parentNode; - var other_rows = []; + + // If you click an action in the preview at the bottom of the page, we + // find the corresponding node and simulate clicking that, if it's + // present on the page. This gives the editor a more consistent view + // of the document. if (JX.Stratcom.hasSigil(node, 'differential-inline-comment-preview')) { - // The DOM structure around the comment is different if it's part of the - // preview, so make sure not to pass the wrong container. - row = node; - if (op === 'delete') { - // Furthermore, deleting a comment in the preview does not automatically - // delete other occurrences of the same comment, so do that manually. - var nodes = JX.DOM.scry( - JX.DOM.getContentFrame(), - 'div', - 'differential-inline-comment'); - for (var i = 0; i < nodes.length; ++i) { - if (JX.Stratcom.getData(nodes[i]).id === data.id) { - other_rows.push(nodes[i]); - } + var nodes = JX.DOM.scry( + JX.DOM.getContentFrame(), + 'div', + 'differential-inline-comment'); + + var found = false; + var node_data; + for (var ii = 0; ii < nodes.length; ++ii) { + if (nodes[ii] == node) { + // Don't match the preview itself. + continue; + } + node_data = JX.Stratcom.getData(nodes[ii]); + if (node_data.id == data.id) { + node = nodes[ii]; + data = node_data; + found = true; + break; } } + + if (!found) { + new JX.DifferentialInlineCommentEditor(config.uri) + .deleteByID(data.id); + return; + } + + op = 'refdelete'; } var original = data.original; @@ -328,6 +343,7 @@ reply_phid = data.phid; } + var row = JX.DOM.findAbove(node, 'tr'); var changeset_root = JX.DOM.findAbove( node, 'div', @@ -344,7 +360,6 @@ .setOnRight(data.on_right) .setOriginalText(original) .setRow(row) - .setOtherRows(other_rows) .setTable(row.parentNode) .setReplyToCommentPHID(reply_phid) .setRenderer(view.getRenderer())