From 2025ecd3d826a5c8918f7e521b1785715b2ca426 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 May 2016 04:42:29 -0700 Subject: [PATCH] Rough cut of inline comment context Summary: Ref T10694. This is still missing some pieces, but seems to get most of the data into the mail in a plausible format: - When an inline remarks on code, show the patch inline in the mail body. - When an inline replies to another inline, show that other inline in the mail body. - Apply remarkup rendering to inline content. - Apply basic styling to mail body blocks. Not covered yet: - Syntax highlighting. - Diff highlighting. - Maybe clearer style/layout hints to connect comments to what they reply to? Current approach might get messy with inlines that have blockquotes and code blocks inside them, for example. - I probably want to cap the amount of diff context we ever show to ~7 lines, even if you drag over 200 lines of code. - CSS is a generally a bit rough still. - The `unified-comment-context` option is effectively always on now, and should be removed. - Text section is getting indented right now but probably shouldn't be. - Spacing, etc., might be a bit off. Test Plan: Rigged Home to render these things, got a plausible-looking render (top is text, bottom is HTML): {F1259052} Sent myself some inline comment mail, got a plausible result. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10694 Differential Revision: https://secure.phabricator.com/D15850 --- src/__phutil_library_map__.php | 2 + .../editor/DifferentialTransactionEditor.php | 131 +----- .../DifferentialInlineCommentMailView.php | 382 ++++++++++++++++++ 3 files changed, 387 insertions(+), 128 deletions(-) create mode 100644 src/applications/differential/mail/DifferentialInlineCommentMailView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ffbda83d63..cf9896fcb5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -459,6 +459,7 @@ phutil_register_library_map(array( 'DifferentialHunkTestCase' => 'applications/differential/storage/__tests__/DifferentialHunkTestCase.php', 'DifferentialInlineComment' => 'applications/differential/storage/DifferentialInlineComment.php', 'DifferentialInlineCommentEditController' => 'applications/differential/controller/DifferentialInlineCommentEditController.php', + 'DifferentialInlineCommentMailView' => 'applications/differential/mail/DifferentialInlineCommentMailView.php', 'DifferentialInlineCommentPreviewController' => 'applications/differential/controller/DifferentialInlineCommentPreviewController.php', 'DifferentialInlineCommentQuery' => 'applications/differential/query/DifferentialInlineCommentQuery.php', 'DifferentialJIRAIssuesField' => 'applications/differential/customfield/DifferentialJIRAIssuesField.php', @@ -4660,6 +4661,7 @@ phutil_register_library_map(array( 'PhabricatorInlineCommentInterface', ), 'DifferentialInlineCommentEditController' => 'PhabricatorInlineCommentController', + 'DifferentialInlineCommentMailView' => 'Phobject', 'DifferentialInlineCommentPreviewController' => 'PhabricatorInlineCommentPreviewController', 'DifferentialInlineCommentQuery' => 'PhabricatorOffsetPagedQuery', 'DifferentialJIRAIssuesField' => 'DifferentialStoredCustomField', diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index fabd2511ba..fed5c3728d 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1374,138 +1374,13 @@ final class DifferentialTransactionEditor return $result; } - protected function indentForMail(array $lines) { - $indented = array(); - foreach ($lines as $line) { - $indented[] = '> '.$line; - } - return $indented; - } - - protected function nestCommentHistory( - DifferentialTransactionComment $comment, array $comments_by_line_number, - array $users_by_phid) { - - $nested = array(); - $previous_comments = $comments_by_line_number[$comment->getChangesetID()] - [$comment->getLineNumber()]; - foreach ($previous_comments as $previous_comment) { - if ($previous_comment->getID() >= $comment->getID()) { - break; - } - $nested = $this->indentForMail( - array_merge( - $nested, - explode("\n", $previous_comment->getContent()))); - $user = idx($users_by_phid, $previous_comment->getAuthorPHID(), null); - if ($user) { - array_unshift($nested, pht('%s wrote:', $user->getUserName())); - } - } - - $nested = array_merge($nested, explode("\n", $comment->getContent())); - return implode("\n", $nested); - } - private function renderInlineCommentsForMail( PhabricatorLiskDAO $object, array $inlines) { - - $context_key = 'metamta.differential.unified-comment-context'; - $show_context = PhabricatorEnv::getEnvConfig($context_key); - - $changeset_ids = array(); - $line_numbers_by_changeset = array(); - foreach ($inlines as $inline) { - $id = $inline->getComment()->getChangesetID(); - $changeset_ids[$id] = $id; - $line_numbers_by_changeset[$id][] = - $inline->getComment()->getLineNumber(); - } - - $changesets = id(new DifferentialChangesetQuery()) + return id(new DifferentialInlineCommentMailView()) ->setViewer($this->getActor()) - ->withIDs($changeset_ids) - ->needHunks(true) - ->execute(); - - $inline_groups = DifferentialTransactionComment::sortAndGroupInlines( - $inlines, - $changesets); - - if ($show_context) { - $hunk_parser = new DifferentialHunkParser(); - $table = new DifferentialTransactionComment(); - $conn_r = $table->establishConnection('r'); - $queries = array(); - foreach ($line_numbers_by_changeset as $id => $line_numbers) { - $queries[] = qsprintf( - $conn_r, - '(changesetID = %d AND lineNumber IN (%Ld))', - $id, $line_numbers); - } - $all_comments = id(new DifferentialTransactionComment())->loadAllWhere( - 'transactionPHID IS NOT NULL AND (%Q)', implode(' OR ', $queries)); - $comments_by_line_number = array(); - foreach ($all_comments as $comment) { - $comments_by_line_number - [$comment->getChangesetID()] - [$comment->getLineNumber()] - [$comment->getID()] = $comment; - } - $author_phids = mpull($all_comments, 'getAuthorPHID'); - $authors = id(new PhabricatorPeopleQuery()) - ->setViewer($this->getActor()) - ->withPHIDs($author_phids) - ->execute(); - $authors_by_phid = mpull($authors, null, 'getPHID'); - } - - $section = new PhabricatorMetaMTAMailSection(); - foreach ($inline_groups as $changeset_id => $group) { - $changeset = idx($changesets, $changeset_id); - if (!$changeset) { - 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 = $comment->getContent(); - - if (!$show_context) { - $section->addFragment("{$file}:{$range} {$inline_content}"); - } else { - $patch = $hunk_parser->makeContextDiff( - $changeset->getHunks(), - $comment->getIsNewFile(), - $comment->getLineNumber(), - $comment->getLineLength(), - 1); - $nested_comments = $this->nestCommentHistory( - $inline->getComment(), $comments_by_line_number, $authors_by_phid); - - $section - ->addFragment('================') - ->addFragment(pht('Comment at: %s:%s', $file, $range)) - ->addPlaintextFragment($patch) - ->addHTMLFragment($this->renderPatchHTMLForMail($patch)) - ->addFragment('----------------') - ->addFragment($nested_comments) - ->addFragment(null); - } - } - } - - return $section; + ->setInlines($inlines) + ->buildMailSection(); } private function loadDiff($phid, $need_changesets = false) { diff --git a/src/applications/differential/mail/DifferentialInlineCommentMailView.php b/src/applications/differential/mail/DifferentialInlineCommentMailView.php new file mode 100644 index 0000000000..8b78fbc752 --- /dev/null +++ b/src/applications/differential/mail/DifferentialInlineCommentMailView.php @@ -0,0 +1,382 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setInlines($inlines) { + $this->inlines = $inlines; + return $this; + } + + public function getInlines() { + return $this->inlines; + } + + public function buildMailSection() { + $inlines = $this->getInlines(); + + $comments = mpull($inlines, 'getComment'); + $comments = mpull($comments, null, 'getPHID'); + $parents = $this->loadParents($comments); + $all_comments = $comments + $parents; + + $this->changesets = $this->loadChangesets($all_comments); + $this->authors = $this->loadAuthors($all_comments); + $groups = $this->groupInlines($inlines); + + $hunk_parser = new DifferentialHunkParser(); + + $spacer_text = null; + $spacer_html = phutil_tag('br'); + + $section = new PhabricatorMetaMTAMailSection(); + + $last_group_key = last_key($groups); + foreach ($groups as $changeset_id => $group) { + $changeset = $this->getChangeset($changeset_id); + if (!$changeset) { + continue; + } + + $is_last_group = ($changeset_id == $last_group_key); + + $last_inline_key = last_key($group); + foreach ($group as $inline_key => $inline) { + $comment = $inline->getComment(); + $parent_phid = $comment->getReplyToCommentPHID(); + + $is_last_inline = ($inline_key == $last_inline_key); + + $context_text = null; + $context_html = null; + + if ($parent_phid) { + $parent = idx($parents, $parent_phid); + if ($parent) { + $context_text = $this->renderInline($parent, false, true); + $context_html = $this->renderInline($parent, true, true); + } + } else { + $patch = $this->getPatch($hunk_parser, $comment); + $context_text = $this->renderPatch($comment, $patch, false); + $context_html = $this->renderPatch($comment, $patch, true); + } + + $render_text = $this->renderInline($comment, false, false); + $render_html = $this->renderInline($comment, true, false); + + $section->addPlaintextFragment($context_text); + $section->addHTMLFragment($context_html); + + $section->addPlaintextFragment($spacer_text); + $section->addHTMLFragment($spacer_html); + + $section->addPlaintextFragment($render_text); + $section->addHTMLFragment($render_html); + + if (!$is_last_group || !$is_last_inline) { + $section->addPlaintextFragment($spacer_text); + $section->addHTMLFragment($spacer_html); + } + } + } + + return $section; + } + + private function loadChangesets(array $comments) { + if (!$comments) { + return array(); + } + + $ids = array(); + foreach ($comments as $comment) { + $ids[] = $comment->getChangesetID(); + } + + $changesets = id(new DifferentialChangesetQuery()) + ->setViewer($this->getViewer()) + ->withIDs($ids) + ->needHunks(true) + ->execute(); + + return mpull($changesets, null, 'getID'); + } + + private function loadParents(array $comments) { + $viewer = $this->getViewer(); + + $phids = array(); + foreach ($comments as $comment) { + $parent_phid = $comment->getReplyToCommentPHID(); + if (!$parent_phid) { + continue; + } + $phids[] = $parent_phid; + } + + if (!$phids) { + return array(); + } + + $parents = id(new DifferentialDiffInlineCommentQuery()) + ->setViewer($viewer) + ->withPHIDs($phids) + ->execute(); + + return mpull($parents, null, 'getPHID'); + } + + private function loadAuthors(array $comments) { + $viewer = $this->getViewer(); + + $phids = array(); + foreach ($comments as $comment) { + $author_phid = $comment->getAuthorPHID(); + if (!$author_phid) { + continue; + } + $phids[] = $author_phid; + } + + if (!$phids) { + return array(); + } + + return $viewer->loadHandles($phids); + } + + private function groupInlines(array $inlines) { + return DifferentialTransactionComment::sortAndGroupInlines( + $inlines, + $this->changesets); + } + + private function renderInline( + DifferentialTransactionComment $comment, + $is_html, + $is_quote) { + + $changeset = $this->getChangeset($comment->getChangesetID()); + if (!$changeset) { + return null; + } + + $content = $comment->getContent(); + $content = $this->renderRemarkupContent($content, $is_html); + + if ($is_quote) { + $header = $this->renderHeader($comment, $is_html, true); + } else { + $header = null; + } + + $parts = array( + $header, + "\n", + $content, + ); + + if (!$is_html) { + $parts = implode('', $parts); + $parts = trim($parts); + } + + if ($is_quote) { + if ($is_html) { + $parts = $this->quoteHTML($parts); + } else { + $parts = $this->quoteText($parts); + } + } + + return $parts; + } + + private function renderRemarkupContent($content, $is_html) { + $viewer = $this->getViewer(); + $production_uri = PhabricatorEnv::getProductionURI('/'); + + if ($is_html) { + $mode = PhutilRemarkupEngine::MODE_HTML_MAIL; + } else { + $mode = PhutilRemarkupEngine::MODE_TEXT; + } + + $engine = PhabricatorMarkupEngine::newMarkupEngine(array()) + ->setConfig('viewer', $viewer) + ->setConfig('uri.base', $production_uri) + ->setMode($mode); + + try { + return $engine->markupText($content); + } catch (Exception $ex) { + return $content; + } + } + + private function getChangeset($id) { + return idx($this->changesets, $id); + } + + private function getAuthor($phid) { + if (isset($this->authors[$phid])) { + return $this->authors[$phid]; + } + return null; + } + + private function quoteText($block) { + $block = phutil_split_lines($block); + foreach ($block as $key => $line) { + $block[$key] = '> '.$line; + } + + return implode('', $block); + } + + private function quoteHTML($block) { + $styles = array( + 'padding: 4px 8px;', + 'background: #F8F9FC;', + 'border-left: 3px solid #a7b5bf;', + ); + + $styles = implode(' ', $styles); + + return phutil_tag( + 'div', + array( + 'style' => $styles, + ), + $block); + } + + private function getPatch( + DifferentialHunkParser $parser, + DifferentialTransactionComment $comment) { + + $changeset = $this->getChangeset($comment->getChangesetID()); + $hunks = $changeset->getHunks(); + + $is_new = $comment->getIsNewFile(); + $start = $comment->getLineNumber(); + $length = $comment->getLineLength(); + + $diff = $parser->makeContextDiff($hunks, $is_new, $start, $length, 1); + + return $diff; + } + + private function renderPatch( + DifferentialTransactionComment $comment, + $patch, + $is_html) { + + $patch = phutil_split_lines($patch); + + // Remove the "@@ -x,y +u,v @@" line. + array_shift($patch); + + $patch = implode('', $patch); + + if ($is_html) { + $style = array( + 'font: 11px/15px "Menlo", "Consolas", "Monaco", monospace;', + 'padding: 0', + 'margin: 0;', + ); + + $style = implode(' ', $style); + $patch = phutil_tag( + 'pre', + array( + 'style' => $style, + ), + $patch); + } + + $header = $this->renderHeader($comment, $is_html, false); + + $patch = array( + $header, + "\n", + $patch, + ); + + if (!$is_html) { + $patch = implode('', $patch); + $patch = $this->quoteText($patch); + } else { + $patch = $this->quoteHTML($patch); + } + + return $patch; + } + + private function renderHeader( + DifferentialTransactionComment $comment, + $is_html, + $with_author) { + + $changeset = $this->getChangeset($comment->getChangesetID()); + $path = $changeset->getFilename(); + + $start = $comment->getLineNumber(); + $length = $comment->getLineLength(); + if ($length) { + $range = pht('%s-%s', $start, $start + $length); + } else { + $range = $start; + } + + $header = "{$path}:{$range}"; + if ($is_html) { + $header = phutil_tag('strong', array(), $header); + } + + if ($with_author) { + $author = $this->getAuthor($comment->getAuthorPHID()); + } else { + $author = null; + } + + if ($author) { + $byline = '@'.$author->getName(); + + if ($is_html) { + $byline = phutil_tag('strong', array(), $byline); + } + + $header = pht('%s wrote in %s', $byline, $header); + } else { + $header = pht('In %s', $header); + } + + if ($is_html) { + $header = phutil_tag( + 'div', + array( + 'style' => 'font-style: italic', + ), + $header); + } + + return $header; + } + +}