revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-23 15:22:41 +01:00
Code Issues Releases Wiki Activity

Make "Reply" on ghost inlines work correctly (or, at least, consistently)

Browse source 
Summary:
Ref T7447. Ref T7870. When you "reply" to a ghost inline, make it work properly.

This exact behavior is arguable. In particular, when you reply to a ghost inline, we //could// put the reply on the same diff as the original.

I suspect it aligns better with user exepectation to put the new inline on the current (visible) diff instead, and generally for inlines to flow forward through time and all of the ghosts to pretty much be older than all of the non-ghosts in most cases. We can see how it feels and adjust things if this turns out to not make sense.

Test Plan:
  - Replied to ghost inlines, got new inlines on the proper display line.
  - Replied to normal inlines, got normal behavior.
  - Made some new inlines.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7870, T7447

Differential Revision: https://secure.phabricator.com/D12492
This commit is contained in:
epriestley 2015-04-21 07:28:02 -07:00
parent 84f62bdf9a
commit c596c358db
1 changed files with 15 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

29
src/infrastructure/diff/PhabricatorInlineCommentController.php
View file

@ -206,20 +206,20 @@ abstract class PhabricatorInlineCommentController
$edit_dialog = $this->buildEditDialog(); $edit_dialog = $this->buildEditDialog();
if ($this->getOperation() == 'reply') { if ($this->getOperation() == 'reply') {
$inline = $this->loadComment($this->getCommentID());
$edit_dialog->setTitle(pht('Reply to Inline Comment')); $edit_dialog->setTitle(pht('Reply to Inline Comment'));
$changeset = $inline->getChangesetID();
$is_new = $inline->getIsNewFile();
$number = $inline->getLineNumber();
$length = $inline->getLineLength();
} else { } else {
$edit_dialog->setTitle(pht('New Inline Comment')); $edit_dialog->setTitle(pht('New Inline Comment'));
$changeset = $this->getChangesetID(); }
// NOTE: We read the values from the client (the display values), not
// the values from the database (the original values) when replying.
// In particular, when replying to a ghost comment which was moved
// across diffs and then moved backward to the most recent visible
// line, we want to reply on the display line (which exists), not on
// the comment's original line (which may not exist in this changeset).
$is_new = $this->getIsNewFile(); $is_new = $this->getIsNewFile();
$number = $this->getLineNumber(); $number = $this->getLineNumber();
$length = $this->getLineLength(); $length = $this->getLineLength();
}
$edit_dialog->addHiddenInput('op', 'create'); $edit_dialog->addHiddenInput('op', 'create');
$edit_dialog->addHiddenInput('is_new', $is_new); $edit_dialog->addHiddenInput('is_new', $is_new);
@ -261,14 +261,11 @@ abstract class PhabricatorInlineCommentController
pht('Failed to load comment "%s".', $reply_phid)); pht('Failed to load comment "%s".', $reply_phid));
} }
if ($reply_comment->getChangesetID() != $this->getChangesetID()) { // NOTE: It's fine to reply to a comment from a different changeset, so
throw new Exception( // the reply comment may not appear on the same changeset that the new
pht( // comment appears on. This is expected in the case of ghost comments.
'Comment "%s" belongs to wrong changeset (%s vs %s).', // We currently put the new comment on the visible changeset, not the
$reply_phid, // original comment's changeset.
$reply_comment->getChangesetID(),
$this->getChangesetID()));
}
} }
} }