From 21826ed7b35a54db1ba32fb7ab72b086518a1ea0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 29 Jun 2014 12:07:46 -0700 Subject: [PATCH] Don't highlight very large files by default Summary: Ref T5644. See some discussion in D8040. When a file is very large (more than 64KB of text), don't activate syntax highlighting by default. This should prevent us from wasting resources running `pygmentize` on enormous files. Users who want the file highlighted can still select "Highlight As...". The tricky part of this diff is separating the headers into "changeset" headers and "undershield" (rendering) headers. Specifically, a file might have these headers/shields: - "This file is newly added." - "This file is generated. Show Changes" - "Highlighting is disabled for this large file." In this case, I want the user to see "added" and "generated" when they load the page, and only see "highlighting disabled" after they click "Show Changes". So there are several categories: - "Changeset" headers, which discuss the changeset as a whole (binary file, image file, moved, added, deleted, etc.) - "Property" headers, which describe metadata changes (not relevant here). - "Shields", which hide files from view by default. - "Undershield" headers, which provide rendering information that is only relevant if there is no shield on the file. Test Plan: - Viewed a diff with the library map, clicked "show changes", got a "highlighting disabled" header back with highlighting disabled. - Enabled highlighting explicitly (this currently restores the shield, which it probably shouldn't, but that feels out of scope for this change). The deshielded file is highlighted per the user's request. - Loaded context on normal files. Reviewers: btrahan Reviewed By: btrahan Subscribers: joshuaspence, epriestley Maniphest Tasks: T5644 Differential Revision: https://secure.phabricator.com/D12132 --- resources/celerity/map.php | 24 +++++++-------- .../parser/DifferentialChangesetParser.php | 23 ++++++++++++++- .../DifferentialChangesetHTMLRenderer.php | 21 ++++++++++++++ .../render/DifferentialChangesetRenderer.php | 29 ++++++++++++++++++- .../DifferentialChangesetTestRenderer.php | 4 +++ .../differential/ChangesetViewManager.js | 14 ++++++++- 6 files changed, 100 insertions(+), 15 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 10afe8a170..7c7c81c9f1 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -363,7 +363,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '82439934', 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', - 'rsrc/js/application/differential/ChangesetViewManager.js' => '88be0133', + 'rsrc/js/application/differential/ChangesetViewManager.js' => '58562350', 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => 'f2431bc1', 'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => 'e10f8e18', 'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d', @@ -510,7 +510,7 @@ return array( 'aphront-two-column-view-css' => '16ab3ad2', 'aphront-typeahead-control-css' => '0e403212', 'auth-css' => '1e655982', - 'changeset-view-manager' => '88be0133', + 'changeset-view-manager' => '58562350', 'config-options-css' => '7fedf08b', 'config-welcome-css' => '6abd79be', 'conpherence-durable-column-view' => 'a27580c5', @@ -1202,6 +1202,16 @@ return array( 'javelin-vector', 'javelin-dom', ), + 58562350 => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + ), '59b251eb' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1492,16 +1502,6 @@ return array( 'javelin-stratcom', 'javelin-dom', ), - '88be0133' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - ), '88f0c5b3' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 3c78415152..f985a8657c 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -2,6 +2,8 @@ final class DifferentialChangesetParser { + const HIGHLIGHT_BYTE_LIMIT = 262144; + protected $visible = array(); protected $new = array(); protected $old = array(); @@ -36,6 +38,7 @@ final class DifferentialChangesetParser { private $isSubparser; private $isTopLevel; + private $coverage; private $markupEngine; private $highlightErrors; @@ -43,6 +46,7 @@ final class DifferentialChangesetParser { private $renderer; private $characterEncoding; private $highlightAs; + private $highlightingDisabled; private $showEditAndReplyLinks = true; private $canMarkDone; @@ -69,6 +73,7 @@ final class DifferentialChangesetParser { $this->showEditAndReplyLinks = $bool; return $this; } + public function getShowEditAndReplyLinks() { return $this->showEditAndReplyLinks; } @@ -416,6 +421,7 @@ final class DifferentialChangesetParser { 'hunkStartLines', 'cacheVersion', 'cacheHost', + 'highlightingDisabled', ); } @@ -537,6 +543,12 @@ final class DifferentialChangesetParser { if (!$language) { $language = $this->highlightEngine->getLanguageFromFilename( $this->filename); + + if (($language != 'txt') && + (strlen($corpus) > self::HIGHLIGHT_BYTE_LIMIT)) { + $this->highlightingDisabled = true; + $language = 'txt'; + } } return $this->highlightEngine->getHighlightFuture( @@ -830,7 +842,8 @@ final class DifferentialChangesetParser { ->setNewLines($this->new) ->setOriginalCharacterEncoding($encoding) ->setShowEditAndReplyLinks($this->getShowEditAndReplyLinks()) - ->setCanMarkDone($this->getCanMarkDone()); + ->setCanMarkDone($this->getCanMarkDone()) + ->setHighlightingDisabled($this->highlightingDisabled); $shield = null; if ($this->isTopLevel && !$this->comments) { @@ -894,6 +907,14 @@ final class DifferentialChangesetParser { return $renderer->renderChangesetTable($shield); } + // This request should render the "undershield" headers if it's a top-level + // request which made it this far (indicating the changeset has no shield) + // or it's a request with no mask information (indicating it's the request + // that removes the rendering shield). Possibly, this second class of + // request might need to be made more explicit. + $is_undershield = (empty($mask_force) || $this->isTopLevel); + $renderer->setIsUndershield($is_undershield); + $old_comments = array(); $new_comments = array(); $old_mask = array(); diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index 3eef085555..f8c9b5592e 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -242,6 +242,16 @@ abstract class DifferentialChangesetHTMLRenderer break; } + return $this->formatHeaderMessages($messages); + } + + protected function renderUndershieldHeader() { + $messages = array(); + + $changeset = $this->getChangeset(); + + $file = $changeset->getFileType(); + // If this is a text file with at least one hunk, we may have converted // the text encoding. In this case, show a note. $show_encoding = ($file == DifferentialChangeType::FILE_TEXT) && @@ -261,6 +271,17 @@ abstract class DifferentialChangesetHTMLRenderer } } + if ($this->getHighlightingDisabled()) { + $messages[] = pht( + 'This file is larger than %s, so syntax highlighting is '. + 'disabled by default.', + phutil_format_bytes(DifferentialChangesetParser::HIGHLIGHT_BYTE_LIMIT)); + } + + return $this->formatHeaderMessages($messages); + } + + private function formatHeaderMessages(array $messages) { if (!$messages) { return null; } diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index 7fa5b37f67..1960d4906a 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -7,6 +7,7 @@ abstract class DifferentialChangesetRenderer { private $renderingReference; private $renderPropertyChangeHeader; private $isTopLevel; + private $isUndershield; private $hunkStartLines; private $oldLines; private $newLines; @@ -31,6 +32,7 @@ abstract class DifferentialChangesetRenderer { private $originalCharacterEncoding; private $showEditAndReplyLinks; private $canMarkDone; + private $highlightingDisabled; private $oldFile = false; private $newFile = false; @@ -41,10 +43,20 @@ abstract class DifferentialChangesetRenderer { $this->showEditAndReplyLinks = $bool; return $this; } + public function getShowEditAndReplyLinks() { return $this->showEditAndReplyLinks; } + public function setHighlightingDisabled($highlighting_disabled) { + $this->highlightingDisabled = $highlighting_disabled; + return $this; + } + + public function getHighlightingDisabled() { + return $this->highlightingDisabled; + } + public function setOriginalCharacterEncoding($original_character_encoding) { $this->originalCharacterEncoding = $original_character_encoding; return $this; @@ -54,6 +66,15 @@ abstract class DifferentialChangesetRenderer { return $this->originalCharacterEncoding; } + public function setIsUndershield($is_undershield) { + $this->isUndershield = $is_undershield; + return $this; + } + + public function getIsUndershield() { + return $this->isUndershield; + } + public function setDepths($depths) { $this->depths = $depths; return $this; @@ -325,7 +346,12 @@ abstract class DifferentialChangesetRenderer { $notice = $this->renderChangeTypeHeader($force); } - $result = $notice.$props.$content; + $undershield = null; + if ($this->getIsUndershield()) { + $undershield = $this->renderUndershieldHeader(); + } + + $result = $notice.$props.$undershield.$content; // TODO: Let the user customize their tab width / display style. // TODO: We should possibly post-process "\r" as well. @@ -347,6 +373,7 @@ abstract class DifferentialChangesetRenderer { $vs = 0); abstract protected function renderChangeTypeHeader($force); + abstract protected function renderUndershieldHeader(); protected function didRenderChangesetTableContents($contents) { return $contents; diff --git a/src/applications/differential/render/DifferentialChangesetTestRenderer.php b/src/applications/differential/render/DifferentialChangesetTestRenderer.php index 3d9c8e701d..429dbeda0a 100644 --- a/src/applications/differential/render/DifferentialChangesetTestRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTestRenderer.php @@ -20,6 +20,10 @@ abstract class DifferentialChangesetTestRenderer "{$away}\n"; } + protected function renderUndershieldHeader() { + return null; + } + public function renderShield($message, $force = 'default') { return "SHIELD ({$force}) {$message}\n"; } diff --git a/webroot/rsrc/js/application/differential/ChangesetViewManager.js b/webroot/rsrc/js/application/differential/ChangesetViewManager.js index 02a1a41b02..588403e9ae 100644 --- a/webroot/rsrc/js/application/differential/ChangesetViewManager.js +++ b/webroot/rsrc/js/application/differential/ChangesetViewManager.js @@ -190,7 +190,19 @@ JX.install('ChangesetViewManager', { * Receive a response to a context request. */ _oncontext: function(target, response) { - var table = JX.$H(response.changeset).getNode(); + // TODO: This should be better structured. + // If the response comes back with several top-level nodes, the last one + // is the actual context; the others are headers. Add any headers first, + // then copy the new rows into the document. + var markup = JX.$H(response.changeset).getFragment(); + var len = markup.childNodes.length; + var diff = JX.DOM.findAbove(target, 'table', 'differential-diff'); + + for (var ii = 0; ii < len - 1; ii++) { + diff.parentNode.insertBefore(markup.firstChild, diff); + } + + var table = markup.firstChild; var root = target.parentNode; this._moveRows(table, root, target); root.removeChild(target);