archive/phabricator
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Add "Undo" for editing Differential inline comments

Browse Source 
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.

This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.

This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).

Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.

Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
This commit is contained in:
epriestley
2011-06-07 16:11:10 -07:00
parent 19d0d28089
commit 94d0adb140
13 changed files with 420 additions and 191 deletions
Download Patch File Download Diff File Expand all files Collapse all files

107
src/__celerity_resource_map__.php
View File

@@ -145,7 +145,7 @@ celerity_register_resource_map(array(
),
'differential-changeset-view-css' =>
array(
'uri' => '/res/4cb57f5f/rsrc/css/application/differential/changeset-view.css',
'uri' => '/res/313e96e9/rsrc/css/application/differential/changeset-view.css',
'type' => 'css',
'requires' =>
array(
@@ -161,6 +161,20 @@ celerity_register_resource_map(array(
),
'disk' => '/rsrc/css/application/differential/core.css',
),
'differential-inline-comment-editor' =>
array(
'uri' => '/res/0ee4fd79/rsrc/js/application/differential/DifferentialInlineCommentEditor.js',
'type' => 'js',
'requires' =>
array(
0 => 'javelin-dom',
1 => 'javelin-workflow',
2 => 'javelin-util',
3 => 'javelin-stratcom',
4 => 'javelin-install',
),
'disk' => '/rsrc/js/application/differential/DifferentialInlineCommentEditor.js',
),
'differential-revision-add-comment-css' =>
array(
'uri' => '/res/849748d3/rsrc/css/application/differential/add-comment.css',
@@ -197,16 +211,6 @@ celerity_register_resource_map(array(
),
'disk' => '/rsrc/css/application/differential/revision-detail.css',
),
0 =>
array(
'uri' => '/res/39de799e/rsrc/js/javelin/docs/Base.js',
'type' => 'js',
'requires' =>
array(
0 => 'javelin-install',
),
'disk' => '/rsrc/js/javelin/docs/Base.js',
),
'differential-revision-history-css' =>
array(
'uri' => '/res/0d7d515d/rsrc/css/application/differential/revision-history.css',
@@ -289,6 +293,16 @@ celerity_register_resource_map(array(
),
'disk' => '/rsrc/js/javelin/lib/behavior.js',
),
0 =>
array(
'uri' => '/res/39de799e/rsrc/js/javelin/docs/Base.js',
'type' => 'js',
'requires' =>
array(
0 => 'javelin-install',
),
'disk' => '/rsrc/js/javelin/docs/Base.js',
),
'javelin-behavior-aphront-basic-tokenizer' =>
array(
'uri' => '/res/bce3961b/rsrc/js/application/core/behavior-tokenizer.js',
@@ -383,15 +397,16 @@ celerity_register_resource_map(array(
),
'javelin-behavior-differential-edit-inline-comments' =>
array(
'uri' => '/res/682d1a9c/rsrc/js/application/differential/behavior-edit-inline-comments.js',
'uri' => '/res/e21bb634/rsrc/js/application/differential/behavior-edit-inline-comments.js',
'type' => 'js',
'requires' =>
array(
0 => 'javelin-behavior',
1 => 'javelin-stratcom',
2 => 'javelin-dom',
3 => 'javelin-workflow',
3 => 'javelin-util',
4 => 'javelin-vector',
5 => 'differential-inline-comment-editor',
),
'disk' => '/rsrc/js/application/differential/behavior-edit-inline-comments.js',
),
@@ -437,7 +452,7 @@ celerity_register_resource_map(array(
),
'javelin-behavior-differential-show-more' =>
array(
'uri' => '/res/9cbf1c9c/rsrc/js/application/differential/behavior-show-more.js',
'uri' => '/res/a766c717/rsrc/js/application/differential/behavior-show-more.js',
'type' => 'js',
'requires' =>
array(
@@ -1042,6 +1057,23 @@ celerity_register_resource_map(array(
'uri' => '/res/pkg/03ef179e/diffusion.pkg.css',
'type' => 'css',
),
'234a821b' =>
array (
'name' => 'differential.pkg.css',
'symbols' =>
array (
0 => 'differential-core-view-css',
1 => 'differential-changeset-view-css',
2 => 'differential-revision-detail-css',
3 => 'differential-revision-history-css',
4 => 'differential-table-of-contents-css',
5 => 'differential-revision-comment-css',
6 => 'differential-revision-add-comment-css',
7 => 'differential-revision-comment-list-css',
),
'uri' => '/res/pkg/234a821b/differential.pkg.css',
'type' => 'css',
),
'33f413ef' =>
array (
'name' => 'typeahead.pkg.js',
@@ -1082,23 +1114,6 @@ celerity_register_resource_map(array(
'uri' => '/res/pkg/64383b02/core.pkg.css',
'type' => 'css',
),
'b1200c80' =>
array (
'name' => 'differential.pkg.css',
'symbols' =>
array (
0 => 'differential-core-view-css',
1 => 'differential-changeset-view-css',
2 => 'differential-revision-detail-css',
3 => 'differential-revision-history-css',
4 => 'differential-table-of-contents-css',
5 => 'differential-revision-comment-css',
6 => 'differential-revision-add-comment-css',
7 => 'differential-revision-comment-list-css',
),
'uri' => '/res/pkg/b1200c80/differential.pkg.css',
'type' => 'css',
),
'db95a6d0' =>
array (
'name' => 'javelin.pkg.js',
@@ -1134,7 +1149,7 @@ celerity_register_resource_map(array(
'uri' => '/res/pkg/e26c5e06/workflow.pkg.js',
'type' => 'js',
),
'ed383f69' =>
'f292b274' =>
array (
'name' => 'differential.pkg.js',
'symbols' =>
@@ -1145,7 +1160,7 @@ celerity_register_resource_map(array(
3 => 'javelin-behavior-differential-show-more',
4 => 'javelin-behavior-differential-diff-radios',
),
'uri' => '/res/pkg/ed383f69/differential.pkg.js',
'uri' => '/res/pkg/f292b274/differential.pkg.js',
'type' => 'js',
),
),
@@ -1160,23 +1175,23 @@ celerity_register_resource_map(array(
'aphront-table-view-css' => '64383b02',
'aphront-tokenizer-control-css' => '64383b02',
'aphront-typeahead-control-css' => '64383b02',
'differential-changeset-view-css' => 'b1200c80',
'differential-core-view-css' => 'b1200c80',
'differential-revision-add-comment-css' => 'b1200c80',
'differential-revision-comment-css' => 'b1200c80',
'differential-revision-comment-list-css' => 'b1200c80',
'differential-revision-detail-css' => 'b1200c80',
'differential-revision-history-css' => 'b1200c80',
'differential-table-of-contents-css' => 'b1200c80',
'differential-changeset-view-css' => '234a821b',
'differential-core-view-css' => '234a821b',
'differential-revision-add-comment-css' => '234a821b',
'differential-revision-comment-css' => '234a821b',
'differential-revision-comment-list-css' => '234a821b',
'differential-revision-detail-css' => '234a821b',
'differential-revision-history-css' => '234a821b',
'differential-table-of-contents-css' => '234a821b',
'diffusion-commit-view-css' => '03ef179e',
'javelin-behavior' => 'db95a6d0',
'javelin-behavior-aphront-basic-tokenizer' => '33f413ef',
'javelin-behavior-aphront-form-disable-on-submit' => 'e26c5e06',
'javelin-behavior-differential-diff-radios' => 'ed383f69',
'javelin-behavior-differential-edit-inline-comments' => 'ed383f69',
'javelin-behavior-differential-feedback-preview' => 'ed383f69',
'javelin-behavior-differential-populate' => 'ed383f69',
'javelin-behavior-differential-show-more' => 'ed383f69',
'javelin-behavior-differential-diff-radios' => 'f292b274',
'javelin-behavior-differential-edit-inline-comments' => 'f292b274',
'javelin-behavior-differential-feedback-preview' => 'f292b274',
'javelin-behavior-differential-populate' => 'f292b274',
'javelin-behavior-differential-show-more' => 'f292b274',
'javelin-behavior-phabricator-keyboard-shortcuts' => 'e26c5e06',
'javelin-behavior-workflow' => 'e26c5e06',
'javelin-dom' => 'db95a6d0',

90
src/applications/differential/controller/inlinecommentedit/DifferentialInlineCommentEditController.php
View File

@@ -49,24 +49,9 @@ class DifferentialInlineCommentEditController extends DifferentialController {
$edit_dialog->addSubmitButton();
$edit_dialog->addCancelButton('#');
$inline = null;
if ($inline_id) {
$inline = id(new DifferentialInlineComment())
->load($inline_id);
if (!$inline ||
$inline->getAuthorPHID() != $user->getPHID() ||
$inline->getCommentID() ||
$inline->getRevisionID() != $this->revisionID) {
throw new Exception("That comment is not editable!");
}
}
switch ($op) {
case 'delete':
if (!$inline) {
return new Aphront400Response();
}
$inline = $this->loadInlineCommentForEditing($inline_id);
if ($request->isFormPost()) {
$inline->delete();
@@ -81,9 +66,7 @@ class DifferentialInlineCommentEditController extends DifferentialController {
return id(new AphrontDialogResponse())->setDialog($edit_dialog);
case 'edit':
if (!$inline) {
return new Aphront400Response();
}
$inline = $this->loadInlineCommentForEditing($inline_id);
if ($request->isFormPost()) {
if (strlen($text)) {
@@ -106,7 +89,7 @@ class DifferentialInlineCommentEditController extends DifferentialController {
$edit_dialog->appendChild(
$this->renderTextArea(
$inline->getContent()));
nonempty($text, $inline->getContent())));
return id(new AphrontDialogResponse())->setDialog($edit_dialog);
case 'create':
@@ -134,8 +117,21 @@ class DifferentialInlineCommentEditController extends DifferentialController {
->save();
return $this->buildRenderedCommentResponse($inline, $on_right);
case 'reply':
default:
$edit_dialog->setTitle('New Inline Comment');
if ($op == 'reply') {
$inline = $this->loadInlineComment($inline_id);
// Override defaults.
$changeset = $inline->getChangesetID();
$is_new = $inline->getIsNewFile();
$number = $inline->getLineNumber();
$length = $inline->getLineLength();
$edit_dialog->setTitle('Reply to Inline Comment');
} else {
$edit_dialog->setTitle('New Inline Comment');
}
$edit_dialog->addHiddenInput('op', 'create');
$edit_dialog->addHiddenInput('changeset', $changeset);
@@ -143,7 +139,7 @@ class DifferentialInlineCommentEditController extends DifferentialController {
$edit_dialog->addHiddenInput('number', $number);
$edit_dialog->addHiddenInput('length', $length);
$edit_dialog->appendChild($this->renderTextArea(''));
$edit_dialog->appendChild($this->renderTextArea($text));
return id(new AphrontDialogResponse())->setDialog($edit_dialog);
}
@@ -189,13 +185,61 @@ class DifferentialInlineCommentEditController extends DifferentialController {
}
private function renderTextArea($text) {
return phutil_render_tag(
return javelin_render_tag(
'textarea',
array(
'class' => 'differential-inline-comment-edit-textarea',
'sigil' => 'differential-inline-comment-edit-textarea',
'name' => 'text',
),
phutil_escape_html($text));
}
private function loadInlineComment($id) {
$inline = null;
if ($id) {
$inline = id(new DifferentialInlineComment())->load($id);
}
if (!$inline) {
throw new Exception("No such inline comment!");
}
return $inline;
}
private function loadInlineCommentForEditing($id) {
$inline = $this->loadInlineComment($id);
$user = $this->getRequest()->getUser();
if (!$this->canEditInlineComment($user, $inline, $this->revisionID)) {
throw new Exception("That comment is not editable!");
}
return $inline;
}
private function canEditInlineComment(
PhabricatorUser $user,
DifferentialInlineComment $inline,
$revision_id) {
// Only the author may edit a comment.
if ($inline->getAuthorPHID() != $user->getPHID()) {
return false;
}
// Saved comments may not be edited.
if ($inline->getCommentID()) {
return false;
}
// Inline must be attached to the active revision.
if ($inline->getRevisionID() != $revision_id) {
return false;
}
return true;
}
}

2
src/applications/differential/controller/inlinecommentedit/__init__.php
View File

@@ -6,7 +6,6 @@
phutil_require_module('phabricator', 'aphront/response/400');
phutil_require_module('phabricator', 'aphront/response/ajax');
phutil_require_module('phabricator', 'aphront/response/dialog');
phutil_require_module('phabricator', 'applications/differential/controller/base');
@@ -16,6 +15,7 @@ phutil_require_module('phabricator', 'applications/differential/storage/inlineco
phutil_require_module('phabricator', 'applications/differential/storage/revision');
phutil_require_module('phabricator', 'applications/differential/view/inlinecomment');
phutil_require_module('phabricator', 'applications/phid/handle/data');
phutil_require_module('phabricator', 'infrastructure/javelin/markup');
phutil_require_module('phabricator', 'view/dialog');
phutil_require_module('phutil', 'markup');

7
src/applications/differential/view/changesetdetailview/DifferentialChangesetDetailView.php
View File

@@ -83,13 +83,6 @@ class DifferentialChangesetDetailView extends AphrontView {
'<div style="clear: both;"></div>'.
$this->renderChildren());
if ($edit) {
Javelin::initBehavior(
'differential-edit-inline-comments', array(
'uri' => '/differential/comment/inline/edit/'.$this->revisionID.'/',
));
}
return $output;
}

1
src/applications/differential/view/changesetdetailview/__init__.php
View File

@@ -7,7 +7,6 @@
phutil_require_module('phabricator', 'infrastructure/celerity/api');
phutil_require_module('phabricator', 'infrastructure/javelin/api');
phutil_require_module('phabricator', 'infrastructure/javelin/markup');
phutil_require_module('phabricator', 'view/base');

35
src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php
View File

@@ -126,9 +126,13 @@ class DifferentialChangesetListView extends AphrontView {
Javelin::initBehavior('differential-comment-jump', array());
if ($this->editable) {
$undo_templates = $this->renderUndoTemplates();
$revision = $this->revision;
Javelin::initBehavior('differential-edit-inline-comments', array(
'uri' => '/differential/comment/inline/edit/'.$revision->getID().'/',
'undo_templates' => $undo_templates,
));
}
@@ -138,4 +142,35 @@ class DifferentialChangesetListView extends AphrontView {
'</div>';
}
/**
* Render the "Undo" markup for the inline comment undo feature.
*/
private function renderUndoTemplates() {
$link = javelin_render_tag(
'a',
array(
'href' => '#',
'sigil' => 'differential-inline-comment-undo',
),
'Undo');
$div = phutil_render_tag(
'div',
array(
'class' => 'differential-inline-undo',
),
'Changes discarded. '.$link);
$content = '<th></th><td>'.$div.'</td>';
$empty = '<th></th><td></td>';
$left = array($content, $empty);
$right = array($empty, $content);
return array(
'l' => '<table><tr>'.implode('', $left).'</tr></table>',
'r' => '<table><tr>'.implode('', $right).'</tr></table>',
);
}
}

1
src/applications/differential/view/changesetlistview/__init__.php
View File

@@ -9,6 +9,7 @@
phutil_require_module('phabricator', 'applications/differential/view/changesetdetailview');
phutil_require_module('phabricator', 'infrastructure/celerity/api');
phutil_require_module('phabricator', 'infrastructure/javelin/api');
phutil_require_module('phabricator', 'infrastructure/javelin/markup');
phutil_require_module('phabricator', 'view/base');
phutil_require_module('phutil', 'markup');

8
src/applications/differential/view/inlinecomment/DifferentialInlineCommentView.php
View File

@@ -73,6 +73,7 @@ final class DifferentialInlineCommentView extends AphrontView {
'number' => $inline->getLineNumber(),
'length' => $inline->getLineLength(),
'on_right' => $this->onRight,
'original' => $inline->getContent(),
);
$sigil = 'differential-inline-comment';
@@ -106,13 +107,6 @@ final class DifferentialInlineCommentView extends AphrontView {
'href' => '#',
'mustcapture' => true,
'sigil' => 'differential-inline-reply',
'meta' => array(
'is_new' => true,
'changeset' => $inline->getChangesetID(),
'number' => $inline->getLineNumber(),
'length' => $inline->getLineLength(),
'on_right' => $this->onRight,
)
),
'Reply');

2
src/infrastructure/lint/linter/javelin/PhabricatorJavelinLinter.php
View File

@@ -78,7 +78,7 @@ class PhabricatorJavelinLinter extends ArcanistLinter {
// TODO: Write build documentation for the Javelin binaries and point
// the user at it.
$this->raiseLintAtLine(
0,
1,
0,
self::LINT_MISSING_BINARY,
"The 'javelinsymbols' binary in the Javelin project has not been ".