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
2016-05-12 09:59:07 -07:00
parent 9d196648f5
commit bd9bcaa8ff
8 changed files with 228 additions and 79 deletions
--- 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',
--- 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) {
--- a/src/applications/differential/mail/DifferentialChangeDetailMailView.php
+++ b/src/applications/differential/mail/DifferentialChangeDetailMailView.php
@@ -0,0 +1,77 @@
+<?php
+
+final class DifferentialChangeDetailMailView
+  extends DifferentialMailView {
+
+  private $viewer;
+  private $diff;
+  private $patch;
+
+  public function setViewer(PhabricatorUser $viewer) {
+    $this->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);
+  }
+
+}
--- a/src/applications/differential/mail/DifferentialInlineCommentMailView.php
+++ b/src/applications/differential/mail/DifferentialInlineCommentMailView.php
@@ -1,7 +1,7 @@
 <?php

 final class DifferentialInlineCommentMailView
-  extends Phobject {
+  extends DifferentialMailView {

  private $viewer;
  private $inlines;
@@ -85,16 +85,7 @@ final class DifferentialInlineCommentMailView
        $section->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;
--- a/src/applications/differential/mail/DifferentialMailView.php
+++ b/src/applications/differential/mail/DifferentialMailView.php
@@ -0,0 +1,62 @@
+<?php
+
+abstract class DifferentialMailView
+  extends Phobject {
+
+  protected function renderCodeBlock($block) {
+    $style = array(
+      'font: 11px/15px "Menlo", "Consolas", "Monaco", monospace;',
+      'white-space: pre-wrap;',
+      'clear: both;',
+      'padding: 4px 0;',
+      'margin: 0;',
+    );
+
+    return phutil_tag(
+      'div',
+      array(
+        'style' => 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);
+  }
+
+}
--- 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;
            }
--- 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));
--- 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;
      }