From dbdb01f8580770c39f0399fd4424d8c8e08e8319 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 11 Jan 2013 15:27:42 -0800 Subject: [PATCH] Fix whitespace and unchanged file shields Summary: Fixes T181. I actually have no idea what the original issue in T181 was, but this fixes several problems: - The code for figuring out whitespace-only changes was extremely confusing and probably buggy (the code for figuring out unchanged files was equally confusing but probably less buggy). - When rendering a whitespace shield, we did not offer a "Show Changes" link. Instead, show the "Show Changes" link: we can always render the content beneath this link. - When clicking "Show Changes", we used the current whitespace mode, which might result in a diff with no changes. Instead, force "show all" whitespace mode. - We never offered a "show changes" link for unchanged files. Instead, offer this link if we can service it. - When clicking "Show Changes", we pierced the shield but didn't force file content, which would fold the entire file even if it was available. Instead, force the file content. Test Plan: Generated whitespace-only and no-change diffs. Clicked shield links, or verified no shield link available in no-change-with-no-content. Generated an "@generated" change, verified shield worked correctly. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T181, T2009 Differential Revision: https://secure.phabricator.com/D4407 --- .../parser/DifferentialChangesetParser.php | 31 ++++--- .../parser/DifferentialHunkParser.php | 2 + .../render/DifferentialChangesetRenderer.php | 83 ++++++++++++++----- .../differential/behavior-show-more.js | 6 +- 4 files changed, 88 insertions(+), 34 deletions(-) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index c8b9ccfe63..8111be2fad 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -690,28 +690,35 @@ final class DifferentialChangesetParser { $shield = $renderer->renderShield( pht( 'This file contains generated code, which does not normally '. - 'need to be reviewed.'), - true); + 'need to be reviewed.')); } else if ($this->isUnchanged()) { - $shield = $renderer->renderShield( - pht("The contents of this file were not changed."), - false); + $type = 'text'; + if (!$rows) { + // NOTE: Normally, diffs which don't change files do not include + // file content (for example, if you "chmod +x" a file and then + // run "git show", the file content is not available). Similarly, + // if you move a file from A to B without changing it, diffs normally + // do not show the file content. In some cases `arc` is able to + // synthetically generate content for these diffs, but for raw diffs + // we'll never have it so we need to be prepared to not render a link. + $type = 'none'; + } + $shield = $renderer->renderShield( + pht('The contents of this file were not changed.'), + $type); } else if ($this->isWhitespaceOnly()) { $shield = $renderer->renderShield( - pht( - 'This file was changed only by adding or removing whitespace.'), - false); + pht('This file was changed only by adding or removing whitespace.'), + 'whitespace'); } else if ($this->isDeleted()) { $shield = $renderer->renderShield( - pht("This file was completely deleted."), - true); + pht('This file was completely deleted.')); } else if ($this->changeset->getAffectedLineCount() > 2500) { $lines = number_format($this->changeset->getAffectedLineCount()); $shield = $renderer->renderShield( pht( 'This file has a very large number of changes ({%s} lines).', - $lines), - true); + $lines)); } } diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php index 424c6cfbf0..5d2f9a6222 100644 --- a/src/applications/differential/parser/DifferentialHunkParser.php +++ b/src/applications/differential/parser/DifferentialHunkParser.php @@ -249,6 +249,7 @@ final class DifferentialHunkParser { $n_type = null; } + // This line does not exist in the new file. if (($o_type != null) && ($n_type == null)) { $rebuild_old[] = $old_line_data; $rebuild_new[] = null; @@ -258,6 +259,7 @@ final class DifferentialHunkParser { continue; } + // This line does not exist in the old file. if (($n_type != null) && ($o_type == null)) { $rebuild_old[] = null; $rebuild_new[] = $new_line_data; diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index 9d7671bbc7..157aff5f97 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -258,28 +258,69 @@ abstract class DifferentialChangesetRenderer { $vs = 0 ); - public function renderShield($message, $more) { + /** + * Render a "shield" over the diff, with a message like "This file is + * generated and does not need to be reviewed." or "This file was completely + * deleted." This UI element hides unimportant text so the reviewer doesn't + * need to scroll past it. + * + * The shield includes a link to view the underlying content. This link + * may force certain rendering modes when the link is clicked: + * + * - `"default"`: Render the diff normally, as though it was not + * shielded. This is the default and appropriate if the underlying + * diff is a normal change, but was hidden for reasons of not being + * important (e.g., generated code). + * - `"text"`: Force the text to be shown. This is probably only relevant + * when a file is not changed. + * - `"whitespace"`: Force the text to be shown, and the diff to be + * rendered with all whitespace shown. This is probably only relevant + * when a file is changed only by altering whitespace. + * - `"none"`: Don't show the link (e.g., text not available). + * + * @param string Message explaining why the diff is hidden. + * @param string|null Force mode, see above. + * @return string Shield markup. + */ + public function renderShield($message, $force = 'default') { - if ($more) { - $end = $this->getLineCount(); - $reference = $this->getRenderingReference(); - $more = - ' '. - javelin_render_tag( - 'a', - array( - 'mustcapture' => true, - 'sigil' => 'show-more', - 'class' => 'complete', - 'href' => '#', - 'meta' => array( - 'ref' => $reference, - 'range' => "0-{$end}", - ), - ), - 'Show File Contents'); - } else { - $more = null; + $end = $this->getLineCount(); + $reference = $this->getRenderingReference(); + + if ($force !== 'text' && + $force !== 'whitespace' && + $force !== 'none' && + $force !== 'default') { + throw new Exception("Invalid 'force' parameter '{$force}'!"); + } + + $range = "0-{$end}"; + if ($force == 'text') { + // If we're forcing text, force the whole file to be rendered. + $range = "{$range}/0-{$end}"; + } + + $meta = array( + 'ref' => $reference, + 'range' => $range, + ); + + if ($force == 'whitespace') { + $meta['whitespace'] = DifferentialChangesetParser::WHITESPACE_SHOW_ALL; + } + + $more = null; + if ($force !== 'none') { + $more = ' '.javelin_render_tag( + 'a', + array( + 'mustcapture' => true, + 'sigil' => 'show-more', + 'class' => 'complete', + 'href' => '#', + 'meta' => $meta, + ), + 'Show File Contents'); } return javelin_render_tag( diff --git a/webroot/rsrc/js/application/differential/behavior-show-more.js b/webroot/rsrc/js/application/differential/behavior-show-more.js index b38f5062b7..2c44b798c3 100644 --- a/webroot/rsrc/js/application/differential/behavior-show-more.js +++ b/webroot/rsrc/js/application/differential/behavior-show-more.js @@ -39,7 +39,11 @@ JX.behavior('differential-show-more', function(config) { var container = JX.DOM.scry(context, 'td')[0]; JX.DOM.setContent(container, 'Loading...'); JX.DOM.alterClass(context, 'differential-show-more-loading', true); - data['whitespace'] = config.whitespace; + + if (!data['whitespace']) { + data['whitespace'] = config.whitespace; + } + new JX.Workflow(config.uri, data) .setHandler(JX.bind(null, onresponse, context)) .start();