Page MenuHomePhabricator
Files F15338262

D8294.diff
No OneTemporary
Actions

D8294.diff
View Options

Index: src/applications/differential/controller/DifferentialChangesetViewController.php
===================================================================
--- src/applications/differential/controller/DifferentialChangesetViewController.php
+++ src/applications/differential/controller/DifferentialChangesetViewController.php
@@ -356,7 +356,7 @@
}
$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);
Index: src/applications/differential/editor/DifferentialCommentEditor.php
===================================================================
--- src/applications/differential/editor/DifferentialCommentEditor.php
+++ src/applications/differential/editor/DifferentialCommentEditor.php
@@ -641,6 +641,7 @@
}
$changesets = array();
+ $mail_inlines = array();
if ($inline_comments) {
$load_ids = mpull($inline_comments, 'getChangesetID');
if ($load_ids) {
@@ -653,10 +654,13 @@
$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 @@
$actor_handle,
$comments,
$changesets,
- $inline_comments))
+ $mail_inlines))
->setActor($actor)
->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs())
->setToPHIDs(
Index: src/applications/differential/mail/DifferentialCommentMail.php
===================================================================
--- src/applications/differential/mail/DifferentialCommentMail.php
+++ src/applications/differential/mail/DifferentialCommentMail.php
@@ -25,7 +25,7 @@
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 @@
$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 @@
$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!');
+ continue;
}
- $file = $changeset->getFilename();
- $start = $inline->getLineNumber();
- $len = $inline->getLineLength();
- if ($len) {
- $range = $start.'-'.($start + $len);
- } else {
- $range = $start;
- }
-
- $inline_content = $inline->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[] = "----------------";
-
- $body[] = $inline_content;
- $body[] = null;
+ 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 = $comment->getContent();
+
+ 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[] = null;
Index: src/applications/differential/mail/DifferentialMail.php
===================================================================
--- src/applications/differential/mail/DifferentialMail.php
+++ src/applications/differential/mail/DifferentialMail.php
@@ -389,7 +389,7 @@
}
public function setInlineComments(array $inline_comments) {
- assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface');
+ assert_instances_of($inline_comments, 'DifferentialTransaction');
$this->inlineComments = $inline_comments;
return $this;
}
Index: src/applications/differential/parser/DifferentialHunkParser.php
===================================================================
--- src/applications/differential/parser/DifferentialHunkParser.php
+++ src/applications/differential/parser/DifferentialHunkParser.php
@@ -545,38 +545,31 @@
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.
Index: src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php
===================================================================
--- src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php
+++ src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php
@@ -5,24 +5,9 @@
$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 @@
$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 @@
$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 @@
$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 @@
);
$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 @@
);
$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 @@
"+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 @@
"+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 @@
// 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 @@
" 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 @@
" 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 @@
"+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 @@
"+n2\n");
$context = $parser->makeContextDiff(
$hunks,
- $this->createNewComment(1, 1),
+ 1,
+ 1,
+ 1,
1);
$this->assertEqual(
"@@ -1,3 +1,2 @@\n".
Index: src/applications/differential/storage/DifferentialComment.php
===================================================================
--- src/applications/differential/storage/DifferentialComment.php
+++ src/applications/differential/storage/DifferentialComment.php
@@ -260,6 +260,7 @@
public function setProxyComment(DifferentialTransactionComment $proxy) {
$this->proxyComment = $proxy;
+ $this->proxy->attachComment($proxy);
return $this;
}
@@ -351,4 +352,8 @@
return $this;
}
+ public function getProxyTransaction() {
+ return $this->proxy;
+ }
+
}
Index: src/applications/differential/storage/DifferentialTransactionComment.php
===================================================================
--- src/applications/differential/storage/DifferentialTransactionComment.php
+++ src/applications/differential/storage/DifferentialTransactionComment.php
@@ -21,4 +21,44 @@
// 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;
+ }
+
}
Index: src/applications/differential/view/DifferentialTransactionView.php
===================================================================
--- src/applications/differential/view/DifferentialTransactionView.php
+++ src/applications/differential/view/DifferentialTransactionView.php
@@ -112,32 +112,11 @@
$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) {
Index: src/infrastructure/diff/PhabricatorInlineCommentController.php
===================================================================
--- src/infrastructure/diff/PhabricatorInlineCommentController.php
+++ src/infrastructure/diff/PhabricatorInlineCommentController.php
@@ -173,7 +173,7 @@
// 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');
Index: src/infrastructure/diff/view/PhabricatorInlineSummaryView.php
===================================================================
--- src/infrastructure/diff/view/PhabricatorInlineSummaryView.php
+++ src/infrastructure/diff/view/PhabricatorInlineSummaryView.php
@@ -40,9 +40,6 @@
phutil_tag('th', array('colspan' => 3), $group));
foreach ($items as $item) {
-
- $items = isort($items, 'line');
-
$line = $item['line'];
$length = $item['length'];
if ($length) {

File Metadata

Mime Type
text/plain
Expires
Mon, Mar 10, 10:12 AM (4 d, 3 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7427072
Default Alt Text
D8294.diff (17 KB)

Event Timeline

Phabricator · Built by Phacility