From 421c2453e5e4bcdc5de97b0f4fd0f72ead89d8a3 Mon Sep 17 00:00:00 2001 From: Giedrius Dubinskas Date: Tue, 20 Oct 2015 19:07:04 +0000 Subject: [PATCH] Truncate long source lines in Paste search result list snippets Summary: An attempt to resolve T9600. - `PhabricatorPasteQuery` builds truncated snippet when requested using `needSnippet()`. - `PhabricatorPasteSearchEngine` uses Paste snippet istead of content. - `PhabricatorSourceCodeView` accepts truncated source and type instead of line limit. Test Plan: Generated some content for Paste application and also added huge JSON oneliner. Checked Paste application pages in browser. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T9600 Differential Revision: https://secure.phabricator.com/D14313 --- src/__phutil_library_map__.php | 2 + .../controller/PhabricatorPasteController.php | 2 - .../PhabricatorPasteViewController.php | 5 +- .../paste/query/PhabricatorPasteQuery.php | 138 ++++++++++++++++-- .../query/PhabricatorPasteSearchEngine.php | 10 +- .../paste/snippet/PhabricatorPasteSnippet.php | 24 +++ .../paste/storage/PhabricatorPaste.php | 10 ++ src/view/layout/PhabricatorSourceCodeView.php | 55 ++++--- 8 files changed, 203 insertions(+), 43 deletions(-) create mode 100644 src/applications/paste/snippet/PhabricatorPasteSnippet.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 89f65a8db0..5c1c071c44 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2552,6 +2552,7 @@ phutil_register_library_map(array( 'PhabricatorPasteRemarkupRule' => 'applications/paste/remarkup/PhabricatorPasteRemarkupRule.php', 'PhabricatorPasteSchemaSpec' => 'applications/paste/storage/PhabricatorPasteSchemaSpec.php', 'PhabricatorPasteSearchEngine' => 'applications/paste/query/PhabricatorPasteSearchEngine.php', + 'PhabricatorPasteSnippet' => 'applications/paste/snippet/PhabricatorPasteSnippet.php', 'PhabricatorPasteTestDataGenerator' => 'applications/paste/lipsum/PhabricatorPasteTestDataGenerator.php', 'PhabricatorPasteTransaction' => 'applications/paste/storage/PhabricatorPasteTransaction.php', 'PhabricatorPasteTransactionComment' => 'applications/paste/storage/PhabricatorPasteTransactionComment.php', @@ -6633,6 +6634,7 @@ phutil_register_library_map(array( 'PhabricatorPasteRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'PhabricatorPasteSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhabricatorPasteSearchEngine' => 'PhabricatorApplicationSearchEngine', + 'PhabricatorPasteSnippet' => 'Phobject', 'PhabricatorPasteTestDataGenerator' => 'PhabricatorTestDataGenerator', 'PhabricatorPasteTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorPasteTransactionComment' => 'PhabricatorApplicationTransactionComment', diff --git a/src/applications/paste/controller/PhabricatorPasteController.php b/src/applications/paste/controller/PhabricatorPasteController.php index 4956e89c28..1f543ac240 100644 --- a/src/applications/paste/controller/PhabricatorPasteController.php +++ b/src/applications/paste/controller/PhabricatorPasteController.php @@ -39,13 +39,11 @@ abstract class PhabricatorPasteController extends PhabricatorController { public function buildSourceCodeView( PhabricatorPaste $paste, - $max_lines = null, $highlights = array()) { $lines = phutil_split_lines($paste->getContent()); return id(new PhabricatorSourceCodeView()) - ->setLimit($max_lines) ->setLines($lines) ->setHighlights($highlights) ->setURI(new PhutilURI($paste->getURI())); diff --git a/src/applications/paste/controller/PhabricatorPasteViewController.php b/src/applications/paste/controller/PhabricatorPasteViewController.php index f8176892bc..46239e7fca 100644 --- a/src/applications/paste/controller/PhabricatorPasteViewController.php +++ b/src/applications/paste/controller/PhabricatorPasteViewController.php @@ -56,10 +56,7 @@ final class PhabricatorPasteViewController extends PhabricatorPasteController { ->setHeader($header) ->addPropertyList($properties); - $source_code = $this->buildSourceCodeView( - $paste, - null, - $this->highlightMap); + $source_code = $this->buildSourceCodeView($paste, $this->highlightMap); require_celerity_resource('paste-css'); $source_code = phutil_tag( diff --git a/src/applications/paste/query/PhabricatorPasteQuery.php b/src/applications/paste/query/PhabricatorPasteQuery.php index 768f0cf20b..2bc9b35168 100644 --- a/src/applications/paste/query/PhabricatorPasteQuery.php +++ b/src/applications/paste/query/PhabricatorPasteQuery.php @@ -10,6 +10,7 @@ final class PhabricatorPasteQuery private $needContent; private $needRawContent; + private $needSnippets; private $languages; private $includeNoLanguage; private $dateCreatedAfter; @@ -47,6 +48,11 @@ final class PhabricatorPasteQuery return $this; } + public function needSnippets($need_snippets) { + $this->needSnippets = $need_snippets; + return $this; + } + public function withLanguages(array $languages) { $this->includeNoLanguage = false; foreach ($languages as $key => $language) { @@ -91,6 +97,10 @@ final class PhabricatorPasteQuery $pastes = $this->loadContent($pastes); } + if ($this->needSnippets) { + $pastes = $this->loadSnippets($pastes); + } + return $pastes; } @@ -166,6 +176,17 @@ final class PhabricatorPasteQuery )); } + private function getSnippetCacheKey(PhabricatorPaste $paste) { + return implode( + ':', + array( + 'P'.$paste->getID(), + $paste->getFilePHID(), + $paste->getLanguage(), + 'snippet', + )); + } + private function loadRawContent(array $pastes) { $file_phids = mpull($pastes, 'getFilePHID'); $files = id(new PhabricatorFileQuery()) @@ -250,19 +271,114 @@ final class PhabricatorPasteQuery return $pastes; } - private function buildContent(PhabricatorPaste $paste) { - $language = $paste->getLanguage(); - $source = $paste->getRawContent(); + private function loadSnippets(array $pastes) { + $cache = new PhabricatorKeyValueDatabaseCache(); - if (empty($language)) { - return PhabricatorSyntaxHighlighter::highlightWithFilename( - $paste->getTitle(), - $source); - } else { - return PhabricatorSyntaxHighlighter::highlightWithLanguage( - $language, - $source); + $cache = new PhutilKeyValueCacheProfiler($cache); + $cache->setProfiler(PhutilServiceProfiler::getInstance()); + + $keys = array(); + foreach ($pastes as $paste) { + $keys[] = $this->getSnippetCacheKey($paste); } + + $caches = $cache->getKeys($keys); + + $need_raw = array(); + $have_cache = array(); + foreach ($pastes as $paste) { + $key = $this->getSnippetCacheKey($paste); + if (isset($caches[$key])) { + $snippet_data = phutil_json_decode($caches[$key], true); + $snippet = new PhabricatorPasteSnippet( + phutil_safe_html($snippet_data['content']), + $snippet_data['type']); + $paste->attachSnippet($snippet); + $have_cache[$paste->getPHID()] = true; + } else { + $need_raw[$key] = $paste; + } + } + + if (!$need_raw) { + return $pastes; + } + + $write_data = array(); + + $have_raw = $this->loadRawContent($need_raw); + $have_raw = mpull($have_raw, null, 'getPHID'); + foreach ($pastes as $key => $paste) { + $paste_phid = $paste->getPHID(); + if (isset($have_cache[$paste_phid])) { + continue; + } + + if (empty($have_raw[$paste_phid])) { + unset($pastes[$key]); + continue; + } + + $snippet = $this->buildSnippet($paste); + $paste->attachSnippet($snippet); + $snippet_data = array( + 'content' => (string)$snippet->getContent(), + 'type' => (string)$snippet->getType(), + ); + $write_data[$this->getSnippetCacheKey($paste)] = phutil_json_encode( + $snippet_data); + } + + if ($write_data) { + $cache->setKeys($write_data); + } + + return $pastes; + } + + private function buildContent(PhabricatorPaste $paste) { + return $this->highlightSource( + $paste->getRawContent(), + $paste->getTitle(), + $paste->getLanguage()); + } + + private function buildSnippet(PhabricatorPaste $paste) { + $snippet_type = PhabricatorPasteSnippet::FULL; + $snippet = $paste->getRawContent(); + + if (strlen($snippet) > 1024) { + $snippet_type = PhabricatorPasteSnippet::FIRST_BYTES; + $snippet = id(new PhutilUTF8StringTruncator()) + ->setMaximumBytes(1024) + ->setTerminator('') + ->truncateString($snippet); + } + + $lines = phutil_split_lines($snippet); + if (count($lines) > 5) { + $snippet_type = PhabricatorPasteSnippet::FIRST_LINES; + $snippet = implode('', array_slice($lines, 0, 5)); + } + + return new PhabricatorPasteSnippet( + $this->highlightSource( + $snippet, + $paste->getTitle(), + $paste->getLanguage()), + $snippet_type); + } + + private function highlightSource($source, $title, $language) { + if (empty($language)) { + return PhabricatorSyntaxHighlighter::highlightWithFilename( + $title, + $source); + } else { + return PhabricatorSyntaxHighlighter::highlightWithLanguage( + $language, + $source); + } } public function getQueryApplicationClass() { diff --git a/src/applications/paste/query/PhabricatorPasteSearchEngine.php b/src/applications/paste/query/PhabricatorPasteSearchEngine.php index e27445d078..0b4e47b497 100644 --- a/src/applications/paste/query/PhabricatorPasteSearchEngine.php +++ b/src/applications/paste/query/PhabricatorPasteSearchEngine.php @@ -13,7 +13,7 @@ final class PhabricatorPasteSearchEngine public function newQuery() { return id(new PhabricatorPasteQuery()) - ->needContent(true); + ->needSnippets(true); } protected function buildQueryFromParameters(array $map) { @@ -136,11 +136,15 @@ final class PhabricatorPasteSearchEngine $created = phabricator_date($paste->getDateCreated(), $viewer); $author = $handles[$paste->getAuthorPHID()]->renderLink(); - $lines = phutil_split_lines($paste->getContent()); + $snippet_type = $paste->getSnippet()->getType(); + $lines = phutil_split_lines($paste->getSnippet()->getContent()); $preview = id(new PhabricatorSourceCodeView()) - ->setLimit(5) ->setLines($lines) + ->setTruncatedFirstBytes( + $snippet_type == PhabricatorPasteSnippet::FIRST_BYTES) + ->setTruncatedFirstLines( + $snippet_type == PhabricatorPasteSnippet::FIRST_LINES) ->setURI(new PhutilURI($paste->getURI())); $source_code = phutil_tag( diff --git a/src/applications/paste/snippet/PhabricatorPasteSnippet.php b/src/applications/paste/snippet/PhabricatorPasteSnippet.php new file mode 100644 index 0000000000..4a001e5955 --- /dev/null +++ b/src/applications/paste/snippet/PhabricatorPasteSnippet.php @@ -0,0 +1,24 @@ +content = $content; + $this->type = $type; + } + + public function getContent() { + return $this->content; + } + + public function getType() { + return $this->type; + } +} diff --git a/src/applications/paste/storage/PhabricatorPaste.php b/src/applications/paste/storage/PhabricatorPaste.php index 0fab56b724..8aeda09aae 100644 --- a/src/applications/paste/storage/PhabricatorPaste.php +++ b/src/applications/paste/storage/PhabricatorPaste.php @@ -28,6 +28,7 @@ final class PhabricatorPaste extends PhabricatorPasteDAO private $content = self::ATTACHABLE; private $rawContent = self::ATTACHABLE; + private $snippet = self::ATTACHABLE; public static function initializeNewPaste(PhabricatorUser $actor) { $app = id(new PhabricatorApplicationQuery()) @@ -135,6 +136,15 @@ final class PhabricatorPaste extends PhabricatorPasteDAO return $this; } + public function getSnippet() { + return $this->assertAttached($this->snippet); + } + + public function attachSnippet(PhabricatorPasteSnippet $snippet) { + $this->snippet = $snippet; + return $this; + } + /* -( PhabricatorSubscribableInterface )----------------------------------- */ diff --git a/src/view/layout/PhabricatorSourceCodeView.php b/src/view/layout/PhabricatorSourceCodeView.php index 7fb3f2ca66..eb34925b3f 100644 --- a/src/view/layout/PhabricatorSourceCodeView.php +++ b/src/view/layout/PhabricatorSourceCodeView.php @@ -3,15 +3,11 @@ final class PhabricatorSourceCodeView extends AphrontView { private $lines; - private $limit; private $uri; private $highlights = array(); private $canClickHighlight = true; - - public function setLimit($limit) { - $this->limit = $limit; - return $this; - } + private $truncatedFirstBytes = false; + private $truncatedFirstLines = false; public function setLines(array $lines) { $this->lines = $lines; @@ -33,6 +29,16 @@ final class PhabricatorSourceCodeView extends AphrontView { return $this; } + public function setTruncatedFirstBytes($truncated_first_bytes) { + $this->truncatedFirstBytes = $truncated_first_bytes; + return $this; + } + + public function setTruncatedFirstLines($truncated_first_lines) { + $this->truncatedFirstLines = $truncated_first_lines; + return $this; + } + public function render() { require_celerity_resource('phabricator-source-code-view-css'); require_celerity_resource('syntax-highlighting-css'); @@ -46,24 +52,31 @@ final class PhabricatorSourceCodeView extends AphrontView { $rows = array(); - foreach ($this->lines as $line) { - $hit_limit = $this->limit && - ($line_number == $this->limit) && - (count($this->lines) != $this->limit); - - if ($hit_limit) { - $content_number = ''; - $content_line = phutil_tag( + $lines = $this->lines; + if ($this->truncatedFirstLines) { + $lines[] = phutil_tag( 'span', array( 'class' => 'c', ), pht('...')); - } else { - $content_number = $line_number; - // NOTE: See phabricator-oncopy behavior. - $content_line = hsprintf("\xE2\x80\x8B%s", $line); - } + } else if ($this->truncatedFirstBytes) { + $last_key = last_key($lines); + $lines[$last_key] = hsprintf( + '%s%s', + $lines[$last_key], + phutil_tag( + 'span', + array( + 'class' => 'c', + ), + pht('...'))); + } + + foreach ($lines as $line) { + + // NOTE: See phabricator-oncopy behavior. + $content_line = hsprintf("\xE2\x80\x8B%s", $line); $row_attributes = array(); if (isset($this->highlights[$line_number])) { @@ -106,10 +119,6 @@ final class PhabricatorSourceCodeView extends AphrontView { $content_line), )); - if ($hit_limit) { - break; - } - $line_number++; }