make differential use fluid width for code layout

Summary:
assume at least 360px for a given code pane. that's about when the comment box starts fighting back anyway. we'll use the yet-to-be-built one page render for the narrow viewport cases.

This address the cases as laid out in T2005. It fails the "MMMMM" case pretty horribly. However, if there is a space it works just fine and presumably folks are stretching out their windows on big glorious monitors at 160 characters wide or whatever.

Re-factored things just a tad but figure I'll take a nice big chunk of "renderer" to move forward T2009

Test Plan: looked at all sorts of funky diffs

Reviewers: epriestley

Reviewed By: epriestley

CC: chad, aran, Korvin

Maniphest Tasks: T2005

Differential Revision: https://secure.phabricator.com/D4083

src/__phutil_library_map__.php

@@ -229,7 +229,6 @@ phutil_register_library_map(array(
     'DifferentialChangesetParser' => 'applications/differential/parser/DifferentialChangesetParser.php',
     'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php',
     'DifferentialChangesetViewController' => 'applications/differential/controller/DifferentialChangesetViewController.php',
-    'DifferentialCodeWidthSensitiveView' => 'applications/differential/view/DifferentialCodeWidthSensitiveView.php',
     'DifferentialComment' => 'applications/differential/storage/DifferentialComment.php',
     'DifferentialCommentEditor' => 'applications/differential/editor/DifferentialCommentEditor.php',
     'DifferentialCommentMail' => 'applications/differential/mail/DifferentialCommentMail.php',
@@ -1508,10 +1507,9 @@ phutil_register_library_map(array(
     'DifferentialChangeSetTestCase' => 'PhabricatorTestCase',
     'DifferentialChangeset' => 'DifferentialDAO',
     'DifferentialChangesetDetailView' => 'AphrontView',
-    'DifferentialChangesetListView' => 'DifferentialCodeWidthSensitiveView',
+    'DifferentialChangesetListView' => 'AphrontView',
     'DifferentialChangesetParserTestCase' => 'ArcanistPhutilTestCase',
     'DifferentialChangesetViewController' => 'DifferentialController',
-    'DifferentialCodeWidthSensitiveView' => 'AphrontView',
     'DifferentialComment' =>
     array(
       0 => 'DifferentialDAO',
@@ -1563,7 +1561,7 @@ phutil_register_library_map(array(
     'DifferentialManiphestTasksFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialNewDiffMail' => 'DifferentialReviewRequestMail',
     'DifferentialPathFieldSpecification' => 'DifferentialFieldSpecification',
-    'DifferentialPrimaryPaneView' => 'DifferentialCodeWidthSensitiveView',
+    'DifferentialPrimaryPaneView' => 'AphrontView',
     'DifferentialReplyHandler' => 'PhabricatorMailReplyHandler',
     'DifferentialResultsTableView' => 'AphrontView',
     'DifferentialRevertPlanFieldSpecification' => 'DifferentialFieldSpecification',

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -234,14 +234,11 @@ final class DifferentialChangesetViewController extends DifferentialController {
     $detail->appendChild($output);
     $detail->setVsChangesetID($left_source);
 
-    $panel = id(new DifferentialPrimaryPaneView())
+    $panel = new DifferentialPrimaryPaneView();
-      ->setLineWidthFromChangesets(array($changeset));
-
     $panel->appendChild(phutil_render_tag('div',
       array(
         'class' => 'differential-review-stage',
         'id'    => 'differential-review-stage',
-        'style' => "max-width: {$panel->calculateSideBySideWidth()}px;"
       ), $detail->render())
     );
 

src/applications/differential/controller/DifferentialDiffViewController.php

@@ -130,14 +130,12 @@ final class DifferentialDiffViewController extends DifferentialController {
     $details = id(new DifferentialChangesetListView())
       ->setChangesets($changesets)
       ->setVisibleChangesets($changesets)
-      ->setLineWidthFromChangesets($changesets)
       ->setRenderingReferences($refs)
       ->setStandaloneURI('/differential/changeset/')
       ->setUser($request->getUser());
 
     return $this->buildStandardPageResponse(
       id(new DifferentialPrimaryPaneView())
-        ->setLineWidthFromChangesets($changesets)
         ->appendChild(
           array(
             $top_panel->render(),

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -281,7 +281,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
     }
 
     $changeset_view = new DifferentialChangesetListView();
-    $changeset_view->setLineWidthFromChangesets($changesets);
     $changeset_view->setChangesets($changesets);
     $changeset_view->setVisibleChangesets($visible_changesets);
 
@@ -382,7 +381,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
     Javelin::initBehavior('differential-user-select');
 
     $page_pane = id(new DifferentialPrimaryPaneView())
-      ->setLineWidthFromChangesets($changesets)
       ->setID($pane_id)
       ->appendChild(
         $comment_view->render().

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -38,7 +38,6 @@ final class DifferentialChangesetParser {
   private $renderingReference;
   private $isSubparser;
 
-  private $lineWidth = 80;
   private $isTopLevel;
   private $coverage;
   private $markupEngine;
@@ -185,17 +184,6 @@ final class DifferentialChangesetParser {
     return $this;
   }
 
-  /**
-   * Set the character width at which lines will be wrapped. Defaults to 80.
-   *
-   * @param   int Hard-wrap line-width for diff display.
-   * @return this
-   */
-  public function setLineWidth($width) {
-    $this->lineWidth = $width;
-    return $this;
-  }
-
   private function getRenderCacheKey() {
     return $this->renderCacheKey;
   }
@@ -204,7 +192,6 @@ final class DifferentialChangesetParser {
     $this->changeset = $changeset;
 
     $this->setFilename($changeset->getFilename());
-    $this->setLineWidth($changeset->getWordWrapWidth());
 
     return $this;
   }
@@ -794,20 +781,12 @@ final class DifferentialChangesetParser {
 
   protected function applyIntraline(&$render, $intra, $corpus) {
 
-    $line_break = "<span class=\"over-the-line\">\xE2\xAC\x85</span><br />";
-
     foreach ($render as $key => $text) {
       if (isset($intra[$key])) {
         $render[$key] = ArcanistDiffUtils::applyIntralineDiff(
           $text,
           $intra[$key]);
       }
-      if (isset($corpus[$key]) &&
-          strlen($corpus[$key]) > $this->lineWidth &&
-          strlen(rtrim($corpus[$key], "\r\n")) > $this->lineWidth) {
-        $lines = phutil_utf8_hard_wrap_html($render[$key], $this->lineWidth);
-        $render[$key] = implode($line_break, $lines);
-      }
     }
   }
 
@@ -1054,16 +1033,30 @@ final class DifferentialChangesetParser {
             }
             if ($file->getPHID() == $old_phid) {
               $old = phutil_render_tag(
-                'img',
+                'div',
                 array(
-                  'src' => $file->getBestURI(),
+                  'class' => 'differential-image-stage'
-                ));
+                ),
+                phutil_render_tag(
+                  'img',
+                  array(
+                    'src' => $file->getBestURI(),
+                  )
+                )
+              );
             } else {
               $cur = phutil_render_tag(
-                'img',
+                'div',
                 array(
-                  'src' => $file->getBestURI(),
+                  'class' => 'differential-image-stage'
-                ));
+                ),
+                phutil_render_tag(
+                  'img',
+                  array(
+                    'src' => $file->getBestURI(),
+                  )
+                )
+              );
             }
           }
         }
@@ -1084,28 +1077,33 @@ final class DifferentialChangesetParser {
         foreach ($old_comments as $comment) {
           $xhp = $this->renderInlineComment($comment);
           $html_old[] =
-            '<tr class="inline"><th /><td>'.
+            '<tr class="inline">'.
-              $xhp.
+              '<th />'.
-            '</td><th /><td colspan="2" /></tr>';
+              '<td class="left">'.$xhp.'</td>'.
+              '<th />'.
+              '<td class="right3" colspan="3" />'.
+            '</tr>';
         }
         foreach ($new_comments as $comment) {
           $xhp = $this->renderInlineComment($comment);
           $html_new[] =
-            '<tr class="inline"><th /><td /><th /><td colspan="2">'.
+            '<tr class="inline">'.
-              $xhp.
+              '<th />'.
-            '</td></tr>';
+              '<td class="left" />'.
+              '<th />'.
+              '<td class="right3" colspan="3">'.$xhp.'</td>'.
+            '</tr>';
         }
 
         if (!$old) {
           $th_old = '<th></th>';
-        }
+        } else {
-        else {
           $th_old = '<th id="C'.$vs.'OL1">1</th>';
         }
+
         if (!$cur) {
           $th_new = '<th></th>';
-        }
+        } else {
-        else {
           $th_new = '<th id="C'.$id.'NL1">1</th>';
         }
 
@@ -1113,17 +1111,10 @@ final class DifferentialChangesetParser {
           $this->changeset,
           '<tr class="differential-image-diff">'.
             $th_old.
-            '<td class="differential-old-image">'.
+            '<td class="left differential-old-image">'.$old.'</td>'.
-              '<div class="differential-image-stage">'.
-                $old.
-              '</div>'.
-            '</td>'.
             $th_new.
-            '<td class="copy differential-new-image"></td>'.
+            '<td class="right3 differential-new-image" colspan="3">'.
-            '<td class="differential-new-image">'.
+              $cur.
-              '<div class="differential-image-stage">'.
-                $cur.
-              '</div>'.
             '</td>'.
           '</tr>'.
           implode('', $html_old).
@@ -1302,9 +1293,15 @@ final class DifferentialChangesetParser {
         array(
           'sigil' => 'context-target',
         ),
-        '<td colspan="6" class="show-more">'.
+        phutil_render_tag(
-          'Context not available.'.
+          'td',
-        '</td>');
+          array(
+            'colspan' => 6,
+            'class' => 'show-more'
+          ),
+          pht('Context not available.')
+        )
+      );
     }
 
     $html = array();
@@ -1513,36 +1510,37 @@ final class DifferentialChangesetParser {
         continue;
       }
 
+      $o_num = null;
+      $o_classes = 'left';
+      $o_text = null;
       if (isset($this->old[$ii])) {
         $o_num  = $this->old[$ii]['line'];
         $o_text = isset($this->oldRender[$ii]) ? $this->oldRender[$ii] : null;
-        $o_attr = null;
         if ($this->old[$ii]['type']) {
           if ($this->old[$ii]['type'] == '\\') {
             $o_text = $this->old[$ii]['text'];
-            $o_attr = ' class="comment"';
+            $o_classes .= ' comment';
           } else if ($this->originalLeft && !isset($highlight_old[$o_num])) {
-            $o_attr = ' class="old-rebase"';
+            $o_classes .= ' old-rebase';
           } else if (empty($this->new[$ii])) {
-            $o_attr = ' class="old old-full"';
+            $o_classes .= ' old old-full';
           } else {
-            $o_attr = ' class="old"';
+            $o_classes .= ' old';
           }
         }
-      } else {
-        $o_num  = null;
-        $o_text = null;
-        $o_attr = null;
       }
 
-      $n_copy = '<td class="copy"></td>';
+      $n_copy = null;
+      $n_cov = null;
+      $n_colspan = 3;
+      $n_classes = '';
+      $n_num  = null;
+      $n_text = null;
 
       if (isset($this->new[$ii])) {
         $n_num  = $this->new[$ii]['line'];
         $n_text = isset($this->newRender[$ii]) ? $this->newRender[$ii] : null;
-        $n_attr = null;
 
-        $cov_class = null;
         if ($this->coverage !== null) {
           if (empty($this->coverage[$n_num - 1])) {
             $cov_class = 'N';
@@ -1550,9 +1548,10 @@ final class DifferentialChangesetParser {
             $cov_class = $this->coverage[$n_num - 1];
           }
           $cov_class = 'cov-'.$cov_class;
+          $n_cov = '<td class="cov '.$cov_class.'"></td>';
+          $n_colspan--;
         }
 
-        $n_cov = '<td class="cov '.$cov_class.'"></td>';
 
         if ($this->new[$ii]['type']) {
           if ($this->new[$ii]['type'] == '\\') {
@@ -1565,7 +1564,7 @@ final class DifferentialChangesetParser {
           } else {
             $n_class = 'new';
           }
-          $n_attr = ' class="'.$n_class.'"';
+          $n_classes = $n_class;
 
           if ($this->new[$ii]['type'] == '\\' || !isset($copy_lines[$n_num])) {
             $n_copy = '<td class="copy '.$n_class.'"></td>';
@@ -1592,12 +1591,9 @@ final class DifferentialChangesetParser {
               '');
           }
         }
-      } else {
+        $n_colspan--;
-        $n_num   = null;
-        $n_text  = null;
-        $n_attr  = null;
-        $n_cov = null;
       }
+      $n_classes .= ' right'.$n_colspan;
 
 
       if (($o_num && !empty($this->missingOld[$o_num])) ||
@@ -1619,16 +1615,19 @@ final class DifferentialChangesetParser {
 
       // NOTE: The Javascript is sensitive to whitespace changes in this
       // block!
+
       $html[] =
         '<tr>'.
           '<th'.$o_id.'>'.$o_num.'</th>'.
-          '<td'.$o_attr.'>'.$o_text.'</td>'.
+          '<td class="'.$o_classes.'">'.$o_text.'</td>'.
           '<th'.$n_id.'>'.$n_num.'</th>'.
           $n_copy.
           // NOTE: This is a unicode zero-width space, which we use as a hint
           // when intercepting 'copy' events to make sure sensible text ends
           // up on the clipboard. See the 'phabricator-oncopy' behavior.
-          '<td'.$n_attr.'>'."\xE2\x80\x8B".$n_text.'</td>'.
+          '<td class="'.$n_classes.'" colspan="'.$n_colspan.'">'.
+            "\xE2\x80\x8B".$n_text.
+          '</td>'.
           $n_cov.
         '</tr>';
 
@@ -1649,20 +1648,24 @@ final class DifferentialChangesetParser {
             }
           }
           $html[] =
-            '<tr class="inline"><th /><td>'.
+            '<tr class="inline">'.
-              $xhp.
+              '<th />'.
-            '</td><th /><td colspan="2">'.
+              '<td class="left">'.$xhp.'</td>'.
-              $new.
+              '<th />'.
-            '</td><td class="cov" /></tr>';
+              $this->renderRightCode($new, 3).
+            '</tr>';
         }
       }
       if ($n_num && isset($new_comments[$n_num])) {
         foreach ($new_comments[$n_num] as $comment) {
           $xhp = $this->renderInlineComment($comment);
           $html[] =
-            '<tr class="inline"><th /><td /><th /><td colspan="2">'.
+            '<tr class="inline">'.
-              $xhp.
+              '<th />'.
-            '</td><td class="cov" /></tr>';
+              '<td class="left" />'.
+              '<th />'.
+              $this->renderRightCode($xhp, 3).
+            '</tr>';
         }
       }
     }

src/applications/differential/storage/DifferentialChangeset.php

@@ -176,19 +176,6 @@ final class DifferentialChangeset extends DifferentialDAO {
     return $path;
   }
 
-  /**
-   * Retreive the configured wordwrap width for this changeset.
-   */
-  public function getWordWrapWidth() {
-    $config = PhabricatorEnv::getEnvConfig('differential.wordwrap');
-    foreach ($config as $regexp => $width) {
-      if (preg_match($regexp, $this->getFilename())) {
-        return $width;
-      }
-    }
-    return 80;
-  }
-
   public function getWhitespaceMatters() {
     $config = PhabricatorEnv::getEnvConfig('differential.whitespace-matters');
     foreach ($config as $regexp) {

src/applications/differential/view/DifferentialChangesetListView.php

@@ -1,7 +1,6 @@
 <?php
 
-final class DifferentialChangesetListView
+final class DifferentialChangesetListView extends AphrontView {
-  extends DifferentialCodeWidthSensitiveView {
 
   private $changesets = array();
   private $visibleChangesets = array();
@@ -194,7 +193,6 @@ final class DifferentialChangesetListView
       array(
         'class' => 'differential-review-stage',
         'id'    => 'differential-review-stage',
-        'style' => "max-width: {$this->calculateSideBySideWidth()}px; ",
       ),
       implode("\n", $output));
   }

src/applications/differential/view/DifferentialCodeWidthSensitiveView.php

@@ -1,39 +0,0 @@
-<?php
-
-abstract class DifferentialCodeWidthSensitiveView extends AphrontView {
-
-  private $lineWidth = 80;
-
-  private function setLineWidth($width) {
-    $this->lineWidth = $width;
-    return $this;
-  }
-
-  public function getLineWidth() {
-    return $this->lineWidth;
-  }
-
-  public function setLineWidthFromChangesets(array $changesets) {
-    assert_instances_of($changesets, 'DifferentialChangeset');
-    if (empty($changesets)) {
-      return $this;
-    }
-
-    $max = 1;
-    foreach ($changesets as $changeset) {
-      $max = max($max, $changeset->getWordWrapWidth());
-    }
-    $this->setLineWidth($max);
-
-    return $this;
-  }
-
-  public function calculateSideBySideWidth() {
-    // Width of the constant-width elements (like line numbers, padding,
-    // and borders).
-    $const = 148;
-    $width = ceil(((1162 - $const) / 80) * $this->getLineWidth()) + $const;
-    return  max(1162, $width);
-  }
-
-}

src/applications/differential/view/DifferentialPrimaryPaneView.php

@@ -1,7 +1,6 @@
 <?php
 
-final class DifferentialPrimaryPaneView
+final class DifferentialPrimaryPaneView extends AphrontView {
-  extends DifferentialCodeWidthSensitiveView {
 
   private $id;
 
@@ -12,28 +11,13 @@ final class DifferentialPrimaryPaneView
 
   public function render() {
 
-    // This is chosen somewhat arbitrarily so the math works out correctly
-    // for 80 columns and sets it to the preexisting width (1162px). It may
-    // need some tweaking, but when lineWidth = 80, the computed pixel width
-    // should be 1162px or something along those lines.
-
-    // Override the 'td' width rule with a more specific, inline style tag.
-    // TODO: move this to <head> somehow.
-    $td_width = ceil((88 / 80) * $this->getLineWidth());
-    $style_tag = phutil_render_tag(
-      'style',
-      array(
-        'type' => 'text/css',
-      ),
-      ".differential-diff td { width: {$td_width}ex; }");
-
     return phutil_render_tag(
       'div',
       array(
         'class' => 'differential-primary-pane',
         'id'    => $this->id,
       ),
-      $style_tag.$this->renderChildren());
+      $this->renderChildren());
   }
 
 }

webroot/rsrc/css/application/differential/changeset-view.css

@@ -4,12 +4,6 @@
 
 .differential-review-stage {
   margin: 0 2em 0 2em;
-
-  /**
-   * This is potentially overridden by a 'style' attribute which selects a
-   * different width based on the maximum character width of the diff.
-   */
-  max-width:        1162px;
 }
 
 .differential-changeset {
@@ -23,15 +17,7 @@
 }
 
 .differential-diff td {
-  /* using monospace fonts makes ex/em most useful:
+  min-width: 320px;
-   *
-   * Unfortunately, firefox 3.6 renders diffs columns for added and removed
-   * files "way-too-wide" when given em as the dimension measurement, so we
-   * use an eyeballed ex equivalent and reset it to the ch character width
-   * measurement for browsers that support that css3 measurement.
-   */
-  width:              88ex;
-  width:              81ch;
   /*
     Disable ligatures in Firefox. Firefox 3 has fancypants ligature support, but
     it gets applied to monospaced fonts, which sucks because it means that the
@@ -44,16 +30,17 @@
   */
   letter-spacing:     0.0083334px;
   vertical-align:     top;
-  white-space:        pre;
+  white-space:        pre-wrap;
   padding:            0 8px 1px;
   line-height:        16px;
-  overflow:           hidden;
 }
 
 .differential-diff th {
   text-align:         right;
-  padding:            2px 6px;
+  padding:            2px 6px 0px 0px;
-  width:              44px;
+  width:              4%;
+  min-width:          45px;
+  max-width:          4%;
   vertical-align:     top;
   background:         #eeeeee;
   color:              #888888;
@@ -70,6 +57,21 @@
   -ms-user-select:      none;
   user-select:          none;
 }
+.differential-diff td.left {
+  width: 45%;
+}
+
+.differential-diff td.right,
+.differential-diff td.right1 {
+  width: 43%;
+}
+
+.differential-diff td.right2 {
+  width: 44.3%;
+}
+.differential-diff td.right3 {
+  width: 45%;
+}
 
 .differential-changeset-immutable .differential-diff th {
   cursor: auto;
@@ -102,7 +104,8 @@
 }
 
 .differential-diff td.copy {
-  width: 6px;
+  min-width: 0.7%;
+  width: 0.7%;
   padding: 0;
 }
 
@@ -121,7 +124,8 @@
 }
 
 .differential-diff td.cov {
-  width: 12px;
+  min-width: 1.3%;
+  width: 1.3%;
   padding: 0;
 }
 
@@ -161,7 +165,7 @@ td.cov-X {
 .differential-diff td.show-context,
 .differential-diff td.differential-shield {
   background: #ffffee;
-  padding: 1em;
+  padding: 1em 0em;
   border: 1px solid #ccccaa;
 }
 
@@ -182,11 +186,10 @@ td.cov-X {
 }
 
 .differential-diff td.show-context {
-  padding-left: 14px; /* td.copy + .differential-diff td */
+  padding-left: 14px;
 }
 
 .differential-diff td.differential-shield {
-  max-width: 1160px;
   text-align: center;
 }
 
@@ -244,7 +247,6 @@ td.cov-X {
   -moz-box-sizing:    border-box;
   -webkit-box-sizing: border-box;
   overflow:           hidden;
-  max-width:          520px;
   white-space:        normal;
 }