From 8ac2da9850b0b3593050b19b325f3516df35fbc8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Aug 2013 07:51:01 -0700 Subject: [PATCH] Provide hasChildren() to replace isEmptyContent() Summary: Fixes T3698. Sometimes views need to render differently depending on whether they contain content or not. The existing approach for this is `isEmptyContent()`, which doesn't work well and is sort of hacky (it implies double-rendering content, which is not always free or side-effect free). Instead, provide a test for an element without children. This test is powerful enough to catch the easy cases of `null`, etc., and just do the expected thing, but will not catch a View which is reduced upon rendering. Since this is rare and we have no actual need for it today, just accept that as a limitation. Test Plan: Viewed Timeline and Feed UI examples. Viewed Feed (feed), Pholio (timelineview), and Differential (old transactionview). {F53915} Reviewers: chad, btrahan Reviewed By: chad CC: aran Maniphest Tasks: T3698 Differential Revision: https://secure.phabricator.com/D6718 --- src/__phutil_library_map__.php | 2 + src/view/AphrontNullView.php | 2 +- src/view/AphrontView.php | 109 +++++++++++++++--- .../PhabricatorAphrontViewTestCase.php | 25 ++++ .../layout/PhabricatorTimelineEventView.php | 8 +- .../layout/PhabricatorTransactionView.php | 5 +- src/view/phui/PHUIFeedStoryView.php | 2 +- 7 files changed, 128 insertions(+), 25 deletions(-) create mode 100644 src/view/__tests__/PhabricatorAphrontViewTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 009cf3f170..4377ad50e3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -775,6 +775,7 @@ phutil_register_library_map(array( 'PhabricatorAllCapsTranslation' => 'infrastructure/internationalization/PhabricatorAllCapsTranslation.php', 'PhabricatorAnchorView' => 'view/layout/PhabricatorAnchorView.php', 'PhabricatorAphrontBarExample' => 'applications/uiexample/examples/PhabricatorAphrontBarExample.php', + 'PhabricatorAphrontViewTestCase' => 'view/__tests__/PhabricatorAphrontViewTestCase.php', 'PhabricatorApplication' => 'applications/base/PhabricatorApplication.php', 'PhabricatorApplicationApplications' => 'applications/meta/application/PhabricatorApplicationApplications.php', 'PhabricatorApplicationAudit' => 'applications/audit/application/PhabricatorApplicationAudit.php', @@ -2804,6 +2805,7 @@ phutil_register_library_map(array( 'PhabricatorAllCapsTranslation' => 'PhabricatorTranslation', 'PhabricatorAnchorView' => 'AphrontView', 'PhabricatorAphrontBarExample' => 'PhabricatorUIExample', + 'PhabricatorAphrontViewTestCase' => 'PhabricatorTestCase', 'PhabricatorApplicationApplications' => 'PhabricatorApplication', 'PhabricatorApplicationAudit' => 'PhabricatorApplication', 'PhabricatorApplicationAuth' => 'PhabricatorApplication', diff --git a/src/view/AphrontNullView.php b/src/view/AphrontNullView.php index 2a218e700a..cfcf48350e 100644 --- a/src/view/AphrontNullView.php +++ b/src/view/AphrontNullView.php @@ -3,7 +3,7 @@ final class AphrontNullView extends AphrontView { public function render() { - return phutil_implode_html('', $this->renderChildren()); + return $this->renderChildren(); } } diff --git a/src/view/AphrontView.php b/src/view/AphrontView.php index c9cf7ae6b6..33bddb701e 100644 --- a/src/view/AphrontView.php +++ b/src/view/AphrontView.php @@ -1,64 +1,143 @@ user = $user; return $this; } + + /** + * @task config + */ protected function getUser() { return $this->user; } + +/* -( Managing Children )-------------------------------------------------- */ + + + /** + * Test if this View accepts children. + * + * By default, views accept children, but subclases may override this method + * to prevent children from being appended. Doing so will cause + * @{method:appendChild} to throw exceptions instead of appending children. + * + * @return bool True if the View should accept children. + * @task children + */ protected function canAppendChild() { return true; } + + /** + * Append a child to the list of children. + * + * This method will only work if the view supports children, which is + * determined by @{method:canAppendChild}. + * + * @param wild Something renderable. + * @return this + */ final public function appendChild($child) { if (!$this->canAppendChild()) { $class = get_class($this); throw new Exception( pht("View '%s' does not support children.", $class)); } + $this->children[] = $child; + return $this; } + + /** + * Produce children for rendering. + * + * Historically, this method reduced children to a string representation, + * but it no longer does. + * + * @return wild Renderable children. + * @task + */ final protected function renderChildren() { return $this->children; } + /** - * @deprecated + * Test if an element has no children. + * + * @return bool True if this element has children. + * @task children */ - final protected function renderSingleView($child) { - phutil_deprecated( - 'AphrontView->renderSingleView()', - "This method no longer does anything; it can be removed and replaced ". - "with its arguments."); - return $child; + final public function hasChildren() { + if ($this->children) { + $this->children = $this->reduceChildren($this->children); + } + return (bool)$this->children; } - final protected function isEmptyContent($content) { - if (is_array($content)) { - foreach ($content as $element) { - if (!$this->isEmptyContent($element)) { - return false; + + /** + * Reduce effectively-empty lists of children to be actually empty. This + * recursively removes `null`, `''`, and `array()` from the list of children + * so that @{method:hasChildren} can more effectively align with expectations. + * + * NOTE: Because View children are not rendered, a View which renders down + * to nothing will not be reduced by this method. + * + * @param list Renderable children. + * @return list Reduced list of children. + * @task children + */ + private function reduceChildren(array $children) { + foreach ($children as $key => $child) { + if ($child === null) { + unset($children[$key]); + } else if ($child === '') { + unset($children[$key]); + } else if (is_array($child)) { + $child = $this->reduceChildren($child); + if ($child) { + $children[$key] = $child; + } else { + unset($children[$key]); } } - return true; - } else { - return !strlen((string)$content); } + return $children; } + +/* -( Rendering )---------------------------------------------------------- */ + + abstract public function render(); + +/* -( PhutilSafeHTMLProducerInterface )------------------------------------ */ + + public function producePhutilSafeHTML() { return $this->render(); } diff --git a/src/view/__tests__/PhabricatorAphrontViewTestCase.php b/src/view/__tests__/PhabricatorAphrontViewTestCase.php new file mode 100644 index 0000000000..2d05151d75 --- /dev/null +++ b/src/view/__tests__/PhabricatorAphrontViewTestCase.php @@ -0,0 +1,25 @@ +assertEqual(false, $view->hasChildren()); + + $values = array( + null, + '', + array(), + array(null, ''), + ); + + foreach ($values as $value) { + $view->appendChild($value); + $this->assertEqual(false, $view->hasChildren()); + } + + $view->appendChild("!"); + $this->assertEqual(true, $view->hasChildren()); + } + +} diff --git a/src/view/layout/PhabricatorTimelineEventView.php b/src/view/layout/PhabricatorTimelineEventView.php index fd9faa04a8..0807edcdf4 100644 --- a/src/view/layout/PhabricatorTimelineEventView.php +++ b/src/view/layout/PhabricatorTimelineEventView.php @@ -100,10 +100,8 @@ final class PhabricatorTimelineEventView extends AphrontView { } public function render() { - $content = $this->renderChildren(); - $title = $this->title; - if (($title === null) && $this->isEmptyContent($content)) { + if (($title === null) && !$this->hasChildren()) { $title = ''; } @@ -163,7 +161,7 @@ final class PhabricatorTimelineEventView extends AphrontView { $classes = array(); $classes[] = 'phabricator-timeline-event-view'; $classes[] = 'phabricator-timeline-border'; - if (!$this->isEmptyContent($content)) { + if ($this->hasChildren()) { $classes[] = 'phabricator-timeline-major-event'; $content = phutil_tag( 'div', @@ -182,7 +180,7 @@ final class PhabricatorTimelineEventView extends AphrontView { array( 'class' => 'phabricator-timeline-core-content', ), - $content), + $this->renderChildren()), ))); $content = array($image, $wedge, $content); } else { diff --git a/src/view/layout/PhabricatorTransactionView.php b/src/view/layout/PhabricatorTransactionView.php index a5f4c480c3..7ceb3023bc 100644 --- a/src/view/layout/PhabricatorTransactionView.php +++ b/src/view/layout/PhabricatorTransactionView.php @@ -136,14 +136,13 @@ final class PhabricatorTransactionView extends AphrontView { } private function renderTransactionContent() { - $content = $this->renderChildren(); - if ($this->isEmptyContent($content)) { + if (!$this->hasChildren()) { return null; } return phutil_tag( 'div', array('class' => 'phabricator-transaction-content'), - $content); + $this->renderChildren()); } } diff --git a/src/view/phui/PHUIFeedStoryView.php b/src/view/phui/PHUIFeedStoryView.php index ad7f8e812a..008c286e49 100644 --- a/src/view/phui/PHUIFeedStoryView.php +++ b/src/view/phui/PHUIFeedStoryView.php @@ -124,7 +124,7 @@ final class PHUIFeedStoryView extends AphrontView { require_celerity_resource('phui-feed-story-css'); Javelin::initBehavior('phabricator-hovercards'); - $oneline = $this->isEmptyContent($this->renderChildren()); + $oneline = !$this->hasChildren(); $body = null; $foot = null;