From f1dc56a68778eac99a2d45be4f8f916c790aca8c Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 8 Sep 2013 09:15:22 -0700 Subject: [PATCH] Muck around with Diviner method documentation display Summary: Ref T988. Not sure about this, feel free to push back or tweak it or whatever, but I want to reduce the amount of meta-text in the method documentation. Primarily this: - Shortens "From parent implementation in ClassName:" to "ClassName". - Tries to tweak the styles a bit so that it's relatively obvious what that means (hopefully?). - Fixes an issue with tasks where some methods could be ignored. Test Plan: {F57565} Reviewers: chad Reviewed By: chad CC: aran Maniphest Tasks: T988 Differential Revision: https://secure.phabricator.com/D6911 --- src/__celerity_resource_map__.php | 2 +- .../controller/DivinerAtomController.php | 68 ++++++++++++------- .../engine/PhabricatorFileStorageEngine.php | 6 ++ .../engine/PhabricatorS3FileStorageEngine.php | 18 ++--- webroot/rsrc/css/diviner/diviner-shared.css | 25 +++++-- 5 files changed, 78 insertions(+), 41 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 3a01225ed1..e04bd72db4 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -1149,7 +1149,7 @@ celerity_register_resource_map(array( ), 'diviner-shared-css' => array( - 'uri' => '/res/686727d1/rsrc/css/diviner/diviner-shared.css', + 'uri' => '/res/4237d999/rsrc/css/diviner/diviner-shared.css', 'type' => 'css', 'requires' => array( diff --git a/src/applications/diviner/controller/DivinerAtomController.php b/src/applications/diviner/controller/DivinerAtomController.php index 80cd33c5a2..c4dc4f6a7d 100644 --- a/src/applications/diviner/controller/DivinerAtomController.php +++ b/src/applications/diviner/controller/DivinerAtomController.php @@ -134,17 +134,21 @@ final class DivinerAtomController extends DivinerController { if ($tasks) { $methods_by_task = igroup($methods, 'task'); + // Add phantom tasks for methods which have a "@task" name that isn't + // documented anywhere, or methods that have no "@task" name. + foreach ($methods_by_task as $task => $ignored) { + if (empty($tasks[$task])) { + $tasks[$task] = array( + 'name' => $task, + 'title' => $task ? $task : pht('Other Methods'), + 'defined' => $symbol, + ); + } + } + $section = id(new DivinerSectionView()) ->setHeader(pht('Tasks')); - if (isset($methods_by_task[''])) { - $tasks[''] = array( - 'name' => '', - 'title' => pht('Other Methods'), - 'defined' => $symbol, - ); - } - foreach ($tasks as $spec) { $section->addContent( id(new PhabricatorHeaderView()) @@ -169,7 +173,7 @@ final class DivinerAtomController extends DivinerController { $item = array( $item, " \xE2\x80\x94 ", - phutil_safe_html($atom->getSummary())); + $atom->getSummary()); } $list_items[] = phutil_tag('li', array(), $item); @@ -429,7 +433,15 @@ final class DivinerAtomController extends DivinerController { } } - return $task_specs + $extends_task_specs; + $specs = $task_specs + $extends_task_specs; + + // Reorder "@tasks" in original declaration order. Basically, we want to + // use the documentation of the closest subclass, but put tasks which + // were declared by parents first. + $keys = array_keys($extends_task_specs); + $specs = array_select_keys($specs, $keys) + $specs; + + return $specs; } private function renderFullSignature( @@ -616,21 +628,29 @@ final class DivinerAtomController extends DivinerController { if (!strlen(trim($symbol->getMarkupText($field)))) { continue; } - $out[] = id(new PHUIBoxView()) - ->addPadding(PHUI::PADDING_LARGE_LEFT) - ->addPadding(PHUI::PADDING_LARGE_RIGHT) - ->addClass('diviner-method-implementation-header') - ->appendChild( - pht('From parent implementation in %s:', $impl->getName())); - } else if ($out) { - $out[] = id(new PHUIBoxView()) - ->addPadding(PHUI::PADDING_LARGE_LEFT) - ->addPadding(PHUI::PADDING_LARGE_RIGHT) - ->addClass('diviner-method-implementation-header') - ->appendChild( - pht('From this implementation:')); } - $out[] = $this->renderDocumentationText($symbol, $engine); + + $doc = $this->renderDocumentationText($symbol, $engine); + + if (($impl !== $parent) || $out) { + $where = id(new PHUIBoxView()) + ->addPadding(PHUI::PADDING_MEDIUM_LEFT) + ->addPadding(PHUI::PADDING_MEDIUM_RIGHT) + ->addClass('diviner-method-implementation-header') + ->appendChild($impl->getName()); + $doc = array($where, $doc); + + if ($impl !== $parent) { + $doc = phutil_tag( + 'div', + array( + 'class' => 'diviner-method-implementation-inherited', + ), + $doc); + } + } + + $out[] = $doc; } // If we only have inherited implementations but none have documentation, diff --git a/src/applications/files/engine/PhabricatorFileStorageEngine.php b/src/applications/files/engine/PhabricatorFileStorageEngine.php index ee4b691454..393357c054 100644 --- a/src/applications/files/engine/PhabricatorFileStorageEngine.php +++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php @@ -9,12 +9,18 @@ * * For more information, see @{article:File Storage Technical Documentation}. * + * @task construct Constructing an Engine * @task meta Engine Metadata * @task file Managing File Data * @group filestorage */ abstract class PhabricatorFileStorageEngine { + /** + * Construct a new storage engine. + * + * @task construct + */ final public function __construct() { // } diff --git a/src/applications/files/engine/PhabricatorS3FileStorageEngine.php b/src/applications/files/engine/PhabricatorS3FileStorageEngine.php index b1ca579394..1dc8c7d9cd 100644 --- a/src/applications/files/engine/PhabricatorS3FileStorageEngine.php +++ b/src/applications/files/engine/PhabricatorS3FileStorageEngine.php @@ -4,20 +4,17 @@ * Amazon S3 file storage engine. This engine scales well but is relatively * high-latency since data has to be pulled off S3. * - * @task impl Implementation - * @task internal Internals - * @group filestorage + * @task internal Internals */ final class PhabricatorS3FileStorageEngine extends PhabricatorFileStorageEngine { + /* -( Implementation )----------------------------------------------------- */ /** - * This engine identifies as "amazon-s3". - * - * @task impl + * This engine identifies as `amazon-s3`. */ public function getEngineIdentifier() { return 'amazon-s3'; @@ -25,8 +22,7 @@ final class PhabricatorS3FileStorageEngine /** - * Write file data into S3. - * @task impl + * Writes file data into Amazon S3. */ public function writeFile($data, array $params) { $s3 = $this->newS3API(); @@ -55,8 +51,7 @@ final class PhabricatorS3FileStorageEngine /** - * Load a stored blob from S3. - * @task impl + * Load a stored blob from Amazon S3. */ public function readFile($handle) { $result = $this->newS3API()->getObject( @@ -74,8 +69,7 @@ final class PhabricatorS3FileStorageEngine /** - * Delete a blob from S3. - * @task impl + * Delete a blob from Amazon S3. */ public function deleteFile($handle) { AphrontWriteGuard::willWrite(); diff --git a/webroot/rsrc/css/diviner/diviner-shared.css b/webroot/rsrc/css/diviner/diviner-shared.css index 17b3ce1377..f904993b15 100644 --- a/webroot/rsrc/css/diviner/diviner-shared.css +++ b/webroot/rsrc/css/diviner/diviner-shared.css @@ -71,10 +71,6 @@ margin: 16px; } -.diviner-method-implementation-header { - color: {$lightgreytext}; -} - .diviner-atom-signature { font-weight: normal; } @@ -90,3 +86,24 @@ .diviner-list a { font-weight: bold; } + +.diviner-method-implementation-header { + color: {$greytext}; + margin-bottom: -8px; +} + +.diviner-method-implementation-inherited { + color: {$darkgreytext}; +} + +.diviner-method-implementation-inherited .diviner-method-implementation-header { + color: {$lightgreytext}; +} + +/** + * Fix excessive padding between method headers and method documentation for + * methods with no inherited context. + */ +.diviner-document-section .phabricator-header-shell + .phabricator-remarkup { + padding-top: 0; +}