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) {