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

Rough cut of inline comment context

Browse source 
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
This commit is contained in:
epriestley 2016-05-05 04:42:29 -07:00
parent 19aac8e8d3
commit 2025ecd3d8
3 changed files with 387 additions and 128 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/__phutil_library_map__.php
View file

@ -459,6 +459,7 @@ phutil_register_library_map(array(
'DifferentialHunkTestCase' => 'applications/differential/storage/__tests__/DifferentialHunkTestCase.php', 'DifferentialHunkTestCase' => 'applications/differential/storage/__tests__/DifferentialHunkTestCase.php',
'DifferentialInlineComment' => 'applications/differential/storage/DifferentialInlineComment.php', 'DifferentialInlineComment' => 'applications/differential/storage/DifferentialInlineComment.php',
'DifferentialInlineCommentEditController' => 'applications/differential/controller/DifferentialInlineCommentEditController.php', 'DifferentialInlineCommentEditController' => 'applications/differential/controller/DifferentialInlineCommentEditController.php',
'DifferentialInlineCommentMailView' => 'applications/differential/mail/DifferentialInlineCommentMailView.php',
'DifferentialInlineCommentPreviewController' => 'applications/differential/controller/DifferentialInlineCommentPreviewController.php', 'DifferentialInlineCommentPreviewController' => 'applications/differential/controller/DifferentialInlineCommentPreviewController.php',
'DifferentialInlineCommentQuery' => 'applications/differential/query/DifferentialInlineCommentQuery.php', 'DifferentialInlineCommentQuery' => 'applications/differential/query/DifferentialInlineCommentQuery.php',
'DifferentialJIRAIssuesField' => 'applications/differential/customfield/DifferentialJIRAIssuesField.php', 'DifferentialJIRAIssuesField' => 'applications/differential/customfield/DifferentialJIRAIssuesField.php',
@ -4660,6 +4661,7 @@ phutil_register_library_map(array(
'PhabricatorInlineCommentInterface', 'PhabricatorInlineCommentInterface',
), ),
'DifferentialInlineCommentEditController' => 'PhabricatorInlineCommentController', 'DifferentialInlineCommentEditController' => 'PhabricatorInlineCommentController',
'DifferentialInlineCommentMailView' => 'Phobject',
'DifferentialInlineCommentPreviewController' => 'PhabricatorInlineCommentPreviewController', 'DifferentialInlineCommentPreviewController' => 'PhabricatorInlineCommentPreviewController',
'DifferentialInlineCommentQuery' => 'PhabricatorOffsetPagedQuery', 'DifferentialInlineCommentQuery' => 'PhabricatorOffsetPagedQuery',
'DifferentialJIRAIssuesField' => 'DifferentialStoredCustomField', 'DifferentialJIRAIssuesField' => 'DifferentialStoredCustomField',

131
src/applications/differential/editor/DifferentialTransactionEditor.php
View file

@ -1374,138 +1374,13 @@ final class DifferentialTransactionEditor
return $result; 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( private function renderInlineCommentsForMail(
PhabricatorLiskDAO $object, PhabricatorLiskDAO $object,
array $inlines) { array $inlines) {
return id(new DifferentialInlineCommentMailView())
$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())
->setViewer($this->getActor()) ->setViewer($this->getActor())
->withIDs($changeset_ids) ->setInlines($inlines)
->needHunks(true) ->buildMailSection();
->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;
} }
private function loadDiff($phid, $need_changesets = false) { private function loadDiff($phid, $need_changesets = false) {

382
src/applications/differential/mail/DifferentialInlineCommentMailView.php Normal file
View file

@ -0,0 +1,382 @@
<?php
final class DifferentialInlineCommentMailView
extends Phobject {
private $viewer;
private $inlines;
private $changesets;
private $authors;
public function setViewer(PhabricatorUser $viewer) {
$this->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;
}
}