Page MenuHomePhabricator
Files F15437610

D12032.id28963.diff
No OneTemporary
Actions

D12032.id28963.diff
View Options

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())

File Metadata

Mime Type
text/plain
Expires
Wed, Mar 26, 8:40 PM (1 w, 5 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7592169
Default Alt Text
D12032.id28963.diff (21 KB)

Event Timeline

Phabricator · Built by Phacility