From dd501117e80cde15b8bc208f6a399a885c9c2c16 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Mar 2015 17:27:51 -0700 Subject: [PATCH] When deleting inline comments, offer "undo" instead of prompting Summary: Ref T2009. Ref T1460. Fixes T2618. When users hit "Delete" on inline comments, delete immediately and offer them "Undo". If they delete indirectly (e.g., by clicking "Delete" from the preview at the bottom of the page), we still prompt them, because the "Undo" action either won't be available or may not be easy to find. This is a "refdelete". Fixes T6464. This was just a mess. Make it not as much of a mess. It should work now. Pretty sure. Fixes T4999. We did not refresh these links often enough to find targets for them, so they could race with content. Reevaluate them after loading new changes. Test Plan: - Deleted and undid deletion of inlines from main view and preview. - Clicked "View" on inlines. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T6464, T4999, T2618, T1460, T2009 Differential Revision: https://secure.phabricator.com/D12032 --- resources/celerity/map.php | 86 +++++++++---------- .../storage/PhabricatorAuditInlineComment.php | 9 ++ .../query/DifferentialInlineCommentQuery.php | 7 +- .../storage/DifferentialInlineComment.php | 9 ++ .../PhabricatorInlineCommentController.php | 44 ++++++---- .../PhabricatorInlineCommentInterface.php | 3 + .../differential/ChangesetViewManager.js | 2 + .../DifferentialInlineCommentEditor.js | 81 +++++++++++------ .../differential/behavior-comment-preview.js | 61 +++++++------ .../behavior-edit-inline-comments.js | 49 +++++++---- 10 files changed, 220 insertions(+), 131 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 9681e5c69b..ed08feeb47 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 @@ return array( '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 index 37f18c96e7..5483c58494 100644 --- a/src/applications/audit/storage/PhabricatorAuditInlineComment.php +++ b/src/applications/audit/storage/PhabricatorAuditInlineComment.php @@ -290,6 +290,15 @@ final class PhabricatorAuditInlineComment 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 index c60316336e..f605a78b02 100644 --- a/src/applications/differential/query/DifferentialInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php @@ -129,7 +129,7 @@ final class DifferentialInlineCommentQuery $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 @@ final class DifferentialInlineCommentQuery $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 @@ final class DifferentialInlineCommentQuery 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 index 8701363708..711e50020f 100644 --- a/src/applications/differential/storage/DifferentialInlineComment.php +++ b/src/applications/differential/storage/DifferentialInlineComment.php @@ -207,6 +207,15 @@ final class DifferentialInlineComment 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 index a23efbc9a4..d6f6db44b6 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -79,32 +79,40 @@ abstract class PhabricatorInlineCommentController $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). - $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?'))); + 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->addCancelButton('#'); - $dialog->addSubmitButton(pht('Delete')); + $is_delete = ($op == 'delete' || $op == 'refdelete'); - return id(new AphrontDialogResponse())->setDialog($dialog); + $inline = $this->loadCommentForEdit($this->getCommentID()); + $inline->setIsDeleted((int)$is_delete)->save(); + + 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 index 01914e03db..ba47295cf6 100644 --- a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php +++ b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php @@ -25,6 +25,9 @@ interface PhabricatorInlineCommentInterface extends PhabricatorMarkupInterface { 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 index df50e7e9c7..02a1a41b02 100644 --- a/webroot/rsrc/js/application/differential/ChangesetViewManager.js +++ b/webroot/rsrc/js/application/differential/ChangesetViewManager.js @@ -359,6 +359,8 @@ JX.install('ChangesetViewManager', { 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 index 98b2745633..6055c8ff52 100644 --- a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js +++ b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js @@ -19,6 +19,7 @@ JX.install('DifferentialInlineCommentEditor', { 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.install('DifferentialInlineCommentEditor', { 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 @@ JX.install('DifferentialInlineCommentEditor', { '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 @@ JX.install('DifferentialInlineCommentEditor', { } // 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 @@ JX.install('DifferentialInlineCommentEditor', { // 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 @@ JX.install('DifferentialInlineCommentEditor', { 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 @@ JX.install('DifferentialInlineCommentEditor', { } 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 @@ JX.install('DifferentialInlineCommentEditor', { 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 index 2fae03fd8c..3cf50535fd 100644 --- a/webroot/rsrc/js/application/differential/behavior-comment-preview.js +++ b/webroot/rsrc/js/application/differential/behavior-comment-preview.js @@ -49,42 +49,51 @@ JX.behavior('differential-feedback-preview', function(config) { request.start(); - function refreshInlinePreview() { new JX.Request(config.inlineuri, function(r) { - var inline = JX.$(config.inline); + var inline = JX.$(config.inline); - JX.DOM.setContent(inline, JX.$H(r)); - JX.Stratcom.invoke('differential-preview-update', null, { - container: 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(); + 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 index 7d10be8592..0c27a7472a 100644 --- 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 @@ JX.behavior('differential-edit-inline-comments', function(config) { 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 @@ JX.behavior('differential-edit-inline-comments', function(config) { reply_phid = data.phid; } + var row = JX.DOM.findAbove(node, 'tr'); var changeset_root = JX.DOM.findAbove( node, 'div', @@ -344,7 +360,6 @@ JX.behavior('differential-edit-inline-comments', function(config) { .setOnRight(data.on_right) .setOriginalText(original) .setRow(row) - .setOtherRows(other_rows) .setTable(row.parentNode) .setReplyToCommentPHID(reply_phid) .setRenderer(view.getRenderer())