2011-02-01 16:42:36 -08:00
|
|
|
<?php
|
|
|
|
|
|
2012-03-09 15:46:25 -08:00
|
|
|
final class DifferentialInlineCommentEditController
|
Add inline comments to Diffusion/Audit
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
2012-03-14 12:56:01 -07:00
|
|
|
extends PhabricatorInlineCommentController {
|
2011-02-01 16:42:36 -08:00
|
|
|
|
2015-03-27 17:08:31 -07:00
|
|
|
private function getRevisionID() {
|
|
|
|
|
return $this->getRequest()->getURIData('id');
|
2011-02-01 16:42:36 -08:00
|
|
|
}
|
|
|
|
|
|
2015-03-27 17:08:31 -07:00
|
|
|
private function loadRevision() {
|
|
|
|
|
$viewer = $this->getViewer();
|
|
|
|
|
$revision_id = $this->getRevisionID();
|
2011-02-01 16:42:36 -08:00
|
|
|
|
2013-09-26 12:37:19 -07:00
|
|
|
$revision = id(new DifferentialRevisionQuery())
|
|
|
|
|
->setViewer($viewer)
|
|
|
|
|
->withIDs(array($revision_id))
|
|
|
|
|
->executeOne();
|
2015-03-27 17:08:31 -07:00
|
|
|
if (!$revision) {
|
|
|
|
|
throw new Exception(pht('Invalid revision ID "%s".', $revision_id));
|
2011-02-01 16:42:36 -08:00
|
|
|
}
|
2011-02-02 10:10:25 -08:00
|
|
|
|
2015-03-27 17:08:31 -07:00
|
|
|
return $revision;
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
protected function createComment() {
|
|
|
|
|
// Verify revision and changeset correspond to actual objects.
|
|
|
|
|
$changeset_id = $this->getChangesetID();
|
|
|
|
|
|
|
|
|
|
$revision = $this->loadRevision();
|
|
|
|
|
|
Add inline comments to Diffusion/Audit
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
2012-03-14 12:56:01 -07:00
|
|
|
if (!id(new DifferentialChangeset())->load($changeset_id)) {
|
2015-05-22 17:27:56 +10:00
|
|
|
throw new Exception(pht('Invalid changeset ID!'));
|
Add "Undo" for editing Differential inline comments
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
2011-06-07 16:11:10 -07:00
|
|
|
}
|
|
|
|
|
|
Add inline comments to Diffusion/Audit
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
2012-03-14 12:56:01 -07:00
|
|
|
return id(new DifferentialInlineComment())
|
Migrate all Differential inline comments to ApplicationTransactions
Summary:
Ref T2222. This implements step (1) described there, which is moving over all the inline comments.
The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later.
Two risks here:
- I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after.
- This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this.
I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive.
Test Plan:
- Before migrating, then after migrating:
- Made a bunch of inlines (drafts, submitted).
- Edited and deleted inlines.
- Verified inlines showed up in preview.
- Verified that inlines aren't indexed when they're drafts (`bin/search index D935`).
- Verified that inlines ARE indexed when they're not drafts.
- Verified that drafts inlines make revisions appear as "with draft" in the revision list.
- Made left, right, and draft inlines.
- Migrated (`bin/storage upgrade`).
- Verified that my inlines from before the migration still showed up.
- (Repeated all the stuff above.)
- Manually inspected the inline comment table.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D7139
2013-10-19 05:03:25 -07:00
|
|
|
->setRevision($revision)
|
Add inline comments to Diffusion/Audit
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
2012-03-14 12:56:01 -07:00
|
|
|
->setChangesetID($changeset_id);
|
|
|
|
|
}
|
Add "Undo" for editing Differential inline comments
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
2011-06-07 16:11:10 -07:00
|
|
|
|
Add inline comments to Diffusion/Audit
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
2012-03-14 12:56:01 -07:00
|
|
|
protected function loadComment($id) {
|
2013-06-21 12:54:56 -07:00
|
|
|
return id(new DifferentialInlineCommentQuery())
|
2015-04-20 14:33:58 -07:00
|
|
|
->setViewer($this->getViewer())
|
2013-06-21 12:54:56 -07:00
|
|
|
->withIDs(array($id))
|
2015-04-20 14:33:58 -07:00
|
|
|
->withDeletedDrafts(true)
|
Allow inline comments to be individually hidden
Summary:
Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely.
This is sticky per-user.
My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all).
Specifically, this adds a new action here:
{F435621}
Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you:
{F435626}
You can click the icon to reveal all hidden comments below the line.
Test Plan:
- Hid comments.
- Showed comments.
- Created, edited, deleted and submitted comments.
- Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential).
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: jparise, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D13009
2015-05-27 10:28:38 -07:00
|
|
|
->needHidden(true)
|
2013-06-21 12:54:56 -07:00
|
|
|
->executeOne();
|
Add "Undo" for editing Differential inline comments
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
2011-06-07 16:11:10 -07:00
|
|
|
}
|
|
|
|
|
|
2015-03-08 13:04:38 -07:00
|
|
|
protected function loadCommentByPHID($phid) {
|
|
|
|
|
return id(new DifferentialInlineCommentQuery())
|
2015-04-20 14:33:58 -07:00
|
|
|
->setViewer($this->getViewer())
|
2015-03-08 13:04:38 -07:00
|
|
|
->withPHIDs(array($phid))
|
2015-04-20 14:33:58 -07:00
|
|
|
->withDeletedDrafts(true)
|
Allow inline comments to be individually hidden
Summary:
Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely.
This is sticky per-user.
My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all).
Specifically, this adds a new action here:
{F435621}
Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you:
{F435626}
You can click the icon to reveal all hidden comments below the line.
Test Plan:
- Hid comments.
- Showed comments.
- Created, edited, deleted and submitted comments.
- Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential).
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: jparise, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D13009
2015-05-27 10:28:38 -07:00
|
|
|
->needHidden(true)
|
2015-03-08 13:04:38 -07:00
|
|
|
->executeOne();
|
|
|
|
|
}
|
|
|
|
|
|
Add inline comments to Diffusion/Audit
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
2012-03-14 12:56:01 -07:00
|
|
|
protected function loadCommentForEdit($id) {
|
|
|
|
|
$request = $this->getRequest();
|
|
|
|
|
$user = $request->getUser();
|
Add "Undo" for editing Differential inline comments
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
2011-06-07 16:11:10 -07:00
|
|
|
|
Add inline comments to Diffusion/Audit
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
2012-03-14 12:56:01 -07:00
|
|
|
$inline = $this->loadComment($id);
|
|
|
|
|
if (!$this->canEditInlineComment($user, $inline)) {
|
2015-05-22 17:27:56 +10:00
|
|
|
throw new Exception(pht('That comment is not editable!'));
|
Add "Undo" for editing Differential inline comments
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
2011-06-07 16:11:10 -07:00
|
|
|
}
|
|
|
|
|
return $inline;
|
|
|
|
|
}
|
|
|
|
|
|
Track a "Done" state on inline comments
Summary:
Ref T1460. This just barely works, but throwing it up in case any of it sounds mechanically crazy before we build integrations/UI/etc.
Specifically, these are the behaviors:
- You can mark your own draft comments as "done" before you submit them. The intent is to let reviewers mark their stuff advisory/minor/not-important before they submit it, to hint to authors that they don't expect the feedback to necessarily be addressed (maybe it's a joke, maybe it's just discussion, maybe it's "consider..").
- You can mark others' published comments as "done" if you're the revision/commit author. The intent is to keep this lightweight by not requiring an audit trail of who marked what done when. If anyone could mark anything done, we'd have to have some way to show who marked stuff.
- When you mark stuff done (or unmark it), it goes into a "draft" state, where you see the change but others don't see it yet. The intent is twofold:
- Be consistent with how inlines work.
- Allow us to publish a "epriestley updated this revision + epriestley marked 15 inlines as done" story later if we want. This seems more useful than publishing 15 "epriestley marked one thing as done" stories.
- The actual bit where done-ness publishes isn't implemented.
- UI is bare bones.
- No integration with the rest of the UI yet.
Test Plan: Clicked some checkboxes.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: paulshen, chasemp, epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12033
2015-03-09 18:41:47 -07:00
|
|
|
protected function loadCommentForDone($id) {
|
|
|
|
|
$request = $this->getRequest();
|
|
|
|
|
$viewer = $request->getUser();
|
|
|
|
|
|
|
|
|
|
$inline = $this->loadComment($id);
|
|
|
|
|
if (!$inline) {
|
|
|
|
|
throw new Exception(pht('Unable to load inline "%d".', $id));
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
$changeset = id(new DifferentialChangesetQuery())
|
|
|
|
|
->setViewer($viewer)
|
|
|
|
|
->withIDs(array($inline->getChangesetID()))
|
|
|
|
|
->executeOne();
|
|
|
|
|
if (!$changeset) {
|
|
|
|
|
throw new Exception(pht('Unable to load changeset.'));
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
$diff = id(new DifferentialDiffQuery())
|
|
|
|
|
->setViewer($viewer)
|
|
|
|
|
->withIDs(array($changeset->getDiffID()))
|
|
|
|
|
->executeOne();
|
|
|
|
|
if (!$diff) {
|
|
|
|
|
throw new Exception(pht('Unable to load diff.'));
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
$revision = id(new DifferentialRevisionQuery())
|
|
|
|
|
->setViewer($viewer)
|
|
|
|
|
->withIDs(array($diff->getRevisionID()))
|
|
|
|
|
->executeOne();
|
|
|
|
|
if (!$revision) {
|
|
|
|
|
throw new Exception(pht('Unable to load revision.'));
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
if ($revision->getAuthorPHID() !== $viewer->getPHID()) {
|
|
|
|
|
throw new Exception(pht('You are not the revision owner.'));
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
return $inline;
|
|
|
|
|
}
|
|
|
|
|
|
Add "Undo" for editing Differential inline comments
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
2011-06-07 16:11:10 -07:00
|
|
|
private function canEditInlineComment(
|
|
|
|
|
PhabricatorUser $user,
|
Add inline comments to Diffusion/Audit
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
2012-03-14 12:56:01 -07:00
|
|
|
DifferentialInlineComment $inline) {
|
Add "Undo" for editing Differential inline comments
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
2011-06-07 16:11:10 -07:00
|
|
|
|
|
|
|
|
// Only the author may edit a comment.
|
|
|
|
|
if ($inline->getAuthorPHID() != $user->getPHID()) {
|
|
|
|
|
return false;
|
|
|
|
|
}
|
|
|
|
|
|
2014-02-14 15:56:25 -08:00
|
|
|
// Saved comments may not be edited, for now, although the schema now
|
|
|
|
|
// supports it.
|
|
|
|
|
if (!$inline->isDraft()) {
|
Add "Undo" for editing Differential inline comments
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
2011-06-07 16:11:10 -07:00
|
|
|
return false;
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// Inline must be attached to the active revision.
|
2015-03-27 17:08:31 -07:00
|
|
|
if ($inline->getRevisionID() != $this->getRevisionID()) {
|
Add "Undo" for editing Differential inline comments
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
2011-06-07 16:11:10 -07:00
|
|
|
return false;
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
return true;
|
|
|
|
|
}
|
|
|
|
|
|
2014-02-18 16:25:16 -08:00
|
|
|
protected function deleteComment(PhabricatorInlineCommentInterface $inline) {
|
|
|
|
|
$inline->openTransaction();
|
|
|
|
|
DifferentialDraft::deleteHasDraft(
|
|
|
|
|
$inline->getAuthorPHID(),
|
|
|
|
|
$inline->getRevisionPHID(),
|
|
|
|
|
$inline->getPHID());
|
|
|
|
|
$inline->delete();
|
|
|
|
|
$inline->saveTransaction();
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
protected function saveComment(PhabricatorInlineCommentInterface $inline) {
|
|
|
|
|
$inline->openTransaction();
|
|
|
|
|
$inline->save();
|
|
|
|
|
DifferentialDraft::markHasDraft(
|
|
|
|
|
$inline->getAuthorPHID(),
|
|
|
|
|
$inline->getRevisionPHID(),
|
|
|
|
|
$inline->getPHID());
|
|
|
|
|
$inline->saveTransaction();
|
|
|
|
|
}
|
2015-03-27 17:08:31 -07:00
|
|
|
|
|
|
|
|
protected function loadObjectOwnerPHID(
|
|
|
|
|
PhabricatorInlineCommentInterface $inline) {
|
|
|
|
|
return $this->loadRevision()->getAuthorPHID();
|
|
|
|
|
}
|
|
|
|
|
|
Allow inline comments to be individually hidden
Summary:
Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely.
This is sticky per-user.
My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all).
Specifically, this adds a new action here:
{F435621}
Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you:
{F435626}
You can click the icon to reveal all hidden comments below the line.
Test Plan:
- Hid comments.
- Showed comments.
- Created, edited, deleted and submitted comments.
- Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential).
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: jparise, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D13009
2015-05-27 10:28:38 -07:00
|
|
|
protected function hideComments(array $ids) {
|
|
|
|
|
$viewer = $this->getViewer();
|
|
|
|
|
$table = new DifferentialHiddenComment();
|
|
|
|
|
$conn_w = $table->establishConnection('w');
|
|
|
|
|
|
|
|
|
|
$sql = array();
|
|
|
|
|
foreach ($ids as $id) {
|
|
|
|
|
$sql[] = qsprintf(
|
|
|
|
|
$conn_w,
|
|
|
|
|
'(%s, %d)',
|
|
|
|
|
$viewer->getPHID(),
|
|
|
|
|
$id);
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
queryfx(
|
|
|
|
|
$conn_w,
|
|
|
|
|
'INSERT IGNORE INTO %T (userPHID, commentID) VALUES %Q',
|
|
|
|
|
$table->getTableName(),
|
|
|
|
|
implode(', ', $sql));
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
protected function showComments(array $ids) {
|
|
|
|
|
$viewer = $this->getViewer();
|
|
|
|
|
$table = new DifferentialHiddenComment();
|
|
|
|
|
$conn_w = $table->establishConnection('w');
|
|
|
|
|
|
|
|
|
|
queryfx(
|
|
|
|
|
$conn_w,
|
|
|
|
|
'DELETE FROM %T WHERE userPHID = %s AND commentID IN (%Ld)',
|
|
|
|
|
$table->getTableName(),
|
|
|
|
|
$viewer->getPHID(),
|
|
|
|
|
$ids);
|
|
|
|
|
}
|
|
|
|
|
|
2011-02-01 16:42:36 -08:00
|
|
|
}
|