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

Make Differential inline comment rendering more consistent and somewhat more modern

Browse source 
Summary: Ref T2222. Ref T1790. I partially modernized this recently, but bring it to the mail version too.

Test Plan: See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: zeeg, aran

Maniphest Tasks: T1790, T2222

Differential Revision: https://secure.phabricator.com/D8294
This commit is contained in:
epriestley 2014-02-21 11:52:03 -08:00
parent 9384c9e36b
commit 92ac8b5ad9
11 changed files with 145 additions and 108 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/applications/differential/controller/DifferentialChangesetViewController.php
View file

@ -356,7 +356,7 @@ final class DifferentialChangesetViewController extends DifferentialController {
}
$inline = new DifferentialInlineComment();
$inline->setChangesetID($changeset->getID());
$inline->setIsNewFile(true);
$inline->setIsNewFile(1);
$inline->setSyntheticAuthor('Lint: '.$msg['name']);
$inline->setLineNumber($msg['line']);
$inline->setLineLength(0);

8
src/applications/differential/editor/DifferentialCommentEditor.php
View file

@ -641,6 +641,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
}
$changesets = array();
$mail_inlines = array();
if ($inline_comments) {
$load_ids = mpull($inline_comments, 'getChangesetID');
if ($load_ids) {
@ -653,10 +654,13 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
$inline_xaction_comment = $inline->getTransactionCommentForSave();
$inline_xaction_comment->setRevisionPHID($revision->getPHID());
$comments[] = id(clone $template)
$inline_xaction = id(clone $template)
->setAction(DifferentialTransaction::TYPE_INLINE)
->setProxyComment($inline_xaction_comment)
->save();
$comments[] = $inline_xaction;
$mail_inlines[] = $inline_xaction->getProxyTransaction();
}
}
@ -679,7 +683,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
$actor_handle,
$comments,
$changesets,
$inline_comments))
$mail_inlines))
->setActor($actor)
->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs())
->setToPHIDs(

72
src/applications/differential/mail/DifferentialCommentMail.php
View file

@ -25,7 +25,7 @@ final class DifferentialCommentMail extends DifferentialMail {
assert_instances_of($comments, 'DifferentialComment');
assert_instances_of($changesets, 'DifferentialChangeset');
assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface');
assert_instances_of($inline_comments, 'DifferentialTransaction');
$this->setRevision($revision);
$this->setActorHandle($actor);
@ -153,12 +153,12 @@ final class DifferentialCommentMail extends DifferentialMail {
$body[] = null;
foreach ($this->getComments() as $comment) {
if ($comment->getAction() == DifferentialTransaction::TYPE_INLINE) {
foreach ($this->getComments() as $dcomment) {
if ($dcomment->getAction() == DifferentialTransaction::TYPE_INLINE) {
// These have comment content now, but are rendered below.
continue;
}
$content = $comment->getContent();
$content = $dcomment->getContent();
if (strlen($content)) {
$body[] = $this->formatText($content);
$body[] = null;
@ -171,48 +171,58 @@ final class DifferentialCommentMail extends DifferentialMail {
$body[] = null;
}
$context_key = 'metamta.differential.unified-comment-context';
$show_context = PhabricatorEnv::getEnvConfig($context_key);
$inlines = $this->getInlineComments();
if ($inlines) {
$body[] = 'INLINE COMMENTS';
$changesets = $this->getChangesets();
$hunk_parser = new DifferentialHunkParser();
if (PhabricatorEnv::getEnvConfig(
'metamta.differential.unified-comment-context')) {
if ($show_context) {
foreach ($changesets as $changeset) {
$changeset->attachHunks($changeset->loadHunks());
}
}
foreach ($inlines as $inline) {
$changeset = $changesets[$inline->getChangesetID()];
$inline_groups = DifferentialTransactionComment::sortAndGroupInlines(
$inlines,
$changesets);
foreach ($inline_groups as $changeset_id => $group) {
$changeset = $changesets[$changeset_id];
if (!$changeset) {
throw new Exception('Changeset missing!');
}
$file = $changeset->getFilename();
$start = $inline->getLineNumber();
$len = $inline->getLineLength();
if ($len) {
$range = $start.'-'.($start + $len);
} else {
$range = $start;
continue;
}
foreach ($group as $inline) {
$comment = $inline->getComment();
$file = $changeset->getFilename();
$start = $comment->getLineNumber();
$len = $comment->getLineLength();
if ($len) {
$range = $start.'-'.($start + $len);
} else {
$range = $start;
}
$inline_content = $inline->getContent();
$inline_content = $comment->getContent();
if (!PhabricatorEnv::getEnvConfig(
'metamta.differential.unified-comment-context')) {
$body[] = $this->formatText("{$file}:{$range} {$inline_content}");
} else {
$body[] = "================";
$body[] = "Comment at: " . $file . ":" . $range;
$body[] = $hunk_parser->makeContextDiff(
$changeset->getHunks(),
$inline,
1);
$body[] = "----------------";
if (!$show_context) {
$body[] = $this->formatText("{$file}:{$range} {$inline_content}");
} else {
$body[] = "================";
$body[] = "Comment at: " . $file . ":" . $range;
$body[] = $hunk_parser->makeContextDiff(
$changeset->getHunks(),
$comment->getIsNewFile(),
$comment->getLineNumber(),
$comment->getLineLength(),
1);
$body[] = "----------------";
$body[] = $inline_content;
$body[] = null;
$body[] = $inline_content;
$body[] = null;
}
}
}
$body[] = null;

2
src/applications/differential/mail/DifferentialMail.php
View file

@ -389,7 +389,7 @@ abstract class DifferentialMail extends PhabricatorMail {
}
public function setInlineComments(array $inline_comments) {
assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface');
assert_instances_of($inline_comments, 'DifferentialTransaction');
$this->inlineComments = $inline_comments;
return $this;
}

23
src/applications/differential/parser/DifferentialHunkParser.php
View file

@ -545,38 +545,31 @@ final class DifferentialHunkParser {
public function makeContextDiff(
array $hunks,
PhabricatorInlineCommentInterface $inline,
$is_new,
$line_number,
$line_length,
$add_context) {
assert_instances_of($hunks, 'DifferentialHunk');
$context = array();
$debug = false;
if ($debug) {
$context[] = 'Inline: '.$inline->getIsNewFile().' '.
$inline->getLineNumber().' '.$inline->getLineLength();
foreach ($hunks as $hunk) {
$context[] = 'hunk: '.$hunk->getOldOffset().'-'.
$hunk->getOldLen().'; '.$hunk->getNewOffset().'-'.$hunk->getNewLen();
$context[] = $hunk->getChanges();
}
}
if ($inline->getIsNewFile()) {
if ($is_new) {
$prefix = '+';
} else {
$prefix = '-';
}
foreach ($hunks as $hunk) {
if ($inline->getIsNewFile()) {
if ($is_new) {
$offset = $hunk->getNewOffset();
$length = $hunk->getNewLen();
} else {
$offset = $hunk->getOldOffset();
$length = $hunk->getOldLen();
}
$start = $inline->getLineNumber() - $offset;
$end = $start + $inline->getLineLength();
$start = $line_number - $offset;
$end = $start + $line_length;
// We need to go in if $start == $length, because the last line
// might be a "\No newline at end of file" marker, which we want
// to show if the additional context is > 0.

69
src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php
View file

@ -5,24 +5,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
$comment = new DifferentialInlineComment();
return $comment;
}
// $line: 1 based
// $length: 0 based (0 meaning 1 line)
private function createNewComment($line, $length) {
$comment = $this->createComment();
$comment->setIsNewFile(true);
$comment->setLineNumber($line);
$comment->setLineLength($length);
return $comment;
}
// $line: 1 based
// $length: 0 based (0 meaning 1 line)
private function createOldComment($line, $length) {
$comment = $this->createComment();
$comment->setIsNewFile(false);
$comment->setLineNumber($line);
$comment->setLineLength($length);
return $comment;
}
private function createHunk($oldOffset, $oldLen, $newOffset, $newLen, $changes) {
$hunk = new DifferentialHunk();
$hunk->setOldOffset($oldOffset);
@ -57,7 +42,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
$hunks = $this->createSingleChange(1, 0, "-a");
$context = $parser->makeContextDiff(
$hunks,
$this->createOldComment(1, 0),
0,
1,
0,
0);
$this->assertEqual("@@ -1,1 @@\n-a", $context);
}
@ -67,7 +54,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
$hunks = $this->createSingleChange(0, 1, "+a");
$context = $parser->makeContextDiff(
$hunks,
$this->createNewComment(1, 0),
1,
1,
0,
0);
$this->assertEqual("@@ +1,1 @@\n+a", $context);
}
@ -77,7 +66,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
$hunks = $this->createSingleChange(0, 1, "+a");
$context = $parser->makeContextDiff(
$hunks,
$this->createNewComment(2, 0),
1,
2,
0,
0);
$this->assertEqual("", $context);
}
@ -89,7 +80,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
);
$context = $parser->makeContextDiff(
$hunks,
$this->createNewComment(41, 1),
1,
41,
1,
0);
$this->assertEqual("@@ -23,1 +42,1 @@\n 1", $context);
}
@ -101,7 +94,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
);
$context = $parser->makeContextDiff(
$hunks,
$this->createNewComment(43, 1),
1,
43,
1,
0);
$this->assertEqual("@@ -24,1 +43,1 @@\n 2", $context);
}
@ -115,7 +110,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
"+n3\n");
$context = $parser->makeContextDiff(
$hunks,
$this->createOldComment(1, 1),
0,
1,
1,
0);
$this->assertEqual(
"@@ -1,2 +2,1 @@\n".
@ -132,7 +129,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
"+n2\n");
$context = $parser->makeContextDiff(
$hunks,
$this->createNewComment(1, 1),
1,
1,
1,
0);
$this->assertEqual(
"@@ -2,1 +1,2 @@\n".
@ -148,7 +147,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
// Note that this only works with additional context.
$context = $parser->makeContextDiff(
$hunks,
$this->createNewComment(2, 0),
1,
2,
0,
1);
$this->assertEqual(
"@@ +1,1 @@\n".
@ -170,7 +171,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
" e7\n");
$context = $parser->makeContextDiff(
$hunks,
$this->createNewComment(2, 4),
1,
2,
4,
0);
$this->assertEqual(
"@@ -2,5 +2,5 @@\n".
@ -197,7 +200,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
" e7\n");
$context = $parser->makeContextDiff(
$hunks,
$this->createOldComment(2, 4),
0,
2,
4,
0);
$this->assertEqual(
"@@ -2,5 +2,4 @@\n".
@ -218,7 +223,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
"+n3\n");
$context = $parser->makeContextDiff(
$hunks,
$this->createOldComment(1, 1),
0,
1,
1,
1);
$this->assertEqual(
"@@ -1,2 +1,2 @@\n".
@ -236,7 +243,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
"+n2\n");
$context = $parser->makeContextDiff(
$hunks,
$this->createNewComment(1, 1),
1,
1,
1,
1);
$this->assertEqual(
"@@ -1,3 +1,2 @@\n".

5
src/applications/differential/storage/DifferentialComment.php
View file

@ -260,6 +260,7 @@ final class DifferentialComment
public function setProxyComment(DifferentialTransactionComment $proxy) {
$this->proxyComment = $proxy;
$this->proxy->attachComment($proxy);
return $this;
}
@ -351,4 +352,8 @@ final class DifferentialComment
return $this;
}
public function getProxyTransaction() {
return $this->proxy;
}
}

40
src/applications/differential/storage/DifferentialTransactionComment.php
View file

@ -21,4 +21,44 @@ final class DifferentialTransactionComment
// Only cache submitted comments.
return ($this->getTransactionPHID() != null);
}
public static function sortAndGroupInlines(
array $inlines,
array $changesets) {
assert_instances_of($inlines, 'DifferentialTransaction');
assert_instances_of($changesets, 'DifferentialChangeset');
$changesets = mpull($changesets, null, 'getID');
$changesets = msort($changesets, 'getFilename');
// Group the changesets by file and reorder them by display order.
$inline_groups = array();
foreach ($inlines as $inline) {
$changeset_id = $inline->getComment()->getChangesetID();
$inline_groups[$changeset_id][] = $inline;
}
$inline_groups = array_select_keys($inline_groups, array_keys($changesets));
foreach ($inline_groups as $changeset_id => $group) {
// Sort the group of inlines by line number.
$items = array();
foreach ($group as $inline) {
$comment = $inline->getComment();
$num = (int)$comment->getLineNumber();
$len = (int)$comment->getLineLength();
$items[] = array(
'inline' => $inline,
'sort' => ($num << 16) + $len,
);
}
$items = isort($items, 'sort');
$items = ipull($items, 'inline');
$inline_groups[$changeset_id] = $items;
}
return $inline_groups;
}
}

27
src/applications/differential/view/DifferentialTransactionView.php
View file

@ -112,32 +112,11 @@ final class DifferentialTransactionView
$inline_view = new PhabricatorInlineSummaryView();
$changesets = $this->getChangesets();
$changesets = mpull($changesets, null, 'getID');
// Group the changesets by file and reorder them by display order.
$inline_groups = array();
foreach ($inlines as $inline) {
$inline_groups[$inline->getComment()->getChangesetID()][] = $inline;
}
$changsets = msort($changesets, 'getFilename');
$inline_groups = array_select_keys(
$inline_groups,
array_keys($changesets));
$inline_groups = DifferentialTransactionComment::sortAndGroupInlines(
$inlines,
$changesets);
foreach ($inline_groups as $changeset_id => $group) {
// Sort the group of inlines by line number.
$by_line = array();
foreach ($group as $inline) {
$by_line[] = array(
'line' => ((int)$inline->getComment()->getLineNumber() << 16) +
((int)$inline->getComment()->getLineLength()),
'inline' => $inline,
);
}
$by_line = isort($by_line, 'line');
$group = ipull($by_line, 'inline');
$changeset = $changesets[$changeset_id];
$items = array();
foreach ($group as $inline) {

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

@ -173,7 +173,7 @@ abstract class PhabricatorInlineCommentController
// application identifier for the changeset. In Diffusion, it's a Path ID.
$this->changesetID = $request->getInt('changeset');
$this->isNewFile = $request->getBool('is_new');
$this->isNewFile = (int)$request->getBool('is_new');
$this->isOnRight = $request->getBool('on_right');
$this->lineNumber = $request->getInt('number');
$this->lineLength = $request->getInt('length');

3
src/infrastructure/diff/view/PhabricatorInlineSummaryView.php
View file

@ -40,9 +40,6 @@ final class PhabricatorInlineSummaryView extends AphrontView {
phutil_tag('th', array('colspan' => 3), $group));
foreach ($items as $item) {
$items = isort($items, 'line');
$line = $item['line'];
$length = $item['length'];
if ($length) {