From 7e43b740551265378097dba614c0eda0f07ed01c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 15 Mar 2018 13:34:34 -0700 Subject: [PATCH 01/20] Give all commands from DiffusionCommandEngine a default 15 minute timeout Summary: Ref T13108. See PHI364. See the task and issue for discussion. If a `git fetch` during synchronization hangs, the whole node currently hangs. While the causes of a `git fetch` hang aren't clear, we don't expect synchronization to ever reasonably take more than 15 minutes, so add a default timeout. Test Plan: Will deploy and observe; this is difficult to reproduce or test directly. Maniphest Tasks: T13108 Differential Revision: https://secure.phabricator.com/D19235 --- .../diffusion/protocol/DiffusionCommandEngine.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/applications/diffusion/protocol/DiffusionCommandEngine.php b/src/applications/diffusion/protocol/DiffusionCommandEngine.php index 53a086db33..d7da62b11d 100644 --- a/src/applications/diffusion/protocol/DiffusionCommandEngine.php +++ b/src/applications/diffusion/protocol/DiffusionCommandEngine.php @@ -135,6 +135,11 @@ abstract class DiffusionCommandEngine extends Phobject { $future->setEnv($env); + // See T13108. By default, don't let any cluster command run indefinitely + // to try to avoid cases where `git fetch` hangs for some reason and we're + // left sitting with a held lock forever. + $future->setTimeout(phutil_units('15 minutes in seconds')); + return $future; } From c5e4bd8187c6a0529b1a509df8b5d43036e17a3b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 18 Mar 2018 14:33:40 -0700 Subject: [PATCH 02/20] Fix some minor errors (DarkConsole warning, unstable Ferret sort) Summary: DarkConsole could warn when "Analyze Query Plans" was not active. `msort()` is not stable, so Ferret results with similar relevance could be returned out-of-order. Test Plan: Saw fewer traces and more-stable result ordering. Differential Revision: https://secure.phabricator.com/D19236 --- src/applications/console/plugin/DarkConsoleServicesPlugin.php | 2 +- .../fulltextstorage/PhabricatorFerretFulltextStorageEngine.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/console/plugin/DarkConsoleServicesPlugin.php b/src/applications/console/plugin/DarkConsoleServicesPlugin.php index a14ed4b541..4a26665e0a 100644 --- a/src/applications/console/plugin/DarkConsoleServicesPlugin.php +++ b/src/applications/console/plugin/DarkConsoleServicesPlugin.php @@ -279,7 +279,7 @@ final class DarkConsoleServicesPlugin extends DarkConsolePlugin { $analysis, ); - if ($row['trace']) { + if (isset($row['trace'])) { $rows[] = array( null, null, diff --git a/src/applications/search/fulltextstorage/PhabricatorFerretFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorFerretFulltextStorageEngine.php index a7d9570c57..fa03bcafdd 100644 --- a/src/applications/search/fulltextstorage/PhabricatorFerretFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorFerretFulltextStorageEngine.php @@ -104,7 +104,7 @@ final class PhabricatorFerretFulltextStorageEngine // Reorder the results so that the highest-ranking results come first, // no matter which object types they belong to. - $metadata = msort($metadata, 'getRelevanceSortVector'); + $metadata = msortv($metadata, 'getRelevanceSortVector'); $list = array_select_keys($list, array_keys($metadata)) + $list; $result_slice = array_slice($list, $offset, $limit, true); From 01f22a8d06c9a85dc6bf2deebfab55281153a851 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Mar 2018 09:00:48 -0700 Subject: [PATCH 03/20] Roughly modularize document rendering in Files Summary: Ref T13105. This change begins modularizing document rendering. I'm starting in Files since it's the use case with the smallest amount of complexity. Currently, we hard-coding the inline rendering for images, audio, and video. Instead, use the modular engine pattern to make rendering flexible and extensible. There aren't any options for switching modes yet and none of the renderers do anything fancy. This API is also probably very unstable. Test Plan: Viewwed images, audio, video, and other files. Saw reasonable renderings, with "nothing can render this" for any other file type. Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19237 --- resources/celerity/map.php | 6 +- src/__phutil_library_map__.php | 12 ++ .../PhabricatorFileInfoController.php | 122 +++++++++--------- .../PhabricatorAudioDocumentEngine.php | 61 +++++++++ .../document/PhabricatorDocumentEngine.php | 62 +++++++++ .../files/document/PhabricatorDocumentRef.php | 93 +++++++++++++ .../PhabricatorImageDocumentEngine.php | 63 +++++++++ .../PhabricatorVideoDocumentEngine.php | 61 +++++++++ .../PhabricatorVoidDocumentEngine.php | 34 +++++ src/view/phui/PHUIHeaderView.php | 11 +- .../rsrc/css/phui/phui-property-list-view.css | 47 ++++--- 11 files changed, 485 insertions(+), 87 deletions(-) create mode 100644 src/applications/files/document/PhabricatorAudioDocumentEngine.php create mode 100644 src/applications/files/document/PhabricatorDocumentEngine.php create mode 100644 src/applications/files/document/PhabricatorDocumentRef.php create mode 100644 src/applications/files/document/PhabricatorImageDocumentEngine.php create mode 100644 src/applications/files/document/PhabricatorVideoDocumentEngine.php create mode 100644 src/applications/files/document/PhabricatorVoidDocumentEngine.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index f5a5a57904..88a4a366d7 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => 'c218ed53', + 'core.pkg.css' => '6a8ba174', 'core.pkg.js' => '8581cd02', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -168,7 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => '9cff003c', 'rsrc/css/phui/phui-pager.css' => 'edcbc226', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '2dc7993f', + 'rsrc/css/phui/phui-property-list-view.css' => '79fc3a02', 'rsrc/css/phui/phui-remarkup-preview.css' => '54a34863', 'rsrc/css/phui/phui-segment-bar-view.css' => 'b1d1b892', 'rsrc/css/phui/phui-spacing.css' => '042804d6', @@ -848,7 +848,7 @@ return array( 'phui-oi-simple-ui-css' => 'a8beebea', 'phui-pager-css' => 'edcbc226', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '2dc7993f', + 'phui-property-list-view-css' => '79fc3a02', 'phui-remarkup-preview-css' => '54a34863', 'phui-segment-bar-view-css' => 'b1d1b892', 'phui-spacing-css' => '042804d6', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4caa7f563d..5fff5b53df 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2066,6 +2066,7 @@ phutil_register_library_map(array( 'PhabricatorAsanaConfigOptions' => 'applications/doorkeeper/option/PhabricatorAsanaConfigOptions.php', 'PhabricatorAsanaSubtaskHasObjectEdgeType' => 'applications/doorkeeper/edge/PhabricatorAsanaSubtaskHasObjectEdgeType.php', 'PhabricatorAsanaTaskHasObjectEdgeType' => 'applications/doorkeeper/edge/PhabricatorAsanaTaskHasObjectEdgeType.php', + 'PhabricatorAudioDocumentEngine' => 'applications/files/document/PhabricatorAudioDocumentEngine.php', 'PhabricatorAuditActionConstants' => 'applications/audit/constants/PhabricatorAuditActionConstants.php', 'PhabricatorAuditApplication' => 'applications/audit/application/PhabricatorAuditApplication.php', 'PhabricatorAuditCommentEditor' => 'applications/audit/editor/PhabricatorAuditCommentEditor.php', @@ -2808,6 +2809,8 @@ phutil_register_library_map(array( 'PhabricatorDividerEditField' => 'applications/transactions/editfield/PhabricatorDividerEditField.php', 'PhabricatorDividerProfileMenuItem' => 'applications/search/menuitem/PhabricatorDividerProfileMenuItem.php', 'PhabricatorDivinerApplication' => 'applications/diviner/application/PhabricatorDivinerApplication.php', + 'PhabricatorDocumentEngine' => 'applications/files/document/PhabricatorDocumentEngine.php', + 'PhabricatorDocumentRef' => 'applications/files/document/PhabricatorDocumentRef.php', 'PhabricatorDoorkeeperApplication' => 'applications/doorkeeper/application/PhabricatorDoorkeeperApplication.php', 'PhabricatorDraft' => 'applications/draft/storage/PhabricatorDraft.php', 'PhabricatorDraftDAO' => 'applications/draft/storage/PhabricatorDraftDAO.php', @@ -3155,6 +3158,7 @@ phutil_register_library_map(array( 'PhabricatorIconSet' => 'applications/files/iconset/PhabricatorIconSet.php', 'PhabricatorIconSetEditField' => 'applications/transactions/editfield/PhabricatorIconSetEditField.php', 'PhabricatorIconSetIcon' => 'applications/files/iconset/PhabricatorIconSetIcon.php', + 'PhabricatorImageDocumentEngine' => 'applications/files/document/PhabricatorImageDocumentEngine.php', 'PhabricatorImageMacroRemarkupRule' => 'applications/macro/markup/PhabricatorImageMacroRemarkupRule.php', 'PhabricatorImageRemarkupRule' => 'applications/files/markup/PhabricatorImageRemarkupRule.php', 'PhabricatorImageTransformer' => 'applications/files/PhabricatorImageTransformer.php', @@ -4481,7 +4485,9 @@ phutil_register_library_map(array( 'PhabricatorVCSResponse' => 'applications/repository/response/PhabricatorVCSResponse.php', 'PhabricatorVersionedDraft' => 'applications/draft/storage/PhabricatorVersionedDraft.php', 'PhabricatorVeryWowEnglishTranslation' => 'infrastructure/internationalization/translation/PhabricatorVeryWowEnglishTranslation.php', + 'PhabricatorVideoDocumentEngine' => 'applications/files/document/PhabricatorVideoDocumentEngine.php', 'PhabricatorViewerDatasource' => 'applications/people/typeahead/PhabricatorViewerDatasource.php', + 'PhabricatorVoidDocumentEngine' => 'applications/files/document/PhabricatorVoidDocumentEngine.php', 'PhabricatorWatcherHasObjectEdgeType' => 'applications/transactions/edges/PhabricatorWatcherHasObjectEdgeType.php', 'PhabricatorWebContentSource' => 'infrastructure/contentsource/PhabricatorWebContentSource.php', 'PhabricatorWebServerSetupCheck' => 'applications/config/check/PhabricatorWebServerSetupCheck.php', @@ -7497,6 +7503,7 @@ phutil_register_library_map(array( 'PhabricatorAsanaConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorAsanaSubtaskHasObjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorAsanaTaskHasObjectEdgeType' => 'PhabricatorEdgeType', + 'PhabricatorAudioDocumentEngine' => 'PhabricatorDocumentEngine', 'PhabricatorAuditActionConstants' => 'Phobject', 'PhabricatorAuditApplication' => 'PhabricatorApplication', 'PhabricatorAuditCommentEditor' => 'PhabricatorEditor', @@ -8362,6 +8369,8 @@ phutil_register_library_map(array( 'PhabricatorDividerEditField' => 'PhabricatorEditField', 'PhabricatorDividerProfileMenuItem' => 'PhabricatorProfileMenuItem', 'PhabricatorDivinerApplication' => 'PhabricatorApplication', + 'PhabricatorDocumentEngine' => 'Phobject', + 'PhabricatorDocumentRef' => 'Phobject', 'PhabricatorDoorkeeperApplication' => 'PhabricatorApplication', 'PhabricatorDraft' => 'PhabricatorDraftDAO', 'PhabricatorDraftDAO' => 'PhabricatorLiskDAO', @@ -8756,6 +8765,7 @@ phutil_register_library_map(array( 'PhabricatorIconSet' => 'Phobject', 'PhabricatorIconSetEditField' => 'PhabricatorEditField', 'PhabricatorIconSetIcon' => 'Phobject', + 'PhabricatorImageDocumentEngine' => 'PhabricatorDocumentEngine', 'PhabricatorImageMacroRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorImageRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorImageTransformer' => 'Phobject', @@ -10330,7 +10340,9 @@ phutil_register_library_map(array( 'PhabricatorVCSResponse' => 'AphrontResponse', 'PhabricatorVersionedDraft' => 'PhabricatorDraftDAO', 'PhabricatorVeryWowEnglishTranslation' => 'PhutilTranslation', + 'PhabricatorVideoDocumentEngine' => 'PhabricatorDocumentEngine', 'PhabricatorViewerDatasource' => 'PhabricatorTypeaheadDatasource', + 'PhabricatorVoidDocumentEngine' => 'PhabricatorDocumentEngine', 'PhabricatorWatcherHasObjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorWebContentSource' => 'PhabricatorContentSource', 'PhabricatorWebServerSetupCheck' => 'PhabricatorSetupCheck', diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php index 976324d0b2..fe8d37ab80 100644 --- a/src/applications/files/controller/PhabricatorFileInfoController.php +++ b/src/applications/files/controller/PhabricatorFileInfoController.php @@ -23,6 +23,7 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { } return id(new AphrontRedirectResponse())->setURI($file->getInfoURI()); } + $file = id(new PhabricatorFileQuery()) ->setViewer($viewer) ->withIDs(array($id)) @@ -62,31 +63,34 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { $timeline = $this->buildTransactionView($file); $crumbs = $this->buildApplicationCrumbs(); $crumbs->addTextCrumb( - 'F'.$file->getID(), - $this->getApplicationURI("/info/{$phid}/")); + $file->getMonogram(), + $file->getInfoURI()); $crumbs->setBorder(true); $object_box = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('File')) + ->setHeaderText(pht('File Metadata')) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY); $this->buildPropertyViews($object_box, $file); $title = $file->getName(); + $file_content = $this->newFileContent($file); + $view = id(new PHUITwoColumnView()) ->setHeader($header) ->setCurtain($curtain) - ->setMainColumn(array( - $object_box, - $timeline, - )); + ->setMainColumn( + array( + $object_box, + $file_content, + $timeline, + )); return $this->newPage() ->setTitle($title) ->setCrumbs($crumbs) ->setPageObjectPHIDs(array($file->getPHID())) ->appendChild($view); - } private function buildTransactionView(PhabricatorFile $file) { @@ -325,61 +329,6 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { $viewer->renderHandleList($phids)); } - if ($file->isViewableImage()) { - $image = phutil_tag( - 'img', - array( - 'src' => $file->getViewURI(), - 'class' => 'phui-property-list-image', - )); - - $linked_image = phutil_tag( - 'a', - array( - 'href' => $file->getViewURI(), - ), - $image); - - $media = id(new PHUIPropertyListView()) - ->addImageContent($linked_image); - - $box->addPropertyList($media); - } else if ($file->isVideo()) { - $video = phutil_tag( - 'video', - array( - 'controls' => 'controls', - 'class' => 'phui-property-list-video', - ), - phutil_tag( - 'source', - array( - 'src' => $file->getViewURI(), - 'type' => $file->getMimeType(), - ))); - $media = id(new PHUIPropertyListView()) - ->addImageContent($video); - - $box->addPropertyList($media); - } else if ($file->isAudio()) { - $audio = phutil_tag( - 'audio', - array( - 'controls' => 'controls', - 'class' => 'phui-property-list-audio', - ), - phutil_tag( - 'source', - array( - 'src' => $file->getViewURI(), - 'type' => $file->getMimeType(), - ))); - $media = id(new PHUIPropertyListView()) - ->addImageContent($audio); - - $box->addPropertyList($media); - } - $engine = $this->loadStorageEngine($file); if ($engine) { if ($engine->isChunkEngine()) { @@ -453,5 +402,52 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { return $engine; } + private function newFileContent(PhabricatorFile $file) { + $viewer = $this->getViewer(); + $engines = PhabricatorDocumentEngine::getAllEngines(); + + $ref = id(new PhabricatorDocumentRef()) + ->setFile($file); + + foreach ($engines as $key => $engine) { + $engine = id(clone $engine) + ->setViewer($viewer); + + if (!$engine->canRenderDocument($ref)) { + unset($engines[$key]); + continue; + } + + $engines[$key] = $engine; + } + + if (!$engines) { + throw new Exception(pht('No engine can render this document.')); + } + + $vectors = array(); + foreach ($engines as $key => $usable_engine) { + $vectors[$key] = $usable_engine->newSortVector($ref); + } + $vectors = msortv($vectors, 'getSelf'); + + $engine = $engines[head_key($vectors)]; + + $content = $engine->newDocument($ref); + if (!$content) { + return null; + } + + $icon = $engine->newDocumentIcon($ref); + + $header = id(new PHUIHeaderView()) + ->setHeaderIcon($icon) + ->setHeader($ref->getName()); + + return id(new PHUIObjectBoxView()) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setHeader($header) + ->appendChild($content); + } } diff --git a/src/applications/files/document/PhabricatorAudioDocumentEngine.php b/src/applications/files/document/PhabricatorAudioDocumentEngine.php new file mode 100644 index 0000000000..afbc10e70b --- /dev/null +++ b/src/applications/files/document/PhabricatorAudioDocumentEngine.php @@ -0,0 +1,61 @@ +getFile(); + if ($file) { + return $file->isAudio(); + } + + $viewable_types = PhabricatorEnv::getEnvConfig('files.viewable-mime-types'); + $viewable_types = array_keys($viewable_types); + + $audio_types = PhabricatorEnv::getEnvConfig('files.audio-mime-types'); + $audio_types = array_keys($audio_types); + + return + $ref->hasAnyMimeType($viewable_types) && + $ref->hasAnyMimeType($audio_types); + } + + protected function newDocumentContent(PhabricatorDocumentRef $ref) { + $file = $ref->getFile(); + if ($file) { + $source_uri = $file->getViewURI(); + } else { + throw new PhutilMethodNotImplementedException(); + } + + $mime_type = $ref->getMimeType(); + + $audio = phutil_tag( + 'audio', + array( + 'controls' => 'controls', + ), + phutil_tag( + 'source', + array( + 'src' => $source_uri, + 'type' => $mime_type, + ))); + + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-audio', + ), + $audio); + + return $container; + } + +} diff --git a/src/applications/files/document/PhabricatorDocumentEngine.php b/src/applications/files/document/PhabricatorDocumentEngine.php new file mode 100644 index 0000000000..0c72ce9fd1 --- /dev/null +++ b/src/applications/files/document/PhabricatorDocumentEngine.php @@ -0,0 +1,62 @@ +viewer = $viewer; + return $this; + } + + final public function getViewer() { + return $this->viewer; + } + + final public function canRenderDocument(PhabricatorDocumentRef $ref) { + return $this->canRenderDocumentType($ref); + } + + abstract protected function canRenderDocumentType( + PhabricatorDocumentRef $ref); + + final public function newDocument(PhabricatorDocumentRef $ref) { + return $this->newDocumentContent($ref); + } + + final public function newDocumentIcon(PhabricatorDocumentRef $ref) { + return id(new PHUIIconView()) + ->setIcon($this->getDocumentIconIcon($ref)); + } + + abstract protected function newDocumentContent( + PhabricatorDocumentRef $ref); + + protected function getDocumentIconIcon(PhabricatorDocumentRef $ref) { + return 'fa-file-o'; + } + + final public function getDocumentEngineKey() { + return $this->getPhobjectClassConstant('ENGINEKEY'); + } + + final public static function getAllEngines() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->setUniqueMethod('getDocumentEngineKey') + ->execute(); + } + + final public function newSortVector(PhabricatorDocumentRef $ref) { + $content_score = $this->getContentScore($ref); + + return id(new PhutilSortVector()) + ->addInt(-$content_score); + } + + protected function getContentScore() { + return 2000; + } + +} diff --git a/src/applications/files/document/PhabricatorDocumentRef.php b/src/applications/files/document/PhabricatorDocumentRef.php new file mode 100644 index 0000000000..cb36cbaed7 --- /dev/null +++ b/src/applications/files/document/PhabricatorDocumentRef.php @@ -0,0 +1,93 @@ +file = $file; + return $this; + } + + public function getFile() { + return $this->file; + } + + public function setMimeType($mime_type) { + $this->mimeType = $mime_type; + return $this; + } + + public function getMimeType() { + if ($this->mimeType !== null) { + return $this->mimeType; + } + + if ($this->file) { + return $this->file->getMimeType(); + } + + return null; + } + + public function setName($name) { + $this->name = $name; + return $this; + } + + public function getName() { + if ($this->name !== null) { + return $this->name; + } + + if ($this->file) { + return $this->file->getName(); + } + + return null; + } + + public function setByteLength($length) { + $this->byteLength = $length; + return $this; + } + + public function getLength() { + if ($this->byteLength !== null) { + return $this->byteLength; + } + + if ($this->file) { + return (int)$this->file->getByteSize(); + } + + return null; + } + + public function hasAnyMimeType(array $candidate_types) { + $mime_full = $this->getMimeType(); + $mime_parts = explode(';', $mime_full); + + $mime_type = head($mime_parts); + $mime_type = $this->normalizeMimeType($mime_type); + + foreach ($candidate_types as $candidate_type) { + if ($this->normalizeMimeType($candidate_type) === $mime_type) { + return true; + } + } + + return false; + } + + private function normalizeMimeType($mime_type) { + $mime_type = trim($mime_type); + $mime_type = phutil_utf8_strtolower($mime_type); + return $mime_type; + } + +} diff --git a/src/applications/files/document/PhabricatorImageDocumentEngine.php b/src/applications/files/document/PhabricatorImageDocumentEngine.php new file mode 100644 index 0000000000..ab5753680b --- /dev/null +++ b/src/applications/files/document/PhabricatorImageDocumentEngine.php @@ -0,0 +1,63 @@ +getFile(); + if ($file) { + return $file->isViewableImage(); + } + + $viewable_types = PhabricatorEnv::getEnvConfig('files.viewable-mime-types'); + $viewable_types = array_keys($viewable_types); + + $image_types = PhabricatorEnv::getEnvConfig('files.image-mime-types'); + $image_types = array_keys($image_types); + + return + $ref->hasAnyMimeType($viewable_types) && + $ref->hasAnyMimeType($image_types); + } + + protected function newDocumentContent(PhabricatorDocumentRef $ref) { + $file = $ref->getFile(); + if ($file) { + $source_uri = $file->getViewURI(); + } else { + // We could use a "data:" URI here. It's not yet clear if or when we'll + // have a ref but no backing file. + throw new PhutilMethodNotImplementedException(); + } + + $image = phutil_tag( + 'img', + array( + 'src' => $source_uri, + )); + + $linked_image = phutil_tag( + 'a', + array( + 'href' => $source_uri, + 'rel' => 'noreferrer', + ), + $image); + + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-image', + ), + $linked_image); + + return $container; + } + +} diff --git a/src/applications/files/document/PhabricatorVideoDocumentEngine.php b/src/applications/files/document/PhabricatorVideoDocumentEngine.php new file mode 100644 index 0000000000..81573b6e93 --- /dev/null +++ b/src/applications/files/document/PhabricatorVideoDocumentEngine.php @@ -0,0 +1,61 @@ +getFile(); + if ($file) { + return $file->isVideo(); + } + + $viewable_types = PhabricatorEnv::getEnvConfig('files.viewable-mime-types'); + $viewable_types = array_keys($viewable_types); + + $video_types = PhabricatorEnv::getEnvConfig('files.video-mime-types'); + $video_types = array_keys($video_types); + + return + $ref->hasAnyMimeType($viewable_types) && + $ref->hasAnyMimeType($video_types); + } + + protected function newDocumentContent(PhabricatorDocumentRef $ref) { + $file = $ref->getFile(); + if ($file) { + $source_uri = $file->getViewURI(); + } else { + throw new PhutilMethodNotImplementedException(); + } + + $mime_type = $ref->getMimeType(); + + $video = phutil_tag( + 'video', + array( + 'controls' => 'controls', + ), + phutil_tag( + 'source', + array( + 'src' => $source_uri, + 'type' => $mime_type, + ))); + + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-video', + ), + $video); + + return $container; + } + +} diff --git a/src/applications/files/document/PhabricatorVoidDocumentEngine.php b/src/applications/files/document/PhabricatorVoidDocumentEngine.php new file mode 100644 index 0000000000..a57514edb8 --- /dev/null +++ b/src/applications/files/document/PhabricatorVoidDocumentEngine.php @@ -0,0 +1,34 @@ + 'document-engine-message', + ), + $message); + + return $container; + } + +} diff --git a/src/view/phui/PHUIHeaderView.php b/src/view/phui/PHUIHeaderView.php index 53ec096265..4ac4b2de54 100644 --- a/src/view/phui/PHUIHeaderView.php +++ b/src/view/phui/PHUIHeaderView.php @@ -307,9 +307,14 @@ final class PHUIHeaderView extends AphrontTagView { $icon = null; if ($this->headerIcon) { - $icon = id(new PHUIIconView()) - ->setIcon($this->headerIcon) - ->addClass('phui-header-icon'); + if ($this->headerIcon instanceof PHUIIconView) { + $icon = id(clone $this->headerIcon) + ->addClass('phui-header-icon'); + } else { + $icon = id(new PHUIIconView()) + ->setIcon($this->headerIcon) + ->addClass('phui-header-icon'); + } } $header_content = $this->header; diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index 16a78e219c..a598438d99 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -149,24 +149,6 @@ div.phui-property-list-stacked .phui-property-list-properties } -.phui-property-list-image { - margin: auto; - max-width: 95%; -} - -.phui-property-list-audio { - display: block; - margin: 16px auto; - width: 50%; - min-width: 240px; -} - -.phui-property-list-video { - display: block; - margin: 0 auto; - max-width: 95%; -} - /* When tags appear in property lists, give them a little more vertical spacing. */ .phui-property-list-value .phui-tag-view { @@ -220,3 +202,32 @@ div.phui-property-list-stacked .phui-property-list-properties border-right: 1px solid {$lightblueborder}; border-bottom: 1px solid {$blueborder}; } + + +.document-engine-image img { + margin: 20px auto; + background: url('/rsrc/image/checker_light.png'); +} + +.device-desktop .document-engine-image img:hover { + background: url('/rsrc/image/checker_dark.png'); +} + +.document-engine-video video { + margin: 20px auto; + display: block; + max-width: 95%; +} + +.document-engine-audio audio { + display: block; + margin: 16px auto; + width: 50%; + min-width: 240px; +} + +.document-engine-message { + margin: 20px auto; + text-align: center; + color: {$greytext}; +} From f646153f4dbe6303856a2f383526d1c539a3b060 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Mar 2018 10:55:02 -0700 Subject: [PATCH 04/20] Add an async driver for document rendering and a crude "Hexdump" document engine Summary: Depends on D19237. Ref T13105. This adds a (very basic) "Hexdump" engine (mostly just to have a second option to switch to) and a selector for choosing view modes. Test Plan: Viewed some files, switched between audio/video/image/hexdump. Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19238 --- resources/celerity/map.php | 13 +- src/__phutil_library_map__.php | 4 + .../PhabricatorFilesApplication.php | 2 + .../PhabricatorFileDocumentController.php | 113 ++++++++++++++++++ .../PhabricatorFileInfoController.php | 78 +++++++----- .../PhabricatorAudioDocumentEngine.php | 4 + .../document/PhabricatorDocumentEngine.php | 48 ++++++++ .../files/document/PhabricatorDocumentRef.php | 8 ++ .../PhabricatorHexdumpDocumentEngine.php | 83 +++++++++++++ .../PhabricatorImageDocumentEngine.php | 4 + .../PhabricatorVideoDocumentEngine.php | 4 + .../PhabricatorVoidDocumentEngine.php | 4 + .../rsrc/css/phui/phui-property-list-view.css | 11 ++ .../files/behavior-document-engine.js | 67 +++++++++++ 14 files changed, 411 insertions(+), 32 deletions(-) create mode 100644 src/applications/files/controller/PhabricatorFileDocumentController.php create mode 100644 src/applications/files/document/PhabricatorHexdumpDocumentEngine.php create mode 100644 webroot/rsrc/js/application/files/behavior-document-engine.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 88a4a366d7..8e7ce6e258 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => '6a8ba174', + 'core.pkg.css' => '97dc0e74', 'core.pkg.js' => '8581cd02', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -168,7 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => '9cff003c', 'rsrc/css/phui/phui-pager.css' => 'edcbc226', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '79fc3a02', + 'rsrc/css/phui/phui-property-list-view.css' => 'ef864066', 'rsrc/css/phui/phui-remarkup-preview.css' => '54a34863', 'rsrc/css/phui/phui-segment-bar-view.css' => 'b1d1b892', 'rsrc/css/phui/phui-spacing.css' => '042804d6', @@ -392,6 +392,7 @@ return array( 'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'f01586dc', 'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '1db13e70', 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef', + 'rsrc/js/application/files/behavior-document-engine.js' => 'f6d6f389', 'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '191b4909', @@ -606,6 +607,7 @@ return array( 'javelin-behavior-diffusion-jump-to' => '73d09eef', 'javelin-behavior-diffusion-locate-file' => '6d3e1947', 'javelin-behavior-diffusion-pull-lastmodified' => 'f01586dc', + 'javelin-behavior-document-engine' => 'f6d6f389', 'javelin-behavior-doorkeeper-tag' => '1db13e70', 'javelin-behavior-drydock-live-operation-status' => '901935ef', 'javelin-behavior-durable-column' => '2ae077e1', @@ -848,7 +850,7 @@ return array( 'phui-oi-simple-ui-css' => 'a8beebea', 'phui-pager-css' => 'edcbc226', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '79fc3a02', + 'phui-property-list-view-css' => 'ef864066', 'phui-remarkup-preview-css' => '54a34863', 'phui-segment-bar-view-css' => 'b1d1b892', 'phui-spacing-css' => '042804d6', @@ -2151,6 +2153,11 @@ return array( 'javelin-util', 'javelin-reactor', ), + 'f6d6f389' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + ), 'f829edb3' => array( 'javelin-view', 'javelin-install', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5fff5b53df..aef91c2e6b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3001,6 +3001,7 @@ phutil_register_library_map(array( 'PhabricatorFileDataController' => 'applications/files/controller/PhabricatorFileDataController.php', 'PhabricatorFileDeleteController' => 'applications/files/controller/PhabricatorFileDeleteController.php', 'PhabricatorFileDeleteTransaction' => 'applications/files/xaction/PhabricatorFileDeleteTransaction.php', + 'PhabricatorFileDocumentController' => 'applications/files/controller/PhabricatorFileDocumentController.php', 'PhabricatorFileDropUploadController' => 'applications/files/controller/PhabricatorFileDropUploadController.php', 'PhabricatorFileEditController' => 'applications/files/controller/PhabricatorFileEditController.php', 'PhabricatorFileEditEngine' => 'applications/files/editor/PhabricatorFileEditEngine.php', @@ -3140,6 +3141,7 @@ phutil_register_library_map(array( 'PhabricatorHelpKeyboardShortcutController' => 'applications/help/controller/PhabricatorHelpKeyboardShortcutController.php', 'PhabricatorHeraldApplication' => 'applications/herald/application/PhabricatorHeraldApplication.php', 'PhabricatorHeraldContentSource' => 'applications/herald/contentsource/PhabricatorHeraldContentSource.php', + 'PhabricatorHexdumpDocumentEngine' => 'applications/files/document/PhabricatorHexdumpDocumentEngine.php', 'PhabricatorHighSecurityRequestExceptionHandler' => 'aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php', 'PhabricatorHomeApplication' => 'applications/home/application/PhabricatorHomeApplication.php', 'PhabricatorHomeConstants' => 'applications/home/constants/PhabricatorHomeConstants.php', @@ -8590,6 +8592,7 @@ phutil_register_library_map(array( 'PhabricatorFileDataController' => 'PhabricatorFileController', 'PhabricatorFileDeleteController' => 'PhabricatorFileController', 'PhabricatorFileDeleteTransaction' => 'PhabricatorFileTransactionType', + 'PhabricatorFileDocumentController' => 'PhabricatorFileController', 'PhabricatorFileDropUploadController' => 'PhabricatorFileController', 'PhabricatorFileEditController' => 'PhabricatorFileController', 'PhabricatorFileEditEngine' => 'PhabricatorEditEngine', @@ -8747,6 +8750,7 @@ phutil_register_library_map(array( 'PhabricatorHelpKeyboardShortcutController' => 'PhabricatorHelpController', 'PhabricatorHeraldApplication' => 'PhabricatorApplication', 'PhabricatorHeraldContentSource' => 'PhabricatorContentSource', + 'PhabricatorHexdumpDocumentEngine' => 'PhabricatorDocumentEngine', 'PhabricatorHighSecurityRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorHomeApplication' => 'PhabricatorApplication', 'PhabricatorHomeConstants' => 'PhabricatorHomeController', diff --git a/src/applications/files/application/PhabricatorFilesApplication.php b/src/applications/files/application/PhabricatorFilesApplication.php index 2d3a1b35c6..d95972d10e 100644 --- a/src/applications/files/application/PhabricatorFilesApplication.php +++ b/src/applications/files/application/PhabricatorFilesApplication.php @@ -89,6 +89,8 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { 'iconset/(?P[^/]+)/' => array( 'select/' => 'PhabricatorFileIconSetSelectController', ), + 'document/(?P[^/]+)/(?P[^/]+)/' + => 'PhabricatorFileDocumentController', ) + $this->getResourceSubroutes(), ); } diff --git a/src/applications/files/controller/PhabricatorFileDocumentController.php b/src/applications/files/controller/PhabricatorFileDocumentController.php new file mode 100644 index 0000000000..b74d98f48e --- /dev/null +++ b/src/applications/files/controller/PhabricatorFileDocumentController.php @@ -0,0 +1,113 @@ +getViewer(); + + $file_phid = $request->getURIData('phid'); + + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$file) { + return $this->newErrorResponse( + pht( + 'This file ("%s") does not exist or could not be loaded.', + $file_phid)); + } + $this->file = $file; + + $ref = id(new PhabricatorDocumentRef()) + ->setFile($file); + $this->ref = $ref; + + $engines = PhabricatorDocumentEngine::getEnginesForRef($viewer, $ref); + $engine_key = $request->getURIData('engineKey'); + if (!isset($engines[$engine_key])) { + return $this->newErrorResponse( + pht( + 'The engine ("%s") is unknown, or unable to render this document.', + $engine_key)); + } + $engine = $engines[$engine_key]; + $this->engine = $engine; + + try { + $content = $engine->newDocument($ref); + } catch (Exception $ex) { + return $this->newErrorResponse($ex->getMessage()); + } + + return $this->newContentResponse($content); + } + + private function newErrorResponse($message) { + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-error', + ), + array( + id(new PHUIIconView()) + ->setIcon('fa-exclamation-triangle red'), + ' ', + $message, + )); + + return $this->newContentResponse($container); + } + + + private function newContentResponse($content) { + $viewer = $this->getViewer(); + $request = $this->getRequest(); + + $file = $this->file; + $engine = $this->engine; + $ref = $this->ref; + + if ($request->isAjax()) { + return id(new AphrontAjaxResponse()) + ->setContent( + array( + 'markup' => hsprintf('%s', $content), + )); + } + + $crumbs = $this->buildApplicationCrumbs(); + if ($file) { + $crumbs->addTextCrumb($file->getMonogram(), $file->getInfoURI()); + } + + $label = $engine->getViewAsLabel($ref); + if ($label) { + $crumbs->addTextCrumb($label); + } + + $crumbs->setBorder(true); + + $content_frame = id(new PHUIObjectBoxView()) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->appendChild($content); + + $page_frame = id(new PHUITwoColumnView()) + ->setFooter($content_frame); + + return $this->newPage() + ->setCrumbs($crumbs) + ->setTitle( + array( + $ref->getName(), + pht('Standalone'), + )) + ->appendChild($page_frame); + } + +} diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php index fe8d37ab80..4badb7a140 100644 --- a/src/applications/files/controller/PhabricatorFileInfoController.php +++ b/src/applications/files/controller/PhabricatorFileInfoController.php @@ -404,50 +404,70 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { private function newFileContent(PhabricatorFile $file) { $viewer = $this->getViewer(); - $engines = PhabricatorDocumentEngine::getAllEngines(); $ref = id(new PhabricatorDocumentRef()) ->setFile($file); - foreach ($engines as $key => $engine) { - $engine = id(clone $engine) - ->setViewer($viewer); - - if (!$engine->canRenderDocument($ref)) { - unset($engines[$key]); - continue; - } - - $engines[$key] = $engine; - } - - if (!$engines) { - throw new Exception(pht('No engine can render this document.')); - } - - $vectors = array(); - foreach ($engines as $key => $usable_engine) { - $vectors[$key] = $usable_engine->newSortVector($ref); - } - $vectors = msortv($vectors, 'getSelf'); - - $engine = $engines[head_key($vectors)]; + $engines = PhabricatorDocumentEngine::getEnginesForRef($viewer, $ref); + $engine = head($engines); $content = $engine->newDocument($ref); - if (!$content) { - return null; - } $icon = $engine->newDocumentIcon($ref); + $views = array(); + foreach ($engines as $candidate_engine) { + $label = $candidate_engine->getViewAsLabel($ref); + if ($label === null) { + continue; + } + + $view_icon = $candidate_engine->getViewAsIconIcon($ref); + + $views[] = array( + 'viewKey' => $candidate_engine->getDocumentEngineKey(), + 'icon' => $view_icon, + 'name' => $label, + 'engineURI' => $candidate_engine->getRenderURI($ref), + ); + } + + Javelin::initBehavior('document-engine'); + + $viewport_id = celerity_generate_unique_node_id(); + + $viewport = phutil_tag( + 'div', + array( + 'id' => $viewport_id, + ), + $content); + + $meta = array( + 'viewportID' => $viewport_id, + 'viewKey' => $engine->getDocumentEngineKey(), + 'views' => $views, + 'standaloneURI' => $engine->getRenderURI($ref), + ); + + $view_button = id(new PHUIButtonView()) + ->setTag('a') + ->setText(pht('View Options')) + ->setIcon('fa-file-image-o') + ->setColor(PHUIButtonView::GREY) + ->setMetadata($meta) + ->setDropdown(true) + ->addSigil('document-engine-view-dropdown'); + $header = id(new PHUIHeaderView()) ->setHeaderIcon($icon) - ->setHeader($ref->getName()); + ->setHeader($ref->getName()) + ->addActionLink($view_button); return id(new PHUIObjectBoxView()) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->setHeader($header) - ->appendChild($content); + ->appendChild($viewport); } } diff --git a/src/applications/files/document/PhabricatorAudioDocumentEngine.php b/src/applications/files/document/PhabricatorAudioDocumentEngine.php index afbc10e70b..9f6274de84 100644 --- a/src/applications/files/document/PhabricatorAudioDocumentEngine.php +++ b/src/applications/files/document/PhabricatorAudioDocumentEngine.php @@ -5,6 +5,10 @@ final class PhabricatorAudioDocumentEngine const ENGINEKEY = 'audio'; + public function getViewAsLabel(PhabricatorDocumentRef $ref) { + return pht('View as Audio'); + } + protected function getDocumentIconIcon(PhabricatorDocumentRef $ref) { return 'fa-file-sound-o'; } diff --git a/src/applications/files/document/PhabricatorDocumentEngine.php b/src/applications/files/document/PhabricatorDocumentEngine.php index 0c72ce9fd1..65596975c9 100644 --- a/src/applications/files/document/PhabricatorDocumentEngine.php +++ b/src/applications/files/document/PhabricatorDocumentEngine.php @@ -59,4 +59,52 @@ abstract class PhabricatorDocumentEngine return 2000; } + abstract public function getViewAsLabel(PhabricatorDocumentRef $ref); + + public function getViewAsIconIcon(PhabricatorDocumentRef $ref) { + return $this->getDocumentIconIcon($ref); + } + + public function getRenderURI(PhabricatorDocumentRef $ref) { + $file = $ref->getFile(); + if (!$file) { + throw new PhutilMethodNotImplementedException(); + } + + $engine_key = $this->getDocumentEngineKey(); + $file_phid = $file->getPHID(); + + return "/file/document/{$engine_key}/{$file_phid}/"; + } + + final public static function getEnginesForRef( + PhabricatorUser $viewer, + PhabricatorDocumentRef $ref) { + $engines = self::getAllEngines(); + + foreach ($engines as $key => $engine) { + $engine = id(clone $engine) + ->setViewer($viewer); + + if (!$engine->canRenderDocument($ref)) { + unset($engines[$key]); + continue; + } + + $engines[$key] = $engine; + } + + if (!$engines) { + throw new Exception(pht('No content engine can render this document.')); + } + + $vectors = array(); + foreach ($engines as $key => $usable_engine) { + $vectors[$key] = $usable_engine->newSortVector($ref); + } + $vectors = msortv($vectors, 'getSelf'); + + return array_select_keys($engines, array_keys($vectors)); + } + } diff --git a/src/applications/files/document/PhabricatorDocumentRef.php b/src/applications/files/document/PhabricatorDocumentRef.php index cb36cbaed7..91f41eb871 100644 --- a/src/applications/files/document/PhabricatorDocumentRef.php +++ b/src/applications/files/document/PhabricatorDocumentRef.php @@ -68,6 +68,14 @@ final class PhabricatorDocumentRef return null; } + public function loadData() { + if ($this->file) { + return $this->file->loadFileData(); + } + + throw new PhutilMethodNotImplementedException(); + } + public function hasAnyMimeType(array $candidate_types) { $mime_full = $this->getMimeType(); $mime_parts = explode(';', $mime_full); diff --git a/src/applications/files/document/PhabricatorHexdumpDocumentEngine.php b/src/applications/files/document/PhabricatorHexdumpDocumentEngine.php new file mode 100644 index 0000000000..4ceeca9cf3 --- /dev/null +++ b/src/applications/files/document/PhabricatorHexdumpDocumentEngine.php @@ -0,0 +1,83 @@ +loadData(); + + $output = array(); + $offset = 0; + + $lines = str_split($content, 16); + foreach ($lines as $line) { + $output[] = sprintf( + '%08x %- 23s %- 23s %- 16s', + $offset, + $this->renderHex(substr($line, 0, 8)), + $this->renderHex(substr($line, 8)), + $this->renderBytes($line)); + + $offset += 16; + } + + $output = implode("\n", $output); + + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-hexdump PhabricatorMonospaced', + ), + $output); + + return $container; + } + + private function renderHex($bytes) { + $length = strlen($bytes); + + $output = array(); + for ($ii = 0; $ii < $length; $ii++) { + $output[] = sprintf('%02x', ord($bytes[$ii])); + } + + return implode(' ', $output); + } + + private function renderBytes($bytes) { + $length = strlen($bytes); + + $output = array(); + for ($ii = 0; $ii < $length; $ii++) { + $chr = $bytes[$ii]; + $ord = ord($chr); + + if ($ord < 0x20 || $ord >= 0x7F) { + $chr = '.'; + } + + $output[] = $chr; + } + + return implode('', $output); + } + +} diff --git a/src/applications/files/document/PhabricatorImageDocumentEngine.php b/src/applications/files/document/PhabricatorImageDocumentEngine.php index ab5753680b..84ad3110df 100644 --- a/src/applications/files/document/PhabricatorImageDocumentEngine.php +++ b/src/applications/files/document/PhabricatorImageDocumentEngine.php @@ -5,6 +5,10 @@ final class PhabricatorImageDocumentEngine const ENGINEKEY = 'image'; + public function getViewAsLabel(PhabricatorDocumentRef $ref) { + return pht('View as Image'); + } + protected function getDocumentIconIcon(PhabricatorDocumentRef $ref) { return 'fa-file-image-o'; } diff --git a/src/applications/files/document/PhabricatorVideoDocumentEngine.php b/src/applications/files/document/PhabricatorVideoDocumentEngine.php index 81573b6e93..591a24cb3e 100644 --- a/src/applications/files/document/PhabricatorVideoDocumentEngine.php +++ b/src/applications/files/document/PhabricatorVideoDocumentEngine.php @@ -5,6 +5,10 @@ final class PhabricatorVideoDocumentEngine const ENGINEKEY = 'video'; + public function getViewAsLabel(PhabricatorDocumentRef $ref) { + return pht('View as Video'); + } + protected function getDocumentIconIcon(PhabricatorDocumentRef $ref) { return 'fa-film'; } diff --git a/src/applications/files/document/PhabricatorVoidDocumentEngine.php b/src/applications/files/document/PhabricatorVoidDocumentEngine.php index a57514edb8..75d74e05a5 100644 --- a/src/applications/files/document/PhabricatorVoidDocumentEngine.php +++ b/src/applications/files/document/PhabricatorVoidDocumentEngine.php @@ -5,6 +5,10 @@ final class PhabricatorVoidDocumentEngine const ENGINEKEY = 'void'; + public function getViewAsLabel(PhabricatorDocumentRef $ref) { + return null; + } + protected function getDocumentIconIcon(PhabricatorDocumentRef $ref) { return 'fa-file'; } diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index a598438d99..1ca39a8486 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -231,3 +231,14 @@ div.phui-property-list-stacked .phui-property-list-properties text-align: center; color: {$greytext}; } + +.document-engine-error { + margin: 20px auto; + text-align: center; + color: {$redtext}; +} + +.document-engine-hexdump { + margin: 20px; + white-space: pre; +} diff --git a/webroot/rsrc/js/application/files/behavior-document-engine.js b/webroot/rsrc/js/application/files/behavior-document-engine.js new file mode 100644 index 0000000000..3af1d681fd --- /dev/null +++ b/webroot/rsrc/js/application/files/behavior-document-engine.js @@ -0,0 +1,67 @@ +/** + * @provides javelin-behavior-document-engine + * @requires javelin-behavior + * javelin-dom + * javelin-stratcom + */ + +JX.behavior('document-engine', function() { + + function onmenu(e) { + var node = e.getNode('document-engine-view-dropdown'); + var data = JX.Stratcom.getData(node); + + if (data.menu) { + return; + } + + e.prevent(); + + var menu = new JX.PHUIXDropdownMenu(node); + var list = new JX.PHUIXActionListView(); + + var view; + for (var ii = 0; ii < data.views.length; ii++) { + var spec = data.views[ii]; + + view = new JX.PHUIXActionView() + .setName(spec.name) + .setIcon(spec.icon) + .setHref(spec.engineURI); + + view.setHandler(JX.bind(null, function(spec, e) { + if (!e.isNormalClick()) { + return; + } + + e.prevent(); + menu.close(); + + onview(data, spec); + }, spec)); + + list.addItem(view); + } + + menu.setContent(list.getNode()); + + data.menu = menu; + menu.open(); + } + + function onview(data, spec) { + var handler = JX.bind(null, onrender, data); + + new JX.Request(spec.engineURI, handler) + .send(); + } + + function onrender(data, r) { + var viewport = JX.$(data.viewportID); + + JX.DOM.setContent(viewport, JX.$H(r.markup)); + } + + JX.Stratcom.listen('click', 'document-engine-view-dropdown', onmenu); + +}); From 4aafce686202aca64d945a3c2798837de62c1cd1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Mar 2018 12:36:39 -0700 Subject: [PATCH 05/20] Add filesize limits for document rendering engines and support partial/complete rendering Summary: Depends on D19238. Ref T13105. Give document engines some reasonable automatic support for degrading gracefully when someone tries to hexdump a 100MB file or similar. Also, make "Video" sort above "Audio" for files which could be rendered either way. Test Plan: Viewed audio, video, image, and other files. Adjusted limits and saw full, partial, and fallback/error rendering. Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19239 --- resources/celerity/map.php | 36 +++++----- .../PhabricatorFileInfoController.php | 2 + .../PhabricatorAudioDocumentEngine.php | 4 ++ .../document/PhabricatorDocumentEngine.php | 72 ++++++++++++++++++- .../files/document/PhabricatorDocumentRef.php | 12 +++- .../PhabricatorHexdumpDocumentEngine.php | 37 +++++++++- .../PhabricatorImageDocumentEngine.php | 4 ++ .../PhabricatorVideoDocumentEngine.php | 10 +++ .../PhabricatorVoidDocumentEngine.php | 6 +- .../rsrc/css/phui/phui-property-list-view.css | 4 +- .../files/behavior-document-engine.js | 1 + webroot/rsrc/js/phuix/PHUIXActionView.js | 11 +++ 12 files changed, 172 insertions(+), 27 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 8e7ce6e258..562bd50fc1 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,8 +9,8 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => '97dc0e74', - 'core.pkg.js' => '8581cd02', + 'core.pkg.css' => '6da3c0e5', + 'core.pkg.js' => '932d60d4', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', 'diffusion.pkg.css' => 'a2d17c7d', @@ -168,7 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => '9cff003c', 'rsrc/css/phui/phui-pager.css' => 'edcbc226', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => 'ef864066', + 'rsrc/css/phui/phui-property-list-view.css' => '6ef560df', 'rsrc/css/phui/phui-remarkup-preview.css' => '54a34863', 'rsrc/css/phui/phui-segment-bar-view.css' => 'b1d1b892', 'rsrc/css/phui/phui-spacing.css' => '042804d6', @@ -392,7 +392,7 @@ return array( 'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'f01586dc', 'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '1db13e70', 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef', - 'rsrc/js/application/files/behavior-document-engine.js' => 'f6d6f389', + 'rsrc/js/application/files/behavior-document-engine.js' => '396ef112', 'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '191b4909', @@ -508,7 +508,7 @@ return array( 'rsrc/js/phui/behavior-phui-submenu.js' => 'a6f7a73b', 'rsrc/js/phui/behavior-phui-tab-group.js' => '0a0b10e9', 'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8', - 'rsrc/js/phuix/PHUIXActionView.js' => '442efd08', + 'rsrc/js/phuix/PHUIXActionView.js' => 'ed18356a', 'rsrc/js/phuix/PHUIXAutocomplete.js' => '7fa5c915', 'rsrc/js/phuix/PHUIXButtonView.js' => '8a91e1ac', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => '04b2ae03', @@ -607,7 +607,7 @@ return array( 'javelin-behavior-diffusion-jump-to' => '73d09eef', 'javelin-behavior-diffusion-locate-file' => '6d3e1947', 'javelin-behavior-diffusion-pull-lastmodified' => 'f01586dc', - 'javelin-behavior-document-engine' => 'f6d6f389', + 'javelin-behavior-document-engine' => '396ef112', 'javelin-behavior-doorkeeper-tag' => '1db13e70', 'javelin-behavior-drydock-live-operation-status' => '901935ef', 'javelin-behavior-durable-column' => '2ae077e1', @@ -850,7 +850,7 @@ return array( 'phui-oi-simple-ui-css' => 'a8beebea', 'phui-pager-css' => 'edcbc226', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => 'ef864066', + 'phui-property-list-view-css' => '6ef560df', 'phui-remarkup-preview-css' => '54a34863', 'phui-segment-bar-view-css' => 'b1d1b892', 'phui-spacing-css' => '042804d6', @@ -864,7 +864,7 @@ return array( 'phui-workcard-view-css' => 'cca5fa92', 'phui-workpanel-view-css' => 'a3a63478', 'phuix-action-list-view' => 'b5c256b8', - 'phuix-action-view' => '442efd08', + 'phuix-action-view' => 'ed18356a', 'phuix-autocomplete' => '7fa5c915', 'phuix-button-view' => '8a91e1ac', 'phuix-dropdown-menu' => '04b2ae03', @@ -1114,6 +1114,11 @@ return array( 'javelin-dom', 'javelin-vector', ), + '396ef112' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + ), '3ab51e2c' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -1184,11 +1189,6 @@ return array( 'javelin-workflow', 'javelin-workboard-controller', ), - '442efd08' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-util', - ), '44959b73' => array( 'javelin-util', 'javelin-uri', @@ -2125,6 +2125,11 @@ return array( 'javelin-stratcom', 'javelin-vector', ), + 'ed18356a' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-util', + ), 'edf8a145' => array( 'javelin-behavior', 'javelin-uri', @@ -2153,11 +2158,6 @@ return array( 'javelin-util', 'javelin-reactor', ), - 'f6d6f389' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - ), 'f829edb3' => array( 'javelin-view', 'javelin-install', diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php index 4badb7a140..e25cf8d74c 100644 --- a/src/applications/files/controller/PhabricatorFileInfoController.php +++ b/src/applications/files/controller/PhabricatorFileInfoController.php @@ -423,10 +423,12 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { } $view_icon = $candidate_engine->getViewAsIconIcon($ref); + $view_color = $candidate_engine->getViewAsIconColor($ref); $views[] = array( 'viewKey' => $candidate_engine->getDocumentEngineKey(), 'icon' => $view_icon, + 'color' => $view_color, 'name' => $label, 'engineURI' => $candidate_engine->getRenderURI($ref), ); diff --git a/src/applications/files/document/PhabricatorAudioDocumentEngine.php b/src/applications/files/document/PhabricatorAudioDocumentEngine.php index 9f6274de84..859f167627 100644 --- a/src/applications/files/document/PhabricatorAudioDocumentEngine.php +++ b/src/applications/files/document/PhabricatorAudioDocumentEngine.php @@ -13,6 +13,10 @@ final class PhabricatorAudioDocumentEngine return 'fa-file-sound-o'; } + protected function getByteLengthLimit() { + return null; + } + protected function canRenderDocumentType(PhabricatorDocumentRef $ref) { $file = $ref->getFile(); if ($file) { diff --git a/src/applications/files/document/PhabricatorDocumentEngine.php b/src/applications/files/document/PhabricatorDocumentEngine.php index 65596975c9..6ad7f74f9a 100644 --- a/src/applications/files/document/PhabricatorDocumentEngine.php +++ b/src/applications/files/document/PhabricatorDocumentEngine.php @@ -22,6 +22,18 @@ abstract class PhabricatorDocumentEngine PhabricatorDocumentRef $ref); final public function newDocument(PhabricatorDocumentRef $ref) { + $can_complete = $this->canRenderCompleteDocument($ref); + $can_partial = $this->canRenderPartialDocument($ref); + + if (!$can_complete && !$can_partial) { + return $this->newMessage( + pht( + 'This document is too large to be rendered inline. (The document '. + 'is %s bytes, the limit for this engine is %s bytes.)', + new PhutilNumber($ref->getByteLength()), + new PhutilNumber($this->getByteLengthLimit()))); + } + return $this->newDocumentContent($ref); } @@ -51,20 +63,49 @@ abstract class PhabricatorDocumentEngine final public function newSortVector(PhabricatorDocumentRef $ref) { $content_score = $this->getContentScore($ref); + // Prefer engines which can render the entire file over engines which + // can only render a header, and engines which can render a header over + // engines which can't render anything. + if ($this->canRenderCompleteDocument($ref)) { + $limit_score = 0; + } else if ($this->canRenderPartialDocument($ref)) { + $limit_score = 1; + } else { + $limit_score = 2; + } + return id(new PhutilSortVector()) + ->addInt($limit_score) ->addInt(-$content_score); } - protected function getContentScore() { + protected function getContentScore(PhabricatorDocumentRef $ref) { return 2000; } abstract public function getViewAsLabel(PhabricatorDocumentRef $ref); public function getViewAsIconIcon(PhabricatorDocumentRef $ref) { + $can_complete = $this->canRenderCompleteDocument($ref); + $can_partial = $this->canRenderPartialDocument($ref); + + if (!$can_complete && !$can_partial) { + return 'fa-times'; + } + return $this->getDocumentIconIcon($ref); } + public function getViewAsIconColor(PhabricatorDocumentRef $ref) { + $can_complete = $this->canRenderCompleteDocument($ref); + + if (!$can_complete) { + return 'grey'; + } + + return null; + } + public function getRenderURI(PhabricatorDocumentRef $ref) { $file = $ref->getFile(); if (!$file) { @@ -107,4 +148,33 @@ abstract class PhabricatorDocumentEngine return array_select_keys($engines, array_keys($vectors)); } + protected function getByteLengthLimit() { + return (1024 * 1024 * 8); + } + + protected function canRenderCompleteDocument(PhabricatorDocumentRef $ref) { + $limit = $this->getByteLengthLimit(); + if ($limit) { + $length = $ref->getByteLength(); + if ($length > $limit) { + return false; + } + } + + return true; + } + + protected function canRenderPartialDocument(PhabricatorDocumentRef $ref) { + return false; + } + + protected function newMessage($message) { + return phutil_tag( + 'div', + array( + 'class' => 'document-engine-error', + ), + $message); + } + } diff --git a/src/applications/files/document/PhabricatorDocumentRef.php b/src/applications/files/document/PhabricatorDocumentRef.php index 91f41eb871..862c0596d3 100644 --- a/src/applications/files/document/PhabricatorDocumentRef.php +++ b/src/applications/files/document/PhabricatorDocumentRef.php @@ -56,7 +56,7 @@ final class PhabricatorDocumentRef return $this; } - public function getLength() { + public function getByteLength() { if ($this->byteLength !== null) { return $this->byteLength; } @@ -68,9 +68,15 @@ final class PhabricatorDocumentRef return null; } - public function loadData() { + public function loadData($begin = null, $end = null) { if ($this->file) { - return $this->file->loadFileData(); + $iterator = $this->file->getFileDataIterator($begin, $end); + + $result = ''; + foreach ($iterator as $chunk) { + $result .= $chunk; + } + return $result; } throw new PhutilMethodNotImplementedException(); diff --git a/src/applications/files/document/PhabricatorHexdumpDocumentEngine.php b/src/applications/files/document/PhabricatorHexdumpDocumentEngine.php index 4ceeca9cf3..4217c24d62 100644 --- a/src/applications/files/document/PhabricatorHexdumpDocumentEngine.php +++ b/src/applications/files/document/PhabricatorHexdumpDocumentEngine.php @@ -13,7 +13,11 @@ final class PhabricatorHexdumpDocumentEngine return 'fa-microchip'; } - protected function getContentScore() { + protected function getByteLengthLimit() { + return (1024 * 1024 * 1); + } + + protected function getContentScore(PhabricatorDocumentRef $ref) { return 500; } @@ -21,8 +25,23 @@ final class PhabricatorHexdumpDocumentEngine return true; } + protected function canRenderPartialDocument(PhabricatorDocumentRef $ref) { + return true; + } + protected function newDocumentContent(PhabricatorDocumentRef $ref) { - $content = $ref->loadData(); + $limit = $this->getByteLengthLimit(); + $length = $ref->getByteLength(); + + $is_partial = false; + if ($limit) { + if ($length > $limit) { + $is_partial = true; + $length = $limit; + } + } + + $content = $ref->loadData(null, $length); $output = array(); $offset = 0; @@ -48,7 +67,19 @@ final class PhabricatorHexdumpDocumentEngine ), $output); - return $container; + $message = null; + if ($is_partial) { + $message = $this->newMessage( + pht( + 'This document is too large to be completely rendered inline. The '. + 'first %s bytes are shown.', + new PhutilNumber($limit))); + } + + return array( + $message, + $container, + ); } private function renderHex($bytes) { diff --git a/src/applications/files/document/PhabricatorImageDocumentEngine.php b/src/applications/files/document/PhabricatorImageDocumentEngine.php index 84ad3110df..d0b2099dd0 100644 --- a/src/applications/files/document/PhabricatorImageDocumentEngine.php +++ b/src/applications/files/document/PhabricatorImageDocumentEngine.php @@ -13,6 +13,10 @@ final class PhabricatorImageDocumentEngine return 'fa-file-image-o'; } + protected function getByteLengthLimit() { + return (1024 * 1024 * 64); + } + protected function canRenderDocumentType(PhabricatorDocumentRef $ref) { $file = $ref->getFile(); if ($file) { diff --git a/src/applications/files/document/PhabricatorVideoDocumentEngine.php b/src/applications/files/document/PhabricatorVideoDocumentEngine.php index 591a24cb3e..4e03c62ebc 100644 --- a/src/applications/files/document/PhabricatorVideoDocumentEngine.php +++ b/src/applications/files/document/PhabricatorVideoDocumentEngine.php @@ -9,6 +9,16 @@ final class PhabricatorVideoDocumentEngine return pht('View as Video'); } + protected function getContentScore(PhabricatorDocumentRef $ref) { + // Some video documents can be rendered as either video or audio, but we + // want to prefer video. + return 2500; + } + + protected function getByteLengthLimit() { + return null; + } + protected function getDocumentIconIcon(PhabricatorDocumentRef $ref) { return 'fa-film'; } diff --git a/src/applications/files/document/PhabricatorVoidDocumentEngine.php b/src/applications/files/document/PhabricatorVoidDocumentEngine.php index 75d74e05a5..c5395632eb 100644 --- a/src/applications/files/document/PhabricatorVoidDocumentEngine.php +++ b/src/applications/files/document/PhabricatorVoidDocumentEngine.php @@ -13,10 +13,14 @@ final class PhabricatorVoidDocumentEngine return 'fa-file'; } - protected function getContentScore() { + protected function getContentScore(PhabricatorDocumentRef $ref) { return 1000; } + protected function getByteLengthLimit() { + return null; + } + protected function canRenderDocumentType(PhabricatorDocumentRef $ref) { return true; } diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index 1ca39a8486..840364723d 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -233,9 +233,11 @@ div.phui-property-list-stacked .phui-property-list-properties } .document-engine-error { - margin: 20px auto; + margin: 20px; + padding: 12px; text-align: center; color: {$redtext}; + background: {$sh-redbackground}; } .document-engine-hexdump { diff --git a/webroot/rsrc/js/application/files/behavior-document-engine.js b/webroot/rsrc/js/application/files/behavior-document-engine.js index 3af1d681fd..6b9b3eeb24 100644 --- a/webroot/rsrc/js/application/files/behavior-document-engine.js +++ b/webroot/rsrc/js/application/files/behavior-document-engine.js @@ -27,6 +27,7 @@ JX.behavior('document-engine', function() { view = new JX.PHUIXActionView() .setName(spec.name) .setIcon(spec.icon) + .setIconColor(spec.color) .setHref(spec.engineURI); view.setHandler(JX.bind(null, function(spec, e) { diff --git a/webroot/rsrc/js/phuix/PHUIXActionView.js b/webroot/rsrc/js/phuix/PHUIXActionView.js index 2db58dbdc4..7967fa7366 100644 --- a/webroot/rsrc/js/phuix/PHUIXActionView.js +++ b/webroot/rsrc/js/phuix/PHUIXActionView.js @@ -12,6 +12,7 @@ JX.install('PHUIXActionView', { _node: null, _name: null, _icon: 'none', + _iconColor: null, _disabled: false, _label: false, _handler: null, @@ -79,6 +80,12 @@ JX.install('PHUIXActionView', { return this; }, + setIconColor: function(color) { + this._iconColor = color; + this._buildIconNode(true); + return this; + }, + setHref: function(href) { this._href = href; this._buildNameNode(true); @@ -129,6 +136,10 @@ JX.install('PHUIXActionView', { icon_class = icon_class + ' grey'; } + if (this._iconColor) { + icon_class = icon_class + ' ' + this._iconColor; + } + JX.DOM.alterClass(node, icon_class, true); if (this._iconNode && this._iconNode.parentNode) { From 3bf8d5682ed06a6905fc0ca896531312427de1d9 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 20 Mar 2018 15:50:51 -0700 Subject: [PATCH 06/20] Fix outdated link for Font Awesome icon set Summary: The current link has a redirect for a while now, from http://fortawesome.github.io/Font-Awesome/ to https://fontawesome.com However, since the release of Version 5, the docs no longer match the icons that are valid for use in Phabricator, which uses Version 4. Update the reference to link to the same logical content as before. Test Plan: The content now lives at . Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Differential Revision: https://secure.phabricator.com/D19241 --- src/docs/user/userguide/remarkup.diviner | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/docs/user/userguide/remarkup.diviner b/src/docs/user/userguide/remarkup.diviner index 28a1a31fc4..d052a9a248 100644 --- a/src/docs/user/userguide/remarkup.diviner +++ b/src/docs/user/userguide/remarkup.diviner @@ -530,7 +530,7 @@ This renders: {icon camera color=blue} For a list of available icons and colors, check the UIExamples application. (The icons are sourced from -[[ http://fortawesome.github.io/Font-Awesome/ | FontAwesome ]], so you can also +[[ https://fontawesome.com/v4.7.0/icons/ | FontAwesome ]], so you can also browse the collection there.) You can add `spin` to make the icon spin: From 73b68bc2a61910b6e6fc291a9109eb492f9dfaec Mon Sep 17 00:00:00 2001 From: Tino Breddin Date: Wed, 21 Mar 2018 07:39:34 -0700 Subject: [PATCH 07/20] Fix a possible `count(null)` in DifferentialRevisionActionTransaction Summary: This change prevents the following error when using PHP 7.2: ``` ERROR 2: count(): Parameter must be an array or an object that implements Countable at [/usr/local/lib/php/phabricator/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php:132] ``` A similar issue was fixed in D18964 Test Plan: Tested in a live system. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19242 --- .../xaction/DifferentialRevisionActionTransaction.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php index a1e66379f7..07809dca59 100644 --- a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php @@ -80,7 +80,7 @@ abstract class DifferentialRevisionActionTransaction DifferentialRevision $revision) { return array( array(), - null, + array(), ); } From 6ed123e0801a980e47624341e7f1f913694101cf Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Mar 2018 11:11:43 -0700 Subject: [PATCH 08/20] Propagate "unexpandable" PHIDs to feed notification recipient expansion Summary: See PHI466. Ref T13108. Somewhat recently, new rules were added so that "Resigning" from a revision takes you off the default recipient list, even if you're still a member of a project or package that is still a reviewer or subscriber. However, these rules don't currently apply to the similar expansion which occurs in notifications. If you resign from a revision you may still get some notifications (just not email) if a package or project you're a member of is a reviewer or subscriber. (Possibly these should eventually share more code, but just get things working for now.) Test Plan: - Created a revision as A. - Added B as a reviewer. - Added a package B is an owner for as a reviewer. - As B, resigned. (Make sure B is also not an explicit subscriber.) - Commented on the revision as A. - Before: B is included in the expanded notification recipient list. - After: B is no longer included in the expanded notification recipient list. Maniphest Tasks: T13108 Differential Revision: https://secure.phabricator.com/D19244 --- .../feed/PhabricatorFeedStoryPublisher.php | 38 ++++++++++++++++++- ...habricatorApplicationTransactionEditor.php | 6 +++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/applications/feed/PhabricatorFeedStoryPublisher.php b/src/applications/feed/PhabricatorFeedStoryPublisher.php index 8d018c61b3..47dde8c98a 100644 --- a/src/applications/feed/PhabricatorFeedStoryPublisher.php +++ b/src/applications/feed/PhabricatorFeedStoryPublisher.php @@ -12,6 +12,7 @@ final class PhabricatorFeedStoryPublisher extends Phobject { private $mailRecipientPHIDs = array(); private $notifyAuthor; private $mailTags = array(); + private $unexpandablePHIDs = array(); public function setMailTags(array $mail_tags) { $this->mailTags = $mail_tags; @@ -46,6 +47,15 @@ final class PhabricatorFeedStoryPublisher extends Phobject { return $this; } + public function setUnexpandablePHIDs(array $unexpandable_phids) { + $this->unexpandablePHIDs = $unexpandable_phids; + return $this; + } + + public function getUnexpandablePHIDs() { + return $this->unexpandablePHIDs; + } + public function setStoryType($story_type) { $this->storyType = $story_type; return $this; @@ -254,10 +264,36 @@ final class PhabricatorFeedStoryPublisher extends Phobject { } private function expandRecipients(array $phids) { - return id(new PhabricatorMetaMTAMemberQuery()) + $expanded_phids = id(new PhabricatorMetaMTAMemberQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withPHIDs($phids) ->executeExpansion(); + + // Filter out unexpandable PHIDs from the results. The typical case for + // this is that resigned reviewers should not be notified just because + // they are a member of some project or package reviewer. + + $original_map = array_fuse($phids); + $unexpandable_map = array_fuse($this->unexpandablePHIDs); + + foreach ($expanded_phids as $key => $phid) { + // We can keep this expanded PHID if it was present originally. + if (isset($original_map[$phid])) { + continue; + } + + // We can also keep it if it isn't marked as unexpandable. + if (!isset($unexpandable_map[$phid])) { + continue; + } + + // If it's unexpandable and we produced it by expanding recipients, + // throw it away. + unset($expanded_phids[$key]); + } + $expanded_phids = array_values($expanded_phids); + + return $expanded_phids; } /** diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 26be0f87af..8184eafb50 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -3199,6 +3199,11 @@ abstract class PhabricatorApplicationTransactionEditor $story_type = $this->getFeedStoryType(); $story_data = $this->getFeedStoryData($object, $xactions); + $unexpandable_phids = $this->mailUnexpandablePHIDs; + if (!is_array($unexpandable_phids)) { + $unexpandable_phids = array(); + } + id(new PhabricatorFeedStoryPublisher()) ->setStoryType($story_type) ->setStoryData($story_data) @@ -3207,6 +3212,7 @@ abstract class PhabricatorApplicationTransactionEditor ->setRelatedPHIDs($related_phids) ->setPrimaryObjectPHID($object->getPHID()) ->setSubscribedPHIDs($subscribed_phids) + ->setUnexpandablePHIDs($unexpandable_phids) ->setMailRecipientPHIDs($mailed_phids) ->setMailTags($this->getMailTags($object, $xactions)) ->publish(); From 9e278a89ba302307184389be63d390a6a8d2c7ff Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Mar 2018 11:42:58 -0700 Subject: [PATCH 09/20] If a Workflow form receives a redirect response, don't re-enable the submit buttons Summary: See PHI488. Ref T13108. Currently, there is a narrow window between when the response returns and when the browser actually follows the redirect where the form is live and you can click the button again. This is relativey easy if Phabricator is running //too fast// since the button may be disabled only momentarily. This seems to be easier in Firefox/Chrome than Safari. Test Plan: - In Firefox and Chrome, spam-clicked a comment submit button. - Before: could sometimes get a double-submit. - After: couldn't get a double-submit. - This could probably be reproduced more reliabily by adding a `sleep(1)` to whatever we're redirecting //to//. - Submitted an empty comment, got a dialog plus a still-enabled form (so this doesn't break the non-redirect case). Maniphest Tasks: T13108 Differential Revision: https://secure.phabricator.com/D19245 --- resources/celerity/map.php | 28 +++++++++---------- .../rsrc/externals/javelin/lib/Workflow.js | 15 +++++++--- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 562bd50fc1..dab7a5e395 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', 'core.pkg.css' => '6da3c0e5', - 'core.pkg.js' => '932d60d4', + 'core.pkg.js' => 'b305dbe2', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', 'diffusion.pkg.css' => 'a2d17c7d', @@ -253,7 +253,7 @@ return array( 'rsrc/externals/javelin/lib/URI.js' => 'c989ade3', 'rsrc/externals/javelin/lib/Vector.js' => '2caa8fb8', 'rsrc/externals/javelin/lib/WebSocket.js' => '3ffe32d6', - 'rsrc/externals/javelin/lib/Workflow.js' => '0eb1db0c', + 'rsrc/externals/javelin/lib/Workflow.js' => '33fea02f', 'rsrc/externals/javelin/lib/__tests__/Cookie.js' => '5ed109e8', 'rsrc/externals/javelin/lib/__tests__/DOM.js' => 'c984504b', 'rsrc/externals/javelin/lib/__tests__/JSON.js' => '837a7d68', @@ -739,7 +739,7 @@ return array( 'javelin-workboard-card' => 'c587b80f', 'javelin-workboard-column' => '758b4758', 'javelin-workboard-controller' => '26167537', - 'javelin-workflow' => '0eb1db0c', + 'javelin-workflow' => '33fea02f', 'maniphest-report-css' => '9b9580b7', 'maniphest-task-edit-css' => 'fda62a9b', 'maniphest-task-summary-css' => '11cc5344', @@ -960,17 +960,6 @@ return array( 'javelin-dom', 'javelin-router', ), - '0eb1db0c' => array( - 'javelin-stratcom', - 'javelin-request', - 'javelin-dom', - 'javelin-vector', - 'javelin-install', - 'javelin-util', - 'javelin-mask', - 'javelin-uri', - 'javelin-routable', - ), '0f764c35' => array( 'javelin-install', 'javelin-util', @@ -1108,6 +1097,17 @@ return array( 'javelin-util', 'javelin-magical-init', ), + '33fea02f' => array( + 'javelin-stratcom', + 'javelin-request', + 'javelin-dom', + 'javelin-vector', + 'javelin-install', + 'javelin-util', + 'javelin-mask', + 'javelin-uri', + 'javelin-routable', + ), '358b8c04' => array( 'javelin-install', 'javelin-util', diff --git a/webroot/rsrc/externals/javelin/lib/Workflow.js b/webroot/rsrc/externals/javelin/lib/Workflow.js index 995f204c5d..c3a1b90b44 100644 --- a/webroot/rsrc/externals/javelin/lib/Workflow.js +++ b/webroot/rsrc/externals/javelin/lib/Workflow.js @@ -59,12 +59,15 @@ JX.install('Workflow', { workflow.setDataWithListOfPairs(pairs); workflow.setMethod(form.getAttribute('method')); - workflow.listen('finally', function() { - // Re-enable form elements - for (var ii = 0; ii < inputs.length; ii++) { - inputs[ii] && (inputs[ii].disabled = false); + + var onfinally = JX.bind(workflow, function() { + if (!this._keepControlsDisabled) { + for (var ii = 0; ii < inputs.length; ii++) { + inputs[ii] && (inputs[ii].disabled = false); + } } }); + workflow.listen('finally', onfinally); return workflow; }, @@ -242,6 +245,7 @@ JX.install('Workflow', { _form: null, _paused: 0, _nextCallback: null, + _keepControlsDisabled: false, getSourceForm: function() { return this._form; @@ -283,6 +287,9 @@ JX.install('Workflow', { this._pop(); } + // If we're redirecting, don't re-enable for controls. + this._keepControlsDisabled = true; + JX.$U(r.redirect).go(); } else if (r && r.dialog) { this._push(); From c8583b016daf4931f359f128d4d7d96caa3b2139 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Mar 2018 11:51:12 -0700 Subject: [PATCH 10/20] When workflow dialog buttons are clicked, disable the button Summary: Depends on D19245. Fixes T11145. Ref T13108. See PHI488. Disable workflow buttons when they're clicked to prevent accidental client-side double submission. This might have some weird side effects but we should normally never need to re-use a workflow dialog form so it's not immediately obvious that this can break anything. Test Plan: - Added `sleep(1)` to the Mute controller and the Maniphest task controller. - Added `phlog(...)` to the Mute controller. - Opened the mute dialog, mashed the button a thousand times. - Before: Saw a bunch of logs. - After: Button immediately disables, saw only one log. Maniphest Tasks: T13108, T11145 Differential Revision: https://secure.phabricator.com/D19246 --- resources/celerity/map.php | 28 +++++++++---------- .../rsrc/externals/javelin/lib/Workflow.js | 4 +++ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index dab7a5e395..06ab100482 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', 'core.pkg.css' => '6da3c0e5', - 'core.pkg.js' => 'b305dbe2', + 'core.pkg.js' => '27f3489f', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', 'diffusion.pkg.css' => 'a2d17c7d', @@ -253,7 +253,7 @@ return array( 'rsrc/externals/javelin/lib/URI.js' => 'c989ade3', 'rsrc/externals/javelin/lib/Vector.js' => '2caa8fb8', 'rsrc/externals/javelin/lib/WebSocket.js' => '3ffe32d6', - 'rsrc/externals/javelin/lib/Workflow.js' => '33fea02f', + 'rsrc/externals/javelin/lib/Workflow.js' => '7dd6653c', 'rsrc/externals/javelin/lib/__tests__/Cookie.js' => '5ed109e8', 'rsrc/externals/javelin/lib/__tests__/DOM.js' => 'c984504b', 'rsrc/externals/javelin/lib/__tests__/JSON.js' => '837a7d68', @@ -739,7 +739,7 @@ return array( 'javelin-workboard-card' => 'c587b80f', 'javelin-workboard-column' => '758b4758', 'javelin-workboard-controller' => '26167537', - 'javelin-workflow' => '33fea02f', + 'javelin-workflow' => '7dd6653c', 'maniphest-report-css' => '9b9580b7', 'maniphest-task-edit-css' => 'fda62a9b', 'maniphest-task-summary-css' => '11cc5344', @@ -1097,17 +1097,6 @@ return array( 'javelin-util', 'javelin-magical-init', ), - '33fea02f' => array( - 'javelin-stratcom', - 'javelin-request', - 'javelin-dom', - 'javelin-vector', - 'javelin-install', - 'javelin-util', - 'javelin-mask', - 'javelin-uri', - 'javelin-routable', - ), '358b8c04' => array( 'javelin-install', 'javelin-util', @@ -1538,6 +1527,17 @@ return array( 'javelin-request', 'javelin-router', ), + '7dd6653c' => array( + 'javelin-stratcom', + 'javelin-request', + 'javelin-dom', + 'javelin-vector', + 'javelin-install', + 'javelin-util', + 'javelin-mask', + 'javelin-uri', + 'javelin-routable', + ), '7e41274a' => array( 'javelin-install', ), diff --git a/webroot/rsrc/externals/javelin/lib/Workflow.js b/webroot/rsrc/externals/javelin/lib/Workflow.js index c3a1b90b44..c669a98706 100644 --- a/webroot/rsrc/externals/javelin/lib/Workflow.js +++ b/webroot/rsrc/externals/javelin/lib/Workflow.js @@ -151,6 +151,10 @@ JX.install('Workflow', { // NOTE: Don't remove the current dialog yet because additional // handlers may still want to access the nodes. + // Disable whatever button the user clicked to prevent duplicate + // submission mistakes when you accidentally . See T11145. + button.disabled = true; + active .setURI(form.getAttribute('action') || active.getURI()) .setDataWithListOfPairs(data) From 859b2749709583e12ac46c6375cd77dbeecd56a1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Mar 2018 17:51:14 -0700 Subject: [PATCH 11/20] Provide more information to users during `git push` while waiting for write locks Summary: Ref T13109. Make it slightly more clear what the scope of the write and read locks are, and slightly more clear that we're actively acquiring locks, not just sitting around waiting. While waiting on another writer, show who we're waiting on so you can walk over to their desk and glare at them. Test Plan: Added `sleep(15)` after `willWrite()`. Pushed in two windows. Saw new, more informative messages. In the second window, saw the new guidance: > # Waiting for hector to finish writing (on device "repo1.local.phacility.net" for 11s)... Reviewers: asherkin Reviewed By: asherkin Subscribers: asherkin Maniphest Tasks: T13109 Differential Revision: https://secure.phabricator.com/D19247 --- .../DiffusionRepositoryClusterEngine.php | 60 ++++++++++++++++--- ...habricatorRepositoryWorkingCopyVersion.php | 32 ++++++++++ 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php index 35bb4d4acf..c855b6c532 100644 --- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php @@ -151,8 +151,8 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $this->logLine( pht( - 'Waiting up to %s second(s) for a cluster read lock on "%s"...', - new PhutilNumber($lock_wait), + 'Acquiring read lock for repository "%s" on device "%s"...', + $repository->getDisplayName(), $device->getName())); try { @@ -308,18 +308,34 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $write_lock->useSpecificConnection($locked_connection); - $lock_wait = phutil_units('2 minutes in seconds'); - $this->logLine( pht( - 'Waiting up to %s second(s) for a cluster write lock...', - new PhutilNumber($lock_wait))); + 'Acquiring write lock for repository "%s"...', + $repository->getDisplayName())); + $lock_wait = phutil_units('2 minutes in seconds'); try { $start = PhabricatorTime::getNow(); - $write_lock->lock($lock_wait); - $waited = (PhabricatorTime::getNow() - $start); + $step_wait = 1; + while (true) { + try { + $write_lock->lock((int)floor($step_wait)); + break; + } catch (PhutilLockException $ex) { + $waited = (PhabricatorTime::getNow() - $start); + if ($waited > $lock_wait) { + throw $ex; + } + $this->logActiveWriter($viewer, $repository); + } + + // Wait a little longer before the next message we print. + $step_wait = $step_wait + 0.5; + $step_wait = min($step_wait, 3); + } + + $waited = (PhabricatorTime::getNow() - $start); if ($waited) { $this->logLine( pht( @@ -763,4 +779,32 @@ final class DiffusionRepositoryClusterEngine extends Phobject { } } + private function logActiveWriter( + PhabricatorUser $viewer, + PhabricatorRepository $repository) { + + $writer = PhabricatorRepositoryWorkingCopyVersion::loadWriter( + $repository->getPHID()); + if (!$writer) { + $this->logLine(pht('Waiting on another user to finish writing...')); + return; + } + + $user_phid = $writer->getWriteProperty('userPHID'); + $device_phid = $writer->getWriteProperty('devicePHID'); + $epoch = $writer->getWriteProperty('epoch'); + + $phids = array($user_phid, $device_phid); + $handles = $viewer->loadHandles($phids); + + $duration = (PhabricatorTime::getNow() - $epoch) + 1; + + $this->logLine( + pht( + 'Waiting for %s to finish writing (on device "%s" for %ss)...', + $handles[$user_phid]->getName(), + $handles[$device_phid]->getName(), + new PhutilNumber($duration))); + } + } diff --git a/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php b/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php index 750cc91c47..da5d54b57d 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php +++ b/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php @@ -28,6 +28,17 @@ final class PhabricatorRepositoryWorkingCopyVersion ) + parent::getConfiguration(); } + public function getWriteProperty($key, $default = null) { + // The "writeProperties" don't currently get automatically serialized or + // deserialized. Perhaps they should. + try { + $properties = phutil_json_decode($this->writeProperties); + return idx($properties, $key, $default); + } catch (Exception $ex) { + return null; + } + } + public static function loadVersions($repository_phid) { $version = new self(); $conn_w = $version->establishConnection('w'); @@ -43,6 +54,27 @@ final class PhabricatorRepositoryWorkingCopyVersion return $version->loadAllFromArray($rows); } + public static function loadWriter($repository_phid) { + $version = new self(); + $conn_w = $version->establishConnection('w'); + $table = $version->getTableName(); + + // We're forcing this read to go to the master. + $row = queryfx_one( + $conn_w, + 'SELECT * FROM %T WHERE repositoryPHID = %s AND isWriting = 1 + LIMIT 1', + $table, + $repository_phid); + + if (!$row) { + return null; + } + + return $version->loadFromArray($row); + } + + public static function getReadLock($repository_phid, $device_phid) { $repository_hash = PhabricatorHash::digestForIndex($repository_phid); $device_hash = PhabricatorHash::digestForIndex($device_phid); From e010aaca4329d1ff401d4f13938ac4c8dab549d2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Mar 2018 10:58:56 -0700 Subject: [PATCH 12/20] accidentally a word Summary: Sometimes I dream I am a small turtle. Test Plan: squeak squeak Differential Revision: https://secure.phabricator.com/D19248 --- resources/celerity/map.php | 28 +++++++++---------- .../rsrc/externals/javelin/lib/Workflow.js | 3 +- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 06ab100482..cb646d386e 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', 'core.pkg.css' => '6da3c0e5', - 'core.pkg.js' => '27f3489f', + 'core.pkg.js' => 'b9b4a943', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', 'diffusion.pkg.css' => 'a2d17c7d', @@ -253,7 +253,7 @@ return array( 'rsrc/externals/javelin/lib/URI.js' => 'c989ade3', 'rsrc/externals/javelin/lib/Vector.js' => '2caa8fb8', 'rsrc/externals/javelin/lib/WebSocket.js' => '3ffe32d6', - 'rsrc/externals/javelin/lib/Workflow.js' => '7dd6653c', + 'rsrc/externals/javelin/lib/Workflow.js' => '6a726c55', 'rsrc/externals/javelin/lib/__tests__/Cookie.js' => '5ed109e8', 'rsrc/externals/javelin/lib/__tests__/DOM.js' => 'c984504b', 'rsrc/externals/javelin/lib/__tests__/JSON.js' => '837a7d68', @@ -739,7 +739,7 @@ return array( 'javelin-workboard-card' => 'c587b80f', 'javelin-workboard-column' => '758b4758', 'javelin-workboard-controller' => '26167537', - 'javelin-workflow' => '7dd6653c', + 'javelin-workflow' => '6a726c55', 'maniphest-report-css' => '9b9580b7', 'maniphest-task-edit-css' => 'fda62a9b', 'maniphest-task-summary-css' => '11cc5344', @@ -1425,6 +1425,17 @@ return array( '69adf288' => array( 'javelin-install', ), + '6a726c55' => array( + 'javelin-stratcom', + 'javelin-request', + 'javelin-dom', + 'javelin-vector', + 'javelin-install', + 'javelin-util', + 'javelin-mask', + 'javelin-uri', + 'javelin-routable', + ), '6b31879a' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1527,17 +1538,6 @@ return array( 'javelin-request', 'javelin-router', ), - '7dd6653c' => array( - 'javelin-stratcom', - 'javelin-request', - 'javelin-dom', - 'javelin-vector', - 'javelin-install', - 'javelin-util', - 'javelin-mask', - 'javelin-uri', - 'javelin-routable', - ), '7e41274a' => array( 'javelin-install', ), diff --git a/webroot/rsrc/externals/javelin/lib/Workflow.js b/webroot/rsrc/externals/javelin/lib/Workflow.js index c669a98706..d767a58780 100644 --- a/webroot/rsrc/externals/javelin/lib/Workflow.js +++ b/webroot/rsrc/externals/javelin/lib/Workflow.js @@ -152,7 +152,8 @@ JX.install('Workflow', { // handlers may still want to access the nodes. // Disable whatever button the user clicked to prevent duplicate - // submission mistakes when you accidentally . See T11145. + // submission mistakes when you accidentally click a button multiple + // times. See T11145. button.disabled = true; active From 69bff489d4ec80db3f8ba843b7d0f7ba6ff8656b Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Mar 2018 12:42:08 -0700 Subject: [PATCH 13/20] Generate a random unique "Request ID" for SSH requests so processes can coordinate better Summary: Depends on D19247. Ref T13109. When we receive an SSH request, generate a random unique ID for the request. Then thread it down through the process tree. The immediate goal is to let the `ssh-exec` process coordinate with `commit-hook` process and log information about read and write lock wait times. Today, there's no way for `ssh-exec` to interact with the `PushEvent`, but this is the most helpful place to store this data for users. Test Plan: Made pushes, saw the `PushEvent` table populate with a random request ID. Exported data and saw the ID preserved in the export. Maniphest Tasks: T13109 Differential Revision: https://secure.phabricator.com/D19249 --- .../20180322.lock.01.identifier.sql | 5 ++++ scripts/repository/commit_hook.php | 5 ++++ scripts/ssh/ssh-exec.php | 7 +++++ .../PhabricatorAccessLogConfigOptions.php | 4 +++ .../engine/DiffusionCommitHookEngine.php | 26 ++++++++++++++++--- .../diffusion/ssh/DiffusionSSHWorkflow.php | 5 ++++ ...abricatorRepositoryPushLogSearchEngine.php | 4 +++ .../PhabricatorRepositoryPushEvent.php | 6 +++++ .../ssh/PhabricatorSSHWorkflow.php | 10 +++++++ 9 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 resources/sql/autopatches/20180322.lock.01.identifier.sql diff --git a/resources/sql/autopatches/20180322.lock.01.identifier.sql b/resources/sql/autopatches/20180322.lock.01.identifier.sql new file mode 100644 index 0000000000..b115a691fa --- /dev/null +++ b/resources/sql/autopatches/20180322.lock.01.identifier.sql @@ -0,0 +1,5 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_pushevent + ADD requestIdentifier VARBINARY(12); + +ALTER TABLE {$NAMESPACE}_repository.repository_pushevent + ADD UNIQUE KEY `key_request` (requestIdentifier); diff --git a/scripts/repository/commit_hook.php b/scripts/repository/commit_hook.php index 51abcb6c89..07d6d7cfa2 100755 --- a/scripts/repository/commit_hook.php +++ b/scripts/repository/commit_hook.php @@ -187,6 +187,11 @@ if (strlen($remote_protocol)) { $engine->setRemoteProtocol($remote_protocol); } +$request_identifier = getenv(DiffusionCommitHookEngine::ENV_REQUEST); +if (strlen($request_identifier)) { + $engine->setRequestIdentifier($request_identifier); +} + try { $err = $engine->execute(); } catch (DiffusionCommitHookRejectException $ex) { diff --git a/scripts/ssh/ssh-exec.php b/scripts/ssh/ssh-exec.php index 2ff1bdc198..0f2275cda8 100755 --- a/scripts/ssh/ssh-exec.php +++ b/scripts/ssh/ssh-exec.php @@ -8,6 +8,12 @@ require_once $root.'/scripts/__init_script__.php'; $ssh_log = PhabricatorSSHLog::getLog(); +$request_identifier = Filesystem::readRandomCharacters(12); +$ssh_log->setData( + array( + 'Q' => $request_identifier, + )); + $args = new PhutilArgumentParser($argv); $args->setTagline(pht('execute SSH requests')); $args->setSynopsis(<<setSSHUser($user); $workflow->setOriginalArguments($original_argv); $workflow->setIsClusterRequest($is_cluster_request); + $workflow->setRequestIdentifier($request_identifier); $sock_stdin = fopen('php://stdin', 'r'); if (!$sock_stdin) { diff --git a/src/applications/config/option/PhabricatorAccessLogConfigOptions.php b/src/applications/config/option/PhabricatorAccessLogConfigOptions.php index 9ae60825ea..2b9c4bbc75 100644 --- a/src/applications/config/option/PhabricatorAccessLogConfigOptions.php +++ b/src/applications/config/option/PhabricatorAccessLogConfigOptions.php @@ -47,6 +47,10 @@ final class PhabricatorAccessLogConfigOptions 's' => pht('The system user.'), 'S' => pht('The system sudo user.'), 'k' => pht('ID of the SSH key used to authenticate the request.'), + + // TODO: This is a reasonable thing to support in the HTTP access + // log, too. + 'Q' => pht('A random, unique string which identifies the request.'), ); $http_desc = pht( diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index a0769a51f0..cc4526dbdc 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -12,6 +12,7 @@ final class DiffusionCommitHookEngine extends Phobject { const ENV_REPOSITORY = 'PHABRICATOR_REPOSITORY'; const ENV_USER = 'PHABRICATOR_USER'; + const ENV_REQUEST = 'PHABRICATOR_REQUEST'; const ENV_REMOTE_ADDRESS = 'PHABRICATOR_REMOTE_ADDRESS'; const ENV_REMOTE_PROTOCOL = 'PHABRICATOR_REMOTE_PROTOCOL'; @@ -25,6 +26,7 @@ final class DiffusionCommitHookEngine extends Phobject { private $subversionRepository; private $remoteAddress; private $remoteProtocol; + private $requestIdentifier; private $transactionKey; private $mercurialHook; private $mercurialCommits = array(); @@ -58,6 +60,15 @@ final class DiffusionCommitHookEngine extends Phobject { return $this->remoteAddress; } + public function setRequestIdentifier($request_identifier) { + $this->requestIdentifier = $request_identifier; + return $this; + } + + public function getRequestIdentifier() { + return $this->requestIdentifier; + } + public function setSubversionTransactionInfo($transaction, $repository) { $this->subversionTransaction = $transaction; $this->subversionRepository = $repository; @@ -620,6 +631,7 @@ final class DiffusionCommitHookEngine extends Phobject { $env = array( self::ENV_REPOSITORY => $this->getRepository()->getPHID(), self::ENV_USER => $this->getViewer()->getUsername(), + self::ENV_REQUEST => $this->getRequestIdentifier(), self::ENV_REMOTE_PROTOCOL => $this->getRemoteProtocol(), self::ENV_REMOTE_ADDRESS => $this->getRemoteAddress(), ); @@ -1081,16 +1093,24 @@ final class DiffusionCommitHookEngine extends Phobject { ->setDevicePHID($device_phid) ->setRepositoryPHID($this->getRepository()->getPHID()) ->attachRepository($this->getRepository()) - ->setEpoch(time()); + ->setEpoch(PhabricatorTime::getNow()); } private function newPushEvent() { $viewer = $this->getViewer(); - return PhabricatorRepositoryPushEvent::initializeNewEvent($viewer) + + $event = PhabricatorRepositoryPushEvent::initializeNewEvent($viewer) ->setRepositoryPHID($this->getRepository()->getPHID()) ->setRemoteAddress($this->getRemoteAddress()) ->setRemoteProtocol($this->getRemoteProtocol()) - ->setEpoch(time()); + ->setEpoch(PhabricatorTime::getNow()); + + $identifier = $this->getRequestIdentifier(); + if (strlen($identifier)) { + $event->setRequestIdentifier($identifier); + } + + return $event; } private function rejectEnormousChanges(array $content_updates) { diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php index e40d8e1f51..baf1749252 100644 --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php @@ -30,6 +30,11 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { DiffusionCommitHookEngine::ENV_REMOTE_PROTOCOL => 'ssh', ); + $identifier = $this->getRequestIdentifier(); + if ($identifier !== null) { + $env[DiffusionCommitHookEngine::ENV_REQUEST] = $identifier; + } + $remote_address = $this->getSSHRemoteAddress(); if ($remote_address !== null) { $env[DiffusionCommitHookEngine::ENV_REMOTE_ADDRESS] = $remote_address; diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php index 8ad76987f9..5bf2c84aeb 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php +++ b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php @@ -101,6 +101,9 @@ final class PhabricatorRepositoryPushLogSearchEngine $fields[] = id(new PhabricatorIDExportField()) ->setKey('pushID') ->setLabel(pht('Push ID')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('unique') + ->setLabel(pht('Unique')), $fields[] = id(new PhabricatorStringExportField()) ->setKey('protocol') ->setLabel(pht('Protocol')), @@ -209,6 +212,7 @@ final class PhabricatorRepositoryPushLogSearchEngine $map = array( 'pushID' => $event->getID(), + 'unique' => $event->getRequestIdentifier(), 'protocol' => $event->getRemoteProtocol(), 'repositoryPHID' => $repository_phid, 'repository' => $repository_name, diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php index 2bc751ffca..369af15635 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php +++ b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php @@ -11,6 +11,7 @@ final class PhabricatorRepositoryPushEvent protected $repositoryPHID; protected $epoch; protected $pusherPHID; + protected $requestIdentifier; protected $remoteAddress; protected $remoteProtocol; protected $rejectCode; @@ -29,6 +30,7 @@ final class PhabricatorRepositoryPushEvent self::CONFIG_AUX_PHID => true, self::CONFIG_TIMESTAMPS => false, self::CONFIG_COLUMN_SCHEMA => array( + 'requestIdentifier' => 'bytes12?', 'remoteAddress' => 'ipaddress?', 'remoteProtocol' => 'text32?', 'rejectCode' => 'uint32', @@ -38,6 +40,10 @@ final class PhabricatorRepositoryPushEvent 'key_repository' => array( 'columns' => array('repositoryPHID'), ), + 'key_request' => array( + 'columns' => array('requestIdentifier'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } diff --git a/src/infrastructure/ssh/PhabricatorSSHWorkflow.php b/src/infrastructure/ssh/PhabricatorSSHWorkflow.php index 6b6222304c..f7739f1332 100644 --- a/src/infrastructure/ssh/PhabricatorSSHWorkflow.php +++ b/src/infrastructure/ssh/PhabricatorSSHWorkflow.php @@ -13,6 +13,7 @@ abstract class PhabricatorSSHWorkflow private $errorChannel; private $isClusterRequest; private $originalArguments; + private $requestIdentifier; public function isExecutable() { return false; @@ -89,6 +90,15 @@ abstract class PhabricatorSSHWorkflow return $this->originalArguments; } + public function setRequestIdentifier($request_identifier) { + $this->requestIdentifier = $request_identifier; + return $this; + } + + public function getRequestIdentifier() { + return $this->requestIdentifier; + } + public function getSSHRemoteAddress() { $ssh_client = getenv('SSH_CLIENT'); if (!strlen($ssh_client)) { From df3c937dab8c0c56289f9fe59e95e57eaa703417 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Mar 2018 12:08:20 -0700 Subject: [PATCH 14/20] Record lock timing information on PushEvents Summary: Depends on D19249. Ref T13109. Add timing information to the `PushEvent`: - `writeWait`: Time spent waiting for a write lock. - `readWait`: Time spent waiting for a read lock. - `hostWait`: Roughly, total time spent on the leaf node. The primary goal here is to see if `readWait` is meaningful in the wild. If it is, that motivates smarter routing, and the value of smarter routing can be demonstrated by looking for a reduction in read wait times. Test Plan: Pushed some stuff, saw reasonable timing values in the table. Saw timing information in "Export Data". Maniphest Tasks: T13109 Differential Revision: https://secure.phabricator.com/D19250 --- .../sql/autopatches/20180322.lock.02.wait.sql | 8 ++++ .../DiffusionRepositoryClusterEngine.php | 14 ++++++ ...ionRepositoryClusterEngineLogInterface.php | 1 + .../DiffusionGitReceivePackSSHWorkflow.php | 43 +++++++++++++++++ .../diffusion/ssh/DiffusionGitSSHWorkflow.php | 10 ++++ ...abricatorRepositoryPushLogSearchEngine.php | 46 ++++++++++++------- .../PhabricatorRepositoryPushEvent.php | 6 +++ 7 files changed, 111 insertions(+), 17 deletions(-) create mode 100644 resources/sql/autopatches/20180322.lock.02.wait.sql diff --git a/resources/sql/autopatches/20180322.lock.02.wait.sql b/resources/sql/autopatches/20180322.lock.02.wait.sql new file mode 100644 index 0000000000..cba7cc64d0 --- /dev/null +++ b/resources/sql/autopatches/20180322.lock.02.wait.sql @@ -0,0 +1,8 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_pushevent + ADD writeWait BIGINT UNSIGNED; + +ALTER TABLE {$NAMESPACE}_repository.repository_pushevent + ADD readWait BIGINT UNSIGNED; + +ALTER TABLE {$NAMESPACE}_repository.repository_pushevent + ADD hostWait BIGINT UNSIGNED; diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php index c855b6c532..d5ba74a30e 100644 --- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php @@ -315,12 +315,15 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $lock_wait = phutil_units('2 minutes in seconds'); try { + $write_wait_start = microtime(true); + $start = PhabricatorTime::getNow(); $step_wait = 1; while (true) { try { $write_lock->lock((int)floor($step_wait)); + $write_wait_end = microtime(true); break; } catch (PhutilLockException $ex) { $waited = (PhabricatorTime::getNow() - $start); @@ -370,12 +373,14 @@ final class DiffusionRepositoryClusterEngine extends Phobject { 'documentation for instructions.')); } + $read_wait_start = microtime(true); try { $max_version = $this->synchronizeWorkingCopyBeforeRead(); } catch (Exception $ex) { $write_lock->unlock(); throw $ex; } + $read_wait_end = microtime(true); $pid = getmypid(); $hash = Filesystem::readRandomCharacters(12); @@ -394,6 +399,15 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $this->clusterWriteVersion = $max_version; $this->clusterWriteLock = $write_lock; + + $write_wait = ($write_wait_end - $write_wait_start); + $read_wait = ($read_wait_end - $read_wait_start); + + $log = $this->logger; + if ($log) { + $log->writeClusterEngineLogProperty('readWait', $read_wait); + $log->writeClusterEngineLogProperty('writeWait', $write_wait); + } } diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php index 9b1fe9a506..3e43d72779 100644 --- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php @@ -3,5 +3,6 @@ interface DiffusionRepositoryClusterEngineLogInterface { public function writeClusterEngineLogMessage($message); + public function writeClusterEngineLogProperty($key, $value); } diff --git a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php index 91c8238718..400340abeb 100644 --- a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php @@ -14,6 +14,8 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow { } protected function executeRepositoryOperations() { + $host_wait_start = microtime(true); + $repository = $this->getRepository(); $viewer = $this->getSSHUser(); $device = AlmanacKeys::getLiveDevice(); @@ -71,6 +73,14 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow { PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE, PhabricatorRepositoryStatusMessage::CODE_OKAY); $this->waitForGitClient(); + + $host_wait_end = microtime(true); + + $this->updatePushLogWithTimingInformation( + $this->getClusterEngineLogProperty('writeWait'), + $this->getClusterEngineLogProperty('readWait'), + ($host_wait_end - $host_wait_start)); + } return $err; @@ -89,4 +99,37 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow { ->execute(); } + private function updatePushLogWithTimingInformation( + $write_wait, + $read_wait, + $host_wait) { + + if ($write_wait !== null) { + $write_wait = (int)(1000000 * $write_wait); + } + + if ($read_wait !== null) { + $read_wait = (int)(1000000 * $read_wait); + } + + if ($host_wait !== null) { + $host_wait = (int)(1000000 * $host_wait); + } + + $identifier = $this->getRequestIdentifier(); + + $event = new PhabricatorRepositoryPushEvent(); + $conn = $event->establishConnection('w'); + + queryfx( + $conn, + 'UPDATE %T SET writeWait = %nd, readWait = %nd, hostWait = %nd + WHERE requestIdentifier = %s', + $event->getTableName(), + $write_wait, + $read_wait, + $host_wait, + $identifier); + } + } diff --git a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php index 6de16e723d..6bc56d767d 100644 --- a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php @@ -4,6 +4,8 @@ abstract class DiffusionGitSSHWorkflow extends DiffusionSSHWorkflow implements DiffusionRepositoryClusterEngineLogInterface { + private $engineLogProperties = array(); + protected function writeError($message) { // Git assumes we'll add our own newlines. return parent::writeError($message."\n"); @@ -14,6 +16,14 @@ abstract class DiffusionGitSSHWorkflow $this->getErrorChannel()->update(); } + public function writeClusterEngineLogProperty($key, $value) { + $this->engineLogProperties[$key] = $value; + } + + protected function getClusterEngineLogProperty($key, $default = null) { + return idx($this->engineLogProperties, $key, $default); + } + protected function identifyRepository() { $args = $this->getArgs(); $path = head($args->getArg('dir')); diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php index 5bf2c84aeb..7b626fff32 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php +++ b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php @@ -98,57 +98,66 @@ final class PhabricatorRepositoryPushLogSearchEngine $viewer = $this->requireViewer(); $fields = array( - $fields[] = id(new PhabricatorIDExportField()) + id(new PhabricatorIDExportField()) ->setKey('pushID') ->setLabel(pht('Push ID')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('unique') ->setLabel(pht('Unique')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('protocol') ->setLabel(pht('Protocol')), - $fields[] = id(new PhabricatorPHIDExportField()) + id(new PhabricatorPHIDExportField()) ->setKey('repositoryPHID') ->setLabel(pht('Repository PHID')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('repository') ->setLabel(pht('Repository')), - $fields[] = id(new PhabricatorPHIDExportField()) + id(new PhabricatorPHIDExportField()) ->setKey('pusherPHID') ->setLabel(pht('Pusher PHID')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('pusher') ->setLabel(pht('Pusher')), - $fields[] = id(new PhabricatorPHIDExportField()) + id(new PhabricatorPHIDExportField()) ->setKey('devicePHID') ->setLabel(pht('Device PHID')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('device') ->setLabel(pht('Device')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('type') ->setLabel(pht('Ref Type')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('name') ->setLabel(pht('Ref Name')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('old') ->setLabel(pht('Ref Old')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('new') ->setLabel(pht('Ref New')), - $fields[] = id(new PhabricatorIntExportField()) + id(new PhabricatorIntExportField()) ->setKey('flags') ->setLabel(pht('Flags')), - $fields[] = id(new PhabricatorStringListExportField()) + id(new PhabricatorStringListExportField()) ->setKey('flagNames') ->setLabel(pht('Flag Names')), - $fields[] = id(new PhabricatorIntExportField()) + id(new PhabricatorIntExportField()) ->setKey('result') ->setLabel(pht('Result')), - $fields[] = id(new PhabricatorStringExportField()) + id(new PhabricatorStringExportField()) ->setKey('resultName') ->setLabel(pht('Result Name')), + id(new PhabricatorIntExportField()) + ->setKey('writeWait') + ->setLabel(pht('Write Wait (us)')), + id(new PhabricatorIntExportField()) + ->setKey('readWait') + ->setLabel(pht('Read Wait (us)')), + id(new PhabricatorIntExportField()) + ->setKey('hostWait') + ->setLabel(pht('Host Wait (us)')), ); if ($viewer->getIsAdmin()) { @@ -228,6 +237,9 @@ final class PhabricatorRepositoryPushLogSearchEngine 'flagNames' => $flag_names, 'result' => $result, 'resultName' => $result_name, + 'writeWait' => $event->getWriteWait(), + 'readWait' => $event->getReadWait(), + 'hostWait' => $event->getHostWait(), ); if ($viewer->getIsAdmin()) { diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php index 369af15635..451f8acda5 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php +++ b/src/applications/repository/storage/PhabricatorRepositoryPushEvent.php @@ -16,6 +16,9 @@ final class PhabricatorRepositoryPushEvent protected $remoteProtocol; protected $rejectCode; protected $rejectDetails; + protected $writeWait; + protected $readWait; + protected $hostWait; private $repository = self::ATTACHABLE; private $logs = self::ATTACHABLE; @@ -35,6 +38,9 @@ final class PhabricatorRepositoryPushEvent 'remoteProtocol' => 'text32?', 'rejectCode' => 'uint32', 'rejectDetails' => 'text64?', + 'writeWait' => 'uint64?', + 'readWait' => 'uint64?', + 'hostWait' => 'uint64?', ), self::CONFIG_KEY_SCHEMA => array( 'key_repository' => array( From 8b658706a8c4989e7a22b4af23703c04f8b4fb47 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Mar 2018 03:52:11 -0700 Subject: [PATCH 15/20] Add a basic Remarkup document rendering engine Summary: Ref T13105. Although Markdown is trickier to deal with, we can handle Remarkup easily. This may need some support for encoding options. Test Plan: Viewed `.remarkup` files, got remarkup document presentation by default. Viewed other text files, got an option to render as remarkup. Reviewers: avivey Reviewed By: avivey Subscribers: mydeveloperday, avivey Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19251 --- resources/celerity/map.php | 6 +-- src/__phutil_library_map__.php | 2 + .../files/document/PhabricatorDocumentRef.php | 14 ++++++ .../PhabricatorRemarkupDocumentEngine.php | 47 +++++++++++++++++++ .../rsrc/css/phui/phui-property-list-view.css | 4 ++ 5 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 src/applications/files/document/PhabricatorRemarkupDocumentEngine.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index cb646d386e..6fcfc52bab 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => '6da3c0e5', + 'core.pkg.css' => 'afe29a6c', 'core.pkg.js' => 'b9b4a943', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -168,7 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => '9cff003c', 'rsrc/css/phui/phui-pager.css' => 'edcbc226', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '6ef560df', + 'rsrc/css/phui/phui-property-list-view.css' => '94a14381', 'rsrc/css/phui/phui-remarkup-preview.css' => '54a34863', 'rsrc/css/phui/phui-segment-bar-view.css' => 'b1d1b892', 'rsrc/css/phui/phui-spacing.css' => '042804d6', @@ -850,7 +850,7 @@ return array( 'phui-oi-simple-ui-css' => 'a8beebea', 'phui-pager-css' => 'edcbc226', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '6ef560df', + 'phui-property-list-view-css' => '94a14381', 'phui-remarkup-preview-css' => '54a34863', 'phui-segment-bar-view-css' => 'b1d1b892', 'phui-spacing-css' => '042804d6', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index aef91c2e6b..ff7c145fb7 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3967,6 +3967,7 @@ phutil_register_library_map(array( 'PhabricatorRemarkupCowsayBlockInterpreter' => 'infrastructure/markup/interpreter/PhabricatorRemarkupCowsayBlockInterpreter.php', 'PhabricatorRemarkupCustomBlockRule' => 'infrastructure/markup/rule/PhabricatorRemarkupCustomBlockRule.php', 'PhabricatorRemarkupCustomInlineRule' => 'infrastructure/markup/rule/PhabricatorRemarkupCustomInlineRule.php', + 'PhabricatorRemarkupDocumentEngine' => 'applications/files/document/PhabricatorRemarkupDocumentEngine.php', 'PhabricatorRemarkupEditField' => 'applications/transactions/editfield/PhabricatorRemarkupEditField.php', 'PhabricatorRemarkupFigletBlockInterpreter' => 'infrastructure/markup/interpreter/PhabricatorRemarkupFigletBlockInterpreter.php', 'PhabricatorRemarkupUIExample' => 'applications/uiexample/examples/PhabricatorRemarkupUIExample.php', @@ -9710,6 +9711,7 @@ phutil_register_library_map(array( 'PhabricatorRemarkupCowsayBlockInterpreter' => 'PhutilRemarkupBlockInterpreter', 'PhabricatorRemarkupCustomBlockRule' => 'PhutilRemarkupBlockRule', 'PhabricatorRemarkupCustomInlineRule' => 'PhutilRemarkupRule', + 'PhabricatorRemarkupDocumentEngine' => 'PhabricatorDocumentEngine', 'PhabricatorRemarkupEditField' => 'PhabricatorEditField', 'PhabricatorRemarkupFigletBlockInterpreter' => 'PhutilRemarkupBlockInterpreter', 'PhabricatorRemarkupUIExample' => 'PhabricatorUIExample', diff --git a/src/applications/files/document/PhabricatorDocumentRef.php b/src/applications/files/document/PhabricatorDocumentRef.php index 862c0596d3..6b4b6a6796 100644 --- a/src/applications/files/document/PhabricatorDocumentRef.php +++ b/src/applications/files/document/PhabricatorDocumentRef.php @@ -7,6 +7,7 @@ final class PhabricatorDocumentRef private $mimeType; private $file; private $byteLength; + private $snippet; public function setFile(PhabricatorFile $file) { $this->file = $file; @@ -104,4 +105,17 @@ final class PhabricatorDocumentRef return $mime_type; } + public function isProbablyText() { + $snippet = $this->getSnippet(); + return (strpos($snippet, "\0") === false); + } + + public function getSnippet() { + if ($this->snippet === null) { + $this->snippet = $this->loadData(null, (1024 * 1024 * 1)); + } + + return $this->snippet; + } + } diff --git a/src/applications/files/document/PhabricatorRemarkupDocumentEngine.php b/src/applications/files/document/PhabricatorRemarkupDocumentEngine.php new file mode 100644 index 0000000000..296b78196f --- /dev/null +++ b/src/applications/files/document/PhabricatorRemarkupDocumentEngine.php @@ -0,0 +1,47 @@ +getName(); + if (preg_match('/\\.remarkup\z/i', $name)) { + return 2000; + } + + return 500; + } + + protected function canRenderDocumentType(PhabricatorDocumentRef $ref) { + return $ref->isProbablyText(); + } + + protected function newDocumentContent(PhabricatorDocumentRef $ref) { + $viewer = $this->getViewer(); + + $content = $ref->loadData(); + $content = phutil_utf8ize($content); + + $remarkup = new PHUIRemarkupView($viewer, $content); + + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-remarkup', + ), + $remarkup); + + return $container; + } + +} diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index 840364723d..685c01fd0a 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -244,3 +244,7 @@ div.phui-property-list-stacked .phui-property-list-properties margin: 20px; white-space: pre; } + +.document-engine-remarkup { + margin: 20px; +} From fb4ce851c4aeac84b4e1a264bc09019aeb9149f8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Mar 2018 04:50:48 -0700 Subject: [PATCH 16/20] Add a PDF document "rendering" engine Summary: Depends on D19251. Ref T13105. This adds rendering engine support for PDFs. It doesn't actually render them, it just renders a link which you can click to view them in a new window. This is much easier than actually rendering them inline and at least 95% as good most of the time (and probably more-than-100%-as-good some of the time). This makes PDF a viewable MIME type by default and adds a narrow CSP exception for it. See also T13112. Test Plan: - Viewed PDFs in Files, got a link to view them in a new tab. - Clicked the link in Safari, Chrome, and Firefox; got inline PDFs. - Verified primary CSP is still `object-src 'none'` with `curl ...`. - Interacted with the vanilla lightbox element to check that it still works. Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19252 --- resources/celerity/map.php | 10 ++-- src/__phutil_library_map__.php | 2 + src/aphront/response/AphrontResponse.php | 7 ++- .../config/PhabricatorFilesConfigOptions.php | 2 + .../PhabricatorFileDataController.php | 20 ++++++- .../document/PhabricatorPDFDocumentEngine.php | 57 ++++++++++++++++++ .../files/storage/PhabricatorFile.php | 13 +++++ src/view/layout/PhabricatorFileLinkView.php | 58 ++++++++++++------- webroot/rsrc/css/core/remarkup.css | 6 +- .../rsrc/css/phui/phui-property-list-view.css | 9 +++ 10 files changed, 154 insertions(+), 30 deletions(-) create mode 100644 src/applications/files/document/PhabricatorPDFDocumentEngine.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 6fcfc52bab..0d80821050 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => 'afe29a6c', + 'core.pkg.css' => '2d73b2f3', 'core.pkg.js' => 'b9b4a943', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -112,7 +112,7 @@ return array( 'rsrc/css/application/tokens/tokens.css' => '3d0f239e', 'rsrc/css/application/uiexample/example.css' => '528b19de', 'rsrc/css/core/core.css' => '62fa3ace', - 'rsrc/css/core/remarkup.css' => '97dc3523', + 'rsrc/css/core/remarkup.css' => 'b375546d', 'rsrc/css/core/syntax.css' => 'cae95e89', 'rsrc/css/core/z-index.css' => '9d8f7c4b', 'rsrc/css/diviner/diviner-shared.css' => '896f1d43', @@ -168,7 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => '9cff003c', 'rsrc/css/phui/phui-pager.css' => 'edcbc226', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '94a14381', + 'rsrc/css/phui/phui-property-list-view.css' => '47018d3c', 'rsrc/css/phui/phui-remarkup-preview.css' => '54a34863', 'rsrc/css/phui/phui-segment-bar-view.css' => 'b1d1b892', 'rsrc/css/phui/phui-spacing.css' => '042804d6', @@ -780,7 +780,7 @@ return array( 'phabricator-object-selector-css' => '85ee8ce6', 'phabricator-phtize' => 'd254d646', 'phabricator-prefab' => '77b0ae28', - 'phabricator-remarkup-css' => '97dc3523', + 'phabricator-remarkup-css' => 'b375546d', 'phabricator-search-results-css' => '505dd8cf', 'phabricator-shaped-request' => '7cbe244b', 'phabricator-slowvote-css' => 'a94b7230', @@ -850,7 +850,7 @@ return array( 'phui-oi-simple-ui-css' => 'a8beebea', 'phui-pager-css' => 'edcbc226', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '94a14381', + 'phui-property-list-view-css' => '47018d3c', 'phui-remarkup-preview-css' => '54a34863', 'phui-segment-bar-view-css' => 'b1d1b892', 'phui-spacing-css' => '042804d6', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ff7c145fb7..6090161840 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3519,6 +3519,7 @@ phutil_register_library_map(array( 'PhabricatorOwnersPathsSearchEngineAttachment' => 'applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php', 'PhabricatorOwnersSchemaSpec' => 'applications/owners/storage/PhabricatorOwnersSchemaSpec.php', 'PhabricatorOwnersSearchField' => 'applications/owners/searchfield/PhabricatorOwnersSearchField.php', + 'PhabricatorPDFDocumentEngine' => 'applications/files/document/PhabricatorPDFDocumentEngine.php', 'PhabricatorPHDConfigOptions' => 'applications/config/option/PhabricatorPHDConfigOptions.php', 'PhabricatorPHID' => 'applications/phid/storage/PhabricatorPHID.php', 'PhabricatorPHIDConstants' => 'applications/phid/PhabricatorPHIDConstants.php', @@ -9169,6 +9170,7 @@ phutil_register_library_map(array( 'PhabricatorOwnersPathsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'PhabricatorOwnersSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhabricatorOwnersSearchField' => 'PhabricatorSearchTokenizerField', + 'PhabricatorPDFDocumentEngine' => 'PhabricatorDocumentEngine', 'PhabricatorPHDConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorPHID' => 'Phobject', 'PhabricatorPHIDConstants' => 'Phobject', diff --git a/src/aphront/response/AphrontResponse.php b/src/aphront/response/AphrontResponse.php index 2ee222d61c..fe1e80318f 100644 --- a/src/aphront/response/AphrontResponse.php +++ b/src/aphront/response/AphrontResponse.php @@ -28,6 +28,7 @@ abstract class AphrontResponse extends Phobject { 'connect-src' => array(), 'frame-src' => array(), 'form-action' => array(), + 'object-src' => array(), ); } @@ -163,8 +164,10 @@ abstract class AphrontResponse extends Phobject { $csp[] = "frame-ancestors 'none'"; } - // Block relics of the old world: Flash, Java applets, and so on. - $csp[] = "object-src 'none'"; + // Block relics of the old world: Flash, Java applets, and so on. Note + // that Chrome prevents the user from viewing PDF documents if they are + // served with a policy which excludes the domain they are served from. + $csp[] = $this->newContentSecurityPolicy('object-src', "'none'"); // Don't allow forms to submit offsite. diff --git a/src/applications/files/config/PhabricatorFilesConfigOptions.php b/src/applications/files/config/PhabricatorFilesConfigOptions.php index 1493c54f59..751a06ffdb 100644 --- a/src/applications/files/config/PhabricatorFilesConfigOptions.php +++ b/src/applications/files/config/PhabricatorFilesConfigOptions.php @@ -45,6 +45,8 @@ final class PhabricatorFilesConfigOptions 'video/ogg' => 'video/ogg', 'video/webm' => 'video/webm', 'video/quicktime' => 'video/quicktime', + + 'application/pdf' => 'application/pdf', ); $image_default = array( diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php index 91dfb4fa4f..bd3d933283 100644 --- a/src/applications/files/controller/PhabricatorFileDataController.php +++ b/src/applications/files/controller/PhabricatorFileDataController.php @@ -73,11 +73,14 @@ final class PhabricatorFileDataController extends PhabricatorFileController { list($begin, $end) = $response->parseHTTPRange($range); } - $is_viewable = $file->isViewableInBrowser(); + if (!$file->isViewableInBrowser()) { + $is_download = true; + } + $request_type = $request->getHTTPHeader('X-Phabricator-Request-Type'); $is_lfs = ($request_type == 'git-lfs'); - if ($is_viewable && !$is_download) { + if (!$is_download) { $response->setMimeType($file->getViewableMimeType()); } else { $is_post = $request->isHTTPPost(); @@ -109,6 +112,19 @@ final class PhabricatorFileDataController extends PhabricatorFileController { $response->setContentLength($file->getByteSize()); $response->setContentIterator($iterator); + // In Chrome, we must permit this domain in "object-src" CSP when serving a + // PDF or the browser will refuse to render it. + if (!$is_download && $file->isPDF()) { + $request_uri = id(clone $request->getAbsoluteRequestURI()) + ->setPath(null) + ->setFragment(null) + ->setQueryParams(array()); + + $response->addContentSecurityPolicyURI( + 'object-src', + (string)$request_uri); + } + return $response; } diff --git a/src/applications/files/document/PhabricatorPDFDocumentEngine.php b/src/applications/files/document/PhabricatorPDFDocumentEngine.php new file mode 100644 index 0000000000..1e85bd4ae5 --- /dev/null +++ b/src/applications/files/document/PhabricatorPDFDocumentEngine.php @@ -0,0 +1,57 @@ +hasAnyMimeType( + array( + 'application/pdf', + )); + } + + protected function newDocumentContent(PhabricatorDocumentRef $ref) { + $viewer = $this->getViewer(); + + $file = $ref->getFile(); + if ($file) { + $source_uri = $file->getViewURI(); + } else { + throw new PhutilMethodNotImplementedException(); + } + + $name = $ref->getName(); + $length = $ref->getByteLength(); + + $link = id(new PhabricatorFileLinkView()) + ->setViewer($viewer) + ->setFileName($name) + ->setFileViewURI($source_uri) + ->setFileViewable(true) + ->setFileSize(phutil_format_bytes($length)); + + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-pdf', + ), + $link); + + return $container; + } + +} diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 60b4ca0e74..a8d16b7651 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -930,6 +930,19 @@ final class PhabricatorFile extends PhabricatorFileDAO return idx($mime_map, $mime_type); } + public function isPDF() { + if (!$this->isViewableInBrowser()) { + return false; + } + + $mime_map = array( + 'application/pdf' => 'application/pdf', + ); + + $mime_type = $this->getMimeType(); + return idx($mime_map, $mime_type); + } + public function isTransformableImage() { // NOTE: The way the 'gd' extension works in PHP is that you can install it // with support for only some file types, so it might be able to handle diff --git a/src/view/layout/PhabricatorFileLinkView.php b/src/view/layout/PhabricatorFileLinkView.php index 82715c0294..49320c4b8c 100644 --- a/src/view/layout/PhabricatorFileLinkView.php +++ b/src/view/layout/PhabricatorFileLinkView.php @@ -101,26 +101,39 @@ final class PhabricatorFileLinkView extends AphrontTagView { } protected function getTagName() { - return 'div'; + if ($this->getFileDownloadURI()) { + return 'div'; + } else { + return 'a'; + } } protected function getTagAttributes() { - $mustcapture = true; - $sigil = 'lightboxable'; - $meta = $this->getMeta(); - $class = 'phabricator-remarkup-embed-layout-link'; if ($this->getCustomClass()) { $class = $this->getCustomClass(); } - return array( - 'href' => $this->getFileViewURI(), - 'class' => $class, - 'sigil' => $sigil, - 'meta' => $meta, - 'mustcapture' => $mustcapture, + $attributes = array( + 'href' => $this->getFileViewURI(), + 'target' => '_blank', + 'rel' => 'noreferrer', + 'class' => $class, ); + + if ($this->getFilePHID()) { + $mustcapture = true; + $sigil = 'lightboxable'; + $meta = $this->getMeta(); + + $attributes += array( + 'sigil' => $sigil, + 'meta' => $meta, + 'mustcapture' => $mustcapture, + ); + } + + return $attributes; } protected function getTagContent() { @@ -131,16 +144,21 @@ final class PhabricatorFileLinkView extends AphrontTagView { ->setIcon($this->getFileIcon()) ->addClass('phabricator-remarkup-embed-layout-icon'); - $dl_icon = id(new PHUIIconView()) - ->setIcon('fa-download'); + $download_link = null; - $download_link = phutil_tag( - 'a', - array( - 'class' => 'phabricator-remarkup-embed-layout-download', - 'href' => $this->getFileDownloadURI(), - ), - pht('Download')); + $download_uri = $this->getFileDownloadURI(); + if ($download_uri) { + $dl_icon = id(new PHUIIconView()) + ->setIcon('fa-download'); + + $download_link = phutil_tag( + 'a', + array( + 'class' => 'phabricator-remarkup-embed-layout-download', + 'href' => $download_uri, + ), + pht('Download')); + } $info = phutil_tag( 'span', diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css index a51c1ce1d9..949deadbe4 100644 --- a/webroot/rsrc/css/core/remarkup.css +++ b/webroot/rsrc/css/core/remarkup.css @@ -405,8 +405,9 @@ video.phabricator-media { color: {$blacktext}; min-width: 256px; position: relative; - /*height: 22px;*/ line-height: 20px; + overflow: hidden; + min-height: 38px; } .phabricator-remarkup-embed-layout-icon { @@ -426,6 +427,9 @@ video.phabricator-media { .phabricator-remarkup-embed-layout-link:hover { border-color: {$violet}; cursor: pointer; +} + +.device-desktop .phabricator-remarkup-embed-layout-link:hover { text-decoration: none; } diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index 685c01fd0a..59dc6cc054 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -248,3 +248,12 @@ div.phui-property-list-stacked .phui-property-list-properties .document-engine-remarkup { margin: 20px; } + +.document-engine-pdf { + margin: 20px; + text-align: center; +} + +.document-engine-pdf .phabricator-remarkup-embed-layout-link { + text-align: left; +} From cbf3d3c37154ad255ac803627427e9397601689d Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Mar 2018 06:40:30 -0700 Subject: [PATCH 17/20] Add a very rough, proof-of-concept Jupyter notebook document engine Summary: Depends on D19252. Ref T13105. This very roughly renders Jupyter notebooks. It's probably better than showing the raw JSON, but not by much. Test Plan: - Viewed various notebooks with various cell types, including markdown, code, stdout, stderr, images, HTML, and Javascript. - HTML and Javascript are not live-fired since they're wildly dangerous. Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19253 --- resources/celerity/map.php | 6 +- src/__phutil_library_map__.php | 2 + .../files/document/PhabricatorDocumentRef.php | 9 + .../PhabricatorJupyterDocumentEngine.php | 305 ++++++++++++++++++ .../rsrc/css/phui/phui-property-list-view.css | 47 +++ 5 files changed, 366 insertions(+), 3 deletions(-) create mode 100644 src/applications/files/document/PhabricatorJupyterDocumentEngine.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0d80821050..231c7fe5da 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => '2d73b2f3', + 'core.pkg.css' => '7daac340', 'core.pkg.js' => 'b9b4a943', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -168,7 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => '9cff003c', 'rsrc/css/phui/phui-pager.css' => 'edcbc226', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '47018d3c', + 'rsrc/css/phui/phui-property-list-view.css' => '871f6815', 'rsrc/css/phui/phui-remarkup-preview.css' => '54a34863', 'rsrc/css/phui/phui-segment-bar-view.css' => 'b1d1b892', 'rsrc/css/phui/phui-spacing.css' => '042804d6', @@ -850,7 +850,7 @@ return array( 'phui-oi-simple-ui-css' => 'a8beebea', 'phui-pager-css' => 'edcbc226', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '47018d3c', + 'phui-property-list-view-css' => '871f6815', 'phui-remarkup-preview-css' => '54a34863', 'phui-segment-bar-view-css' => 'b1d1b892', 'phui-spacing-css' => '042804d6', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6090161840..a7b85ba4c5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3190,6 +3190,7 @@ phutil_register_library_map(array( 'PhabricatorJSONExportFormat' => 'infrastructure/export/format/PhabricatorJSONExportFormat.php', 'PhabricatorJavelinLinter' => 'infrastructure/lint/linter/PhabricatorJavelinLinter.php', 'PhabricatorJiraIssueHasObjectEdgeType' => 'applications/doorkeeper/edge/PhabricatorJiraIssueHasObjectEdgeType.php', + 'PhabricatorJupyterDocumentEngine' => 'applications/files/document/PhabricatorJupyterDocumentEngine.php', 'PhabricatorKeyValueDatabaseCache' => 'applications/cache/PhabricatorKeyValueDatabaseCache.php', 'PhabricatorKeyValueSerializingCacheProxy' => 'applications/cache/PhabricatorKeyValueSerializingCacheProxy.php', 'PhabricatorKeyboardRemarkupRule' => 'infrastructure/markup/rule/PhabricatorKeyboardRemarkupRule.php', @@ -8800,6 +8801,7 @@ phutil_register_library_map(array( 'PhabricatorJSONExportFormat' => 'PhabricatorExportFormat', 'PhabricatorJavelinLinter' => 'ArcanistLinter', 'PhabricatorJiraIssueHasObjectEdgeType' => 'PhabricatorEdgeType', + 'PhabricatorJupyterDocumentEngine' => 'PhabricatorDocumentEngine', 'PhabricatorKeyValueDatabaseCache' => 'PhutilKeyValueCache', 'PhabricatorKeyValueSerializingCacheProxy' => 'PhutilKeyValueCacheProxy', 'PhabricatorKeyboardRemarkupRule' => 'PhutilRemarkupRule', diff --git a/src/applications/files/document/PhabricatorDocumentRef.php b/src/applications/files/document/PhabricatorDocumentRef.php index 6b4b6a6796..e27d164cd7 100644 --- a/src/applications/files/document/PhabricatorDocumentRef.php +++ b/src/applications/files/document/PhabricatorDocumentRef.php @@ -110,6 +110,15 @@ final class PhabricatorDocumentRef return (strpos($snippet, "\0") === false); } + public function isProbablyJSON() { + if (!$this->isProbablyText()) { + return false; + } + + $snippet = $this->getSnippet(); + return phutil_is_utf8($snippet); + } + public function getSnippet() { if ($this->snippet === null) { $this->snippet = $this->loadData(null, (1024 * 1024 * 1)); diff --git a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php new file mode 100644 index 0000000000..7b2246a315 --- /dev/null +++ b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php @@ -0,0 +1,305 @@ +getName(); + + if (preg_match('/\\.ipynb\z/i', $name)) { + return 2000; + } + + return 500; + } + + protected function canRenderDocumentType(PhabricatorDocumentRef $ref) { + return $ref->isProbablyJSON(); + } + + protected function newDocumentContent(PhabricatorDocumentRef $ref) { + $viewer = $this->getViewer(); + $content = $ref->loadData(); + + try { + $data = phutil_json_decode($content); + } catch (PhutilJSONParserException $ex) { + return $this->newMessage( + pht( + 'This is not a valid JSON document and can not be rendered as '. + 'a Jupyter notebook: %s.', + $ex->getMessage())); + } + + if (!is_array($data)) { + return $this->newMessage( + pht( + 'This document does not encode a valid JSON object and can not '. + 'be rendered as a Jupyter notebook.')); + } + + + $nbformat = idx($data, 'nbformat'); + if (!strlen($nbformat)) { + return $this->newMessage( + pht( + 'This document is missing an "nbformat" field. Jupyter notebooks '. + 'must have this field.')); + } + + if ($nbformat !== 4) { + return $this->newMessage( + pht( + 'This Jupyter notebook uses an unsupported version of the file '. + 'format (found version %s, expected version 4).', + $nbformat)); + } + + $cells = idx($data, 'cells'); + if (!is_array($cells)) { + return $this->newMessage( + pht( + 'This Jupyter notebook does not specify a list of "cells".')); + } + + if (!$cells) { + return $this->newMessage( + pht( + 'This Jupyter notebook does not specify any notebook cells.')); + } + + $rows = array(); + foreach ($cells as $cell) { + $rows[] = $this->renderJupyterCell($viewer, $cell); + } + + $notebook_table = phutil_tag( + 'table', + array( + 'class' => 'jupyter-notebook', + ), + $rows); + + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-jupyter', + ), + $notebook_table); + + return $container; + } + + private function renderJupyterCell( + PhabricatorUser $viewer, + array $cell) { + + list($label, $content) = $this->renderJupyterCellContent($viewer, $cell); + + $label_cell = phutil_tag( + 'th', + array(), + $label); + + $content_cell = phutil_tag( + 'td', + array(), + $content); + + return phutil_tag( + 'tr', + array(), + array( + $label_cell, + $content_cell, + )); + } + + private function renderJupyterCellContent( + PhabricatorUser $viewer, + array $cell) { + + $cell_type = idx($cell, 'cell_type'); + switch ($cell_type) { + case 'markdown': + return $this->newMarkdownCell($cell); + case 'code': + return $this->newCodeCell($cell); + } + + return $this->newRawCell(id(new PhutilJSON())->encodeFormatted($cell)); + } + + private function newRawCell($content) { + return array( + null, + phutil_tag( + 'div', + array( + 'class' => 'jupyter-cell-raw PhabricatorMonospaced', + ), + $content), + ); + } + + private function newMarkdownCell(array $cell) { + $content = idx($cell, 'source'); + if (!is_array($content)) { + $content = array(); + } + + $content = implode('', $content); + $content = phutil_escape_html_newlines($content); + + return array( + null, + phutil_tag( + 'div', + array( + 'class' => 'jupyter-cell-markdown', + ), + $content), + ); + } + + private function newCodeCell(array $cell) { + $execution_count = idx($cell, 'execution_count'); + if ($execution_count) { + $label = 'In ['.$execution_count.']:'; + } else { + $label = null; + } + + $content = idx($cell, 'source'); + if (!is_array($content)) { + $content = array(); + } + + $content = implode('', $content); + + $content = PhabricatorSyntaxHighlighter::highlightWithLanguage( + 'python', + $content); + + $outputs = array(); + $output_list = idx($cell, 'outputs'); + if (is_array($output_list)) { + foreach ($output_list as $output) { + $outputs[] = $this->newOutput($output); + } + } + + return array( + $label, + array( + phutil_tag( + 'div', + array( + 'class' => 'jupyter-cell-code PhabricatorMonospaced remarkup-code', + ), + array( + $content, + )), + $outputs, + ), + ); + } + + private function newOutput(array $output) { + if (!is_array($output)) { + return pht(''); + } + + $classes = array( + 'jupyter-output', + 'PhabricatorMonospaced', + ); + + $output_name = idx($output, 'name'); + switch ($output_name) { + case 'stderr': + $classes[] = 'jupyter-output-stderr'; + break; + } + + $output_type = idx($output, 'output_type'); + switch ($output_type) { + case 'execute_result': + case 'display_data': + $data = idx($output, 'data'); + + $image_formats = array( + 'image/png', + 'image/jpeg', + 'image/jpg', + 'image/gif', + ); + + foreach ($image_formats as $image_format) { + if (!isset($data[$image_format])) { + continue; + } + + $raw_data = $data[$image_format]; + if (!is_array($raw_data)) { + continue; + } + + $raw_data = implode('', $raw_data); + + $content = phutil_tag( + 'img', + array( + 'src' => 'data:'.$image_format.';base64,'.$raw_data, + )); + + break 2; + } + + if (isset($data['text/html'])) { + $content = $data['text/html']; + $classes[] = 'jupyter-output-html'; + break; + } + + if (isset($data['application/javascript'])) { + $content = $data['application/javascript']; + $classes[] = 'jupyter-output-html'; + break; + } + + if (isset($data['text/plain'])) { + $content = $data['text/plain']; + break; + } + + break; + case 'stream': + default: + $content = idx($output, 'text'); + if (!is_array($content)) { + $content = array(); + } + $content = implode('', $content); + break; + } + + return phutil_tag( + 'div', + array( + 'class' => implode(' ', $classes), + ), + $content); + } + +} diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index 59dc6cc054..0ff882a746 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -257,3 +257,50 @@ div.phui-property-list-stacked .phui-property-list-properties .document-engine-pdf .phabricator-remarkup-embed-layout-link { text-align: left; } + +.document-engine-jupyter { + overflow: hidden; + margin: 20px; +} + +.jupyter-cell-raw { + white-space: pre-wrap; + background: {$lightgreybackground}; + color: {$greytext}; + padding: 8px; +} + +.jupyter-cell-code { + white-space: pre-wrap; + background: {$lightgreybackground}; + padding: 8px; + border: 1px solid {$lightgreyborder}; + border-radius: 2px; +} + +.jupyter-notebook > tbody > tr > th, +.jupyter-notebook > tbody > tr > td { + padding: 8px; +} + +.jupyter-notebook > tbody > tr > th { + white-space: nowrap; + text-align: right; + min-width: 48px; + font-weight: bold; +} + +.jupyter-output { + margin: 4px 0; + padding: 8px; + white-space: pre-wrap; + word-break: break-all; +} + +.jupyter-output-stderr { + background: {$sh-redbackground}; +} + +.jupyter-output-html { + background: {$sh-indigobackground}; +} From d2727d24da6e3142fee64ec9edbdcba9fa5dc357 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Mar 2018 07:29:28 -0700 Subject: [PATCH 18/20] Add an abstract "Text" document engine and a "Source" document engine Summary: Ref T13105. Allow normal text files to be rendered as documents, and add a "source code" rendering engine. Test Plan: Viewed some source code. Reviewers: mydeveloperday Reviewed By: mydeveloperday Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19254 --- resources/celerity/map.php | 10 +++--- src/__phutil_library_map__.php | 4 +++ .../PhabricatorSourceDocumentEngine.php | 30 +++++++++++++++++ .../PhabricatorTextDocumentEngine.php | 33 +++++++++++++++++++ .../layout/phabricator-source-code-view.css | 20 ++++------- .../rsrc/css/phui/phui-property-list-view.css | 4 +++ 6 files changed, 82 insertions(+), 19 deletions(-) create mode 100644 src/applications/files/document/PhabricatorSourceDocumentEngine.php create mode 100644 src/applications/files/document/PhabricatorTextDocumentEngine.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 231c7fe5da..798f961f90 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => '7daac340', + 'core.pkg.css' => 'da541195', 'core.pkg.js' => 'b9b4a943', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -120,7 +120,7 @@ return array( 'rsrc/css/font/font-lato.css' => 'c7ccd872', 'rsrc/css/font/phui-font-icon-base.css' => '870a7360', 'rsrc/css/layout/phabricator-filetree-view.css' => 'b912ad97', - 'rsrc/css/layout/phabricator-source-code-view.css' => '926ced2d', + 'rsrc/css/layout/phabricator-source-code-view.css' => '31ee3c83', 'rsrc/css/phui/button/phui-button-bar.css' => 'f1ff5494', 'rsrc/css/phui/button/phui-button-simple.css' => '8e1baf68', 'rsrc/css/phui/button/phui-button.css' => '1863cc6e', @@ -168,7 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => '9cff003c', 'rsrc/css/phui/phui-pager.css' => 'edcbc226', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '871f6815', + 'rsrc/css/phui/phui-property-list-view.css' => '54c071ed', 'rsrc/css/phui/phui-remarkup-preview.css' => '54a34863', 'rsrc/css/phui/phui-segment-bar-view.css' => 'b1d1b892', 'rsrc/css/phui/phui-spacing.css' => '042804d6', @@ -784,7 +784,7 @@ return array( 'phabricator-search-results-css' => '505dd8cf', 'phabricator-shaped-request' => '7cbe244b', 'phabricator-slowvote-css' => 'a94b7230', - 'phabricator-source-code-view-css' => '926ced2d', + 'phabricator-source-code-view-css' => '31ee3c83', 'phabricator-standard-page-view' => '34ee718b', 'phabricator-textareautils' => '320810c8', 'phabricator-title' => '485aaa6c', @@ -850,7 +850,7 @@ return array( 'phui-oi-simple-ui-css' => 'a8beebea', 'phui-pager-css' => 'edcbc226', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '871f6815', + 'phui-property-list-view-css' => '54c071ed', 'phui-remarkup-preview-css' => '54a34863', 'phui-segment-bar-view-css' => 'b1d1b892', 'phui-spacing-css' => '042804d6', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a7b85ba4c5..0e2ff3f0fa 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4222,6 +4222,7 @@ phutil_register_library_map(array( 'PhabricatorSlug' => 'infrastructure/util/PhabricatorSlug.php', 'PhabricatorSlugTestCase' => 'infrastructure/util/__tests__/PhabricatorSlugTestCase.php', 'PhabricatorSourceCodeView' => 'view/layout/PhabricatorSourceCodeView.php', + 'PhabricatorSourceDocumentEngine' => 'applications/files/document/PhabricatorSourceDocumentEngine.php', 'PhabricatorSpaceEditField' => 'applications/transactions/editfield/PhabricatorSpaceEditField.php', 'PhabricatorSpacesApplication' => 'applications/spaces/application/PhabricatorSpacesApplication.php', 'PhabricatorSpacesArchiveController' => 'applications/spaces/controller/PhabricatorSpacesArchiveController.php', @@ -4363,6 +4364,7 @@ phutil_register_library_map(array( 'PhabricatorTestWorker' => 'infrastructure/daemon/workers/__tests__/PhabricatorTestWorker.php', 'PhabricatorTextAreaEditField' => 'applications/transactions/editfield/PhabricatorTextAreaEditField.php', 'PhabricatorTextConfigType' => 'applications/config/type/PhabricatorTextConfigType.php', + 'PhabricatorTextDocumentEngine' => 'applications/files/document/PhabricatorTextDocumentEngine.php', 'PhabricatorTextEditField' => 'applications/transactions/editfield/PhabricatorTextEditField.php', 'PhabricatorTextExportFormat' => 'infrastructure/export/format/PhabricatorTextExportFormat.php', 'PhabricatorTextListConfigType' => 'applications/config/type/PhabricatorTextListConfigType.php', @@ -10041,6 +10043,7 @@ phutil_register_library_map(array( 'PhabricatorSlug' => 'Phobject', 'PhabricatorSlugTestCase' => 'PhabricatorTestCase', 'PhabricatorSourceCodeView' => 'AphrontView', + 'PhabricatorSourceDocumentEngine' => 'PhabricatorTextDocumentEngine', 'PhabricatorSpaceEditField' => 'PhabricatorEditField', 'PhabricatorSpacesApplication' => 'PhabricatorApplication', 'PhabricatorSpacesArchiveController' => 'PhabricatorSpacesController', @@ -10188,6 +10191,7 @@ phutil_register_library_map(array( 'PhabricatorTestWorker' => 'PhabricatorWorker', 'PhabricatorTextAreaEditField' => 'PhabricatorEditField', 'PhabricatorTextConfigType' => 'PhabricatorConfigType', + 'PhabricatorTextDocumentEngine' => 'PhabricatorDocumentEngine', 'PhabricatorTextEditField' => 'PhabricatorEditField', 'PhabricatorTextExportFormat' => 'PhabricatorExportFormat', 'PhabricatorTextListConfigType' => 'PhabricatorTextConfigType', diff --git a/src/applications/files/document/PhabricatorSourceDocumentEngine.php b/src/applications/files/document/PhabricatorSourceDocumentEngine.php new file mode 100644 index 0000000000..1c3e54575a --- /dev/null +++ b/src/applications/files/document/PhabricatorSourceDocumentEngine.php @@ -0,0 +1,30 @@ +loadTextData($ref); + + $content = PhabricatorSyntaxHighlighter::highlightWithFilename( + $ref->getName(), + $content); + + return $this->newTextDocumentContent($content); + } + +} diff --git a/src/applications/files/document/PhabricatorTextDocumentEngine.php b/src/applications/files/document/PhabricatorTextDocumentEngine.php new file mode 100644 index 0000000000..506c73badb --- /dev/null +++ b/src/applications/files/document/PhabricatorTextDocumentEngine.php @@ -0,0 +1,33 @@ +isProbablyText(); + } + + protected function newTextDocumentContent($content) { + $lines = phutil_split_lines($content); + + $view = id(new PhabricatorSourceCodeView()) + ->setLines($lines) + ->disableHighlightOnClick(); + + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-text', + ), + $view); + + return $container; + } + + protected function loadTextData(PhabricatorDocumentRef $ref) { + $content = $ref->loadData(); + $content = phutil_utf8ize($content); + return $content; + } + +} diff --git a/webroot/rsrc/css/layout/phabricator-source-code-view.css b/webroot/rsrc/css/layout/phabricator-source-code-view.css index 446366cdf1..1836d321a5 100644 --- a/webroot/rsrc/css/layout/phabricator-source-code-view.css +++ b/webroot/rsrc/css/layout/phabricator-source-code-view.css @@ -14,14 +14,6 @@ margin-left: 8px; } -.phabricator-source-code-view tr:first-child * { - padding-top: 8px; -} - -.phabricator-source-code-view tr:last-child * { - padding-bottom: 8px; -} - .phabricator-source-code { white-space: pre-wrap; padding: 2px 8px 1px; @@ -45,12 +37,16 @@ white-space: nowrap; } -th.phabricator-source-line a { - color: {$darkbluetext}; +th.phabricator-source-line a, +th.phabricator-source-line span { display: block; padding: 2px 6px 1px 12px; } +th.phabricator-source-line a { + color: {$darkbluetext}; +} + th.phabricator-source-line a:hover { background: {$paste.border}; text-decoration: none; @@ -60,10 +56,6 @@ th.phabricator-source-line a:hover { background: {$paste.highlight}; } -.phabricator-source-highlight th.phabricator-source-line { - background: {$paste.border}; -} - .phabricator-source-code-summary { padding-bottom: 8px; } diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index 0ff882a746..c1f9944cbd 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -258,6 +258,10 @@ div.phui-property-list-stacked .phui-property-list-properties text-align: left; } +.document-engine-text .phabricator-source-code-container { + border: none; +} + .document-engine-jupyter { overflow: hidden; margin: 20px; From 49063647515cabaf6bf9a0c4249c5ce357871240 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Mar 2018 08:00:35 -0700 Subject: [PATCH 19/20] Add a JSON document rendering engine Summary: Depends on D19254. This engine just formats JSON files in a nicer, more readable way. Test Plan: Looked at some JSON files, saw them become formatted nicely. Reviewers: mydeveloperday Reviewed By: mydeveloperday Differential Revision: https://secure.phabricator.com/D19255 --- src/__phutil_library_map__.php | 2 + .../files/document/PhabricatorDocumentRef.php | 4 ++ .../PhabricatorJSONDocumentEngine.php | 59 +++++++++++++++++++ 3 files changed, 65 insertions(+) create mode 100644 src/applications/files/document/PhabricatorJSONDocumentEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0e2ff3f0fa..4c6efe4be2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3187,6 +3187,7 @@ phutil_register_library_map(array( 'PhabricatorIteratorFileUploadSource' => 'applications/files/uploadsource/PhabricatorIteratorFileUploadSource.php', 'PhabricatorJIRAAuthProvider' => 'applications/auth/provider/PhabricatorJIRAAuthProvider.php', 'PhabricatorJSONConfigType' => 'applications/config/type/PhabricatorJSONConfigType.php', + 'PhabricatorJSONDocumentEngine' => 'applications/files/document/PhabricatorJSONDocumentEngine.php', 'PhabricatorJSONExportFormat' => 'infrastructure/export/format/PhabricatorJSONExportFormat.php', 'PhabricatorJavelinLinter' => 'infrastructure/lint/linter/PhabricatorJavelinLinter.php', 'PhabricatorJiraIssueHasObjectEdgeType' => 'applications/doorkeeper/edge/PhabricatorJiraIssueHasObjectEdgeType.php', @@ -8800,6 +8801,7 @@ phutil_register_library_map(array( 'PhabricatorIteratorFileUploadSource' => 'PhabricatorFileUploadSource', 'PhabricatorJIRAAuthProvider' => 'PhabricatorOAuth1AuthProvider', 'PhabricatorJSONConfigType' => 'PhabricatorTextConfigType', + 'PhabricatorJSONDocumentEngine' => 'PhabricatorTextDocumentEngine', 'PhabricatorJSONExportFormat' => 'PhabricatorExportFormat', 'PhabricatorJavelinLinter' => 'ArcanistLinter', 'PhabricatorJiraIssueHasObjectEdgeType' => 'PhabricatorEdgeType', diff --git a/src/applications/files/document/PhabricatorDocumentRef.php b/src/applications/files/document/PhabricatorDocumentRef.php index e27d164cd7..cca0c102e2 100644 --- a/src/applications/files/document/PhabricatorDocumentRef.php +++ b/src/applications/files/document/PhabricatorDocumentRef.php @@ -116,6 +116,10 @@ final class PhabricatorDocumentRef } $snippet = $this->getSnippet(); + if (!preg_match('/^\s*[{[]/', $snippet)) { + return false; + } + return phutil_is_utf8($snippet); } diff --git a/src/applications/files/document/PhabricatorJSONDocumentEngine.php b/src/applications/files/document/PhabricatorJSONDocumentEngine.php new file mode 100644 index 0000000000..331a7e6820 --- /dev/null +++ b/src/applications/files/document/PhabricatorJSONDocumentEngine.php @@ -0,0 +1,59 @@ +getName())) { + return 2000; + } + + if ($ref->isProbablyJSON()) { + return 1750; + } + + return 500; + } + + protected function newDocumentContent(PhabricatorDocumentRef $ref) { + $raw_data = $this->loadTextData($ref); + + try { + $data = phutil_json_decode($raw_data); + + if (preg_match('/^\s*\[/', $raw_data)) { + $content = id(new PhutilJSON())->encodeAsList($data); + } else { + $content = id(new PhutilJSON())->encodeFormatted($data); + } + + $message = null; + $content = PhabricatorSyntaxHighlighter::highlightWithLanguage( + 'json', + $content); + } catch (PhutilJSONParserException $ex) { + $message = $this->newMessage( + pht( + 'This document is not valid JSON: %s', + $ex->getMessage())); + + $content = $raw_data; + } + + return array( + $message, + $this->newTextDocumentContent($content), + ); + } + +} From bba1b185f8345e381461d1181568aeb903d433b2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Mar 2018 13:44:14 -0700 Subject: [PATCH 20/20] Improve minor client behaviors for document rendering Summary: Ref T13105. This adds various small client-side improvements to document rendering. - In the menu, show which renderer is in use. - Make linking to lines work. - Make URIs persist information about which rendering engine is in use. - Improve the UI feedback for transitions between document types. - Load slower documents asynchronously by default. - Discard irrelevant requests if you spam the view menu. Test Plan: Loaded files, linked to lines, swapped between modes, copy/pasted URLs. Maniphest Tasks: T13105 Differential Revision: https://secure.phabricator.com/D19256 --- resources/celerity/map.php | 36 ++++---- src/__phutil_library_map__.php | 4 +- .../PhabricatorFilesApplication.php | 9 +- ....php => PhabricatorFileViewController.php} | 39 +++++++-- .../document/PhabricatorDocumentEngine.php | 34 ++++++++ .../PhabricatorJupyterDocumentEngine.php | 8 ++ .../PhabricatorTextDocumentEngine.php | 4 +- src/view/layout/PhabricatorSourceCodeView.php | 6 +- .../rsrc/css/phui/phui-property-list-view.css | 17 ++++ .../files/behavior-document-engine.js | 87 +++++++++++++++++-- webroot/rsrc/js/core/behavior-line-linker.js | 4 + 11 files changed, 210 insertions(+), 38 deletions(-) rename src/applications/files/controller/{PhabricatorFileInfoController.php => PhabricatorFileViewController.php} (91%) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 798f961f90..dabb6f1d5b 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => 'da541195', + 'core.pkg.css' => '3fd3b7b8', 'core.pkg.js' => 'b9b4a943', 'differential.pkg.css' => '113e692c', 'differential.pkg.js' => 'f6d809c0', @@ -168,7 +168,7 @@ return array( 'rsrc/css/phui/phui-object-box.css' => '9cff003c', 'rsrc/css/phui/phui-pager.css' => 'edcbc226', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '54c071ed', + 'rsrc/css/phui/phui-property-list-view.css' => 'de4754d8', 'rsrc/css/phui/phui-remarkup-preview.css' => '54a34863', 'rsrc/css/phui/phui-segment-bar-view.css' => 'b1d1b892', 'rsrc/css/phui/phui-spacing.css' => '042804d6', @@ -392,7 +392,7 @@ return array( 'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'f01586dc', 'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '1db13e70', 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef', - 'rsrc/js/application/files/behavior-document-engine.js' => '396ef112', + 'rsrc/js/application/files/behavior-document-engine.js' => 'd3f8623c', 'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '191b4909', @@ -473,7 +473,7 @@ return array( 'rsrc/js/core/behavior-keyboard-pager.js' => 'a8da01f0', 'rsrc/js/core/behavior-keyboard-shortcuts.js' => '01fca1f0', 'rsrc/js/core/behavior-lightbox-attachments.js' => '6b31879a', - 'rsrc/js/core/behavior-line-linker.js' => 'a9b946f8', + 'rsrc/js/core/behavior-line-linker.js' => '13e39479', 'rsrc/js/core/behavior-more.js' => 'a80d0378', 'rsrc/js/core/behavior-object-selector.js' => '77c1f0b0', 'rsrc/js/core/behavior-oncopy.js' => '2926fff2', @@ -607,7 +607,7 @@ return array( 'javelin-behavior-diffusion-jump-to' => '73d09eef', 'javelin-behavior-diffusion-locate-file' => '6d3e1947', 'javelin-behavior-diffusion-pull-lastmodified' => 'f01586dc', - 'javelin-behavior-document-engine' => '396ef112', + 'javelin-behavior-document-engine' => 'd3f8623c', 'javelin-behavior-doorkeeper-tag' => '1db13e70', 'javelin-behavior-drydock-live-operation-status' => '901935ef', 'javelin-behavior-durable-column' => '2ae077e1', @@ -638,7 +638,7 @@ return array( 'javelin-behavior-phabricator-gesture-example' => '558829c2', 'javelin-behavior-phabricator-keyboard-pager' => 'a8da01f0', 'javelin-behavior-phabricator-keyboard-shortcuts' => '01fca1f0', - 'javelin-behavior-phabricator-line-linker' => 'a9b946f8', + 'javelin-behavior-phabricator-line-linker' => '13e39479', 'javelin-behavior-phabricator-nav' => '836f966d', 'javelin-behavior-phabricator-notification-example' => '8ce821c5', 'javelin-behavior-phabricator-object-selector' => '77c1f0b0', @@ -850,7 +850,7 @@ return array( 'phui-oi-simple-ui-css' => 'a8beebea', 'phui-pager-css' => 'edcbc226', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '54c071ed', + 'phui-property-list-view-css' => 'de4754d8', 'phui-remarkup-preview-css' => '54a34863', 'phui-segment-bar-view-css' => 'b1d1b892', 'phui-spacing-css' => '042804d6', @@ -964,6 +964,12 @@ return array( 'javelin-install', 'javelin-util', ), + '13e39479' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-history', + ), '15d5ff71' => array( 'aphront-typeahead-control-css', 'phui-tag-view-css', @@ -1103,11 +1109,6 @@ return array( 'javelin-dom', 'javelin-vector', ), - '396ef112' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - ), '3ab51e2c' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -1751,12 +1752,6 @@ return array( 'javelin-uri', 'phabricator-keyboard-shortcut', ), - 'a9b946f8' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-history', - ), 'a9f88de2' => array( 'javelin-behavior', 'javelin-dom', @@ -2007,6 +2002,11 @@ return array( 'd254d646' => array( 'javelin-util', ), + 'd3f8623c' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + ), 'd4505101' => array( 'javelin-stratcom', 'javelin-install', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4c6efe4be2..b1afef3600 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3015,7 +3015,6 @@ phutil_register_library_map(array( 'PhabricatorFileImageMacro' => 'applications/macro/storage/PhabricatorFileImageMacro.php', 'PhabricatorFileImageProxyController' => 'applications/files/controller/PhabricatorFileImageProxyController.php', 'PhabricatorFileImageTransform' => 'applications/files/transform/PhabricatorFileImageTransform.php', - 'PhabricatorFileInfoController' => 'applications/files/controller/PhabricatorFileInfoController.php', 'PhabricatorFileIntegrityException' => 'applications/files/exception/PhabricatorFileIntegrityException.php', 'PhabricatorFileLightboxController' => 'applications/files/controller/PhabricatorFileLightboxController.php', 'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php', @@ -3051,6 +3050,7 @@ phutil_register_library_map(array( 'PhabricatorFileUploadException' => 'applications/files/exception/PhabricatorFileUploadException.php', 'PhabricatorFileUploadSource' => 'applications/files/uploadsource/PhabricatorFileUploadSource.php', 'PhabricatorFileUploadSourceByteLimitException' => 'applications/files/uploadsource/PhabricatorFileUploadSourceByteLimitException.php', + 'PhabricatorFileViewController' => 'applications/files/controller/PhabricatorFileViewController.php', 'PhabricatorFileinfoSetupCheck' => 'applications/config/check/PhabricatorFileinfoSetupCheck.php', 'PhabricatorFilesApplication' => 'applications/files/application/PhabricatorFilesApplication.php', 'PhabricatorFilesApplicationStorageEnginePanel' => 'applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php', @@ -8622,7 +8622,6 @@ phutil_register_library_map(array( ), 'PhabricatorFileImageProxyController' => 'PhabricatorFileController', 'PhabricatorFileImageTransform' => 'PhabricatorFileTransform', - 'PhabricatorFileInfoController' => 'PhabricatorFileController', 'PhabricatorFileIntegrityException' => 'Exception', 'PhabricatorFileLightboxController' => 'PhabricatorFileController', 'PhabricatorFileLinkView' => 'AphrontTagView', @@ -8658,6 +8657,7 @@ phutil_register_library_map(array( 'PhabricatorFileUploadException' => 'Exception', 'PhabricatorFileUploadSource' => 'Phobject', 'PhabricatorFileUploadSourceByteLimitException' => 'Exception', + 'PhabricatorFileViewController' => 'PhabricatorFileController', 'PhabricatorFileinfoSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorFilesApplication' => 'PhabricatorApplication', 'PhabricatorFilesApplicationStorageEnginePanel' => 'PhabricatorApplicationConfigurationPanel', diff --git a/src/applications/files/application/PhabricatorFilesApplication.php b/src/applications/files/application/PhabricatorFilesApplication.php index d95972d10e..5bf1ebccc4 100644 --- a/src/applications/files/application/PhabricatorFilesApplication.php +++ b/src/applications/files/application/PhabricatorFilesApplication.php @@ -69,9 +69,15 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { public function getRoutes() { return array( - '/F(?P[1-9]\d*)' => 'PhabricatorFileInfoController', + '/F(?P[1-9]\d*)(?:\$(?P\d+(?:-\d+)?))?' + => 'PhabricatorFileViewController', '/file/' => array( '(query/(?P[^/]+)/)?' => 'PhabricatorFileListController', + 'view/(?P[^/]+)/'. + '(?:(?P[^/]+)/)?'. + '(?:\$(?P\d+(?:-\d+)?))?' + => 'PhabricatorFileViewController', + 'info/(?P[^/]+)/' => 'PhabricatorFileViewController', 'upload/' => 'PhabricatorFileUploadController', 'dropupload/' => 'PhabricatorFileDropUploadController', 'compose/' => 'PhabricatorFileComposeController', @@ -80,7 +86,6 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { 'delete/(?P[1-9]\d*)/' => 'PhabricatorFileDeleteController', $this->getEditRoutePattern('edit/') => 'PhabricatorFileEditController', - 'info/(?P[^/]+)/' => 'PhabricatorFileInfoController', 'imageproxy/' => 'PhabricatorFileImageProxyController', 'transforms/(?P[1-9]\d*)/' => 'PhabricatorFileTransformListController', diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileViewController.php similarity index 91% rename from src/applications/files/controller/PhabricatorFileInfoController.php rename to src/applications/files/controller/PhabricatorFileViewController.php index e25cf8d74c..5251fdd8f1 100644 --- a/src/applications/files/controller/PhabricatorFileInfoController.php +++ b/src/applications/files/controller/PhabricatorFileViewController.php @@ -1,6 +1,6 @@ getViewer(); + $request = $this->getRequest(); $ref = id(new PhabricatorDocumentRef()) ->setFile($file); $engines = PhabricatorDocumentEngine::getEnginesForRef($viewer, $ref); - $engine = head($engines); - $content = $engine->newDocument($ref); + $engine_key = $request->getURIData('engineKey'); + if (!isset($engines[$engine_key])) { + $engine_key = head_key($engines); + } + $engine = $engines[$engine_key]; - $icon = $engine->newDocumentIcon($ref); + $lines = $request->getURILineRange('lines', 1000); + if ($lines) { + $engine->setHighlightedLines(range($lines[0], $lines[1])); + } $views = array(); - foreach ($engines as $candidate_engine) { + foreach ($engines as $candidate_key => $candidate_engine) { $label = $candidate_engine->getViewAsLabel($ref); if ($label === null) { continue; } + $view_uri = '/file/view/'.$file->getID().'/'.$candidate_key.'/'; + $view_icon = $candidate_engine->getViewAsIconIcon($ref); $view_color = $candidate_engine->getViewAsIconColor($ref); + $loading = $candidate_engine->newLoadingContent($ref); $views[] = array( 'viewKey' => $candidate_engine->getDocumentEngineKey(), @@ -431,12 +441,26 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { 'color' => $view_color, 'name' => $label, 'engineURI' => $candidate_engine->getRenderURI($ref), + 'viewURI' => $view_uri, + 'loadingMarkup' => hsprintf('%s', $loading), ); } - Javelin::initBehavior('document-engine'); - $viewport_id = celerity_generate_unique_node_id(); + $control_id = celerity_generate_unique_node_id(); + $icon = $engine->newDocumentIcon($ref); + + if ($engine->shouldRenderAsync($ref)) { + $content = $engine->newLoadingContent($ref); + $config = array( + 'renderControlID' => $control_id, + ); + } else { + $content = $engine->newDocument($ref); + $config = array(); + } + + Javelin::initBehavior('document-engine', $config); $viewport = phutil_tag( 'div', @@ -457,6 +481,7 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { ->setText(pht('View Options')) ->setIcon('fa-file-image-o') ->setColor(PHUIButtonView::GREY) + ->setID($control_id) ->setMetadata($meta) ->setDropdown(true) ->addSigil('document-engine-view-dropdown'); diff --git a/src/applications/files/document/PhabricatorDocumentEngine.php b/src/applications/files/document/PhabricatorDocumentEngine.php index 6ad7f74f9a..a225d55ea9 100644 --- a/src/applications/files/document/PhabricatorDocumentEngine.php +++ b/src/applications/files/document/PhabricatorDocumentEngine.php @@ -4,6 +4,7 @@ abstract class PhabricatorDocumentEngine extends Phobject { private $viewer; + private $highlightedLines = array(); final public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; @@ -14,10 +15,23 @@ abstract class PhabricatorDocumentEngine return $this->viewer; } + final public function setHighlightedLines(array $highlighted_lines) { + $this->highlightedLines = $highlighted_lines; + return $this; + } + + final public function getHighlightedLines() { + return $this->highlightedLines; + } + final public function canRenderDocument(PhabricatorDocumentRef $ref) { return $this->canRenderDocumentType($ref); } + public function shouldRenderAsync(PhabricatorDocumentRef $ref) { + return false; + } + abstract protected function canRenderDocumentType( PhabricatorDocumentRef $ref); @@ -49,6 +63,10 @@ abstract class PhabricatorDocumentEngine return 'fa-file-o'; } + protected function getDocumentRenderingText(PhabricatorDocumentRef $ref) { + return pht('Loading...'); + } + final public function getDocumentEngineKey() { return $this->getPhobjectClassConstant('ENGINEKEY'); } @@ -177,4 +195,20 @@ abstract class PhabricatorDocumentEngine $message); } + final public function newLoadingContent(PhabricatorDocumentRef $ref) { + $spinner = id(new PHUIIconView()) + ->setIcon('fa-gear') + ->addClass('ph-spin'); + + return phutil_tag( + 'div', + array( + 'class' => 'document-engine-loading', + ), + array( + $spinner, + $this->getDocumentRenderingText($ref), + )); + } + } diff --git a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php index 7b2246a315..f960f5c8c0 100644 --- a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php +++ b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php @@ -13,6 +13,14 @@ final class PhabricatorJupyterDocumentEngine return 'fa-sun-o'; } + protected function getDocumentRenderingText(PhabricatorDocumentRef $ref) { + return pht('Rendering Jupyter Notebook...'); + } + + public function shouldRenderAsync(PhabricatorDocumentRef $ref) { + return true; + } + protected function getContentScore(PhabricatorDocumentRef $ref) { $name = $ref->getName(); diff --git a/src/applications/files/document/PhabricatorTextDocumentEngine.php b/src/applications/files/document/PhabricatorTextDocumentEngine.php index 506c73badb..4fb8d052e1 100644 --- a/src/applications/files/document/PhabricatorTextDocumentEngine.php +++ b/src/applications/files/document/PhabricatorTextDocumentEngine.php @@ -11,8 +11,8 @@ abstract class PhabricatorTextDocumentEngine $lines = phutil_split_lines($content); $view = id(new PhabricatorSourceCodeView()) - ->setLines($lines) - ->disableHighlightOnClick(); + ->setHighlights($this->getHighlightedLines()) + ->setLines($lines); $container = phutil_tag( 'div', diff --git a/src/view/layout/PhabricatorSourceCodeView.php b/src/view/layout/PhabricatorSourceCodeView.php index f5f6c22b51..ce26fa3df7 100644 --- a/src/view/layout/PhabricatorSourceCodeView.php +++ b/src/view/layout/PhabricatorSourceCodeView.php @@ -85,7 +85,11 @@ final class PhabricatorSourceCodeView extends AphrontView { } if ($this->canClickHighlight) { - $line_href = $base_uri.'$'.$line_number; + if ($base_uri) { + $line_href = $base_uri.'$'.$line_number; + } else { + $line_href = null; + } $tag_number = phutil_tag( 'a', diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index c1f9944cbd..15632c902f 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -267,6 +267,23 @@ div.phui-property-list-stacked .phui-property-list-properties margin: 20px; } +.document-engine-in-flight { + opacity: 0.25; +} + +.document-engine-loading { + margin: 20px; + text-align: center; + color: {$lightgreytext}; +} + +.document-engine-loading .phui-icon-view { + display: block; + font-size: 48px; + color: {$lightgreyborder}; + padding: 8px; +} + .jupyter-cell-raw { white-space: pre-wrap; background: {$lightgreybackground}; diff --git a/webroot/rsrc/js/application/files/behavior-document-engine.js b/webroot/rsrc/js/application/files/behavior-document-engine.js index 6b9b3eeb24..4cb7d17723 100644 --- a/webroot/rsrc/js/application/files/behavior-document-engine.js +++ b/webroot/rsrc/js/application/files/behavior-document-engine.js @@ -5,7 +5,9 @@ * javelin-stratcom */ -JX.behavior('document-engine', function() { +JX.behavior('document-engine', function(config, statics) { + + function onmenu(e) { var node = e.getNode('document-engine-view-dropdown'); @@ -21,6 +23,7 @@ JX.behavior('document-engine', function() { var list = new JX.PHUIXActionListView(); var view; + var engines = []; for (var ii = 0; ii < data.views.length; ii++) { var spec = data.views[ii]; @@ -38,31 +41,103 @@ JX.behavior('document-engine', function() { e.prevent(); menu.close(); - onview(data, spec); + onview(data, spec, false); }, spec)); list.addItem(view); + + engines.push({ + spec: spec, + view: view + }); } menu.setContent(list.getNode()); + menu.listen('open', function() { + for (var ii = 0; ii < engines.length; ii++) { + var engine = engines[ii]; + + // Highlight the current rendering engine. + var is_selected = (engine.spec.viewKey == data.viewKey); + engine.view.setSelected(is_selected); + } + }); + data.menu = menu; menu.open(); } - function onview(data, spec) { - var handler = JX.bind(null, onrender, data); + function onview(data, spec, immediate) { + data.sequence = (data.sequence || 0) + 1; + var handler = JX.bind(null, onrender, data, data.sequence); + + data.viewKey = spec.viewKey; + JX.History.replace(spec.viewURI); new JX.Request(spec.engineURI, handler) .send(); + + if (data.loadingView) { + // If we're already showing "Loading...", immediately change it to + // show the new document type. + onloading(data, spec); + } else if (!immediate) { + // Otherwise, grey out the document and show "Loading..." after a + // short delay. This prevents the content from flickering when rendering + // is fast. + var viewport = JX.$(data.viewportID); + JX.DOM.alterClass(viewport, 'document-engine-in-flight', true); + + var load = JX.bind(null, onloading, data, spec); + data.loadTimer = setTimeout(load, 333); + } } - function onrender(data, r) { + function onloading(data, spec) { + data.loadingView = true; + var viewport = JX.$(data.viewportID); + JX.DOM.alterClass(viewport, 'document-engine-in-flight', false); + JX.DOM.setContent(viewport, JX.$H(spec.loadingMarkup)); + } + + function onrender(data, sequence, r) { + // If this isn't the most recent request we sent, throw it away. This can + // happen if the user makes multiple selections from the menu while we are + // still rendering the first view. + if (sequence != data.sequence) { + return; + } + + if (data.loadTimer) { + clearTimeout(data.loadTimer); + data.loadTimer = null; + } + + var viewport = JX.$(data.viewportID); + + JX.DOM.alterClass(viewport, 'document-engine-in-flight', false); + data.loadingView = false; JX.DOM.setContent(viewport, JX.$H(r.markup)); } - JX.Stratcom.listen('click', 'document-engine-view-dropdown', onmenu); + if (!statics.initialized) { + JX.Stratcom.listen('click', 'document-engine-view-dropdown', onmenu); + statics.initialized = true; + } + + if (config.renderControlID) { + var control = JX.$(config.renderControlID); + var data = JX.Stratcom.getData(control); + + for (var ii = 0; ii < data.views.length; ii++) { + if (data.views[ii].viewKey == data.viewKey) { + onview(data, data.views[ii], true); + break; + } + } + } }); diff --git a/webroot/rsrc/js/core/behavior-line-linker.js b/webroot/rsrc/js/core/behavior-line-linker.js index a0f99fb3bc..8a68cec842 100644 --- a/webroot/rsrc/js/core/behavior-line-linker.js +++ b/webroot/rsrc/js/core/behavior-line-linker.js @@ -145,6 +145,10 @@ JX.behavior('phabricator-line-linker', function() { var t = getRowNumber(target); var uri = JX.Stratcom.getData(root).uri; + if (!uri) { + uri = ('' + window.location).split('$')[0]; + } + origin = null; target = null; root = null;