From 612b29fff40aa0aed4943e225d4bc1552f64566d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 14 Jan 2013 14:20:06 -0800 Subject: [PATCH] Implement basic one-up and test renderers Summary: This is a half-step toward one-up and test renderers. This is mostly a structural change, and has no user-facing impact. It splits the rendering hierarchy like this: - Renderer (more methods are abstract than before) - HTML Renderer (most HTML stuff has moved down from the base to here) - HTML 1-up (placeholder only -- not yet a functional implementation) - HTML 2-up (minimal changes, uses mostly old code) - Test Renderer (unit-testable renderer base, implements text versions of the HTML stuff) - Test 1-up (selects 1-up mode for test rendering) - Test 2-up (selects 2-up mode for test rendering) Broadly, I'm trying to share as much code as possible by splitting rendering into more, smaller stages. Specifically, we do this: - Combine the various sorts of inputs (changes, context, inlines, etc.) into a single, relatively homogenous list of "primitives". This happens in the base class. - The primitive types are: old (diff left side), new (diff right side), context ("show more context"), no-context ("context not available") and inline (inline comment). - Possibly, apply a filtering/reordering step to the primitives to get them ready for 1-up rendering. This mostly removes information, and does a small amount of reordering. This also happens in the base class. - Pass the primitives to the actual renderer, to convert them into HTML, text, or whatever else. This happens in the leaf class. The primitive implementation is not yet complete (it doesn't attach as much information to the primitives as it should -- stuff like coverage and copies), but covers the basics. The existing HTMLTwoUp renderer does not use the primitive path; instead, it still goes down the old path. Test Plan: Ran unit tests, looked at a bunch of diffs. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2009 Differential Revision: https://secure.phabricator.com/D4421 --- src/__phutil_library_map__.php | 16 +- .../DifferentialParseRenderTestCase.php | 63 ++ .../differential/__tests__/data/fruit.diff | 15 + .../__tests__/data/fruit.diff.one.expect | 12 + .../__tests__/data/fruit.diff.two.expect | 21 + .../parser/DifferentialChangesetParser.php | 29 +- .../DifferentialChangesetParserTestCase.php | 2 +- .../DifferentialChangesetHTMLRenderer.php | 362 ++++++++++++ .../DifferentialChangesetOneUpRenderer.php | 28 + ...DifferentialChangesetOneUpTestRenderer.php | 10 + .../render/DifferentialChangesetRenderer.php | 540 +++++++----------- .../DifferentialChangesetTestRenderer.php | 86 +++ .../DifferentialChangesetTwoUpRenderer.php | 42 +- ...DifferentialChangesetTwoUpTestRenderer.php | 10 + 14 files changed, 853 insertions(+), 383 deletions(-) create mode 100644 src/applications/differential/__tests__/DifferentialParseRenderTestCase.php create mode 100644 src/applications/differential/__tests__/data/fruit.diff create mode 100644 src/applications/differential/__tests__/data/fruit.diff.one.expect create mode 100644 src/applications/differential/__tests__/data/fruit.diff.two.expect create mode 100644 src/applications/differential/render/DifferentialChangesetHTMLRenderer.php create mode 100644 src/applications/differential/render/DifferentialChangesetOneUpRenderer.php create mode 100644 src/applications/differential/render/DifferentialChangesetOneUpTestRenderer.php create mode 100644 src/applications/differential/render/DifferentialChangesetTestRenderer.php create mode 100644 src/applications/differential/render/DifferentialChangesetTwoUpTestRenderer.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 99c14e949b..60717934a4 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -225,11 +225,16 @@ phutil_register_library_map(array( 'DifferentialChangeset' => 'applications/differential/storage/DifferentialChangeset.php', 'DifferentialChangesetDetailView' => 'applications/differential/view/DifferentialChangesetDetailView.php', 'DifferentialChangesetFileTreeSideNavBuilder' => 'applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php', + 'DifferentialChangesetHTMLRenderer' => 'applications/differential/render/DifferentialChangesetHTMLRenderer.php', 'DifferentialChangesetListView' => 'applications/differential/view/DifferentialChangesetListView.php', + 'DifferentialChangesetOneUpRenderer' => 'applications/differential/render/DifferentialChangesetOneUpRenderer.php', + 'DifferentialChangesetOneUpTestRenderer' => 'applications/differential/render/DifferentialChangesetOneUpTestRenderer.php', 'DifferentialChangesetParser' => 'applications/differential/parser/DifferentialChangesetParser.php', 'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php', 'DifferentialChangesetRenderer' => 'applications/differential/render/DifferentialChangesetRenderer.php', + 'DifferentialChangesetTestRenderer' => 'applications/differential/render/DifferentialChangesetTestRenderer.php', 'DifferentialChangesetTwoUpRenderer' => 'applications/differential/render/DifferentialChangesetTwoUpRenderer.php', + 'DifferentialChangesetTwoUpTestRenderer' => 'applications/differential/render/DifferentialChangesetTwoUpTestRenderer.php', 'DifferentialChangesetViewController' => 'applications/differential/controller/DifferentialChangesetViewController.php', 'DifferentialComment' => 'applications/differential/storage/DifferentialComment.php', 'DifferentialCommentEditor' => 'applications/differential/editor/DifferentialCommentEditor.php', @@ -281,6 +286,7 @@ phutil_register_library_map(array( 'DifferentialMailPhase' => 'applications/differential/constants/DifferentialMailPhase.php', 'DifferentialManiphestTasksFieldSpecification' => 'applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php', 'DifferentialNewDiffMail' => 'applications/differential/mail/DifferentialNewDiffMail.php', + 'DifferentialParseRenderTestCase' => 'applications/differential/__tests__/DifferentialParseRenderTestCase.php', 'DifferentialPathFieldSpecification' => 'applications/differential/field/specification/DifferentialPathFieldSpecification.php', 'DifferentialPrimaryPaneView' => 'applications/differential/view/DifferentialPrimaryPaneView.php', 'DifferentialReplyHandler' => 'applications/differential/DifferentialReplyHandler.php', @@ -1624,9 +1630,14 @@ phutil_register_library_map(array( 'DifferentialCCsFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialChangeset' => 'DifferentialDAO', 'DifferentialChangesetDetailView' => 'AphrontView', + 'DifferentialChangesetHTMLRenderer' => 'DifferentialChangesetRenderer', 'DifferentialChangesetListView' => 'AphrontView', - 'DifferentialChangesetParserTestCase' => 'ArcanistPhutilTestCase', - 'DifferentialChangesetTwoUpRenderer' => 'DifferentialChangesetRenderer', + 'DifferentialChangesetOneUpRenderer' => 'DifferentialChangesetHTMLRenderer', + 'DifferentialChangesetOneUpTestRenderer' => 'DifferentialChangesetTestRenderer', + 'DifferentialChangesetParserTestCase' => 'PhabricatorTestCase', + 'DifferentialChangesetTestRenderer' => 'DifferentialChangesetRenderer', + 'DifferentialChangesetTwoUpRenderer' => 'DifferentialChangesetHTMLRenderer', + 'DifferentialChangesetTwoUpTestRenderer' => 'DifferentialChangesetTestRenderer', 'DifferentialChangesetViewController' => 'DifferentialController', 'DifferentialComment' => array( @@ -1680,6 +1691,7 @@ phutil_register_library_map(array( 'DifferentialLocalCommitsView' => 'AphrontView', 'DifferentialManiphestTasksFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialNewDiffMail' => 'DifferentialReviewRequestMail', + 'DifferentialParseRenderTestCase' => 'PhabricatorTestCase', 'DifferentialPathFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialPrimaryPaneView' => 'AphrontView', 'DifferentialReplyHandler' => 'PhabricatorMailReplyHandler', diff --git a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php new file mode 100644 index 0000000000..0402334c57 --- /dev/null +++ b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php @@ -0,0 +1,63 @@ +buildChangesetParser($type, $data, $file); + $actual = $parser->render(null, null, array()); + + $expect = Filesystem::readFile($dir.$file.'.'.$type.'.expect'); + $this->assertEqual($expect, $actual, $file.'.'.$type); + } + } + } + + private function buildChangesetParser($type, $data, $file) { + $parser = new ArcanistDiffParser(); + $changes = $parser->parseDiff($data); + + $diff = DifferentialDiff::newFromRawChanges($changes); + if (count($diff->getChangesets()) !== 1) { + throw new Exception("Expected one changeset: {$file}"); + } + + $changeset = head($diff->getChangesets()); + + $engine = new PhabricatorMarkupEngine(); + + $cparser = new DifferentialChangesetParser(); + $cparser->setDisableCache(true); + $cparser->setChangeset($changeset); + $cparser->setMarkupEngine($engine); + + if ($type == 'one') { + $cparser->setRenderer(new DifferentialChangesetOneUpTestRenderer()); + } else if ($type == 'two') { + $cparser->setRenderer(new DifferentialChangesetTwoUpTestRenderer()); + } else { + throw new Exception("Unknown renderer type '{$type}'!"); + } + + return $cparser; + } + +} diff --git a/src/applications/differential/__tests__/data/fruit.diff b/src/applications/differential/__tests__/data/fruit.diff new file mode 100644 index 0000000000..bf642e24d2 --- /dev/null +++ b/src/applications/differential/__tests__/data/fruit.diff @@ -0,0 +1,15 @@ +diff --git a/fruit b/fruit +new file mode 100644 +index 0000000..a1f7255 +--- /dev/null ++++ b/fruit +@@ -0,0 +1,9 @@ ++apple ++banana ++cherry ++date ++elderberry ++fig ++grape ++honeydew ++ diff --git a/src/applications/differential/__tests__/data/fruit.diff.one.expect b/src/applications/differential/__tests__/data/fruit.diff.one.expect new file mode 100644 index 0000000000..3fc8172b5f --- /dev/null +++ b/src/applications/differential/__tests__/data/fruit.diff.one.expect @@ -0,0 +1,12 @@ +CTYPE 1 1 (unforced) +- +- +N 1 + apple~ +N 2 + banana~ +N 3 + cherry~ +N 4 + date~ +N 5 + elderberry~ +N 6 + fig~ +N 7 + grape~ +N 8 + honeydew~ +N 9 + ~ diff --git a/src/applications/differential/__tests__/data/fruit.diff.two.expect b/src/applications/differential/__tests__/data/fruit.diff.two.expect new file mode 100644 index 0000000000..034b6ef4d2 --- /dev/null +++ b/src/applications/differential/__tests__/data/fruit.diff.two.expect @@ -0,0 +1,21 @@ +CTYPE 1 1 (unforced) +- +- +O - . ~ +N 1 + apple~ +O - . ~ +N 2 + banana~ +O - . ~ +N 3 + cherry~ +O - . ~ +N 4 + date~ +O - . ~ +N 5 + elderberry~ +O - . ~ +N 6 + fig~ +O - . ~ +N 7 + grape~ +O - . ~ +N 8 + honeydew~ +O - . ~ +N 9 + ~ diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 8111be2fad..f190ab11b3 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -39,6 +39,29 @@ final class DifferentialChangesetParser { private $coverage; private $markupEngine; private $highlightErrors; + private $disableCache; + private $renderer; + + public function setRenderer($renderer) { + $this->renderer = $renderer; + return $this; + } + + public function getRenderer() { + if (!$this->renderer) { + return new DifferentialChangesetTwoUpRenderer(); + } + return $this->renderer; + } + + public function setDisableCache($disable_cache) { + $this->disableCache = $disable_cache; + return $this; + } + + public function getDisableCache() { + return $this->disableCache; + } const CACHE_VERSION = 10; const CACHE_MAX_SIZE = 8e6; @@ -434,6 +457,10 @@ final class DifferentialChangesetParser { } $skip_cache = ($whitespace_mode != self::WHITESPACE_IGNORE_ALL); + if ($this->disableCache) { + $skip_cache = true; + } + $this->whitespaceMode = $whitespace_mode; $changeset = $this->changeset; @@ -664,7 +691,7 @@ final class DifferentialChangesetParser { count($this->old), count($this->new)); - $renderer = id(new DifferentialChangesetTwoUpRenderer()) + $renderer = $this->getRenderer() ->setChangeset($this->changeset) ->setRenderPropertyChangeHeader($render_pch) ->setLineCount($rows) diff --git a/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php b/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php index ab062d361c..d8d10169b5 100644 --- a/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php +++ b/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php @@ -1,6 +1,6 @@ getChangeset(); + + $change = $changeset->getChangeType(); + $file = $changeset->getFileType(); + + $message = null; + if ($change == DifferentialChangeType::TYPE_CHANGE && + $file == DifferentialChangeType::FILE_TEXT) { + if ($force) { + // We have to force something to render because there were no changes + // of other kinds. + $message = pht('This file was not modified.'); + } else { + // Default case of changes to a text file, no metadata. + return null; + } + } else { + switch ($change) { + + case DifferentialChangeType::TYPE_ADD: + switch ($file) { + case DifferentialChangeType::FILE_TEXT: + $message = pht('This file was added.'); + break; + case DifferentialChangeType::FILE_IMAGE: + $message = pht('This image was added.'); + break; + case DifferentialChangeType::FILE_DIRECTORY: + $message = pht('This directory was added.'); + break; + case DifferentialChangeType::FILE_BINARY: + $message = pht('This binary file was added.'); + break; + case DifferentialChangeType::FILE_SYMLINK: + $message = pht('This symlink was added.'); + break; + case DifferentialChangeType::FILE_SUBMODULE: + $message = pht('This submodule was added.'); + break; + } + break; + + case DifferentialChangeType::TYPE_DELETE: + switch ($file) { + case DifferentialChangeType::FILE_TEXT: + $message = pht('This file was deleted.'); + break; + case DifferentialChangeType::FILE_IMAGE: + $message = pht('This image was deleted.'); + break; + case DifferentialChangeType::FILE_DIRECTORY: + $message = pht('This directory was deleted.'); + break; + case DifferentialChangeType::FILE_BINARY: + $message = pht('This binary file was deleted.'); + break; + case DifferentialChangeType::FILE_SYMLINK: + $message = pht('This symlink was deleted.'); + break; + case DifferentialChangeType::FILE_SUBMODULE: + $message = pht('This submodule was deleted.'); + break; + } + break; + + case DifferentialChangeType::TYPE_MOVE_HERE: + $from = + "". + phutil_escape_html($changeset->getOldFile()). + ""; + switch ($file) { + case DifferentialChangeType::FILE_TEXT: + $message = pht('This file was moved from %s.', $from); + break; + case DifferentialChangeType::FILE_IMAGE: + $message = pht('This image was moved from %s.', $from); + break; + case DifferentialChangeType::FILE_DIRECTORY: + $message = pht('This directory was moved from %s.', $from); + break; + case DifferentialChangeType::FILE_BINARY: + $message = pht('This binary file was moved from %s.', $from); + break; + case DifferentialChangeType::FILE_SYMLINK: + $message = pht('This symlink was moved from %s.', $from); + break; + case DifferentialChangeType::FILE_SUBMODULE: + $message = pht('This submodule was moved from %s.', $from); + break; + } + break; + + case DifferentialChangeType::TYPE_COPY_HERE: + $from = + "". + phutil_escape_html($changeset->getOldFile()). + ""; + switch ($file) { + case DifferentialChangeType::FILE_TEXT: + $message = pht('This file was copied from %s.', $from); + break; + case DifferentialChangeType::FILE_IMAGE: + $message = pht('This image was copied from %s.', $from); + break; + case DifferentialChangeType::FILE_DIRECTORY: + $message = pht('This directory was copied from %s.', $from); + break; + case DifferentialChangeType::FILE_BINARY: + $message = pht('This binary file was copied from %s.', $from); + break; + case DifferentialChangeType::FILE_SYMLINK: + $message = pht('This symlink was copied from %s.', $from); + break; + case DifferentialChangeType::FILE_SUBMODULE: + $message = pht('This submodule was copied from %s.', $from); + break; + } + break; + + case DifferentialChangeType::TYPE_MOVE_AWAY: + $paths = + "". + phutil_escape_html(implode(', ', $changeset->getAwayPaths())). + ""; + switch ($file) { + case DifferentialChangeType::FILE_TEXT: + $message = pht('This file was moved to %s.', $paths); + break; + case DifferentialChangeType::FILE_IMAGE: + $message = pht('This image was moved to %s.', $paths); + break; + case DifferentialChangeType::FILE_DIRECTORY: + $message = pht('This directory was moved to %s.', $paths); + break; + case DifferentialChangeType::FILE_BINARY: + $message = pht('This binary file was moved to %s.', $paths); + break; + case DifferentialChangeType::FILE_SYMLINK: + $message = pht('This symlink was moved to %s.', $paths); + break; + case DifferentialChangeType::FILE_SUBMODULE: + $message = pht('This submodule was moved to %s.', $paths); + break; + } + break; + + case DifferentialChangeType::TYPE_COPY_AWAY: + $paths = + "". + phutil_escape_html(implode(', ', $changeset->getAwayPaths())). + ""; + switch ($file) { + case DifferentialChangeType::FILE_TEXT: + $message = pht('This file was copied to %s.', $paths); + break; + case DifferentialChangeType::FILE_IMAGE: + $message = pht('This image was copied to %s.', $paths); + break; + case DifferentialChangeType::FILE_DIRECTORY: + $message = pht('This directory was copied to %s.', $paths); + break; + case DifferentialChangeType::FILE_BINARY: + $message = pht('This binary file was copied to %s.', $paths); + break; + case DifferentialChangeType::FILE_SYMLINK: + $message = pht('This symlink was copied to %s.', $paths); + break; + case DifferentialChangeType::FILE_SUBMODULE: + $message = pht('This submodule was copied to %s.', $paths); + break; + } + break; + + case DifferentialChangeType::TYPE_MULTICOPY: + $paths = + "". + phutil_escape_html(implode(', ', $changeset->getAwayPaths())). + ""; + switch ($file) { + case DifferentialChangeType::FILE_TEXT: + $message = pht( + 'This file was deleted after being copied to %s.', + $paths); + break; + case DifferentialChangeType::FILE_IMAGE: + $message = pht( + 'This image was deleted after being copied to %s.', + $paths); + break; + case DifferentialChangeType::FILE_DIRECTORY: + $message = pht( + 'This directory was deleted after being copied to %s.', + $paths); + break; + case DifferentialChangeType::FILE_BINARY: + $message = pht( + 'This binary file was deleted after being copied to %s.', + $paths); + break; + case DifferentialChangeType::FILE_SYMLINK: + $message = pht( + 'This symlink was deleted after being copied to %s.', + $paths); + break; + case DifferentialChangeType::FILE_SUBMODULE: + $message = pht( + 'This submodule was deleted after being copied to %s.', + $paths); + break; + } + break; + + default: + switch ($file) { + case DifferentialChangeType::FILE_TEXT: + $message = pht('This is a file.'); + break; + case DifferentialChangeType::FILE_IMAGE: + $message = pht('This is an image.'); + break; + case DifferentialChangeType::FILE_DIRECTORY: + $message = pht('This is a directory.'); + break; + case DifferentialChangeType::FILE_BINARY: + $message = pht('This is a binary file.'); + break; + case DifferentialChangeType::FILE_SYMLINK: + $message = pht('This is a symlink.'); + break; + case DifferentialChangeType::FILE_SUBMODULE: + $message = pht('This is a submodule.'); + break; + } + break; + } + } + + return + '
'. + $message. + '
'; + } + + protected function renderPropertyChangeHeader() { + $changeset = $this->getChangeset(); + + $old = $changeset->getOldProperties(); + $new = $changeset->getNewProperties(); + + $keys = array_keys($old + $new); + sort($keys); + + $rows = array(); + foreach ($keys as $key) { + $oval = idx($old, $key); + $nval = idx($new, $key); + if ($oval !== $nval) { + if ($oval === null) { + $oval = 'null'; + } else { + $oval = nl2br(phutil_escape_html($oval)); + } + + if ($nval === null) { + $nval = 'null'; + } else { + $nval = nl2br(phutil_escape_html($nval)); + } + + $rows[] = + ''. + ''.phutil_escape_html($key).''. + ''.$oval.''. + ''.$nval.''. + ''; + } + } + + return + ''. + ''. + ''. + ''. + ''. + ''. + implode('', $rows). + '
Property ChangesOld ValueNew Value
'; + } + + protected function renderShield($message, $force = 'default') { + $end = count($this->getOldLines()); + $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( + 'tr', + array( + 'sigil' => 'context-target', + ), + ''. + phutil_escape_html($message). + $more. + ''); + } + + protected function wrapChangeInTable($content) { + if (!$content) { + return null; + } + + return javelin_render_tag( + 'table', + array( + 'class' => 'differential-diff remarkup-code PhabricatorMonospaced', + 'sigil' => 'differential-diff', + ), + $content); + } + + +} diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php new file mode 100644 index 0000000000..919f753231 --- /dev/null +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -0,0 +1,28 @@ +renderPropertyChangeHeader; } - abstract public function renderChangesetTable($contents); + final public function renderChangesetTable($content) { + $props = null; + if ($this->shouldRenderPropertyChangeHeader()) { + $props = $this->renderPropertyChangeHeader(); + } + + $force = (!$content && !$props); + $notice = $this->renderChangeTypeHeader($force); + + $result = $notice.$props.$content; + + // TODO: Let the user customize their tab width / display style. + // TODO: We should possibly post-process "\r" as well. + // TODO: Both these steps should happen earlier. + $result = str_replace("\t", ' ', $result); + + return $result; + } + + abstract public function isOneUpRenderer(); abstract public function renderTextChange( $range_start, $range_len, @@ -258,6 +277,12 @@ abstract class DifferentialChangesetRenderer { $vs = 0 ); + abstract protected function renderChangeTypeHeader($force); + + protected function didRenderChangesetTableContents($contents) { + return $contents; + } + /** * 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 @@ -282,348 +307,9 @@ abstract class DifferentialChangesetRenderer { * @param string|null Force mode, see above. * @return string Shield markup. */ - public function renderShield($message, $force = 'default') { + abstract protected function renderShield($message, $force = 'default'); - $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( - 'tr', - array( - 'sigil' => 'context-target', - ), - ''. - phutil_escape_html($message). - $more. - ''); - } - - - protected function renderPropertyChangeHeader($changeset) { - if (!$this->shouldRenderPropertyChangeHeader()) { - return null; - } - - $old = $changeset->getOldProperties(); - $new = $changeset->getNewProperties(); - - $keys = array_keys($old + $new); - sort($keys); - - $rows = array(); - foreach ($keys as $key) { - $oval = idx($old, $key); - $nval = idx($new, $key); - if ($oval !== $nval) { - if ($oval === null) { - $oval = 'null'; - } else { - $oval = nl2br(phutil_escape_html($oval)); - } - - if ($nval === null) { - $nval = 'null'; - } else { - $nval = nl2br(phutil_escape_html($nval)); - } - - $rows[] = - ''. - ''.phutil_escape_html($key).''. - ''.$oval.''. - ''.$nval.''. - ''; - } - } - - return - ''. - ''. - ''. - ''. - ''. - ''. - implode('', $rows). - '
Property ChangesOld ValueNew Value
'; - } - - protected function renderChangeTypeHeader($changeset, $force) { - $change = $changeset->getChangeType(); - $file = $changeset->getFileType(); - - $message = null; - if ($change == DifferentialChangeType::TYPE_CHANGE && - $file == DifferentialChangeType::FILE_TEXT) { - if ($force) { - // We have to force something to render because there were no changes - // of other kinds. - $message = pht('This file was not modified.'); - } else { - // Default case of changes to a text file, no metadata. - return null; - } - } else { - switch ($change) { - - case DifferentialChangeType::TYPE_ADD: - switch ($file) { - case DifferentialChangeType::FILE_TEXT: - $message = pht('This file was added.'); - break; - case DifferentialChangeType::FILE_IMAGE: - $message = pht('This image was added.'); - break; - case DifferentialChangeType::FILE_DIRECTORY: - $message = pht('This directory was added.'); - break; - case DifferentialChangeType::FILE_BINARY: - $message = pht('This binary file was added.'); - break; - case DifferentialChangeType::FILE_SYMLINK: - $message = pht('This symlink was added.'); - break; - case DifferentialChangeType::FILE_SUBMODULE: - $message = pht('This submodule was added.'); - break; - } - break; - - case DifferentialChangeType::TYPE_DELETE: - switch ($file) { - case DifferentialChangeType::FILE_TEXT: - $message = pht('This file was deleted.'); - break; - case DifferentialChangeType::FILE_IMAGE: - $message = pht('This image was deleted.'); - break; - case DifferentialChangeType::FILE_DIRECTORY: - $message = pht('This directory was deleted.'); - break; - case DifferentialChangeType::FILE_BINARY: - $message = pht('This binary file was deleted.'); - break; - case DifferentialChangeType::FILE_SYMLINK: - $message = pht('This symlink was deleted.'); - break; - case DifferentialChangeType::FILE_SUBMODULE: - $message = pht('This submodule was deleted.'); - break; - } - break; - - case DifferentialChangeType::TYPE_MOVE_HERE: - $from = - "". - phutil_escape_html($changeset->getOldFile()). - ""; - switch ($file) { - case DifferentialChangeType::FILE_TEXT: - $message = pht('This file was moved from %s.', $from); - break; - case DifferentialChangeType::FILE_IMAGE: - $message = pht('This image was moved from %s.', $from); - break; - case DifferentialChangeType::FILE_DIRECTORY: - $message = pht('This directory was moved from %s.', $from); - break; - case DifferentialChangeType::FILE_BINARY: - $message = pht('This binary file was moved from %s.', $from); - break; - case DifferentialChangeType::FILE_SYMLINK: - $message = pht('This symlink was moved from %s.', $from); - break; - case DifferentialChangeType::FILE_SUBMODULE: - $message = pht('This submodule was moved from %s.', $from); - break; - } - break; - - case DifferentialChangeType::TYPE_COPY_HERE: - $from = - "". - phutil_escape_html($changeset->getOldFile()). - ""; - switch ($file) { - case DifferentialChangeType::FILE_TEXT: - $message = pht('This file was copied from %s.', $from); - break; - case DifferentialChangeType::FILE_IMAGE: - $message = pht('This image was copied from %s.', $from); - break; - case DifferentialChangeType::FILE_DIRECTORY: - $message = pht('This directory was copied from %s.', $from); - break; - case DifferentialChangeType::FILE_BINARY: - $message = pht('This binary file was copied from %s.', $from); - break; - case DifferentialChangeType::FILE_SYMLINK: - $message = pht('This symlink was copied from %s.', $from); - break; - case DifferentialChangeType::FILE_SUBMODULE: - $message = pht('This submodule was copied from %s.', $from); - break; - } - break; - - case DifferentialChangeType::TYPE_MOVE_AWAY: - $paths = - "". - phutil_escape_html(implode(', ', $changeset->getAwayPaths())). - ""; - switch ($file) { - case DifferentialChangeType::FILE_TEXT: - $message = pht('This file was moved to %s.', $paths); - break; - case DifferentialChangeType::FILE_IMAGE: - $message = pht('This image was moved to %s.', $paths); - break; - case DifferentialChangeType::FILE_DIRECTORY: - $message = pht('This directory was moved to %s.', $paths); - break; - case DifferentialChangeType::FILE_BINARY: - $message = pht('This binary file was moved to %s.', $paths); - break; - case DifferentialChangeType::FILE_SYMLINK: - $message = pht('This symlink was moved to %s.', $paths); - break; - case DifferentialChangeType::FILE_SUBMODULE: - $message = pht('This submodule was moved to %s.', $paths); - break; - } - break; - - case DifferentialChangeType::TYPE_COPY_AWAY: - $paths = - "". - phutil_escape_html(implode(', ', $changeset->getAwayPaths())). - ""; - switch ($file) { - case DifferentialChangeType::FILE_TEXT: - $message = pht('This file was copied to %s.', $paths); - break; - case DifferentialChangeType::FILE_IMAGE: - $message = pht('This image was copied to %s.', $paths); - break; - case DifferentialChangeType::FILE_DIRECTORY: - $message = pht('This directory was copied to %s.', $paths); - break; - case DifferentialChangeType::FILE_BINARY: - $message = pht('This binary file was copied to %s.', $paths); - break; - case DifferentialChangeType::FILE_SYMLINK: - $message = pht('This symlink was copied to %s.', $paths); - break; - case DifferentialChangeType::FILE_SUBMODULE: - $message = pht('This submodule was copied to %s.', $paths); - break; - } - break; - - case DifferentialChangeType::TYPE_MULTICOPY: - $paths = - "". - phutil_escape_html(implode(', ', $changeset->getAwayPaths())). - ""; - switch ($file) { - case DifferentialChangeType::FILE_TEXT: - $message = pht( - 'This file was deleted after being copied to %s.', - $paths); - break; - case DifferentialChangeType::FILE_IMAGE: - $message = pht( - 'This image was deleted after being copied to %s.', - $paths); - break; - case DifferentialChangeType::FILE_DIRECTORY: - $message = pht( - 'This directory was deleted after being copied to %s.', - $paths); - break; - case DifferentialChangeType::FILE_BINARY: - $message = pht( - 'This binary file was deleted after being copied to %s.', - $paths); - break; - case DifferentialChangeType::FILE_SYMLINK: - $message = pht( - 'This symlink was deleted after being copied to %s.', - $paths); - break; - case DifferentialChangeType::FILE_SUBMODULE: - $message = pht( - 'This submodule was deleted after being copied to %s.', - $paths); - break; - } - break; - - default: - switch ($file) { - case DifferentialChangeType::FILE_TEXT: - $message = pht('This is a file.'); - break; - case DifferentialChangeType::FILE_IMAGE: - $message = pht('This is an image.'); - break; - case DifferentialChangeType::FILE_DIRECTORY: - $message = pht('This is a directory.'); - break; - case DifferentialChangeType::FILE_BINARY: - $message = pht('This is a binary file.'); - break; - case DifferentialChangeType::FILE_SYMLINK: - $message = pht('This is a symlink.'); - break; - case DifferentialChangeType::FILE_SUBMODULE: - $message = pht('This is a submodule.'); - break; - } - break; - } - } - - return - '
'. - $message. - '
'; - } + abstract protected function renderPropertyChangeHeader(); protected function renderInlineComment( PhabricatorInlineCommentInterface $comment, @@ -645,4 +331,174 @@ abstract class DifferentialChangesetRenderer { ->render(); } + protected function buildPrimitives($range_start, $range_len) { + $primitives = array(); + + $hunk_starts = $this->getHunkStartLines(); + + $mask = $this->getMask(); + $gaps = $this->getGaps(); + + $old = $this->getOldLines(); + $new = $this->getNewLines(); + $old_render = $this->getOldRender(); + $new_render = $this->getNewRender(); + $old_comments = $this->getOldComments(); + $new_comments = $this->getNewComments(); + + $size = count($old); + for ($ii = $range_start; $ii < $range_start + $range_len; $ii++) { + if (empty($mask[$ii])) { + list($top, $len) = array_pop($gaps); + $primitives[] = array( + 'type' => 'context', + 'top' => $top, + 'len' => $len, + ); + + $ii += ($len - 1); + continue; + } + + $ospec = array( + 'type' => 'old', + 'htype' => null, + 'cursor' => $ii, + 'line' => null, + 'render' => null, + ); + + $nspec = array( + 'type' => 'new', + 'htype' => null, + 'cursor' => $ii, + 'line' => null, + 'render' => null, + 'copy' => null, + 'coverage' => null, + ); + + if (isset($old[$ii])) { + $ospec['line'] = $old[$ii]['line']; + $ospec['htype'] = $old[$ii]['type']; + if (isset($old_render[$ii])) { + $ospec['render'] = $old_render[$ii]; + } + } + + if (isset($new[$ii])) { + $nspec['line'] = $new[$ii]['line']; + $nspec['htype'] = $new[$ii]['type']; + if (isset($new_render[$ii])) { + $nspec['render'] = $new_render[$ii]; + } + } + + if (isset($hunk_starts[$ospec['line']])) { + $primitives[] = array( + 'type' => 'no-context', + ); + } + + $primitives[] = $ospec; + $primitives[] = $nspec; + + if ($ospec['line'] !== null && isset($old_comments[$ospec['line']])) { + foreach ($old_comments[$ospec['line']] as $comment) { + $primitives[] = array( + 'type' => 'inline', + 'comment' => $comment, + 'right' => false, + ); + } + } + + if ($nspec['line'] !== null && isset($new_comments[$nspec['line']])) { + foreach ($new_comments[$nspec['line']] as $comment) { + $primitives[] = array( + 'type' => 'inline', + 'comment' => 'comment', + 'right' => true, + ); + } + } + + if ($hunk_starts && ($ii == $size - 1)) { + $primitives[] = array( + 'type' => 'no-context', + ); + } + } + + if ($this->isOneUpRenderer()) { + $primitives = $this->processPrimitivesForOneUp($primitives); + } + + return $primitives; + } + + private function processPrimitivesForOneUp(array $primitives) { + // Primitives come out of buildPrimitives() in two-up format, because it + // is the most general, flexible format. To put them into one-up format, + // we need to filter and reorder them. In particular: + // + // - We discard unchanged lines in the old file; in one-up format, we + // render them only once. + // - We group contiguous blocks of old-modified and new-modified lines, so + // they render in "block of old, block of new" order instead of + // alternating old and new lines. + + $out = array(); + + $old_buf = array(); + $new_buf = array(); + foreach ($primitives as $primitive) { + $type = $primitive['type']; + if ($type == 'old') { + if (!$primitive['htype']) { + // This is a line which appears in both the old file and the new + // file, or the spacer corresponding to a line added in the new file. + // Ignore it when rendering a one-up diff. + continue; + } + if ($new_buf) { + $out[] = $new_buf; + $new_buf = array(); + } + $old_buf[] = $primitive; + } else if ($type == 'new') { + if ($primitive['line'] === null) { + // This is an empty spacer corresponding to a line removed from the + // old file. Ignore it when rendering a one-up diff. + continue; + } + if ($old_buf) { + $out[] = $old_buf; + $old_buf = array(); + } + $new_buf[] = $primitive; + } else if ($type == 'context' || $type == 'no-context') { + $out[] = $old_buf; + $out[] = $new_buf; + $old_buf = array(); + $new_buf = array(); + $out[] = array($primitive); + } else if ($type == 'inline') { + if ($primitive['right']) { + $new_buf[] = $primitive; + } else { + $old_buf[] = $primitive; + } + } else { + throw new Exception("Unknown primitive type '{$primitive}'!"); + } + } + + $out[] = $old_buf; + $out[] = $new_buf; + $out = array_mergev($out); + + return $out; + } + } diff --git a/src/applications/differential/render/DifferentialChangesetTestRenderer.php b/src/applications/differential/render/DifferentialChangesetTestRenderer.php new file mode 100644 index 0000000000..202b783463 --- /dev/null +++ b/src/applications/differential/render/DifferentialChangesetTestRenderer.php @@ -0,0 +1,86 @@ +getChangeset(); + + $old = nonempty($changeset->getOldFile(), '-'); + $away = nonempty(implode(', ', $changeset->getAwayPaths()), '-'); + $ctype = $changeset->getChangeType(); + $ftype = $changeset->getFileType(); + $force = ($force ? '(forced)' : '(unforced)'); + + return "CTYPE {$ctype} {$ftype} {$force}\n". + "{$old}\n". + "{$away}\n"; + } + + protected function renderShield($message, $force = 'default') { + return "SHIELD ({$force}) {$message}\n"; + } + + protected function renderPropertyChangeHeader() { + $changeset = $this->getChangeset(); + $old = $changeset->getOldProperties(); + $new = $changeset->getNewProperties(); + + if (!$old && !$new) { + return null; + } + + $props = ''; + foreach ($old as $key => $value) { + $props .= "P - {$key} {$value}~\n"; + } + foreach ($new as $key => $value) { + $props .= "P + {$key} {$value}~\n"; + } + + return "PROPERTIES\n".$props; + } + + public function renderTextChange( + $range_start, + $range_len, + $rows) { + + $out = array(); + + $primitives = $this->buildPrimitives($range_start, $range_len); + foreach ($primitives as $p) { + $type = $p['type']; + switch ($type) { + case 'old': + case 'new': + $num = nonempty($p['line'], '-'); + $render = $p['render']; + $htype = nonempty($p['htype'], '.'); + + // TODO: This should probably happen earlier, whenever we deal with + // \r and \t normalization? + $render = rtrim($render, "\r\n"); + $t = ($type == 'old') ? 'O' : 'N'; + + $out[] = "{$t} {$num} {$htype} {$render}~"; + break; + default: + $out[] = $type; + break; + } + } + + $out = implode("\n", $out)."\n"; + return $out; + } + + + public function renderFileChange($old_file = null, + $new_file = null, + $id = 0, + $vs = 0) { + throw new Exception("Not implemented!"); + } + +} diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index e49cec8bef..2330134884 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -1,42 +1,10 @@ getChangeset(); - $props = $this->renderPropertyChangeHeader($changeset); - $table = null; - if ($contents) { - $table = javelin_render_tag( - 'table', - array( - 'class' => 'differential-diff remarkup-code PhabricatorMonospaced', - 'sigil' => 'differential-diff', - ), - $contents); - } - - if (!$table && !$props) { - $notice = $this->renderChangeTypeHeader($changeset, true); - } else { - $notice = $this->renderChangeTypeHeader($changeset, false); - } - - $result = implode( - "\n", - array( - $notice, - $props, - $table, - )); - - // TODO: Let the user customize their tab width / display style. - $result = str_replace("\t", ' ', $result); - - // TODO: We should possibly post-process "\r" as well. - - return $result; + public function isOneUpRenderer() { + return false; } public function renderTextChange( @@ -360,7 +328,7 @@ final class DifferentialChangesetTwoUpRenderer } } - return implode('', $html); + return $this->wrapChangeInTable(implode('', $html)); } public function renderFileChange($old_file = null, @@ -450,7 +418,7 @@ final class DifferentialChangesetTwoUpRenderer implode('', $html_old). implode('', $html_new)); - return $output; + return $this->wrapChangeInTable($output); } } diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpTestRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpTestRenderer.php new file mode 100644 index 0000000000..e144577db9 --- /dev/null +++ b/src/applications/differential/render/DifferentialChangesetTwoUpTestRenderer.php @@ -0,0 +1,10 @@ +