From 412fc3455730408d8b868bf30bafda20b803b8cc Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 6 May 2016 11:46:46 -0700
Subject: [PATCH] Improve inline mail snippet rendering, possibly fixing
 Airmail?

Summary:
Ref T10694. General improvements:

  - Remove leading empty lines from context snippets.
  - Remove trailing empty lines from context snippets.
  - If we removed everything, render a note.
  - Try using `style` instead of `<pre>`? My thinking is that maybe Airmail has weird default rules for `<pre>`, since that's the biggest / most obvious thing that's different about this element to me.

Test Plan: Viewed normal comments locally, faked a comment on an empty line in the middle of a lot of other empty lines.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10694

Differential Revision: https://secure.phabricator.com/D15864
---
 .../DifferentialInlineCommentMailView.php     |  4 +-
 ...DifferentialChangesetOneUpMailRenderer.php | 59 ++++++++++++++++---
 2 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/src/applications/differential/mail/DifferentialInlineCommentMailView.php b/src/applications/differential/mail/DifferentialInlineCommentMailView.php
index 5fc13c34c5..554bb66a63 100644
--- a/src/applications/differential/mail/DifferentialInlineCommentMailView.php
+++ b/src/applications/differential/mail/DifferentialInlineCommentMailView.php
@@ -376,13 +376,15 @@ final class DifferentialInlineCommentMailView
     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(
-        'pre',
+        'div',
         array(
           'style' => $style,
         ),
diff --git a/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php
index 4c34ebace5..042047b4a8 100644
--- a/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php
+++ b/src/applications/differential/render/DifferentialChangesetOneUpMailRenderer.php
@@ -50,6 +50,7 @@ final class DifferentialChangesetOneUpMailRenderer
 
   protected function renderPrimitives(array $primitives, $rows) {
     $out = array();
+
     foreach ($primitives as $k => $p) {
       $type = $p['type'];
       switch ($type) {
@@ -73,26 +74,66 @@ final class DifferentialChangesetOneUpMailRenderer
             }
           }
 
-          $style = "padding: 0 8px; margin: 0 4px; {$style}";
-
-          $out[] = phutil_tag(
-            'div',
-            array(
-              'style' => $style,
-            ),
-            $p['render']);
+          $out[] = array(
+            'style' => $style,
+            'render' => $p['render'],
+            'text' => (string)$p['render'],
+          );
           break;
         default:
           break;
       }
     }
 
+    // Remove all leading and trailing empty lines, since these just look kind
+    // of weird in mail.
+    foreach ($out as $key => $line) {
+      if (!strlen(trim($line['text']))) {
+        unset($out[$key]);
+      } else {
+        break;
+      }
+    }
+
+    $keys = array_reverse(array_keys($out));
+    foreach ($keys as $key) {
+      $line = $out[$key];
+      if (!strlen(trim($line['text']))) {
+        unset($out[$key]);
+      } else {
+        break;
+      }
+    }
+
+    // If the user has commented on an empty line in the middle of a bunch of
+    // other empty lines, emit an explicit marker instead of just rendering
+    // nothing.
+    if (!$out) {
+      $out[] = array(
+        'style' => 'color: #888888;',
+        'render' => pht('(Empty.)'),
+      );
+    }
+
+    $render = array();
+    foreach ($out as $line) {
+      $style = $line['style'];
+      $style = "padding: 0 8px; margin: 0 4px; {$style}";
+
+      $render[] = phutil_tag(
+        'div',
+        array(
+          'style' => $style,
+        ),
+        $line['render']);
+    }
+
     $style_map = id(new PhabricatorDefaultSyntaxStyle())
       ->getRemarkupStyleMap();
 
     $styled_body = id(new PhutilPygmentizeParser())
       ->setMap($style_map)
-      ->parse((string)hsprintf('%s', $out));
+      ->parse((string)hsprintf('%s', $render));
 
     return phutil_safe_html($styled_body);
   }