From bd9bcaa8ffea6a84119bcdb57f753bc8fb756456 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 May 2016 09:59:07 -0700 Subject: [PATCH] Improve HTML mail rendering of inline patches Summary: Fixes T9790. This uses a simple renderer, like the inline context renderer, that emphasizes getting a quick glance at small changes and working reasonably on mobile devices. Test Plan: - Set `inline` setting to `9999`. - Created a diff. - Saw it render reasonably in HTML mail. - Also tested text mail to make sure I didn't break that. {F1310137, size=full} Reviewers: chad Reviewed By: chad Maniphest Tasks: T9790 Differential Revision: https://secure.phabricator.com/D15901 --- src/__phutil_library_map__.php | 6 +- .../editor/DifferentialTransactionEditor.php | 54 +++++++++---- .../mail/DifferentialChangeDetailMailView.php | 77 +++++++++++++++++++ .../DifferentialInlineCommentMailView.php | 60 ++------------- .../mail/DifferentialMailView.php | 62 +++++++++++++++ .../parser/DifferentialChangesetParser.php | 26 +++++-- .../parser/DifferentialHunkParser.php | 3 +- ...DifferentialChangesetOneUpMailRenderer.php | 19 +++++ 8 files changed, 228 insertions(+), 79 deletions(-) create mode 100644 src/applications/differential/mail/DifferentialChangeDetailMailView.php create mode 100644 src/applications/differential/mail/DifferentialMailView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 74302c34e4..8f4756e9e4 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -362,6 +362,7 @@ phutil_register_library_map(array( 'DifferentialBlameRevisionField' => 'applications/differential/customfield/DifferentialBlameRevisionField.php', 'DifferentialBlockHeraldAction' => 'applications/differential/herald/DifferentialBlockHeraldAction.php', 'DifferentialBranchField' => 'applications/differential/customfield/DifferentialBranchField.php', + 'DifferentialChangeDetailMailView' => 'applications/differential/mail/DifferentialChangeDetailMailView.php', 'DifferentialChangeHeraldFieldGroup' => 'applications/differential/herald/DifferentialChangeHeraldFieldGroup.php', 'DifferentialChangeType' => 'applications/differential/constants/DifferentialChangeType.php', 'DifferentialChangesSinceLastUpdateField' => 'applications/differential/customfield/DifferentialChangesSinceLastUpdateField.php', @@ -471,6 +472,7 @@ phutil_register_library_map(array( 'DifferentialLintField' => 'applications/differential/customfield/DifferentialLintField.php', 'DifferentialLintStatus' => 'applications/differential/constants/DifferentialLintStatus.php', 'DifferentialLocalCommitsView' => 'applications/differential/view/DifferentialLocalCommitsView.php', + 'DifferentialMailView' => 'applications/differential/mail/DifferentialMailView.php', 'DifferentialManiphestTasksField' => 'applications/differential/customfield/DifferentialManiphestTasksField.php', 'DifferentialModernHunk' => 'applications/differential/storage/DifferentialModernHunk.php', 'DifferentialNextStepField' => 'applications/differential/customfield/DifferentialNextStepField.php', @@ -4551,6 +4553,7 @@ phutil_register_library_map(array( 'DifferentialBlameRevisionField' => 'DifferentialStoredCustomField', 'DifferentialBlockHeraldAction' => 'HeraldAction', 'DifferentialBranchField' => 'DifferentialCustomField', + 'DifferentialChangeDetailMailView' => 'DifferentialMailView', 'DifferentialChangeHeraldFieldGroup' => 'HeraldFieldGroup', 'DifferentialChangeType' => 'Phobject', 'DifferentialChangesSinceLastUpdateField' => 'DifferentialCustomField', @@ -4665,7 +4668,7 @@ phutil_register_library_map(array( 'PhabricatorInlineCommentInterface', ), 'DifferentialInlineCommentEditController' => 'PhabricatorInlineCommentController', - 'DifferentialInlineCommentMailView' => 'Phobject', + 'DifferentialInlineCommentMailView' => 'DifferentialMailView', 'DifferentialInlineCommentPreviewController' => 'PhabricatorInlineCommentPreviewController', 'DifferentialInlineCommentQuery' => 'PhabricatorOffsetPagedQuery', 'DifferentialJIRAIssuesField' => 'DifferentialStoredCustomField', @@ -4676,6 +4679,7 @@ phutil_register_library_map(array( 'DifferentialLintField' => 'DifferentialHarbormasterField', 'DifferentialLintStatus' => 'Phobject', 'DifferentialLocalCommitsView' => 'AphrontView', + 'DifferentialMailView' => 'Phobject', 'DifferentialManiphestTasksField' => 'DifferentialCoreCustomField', 'DifferentialModernHunk' => 'DifferentialHunk', 'DifferentialNextStepField' => 'DifferentialCustomField', diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 0db490769f..3855edeac9 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1251,21 +1251,18 @@ final class DifferentialTransactionEditor $config_attach = PhabricatorEnv::getEnvConfig($config_key_attach); if ($config_inline || $config_attach) { - $patch_section = $this->renderPatchForMail($diff); - $lines = count(phutil_split_lines($patch_section->getPlaintext())); + $patch = $this->buildPatchForMail($diff); + $lines = substr_count($patch, "\n"); if ($config_inline && ($lines <= $config_inline)) { - $body->addTextSection( - pht('CHANGE DETAILS'), - $patch_section); + $this->appendChangeDetailsForMail($object, $diff, $patch, $body); } if ($config_attach) { $name = pht('D%s.%s.patch', $object->getID(), $diff->getID()); $mime_type = 'text/x-patch; charset=utf-8'; $body->addAttachment( - new PhabricatorMetaMTAAttachment( - $patch_section->getPlaintext(), $name, $mime_type)); + new PhabricatorMetaMTAAttachment($patch, $name, $mime_type)); } } } @@ -1387,7 +1384,38 @@ final class DifferentialTransactionEditor $section_text = "\n".$section->getPlaintext(); $style = array( - 'margin: 12px 0;', + 'margin: 6px 0 12px 0;', + ); + + $section_html = phutil_tag( + 'div', + array( + 'style' => implode(' ', $style), + ), + $section->getHTML()); + + $body->addPlaintextSection($header, $section_text, false); + $body->addHTMLSection($header, $section_html); + } + + private function appendChangeDetailsForMail( + PhabricatorLiskDAO $object, + DifferentialDiff $diff, + $patch, + PhabricatorMetaMTAMailBody $body) { + + $section = id(new DifferentialChangeDetailMailView()) + ->setViewer($this->getActor()) + ->setDiff($diff) + ->setPatch($patch) + ->buildMailSection(); + + $header = pht('CHANGE DETAILS'); + + $section_text = "\n".$section->getPlaintext(); + + $style = array( + 'margin: 6px 0 12px 0;', ); $section_html = phutil_tag( @@ -1659,20 +1687,14 @@ final class DifferentialTransactionEditor array('style' => 'font-family: monospace;'), $patch); } - private function renderPatchForMail(DifferentialDiff $diff) { + private function buildPatchForMail(DifferentialDiff $diff) { $format = PhabricatorEnv::getEnvConfig('metamta.differential.patch-format'); - $patch = id(new DifferentialRawDiffRenderer()) + return id(new DifferentialRawDiffRenderer()) ->setViewer($this->getActor()) ->setFormat($format) ->setChangesets($diff->getChangesets()) ->buildPatch(); - - $section = new PhabricatorMetaMTAMailSection(); - $section->addHTMLFragment($this->renderPatchHTMLForMail($patch)); - $section->addPlaintextFragment($patch); - - return $section; } protected function willPublish(PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/differential/mail/DifferentialChangeDetailMailView.php b/src/applications/differential/mail/DifferentialChangeDetailMailView.php new file mode 100644 index 0000000000..d0f0be71b1 --- /dev/null +++ b/src/applications/differential/mail/DifferentialChangeDetailMailView.php @@ -0,0 +1,77 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setDiff(DifferentialDiff $diff) { + $this->diff = $diff; + return $this; + } + + public function getDiff() { + return $this->diff; + } + + public function setPatch($patch) { + $this->patch = $patch; + return $this; + } + + public function getPatch() { + return $this->patch; + } + + public function buildMailSection() { + $viewer = $this->getViewer(); + + $diff = $this->getDiff(); + + $engine = new PhabricatorMarkupEngine(); + + $out = array(); + foreach ($diff->getChangesets() as $changeset) { + $parser = id(new DifferentialChangesetParser()) + ->setUser($viewer) + ->setChangeset($changeset) + ->setLinesOfContext(2) + ->setMarkupEngine($engine); + + $parser->setRenderer(new DifferentialChangesetOneUpMailRenderer()); + $block = $parser->render(); + + $filename = $changeset->getFilename(); + $filename = $this->renderHeaderBold($filename); + $header = $this->renderHeaderBlock($filename); + + $out[] = $this->renderContentBox( + array( + $header, + $this->renderCodeBlock($block), + )); + } + + $out = phutil_implode_html(phutil_tag('br'), $out); + + $patch_html = $out; + + $patch_text = $this->getPatch(); + + return id(new PhabricatorMetaMTAMailSection()) + ->addPlaintextFragment($patch_text) + ->addHTMLFragment($patch_html); + } + +} diff --git a/src/applications/differential/mail/DifferentialInlineCommentMailView.php b/src/applications/differential/mail/DifferentialInlineCommentMailView.php index 554bb66a63..57958202b1 100644 --- a/src/applications/differential/mail/DifferentialInlineCommentMailView.php +++ b/src/applications/differential/mail/DifferentialInlineCommentMailView.php @@ -1,7 +1,7 @@ addPlaintextFragment($spacer_text); $section->addPlaintextFragment($render_text); - $style = array( - 'border: 1px solid #C7CCD9;', - 'border-radius: 3px;', - ); - - $html_fragment = phutil_tag( - 'div', - array( - 'style' => implode(' ', $style), - ), + $html_fragment = $this->renderContentBox( array( $context_html, $render_html, @@ -374,21 +365,7 @@ final class DifferentialInlineCommentMailView $is_html) { if ($is_html) { - $style = array( - 'font: 11px/15px "Menlo", "Consolas", "Monaco", monospace;', - 'white-space: pre-wrap;', - 'clear: both;', - 'padding: 4px 0;', - 'margin: 0;', - ); - - $style = implode(' ', $style); - $patch = phutil_tag( - 'div', - array( - 'style' => $style, - ), - $patch); + $patch = $this->renderCodeBlock($patch); } $header = $this->renderHeader($comment, $is_html, false); @@ -430,12 +407,7 @@ final class DifferentialInlineCommentMailView $header = "{$path}:{$range}"; if ($is_html) { - $header = phutil_tag( - 'span', - array( - 'style' => 'color: #4b4d51; font-weight: bold;', - ), - $header); + $header = $this->renderHeaderBold($header); } if ($with_author) { @@ -448,12 +420,7 @@ final class DifferentialInlineCommentMailView $byline = $author->getName(); if ($is_html) { - $byline = phutil_tag( - 'span', - array( - 'style' => 'color: #4b4d51; font-weight: bold;', - ), - $byline); + $byline = $this->renderHeaderBold($byline); } $header = pht('%s wrote in %s', $byline, $header); @@ -478,22 +445,7 @@ final class DifferentialInlineCommentMailView $link = null; } - $style = array( - 'color: #74777d;', - 'background: #eff2f4;', - 'padding: 6px 8px;', - 'overflow: hidden;', - ); - - $header = phutil_tag( - 'div', - array( - 'style' => implode(' ', $style), - ), - array( - $link, - $header, - )); + $header = $this->renderHeaderBlock(array($link, $header)); } return $header; diff --git a/src/applications/differential/mail/DifferentialMailView.php b/src/applications/differential/mail/DifferentialMailView.php new file mode 100644 index 0000000000..81b7836765 --- /dev/null +++ b/src/applications/differential/mail/DifferentialMailView.php @@ -0,0 +1,62 @@ + implode(' ', $style), + ), + $block); + } + + protected function renderHeaderBlock($block) { + $style = array( + 'color: #74777d;', + 'background: #eff2f4;', + 'padding: 6px 8px;', + 'overflow: hidden;', + ); + + return phutil_tag( + 'div', + array( + 'style' => implode(' ', $style), + ), + $block); + } + + protected function renderHeaderBold($content) { + return phutil_tag( + 'span', + array( + 'style' => 'color: #4b4d51; font-weight: bold;', + ), + $content); + } + + protected function renderContentBox($content) { + $style = array( + 'border: 1px solid #C7CCD9;', + 'border-radius: 3px;', + ); + + return phutil_tag( + 'div', + array( + 'style' => implode(' ', $style), + ), + $content); + } + +} diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index f7815538ac..9fc0adf537 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -55,6 +55,7 @@ final class DifferentialChangesetParser extends Phobject { private $rangeStart; private $rangeEnd; private $mask; + private $linesOfContext = 8; private $highlightEngine; @@ -195,8 +196,6 @@ final class DifferentialChangesetParser extends Phobject { const ATTR_WHITELINES = 'attr:white'; const ATTR_MOVEAWAY = 'attr:moveaway'; - const LINES_CONTEXT = 8; - const WHITESPACE_SHOW_ALL = 'show-all'; const WHITESPACE_IGNORE_TRAILING = 'ignore-trailing'; const WHITESPACE_IGNORE_MOST = 'ignore-most'; @@ -227,6 +226,16 @@ final class DifferentialChangesetParser extends Phobject { return $this; } + public function setLinesOfContext($lines_of_context) { + $this->linesOfContext = $lines_of_context; + return $this; + } + + public function getLinesOfContext() { + return $this->linesOfContext; + } + + /** * Configure which Changeset comments added to the right side of the visible * diff will be attached to. The ID must be the ID of a real Differential @@ -724,8 +733,10 @@ final class DifferentialChangesetParser extends Phobject { self::ATTR_MOVEAWAY => $moveaway, )); + $lines_context = $this->getLinesOfContext(); + $hunk_parser->generateIntraLineDiffs(); - $hunk_parser->generateVisibileLinesMask(); + $hunk_parser->generateVisibileLinesMask($lines_context); $this->setOldLines($hunk_parser->getOldLines()); $this->setNewLines($hunk_parser->getNewLines()); @@ -959,6 +970,7 @@ final class DifferentialChangesetParser extends Phobject { $old_mask = array(); $new_mask = array(); $feedback_mask = array(); + $lines_context = $this->getLinesOfContext(); if ($this->comments) { // If there are any comments which appear in sections of the file which @@ -1001,10 +1013,10 @@ final class DifferentialChangesetParser extends Phobject { } - $start = max($comment->getLineNumber() - self::LINES_CONTEXT, 0); + $start = max($comment->getLineNumber() - $lines_context, 0); $end = $comment->getLineNumber() + $comment->getLineLength() + - self::LINES_CONTEXT; + $lines_context; for ($ii = $start; $ii <= $end; $ii++) { if ($new_side) { $new_mask[$ii] = true; @@ -1189,6 +1201,8 @@ final class DifferentialChangesetParser extends Phobject { $range_start, $range_len) { + $lines_context = $this->getLinesOfContext(); + // Calculate gaps and mask first $gaps = array(); $gap_start = 0; @@ -1199,7 +1213,7 @@ final class DifferentialChangesetParser extends Phobject { if (isset($base_mask[$ii])) { if ($in_gap) { $gap_length = $ii - $gap_start; - if ($gap_length <= self::LINES_CONTEXT) { + if ($gap_length <= $lines_context) { for ($jj = $gap_start; $jj <= $gap_start + $gap_length; $jj++) { $base_mask[$jj] = true; } diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php index 22b82e002e..5bd98e9012 100644 --- a/src/applications/differential/parser/DifferentialHunkParser.php +++ b/src/applications/differential/parser/DifferentialHunkParser.php @@ -353,8 +353,7 @@ final class DifferentialHunkParser extends Phobject { return $this; } - public function generateVisibileLinesMask() { - $lines_context = DifferentialChangesetParser::LINES_CONTEXT; + public function generateVisibileLinesMask($lines_context) { $old = $this->getOldLines(); $new = $this->getNewLines(); $max_length = max(count($old), count($new)); diff --git a/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php index 042047b4a8..e720422ecd 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php @@ -51,6 +51,16 @@ final class DifferentialChangesetOneUpMailRenderer protected function renderPrimitives(array $primitives, $rows) { $out = array(); + $context_style = array( + 'background: #F7F7F7;', + 'color: #74777D;', + 'border-style: dashed;', + 'border-color: #C7CCD9;', + 'border-width: 1px 0;', + ); + + $context_style = implode(' ', $context_style); + foreach ($primitives as $k => $p) { $type = $p['type']; switch ($type) { @@ -80,6 +90,15 @@ final class DifferentialChangesetOneUpMailRenderer 'text' => (string)$p['render'], ); break; + case 'context': + // NOTE: These are being included with no text so they get stripped + // in the header and footer. + $out[] = array( + 'style' => $context_style, + 'render' => '...', + 'text' => '', + ); + break; default: break; }