From b4de56ef4bfdd78bceb55f83b76e2436493395d6 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 6 Dec 2012 11:33:04 -0800 Subject: [PATCH] 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 | 6 +- .../DifferentialChangesetViewController.php | 5 +- .../DifferentialDiffViewController.php | 2 - .../DifferentialRevisionViewController.php | 2 - .../parser/DifferentialChangesetParser.php | 161 +++++++++--------- .../storage/DifferentialChangeset.php | 13 -- .../view/DifferentialChangesetListView.php | 4 +- .../DifferentialCodeWidthSensitiveView.php | 39 ----- .../view/DifferentialPrimaryPaneView.php | 20 +-- .../differential/changeset-view.css | 52 +++--- 10 files changed, 115 insertions(+), 189 deletions(-) delete mode 100644 src/applications/differential/view/DifferentialCodeWidthSensitiveView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a8bc226d64..5de9f3181e 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index ce83be9cbc..437cd66d22 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/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()) - ->setLineWidthFromChangesets(array($changeset)); - + $panel = new DifferentialPrimaryPaneView(); $panel->appendChild(phutil_render_tag('div', array( 'class' => 'differential-review-stage', 'id' => 'differential-review-stage', - 'style' => "max-width: {$panel->calculateSideBySideWidth()}px;" ), $detail->render()) ); diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php index 7a775d859e..c5bc4be9d7 100644 --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/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(), diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 2f7f76fc7c..097d836eab 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/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(). diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 4ad2c02f31..97a331fcca 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/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 = "\xE2\xAC\x85
"; - 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[] = - ''. - $xhp. - ''; + ''. + ''. + ''.$xhp.''. + ''. + ''. + ''; } foreach ($new_comments as $comment) { $xhp = $this->renderInlineComment($comment); $html_new[] = - ''. - $xhp. - ''; + ''. + ''. + ''. + ''. + ''.$xhp.''. + ''; } if (!$old) { $th_old = ''; - } - else { + } else { $th_old = '1'; } + if (!$cur) { $th_new = ''; - } - else { + } else { $th_new = '1'; } @@ -1113,17 +1111,10 @@ final class DifferentialChangesetParser { $this->changeset, ''. $th_old. - ''. - '
'. - $old. - '
'. - ''. + ''.$old.''. $th_new. - ''. - ''. - '
'. - $cur. - '
'. + ''. + $cur. ''. ''. implode('', $html_old). @@ -1302,9 +1293,15 @@ final class DifferentialChangesetParser { array( 'sigil' => 'context-target', ), - ''. - 'Context not available.'. - ''); + phutil_render_tag( + '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 = ''; + $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 = ''; + $n_colspan--; } - $n_cov = ''; 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 = ''; @@ -1592,12 +1591,9 @@ final class DifferentialChangesetParser { ''); } } - } else { - $n_num = null; - $n_text = null; - $n_attr = null; - $n_cov = null; + $n_colspan--; } + $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[] = ''. ''.$o_num.''. - ''.$o_text.''. + ''.$o_text.''. ''.$n_num.''. $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. - ''."\xE2\x80\x8B".$n_text.''. + ''. + "\xE2\x80\x8B".$n_text. + ''. $n_cov. ''; @@ -1649,20 +1648,24 @@ final class DifferentialChangesetParser { } } $html[] = - ''. - $xhp. - ''. - $new. - ''; + ''. + ''. + ''.$xhp.''. + ''. + $this->renderRightCode($new, 3). + ''; } } if ($n_num && isset($new_comments[$n_num])) { foreach ($new_comments[$n_num] as $comment) { $xhp = $this->renderInlineComment($comment); $html[] = - ''. - $xhp. - ''; + ''. + ''. + ''. + ''. + $this->renderRightCode($xhp, 3). + ''; } } } diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index 9e5466c660..3fe45fea95 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/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) { diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 27effc6166..7a05998dc7 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -1,7 +1,6 @@ 'differential-review-stage', 'id' => 'differential-review-stage', - 'style' => "max-width: {$this->calculateSideBySideWidth()}px; ", ), implode("\n", $output)); } diff --git a/src/applications/differential/view/DifferentialCodeWidthSensitiveView.php b/src/applications/differential/view/DifferentialCodeWidthSensitiveView.php deleted file mode 100644 index 2b27718b07..0000000000 --- a/src/applications/differential/view/DifferentialCodeWidthSensitiveView.php +++ /dev/null @@ -1,39 +0,0 @@ -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); - } - -} diff --git a/src/applications/differential/view/DifferentialPrimaryPaneView.php b/src/applications/differential/view/DifferentialPrimaryPaneView.php index a8cd6cdc41..11b8e33241 100644 --- a/src/applications/differential/view/DifferentialPrimaryPaneView.php +++ b/src/applications/differential/view/DifferentialPrimaryPaneView.php @@ -1,7 +1,6 @@ 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()); } } diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 2c94185893..e1e71f2193 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/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: - * - * 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; + min-width: 320px; /* 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; - width: 44px; + padding: 2px 6px 0px 0px; + 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; }