From 5b31c49e15ea20571853151899cece11bbd83431 Mon Sep 17 00:00:00 2001 From: jdlrobson Date: Mon, 11 May 2020 15:09:21 -0700 Subject: [PATCH] BaseTemplate:makeListItem is deprecated Use SkinTemplateNavigation hook instead and copy the collapsible behavior to the menu function The code inside getSkinData that checks VectorUseIconWatch is redundant as it duplicates checks already inside SkinTemplate::buildContentNavigationUrls It is enough to simplify check whether watch or unwatch is present in the array. Bug: T251212 Change-Id: If6b10b0ddcbd4b21dd13a2813e60b604c3a23415 --- includes/Hooks.php | 44 ++++++++++ includes/VectorTemplate.php | 68 +++------------ skin.json | 1 + tests/phpunit/integration/VectorHooksTest.php | 37 +++++++++ .../integration/VectorTemplateTest.php | 82 +++++-------------- 5 files changed, 114 insertions(+), 118 deletions(-) diff --git a/includes/Hooks.php b/includes/Hooks.php index a1233be..af1605d 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -33,6 +33,50 @@ class Hooks { } } + /** + * Add icon class to an existing navigation item inside a menu hook. + * See self::onSkinTemplateNavigation. + * @param array $item + * @return array + */ + private static function navigationLinkToIcon( array $item ) { + if ( !isset( $item['class'] ) ) { + $item['class'] = ''; + } + $item['class'] = rtrim( 'icon ' . $item['class'], ' ' ); + return $item; + } + + /** + * Upgrades Vector's watch action to a watchstar. + * + * @see https://www.mediawiki.org/wiki/Manual:Hooks/SkinTemplateNavigation + * @param SkinTemplate $sk + * @param array &$content_navigation + */ + public static function onSkinTemplateNavigation( $sk, &$content_navigation ) { + if ( + $sk->getSkinName() === 'vector' && + $sk->getConfig()->get( 'VectorUseIconWatch' ) + ) { + $key = null; + if ( isset( $content_navigation['actions']['watch'] ) ) { + $key = 'watch'; + } + if ( isset( $content_navigation['actions']['unwatch'] ) ) { + $key = 'unwatch'; + } + + // Promote watch link from actions to views and add an icon + if ( $key !== null ) { + $content_navigation['views'][$key] = self::navigationLinkToIcon( + $content_navigation['actions'][$key] + ); + unset( $content_navigation['actions'][$key] ); + } + } + } + /** * Add Vector preferences to the user's Special:Preferences page directly underneath skins. * diff --git a/includes/VectorTemplate.php b/includes/VectorTemplate.php index 0504823..b318766 100644 --- a/includes/VectorTemplate.php +++ b/includes/VectorTemplate.php @@ -22,8 +22,6 @@ * @ingroup Skins */ -use MediaWiki\MediaWikiServices; - /** * QuickTemplate subclass for Vector * @ingroup Skins @@ -122,30 +120,6 @@ class VectorTemplate extends BaseTemplate { $out = $skin->getOutput(); $title = $out->getTitle(); - // Move the watch/unwatch star outside of the collapsed "actions" menu to the main "views" menu - if ( $this->getConfig()->get( 'VectorUseIconWatch' ) ) { - $mode = ( $skin->getRelevantTitle()->isWatchable() && - MediaWikiServices::getInstance()->getPermissionManager()->userHasRight( - $skin->getUser(), - 'viewmywatchlist' - ) && - MediaWikiServices::getInstance()->getWatchedItemStore()->isWatched( - $skin->getUser(), - $skin->getRelevantTitle() - ) - ) ? 'unwatch' : 'watch'; - - $actionUrls = $contentNavigation[ 'actions' ] ?? []; - if ( array_key_exists( $mode, $actionUrls ) ) { - $viewUrls = $contentNavigation[ 'views' ] ?? []; - $viewUrls[ $mode ] = $actionUrls[ $mode ]; - unset( $actionUrls[ $mode ] ); - $contentNavigation['actions'] = $actionUrls; - $contentNavigation['views'] = $viewUrls; - $this->set( 'content_navigation', $contentNavigation ); - } - } - ob_start(); Hooks::run( 'VectorBeforeFooter', [], '1.35' ); $htmlHookVectorBeforeFooter = ob_get_contents(); @@ -401,35 +375,6 @@ class VectorTemplate extends BaseTemplate { ]; } - /** - * @inheritDoc - */ - public function makeListItem( $key, $item, $options = [] ) { - // For fancy styling of watch/unwatch star - if ( - $this->getConfig()->get( 'VectorUseIconWatch' ) - && ( $key === 'watch' || $key === 'unwatch' ) - ) { - if ( !isset( $item['class'] ) ) { - $item['class'] = ''; - } - $item['class'] = rtrim( 'icon ' . $item['class'], ' ' ); - $item['primary'] = true; - } - - // Add CSS class 'collapsible' to links which are not marked as "primary" - if ( - isset( $options['vector-collapsible'] ) && $options['vector-collapsible'] ) { - if ( !isset( $item['class'] ) ) { - $item['class'] = ''; - } - /* @phan-suppress-next-line PhanTypePossiblyInvalidDimOffset */ - $item['class'] = rtrim( 'collapsible ' . $item['class'], ' ' ); - } - - return parent::makeListItem( $key, $item, $options ); - } - /** * @param string $label to be used to derive the id and human readable label of the menu * If the key has an entry in the constant MENU_LABEL_KEYS then that message will be used for the @@ -472,7 +417,16 @@ class VectorTemplate extends BaseTemplate { ]; foreach ( $urls as $key => $item ) { - $props['html-items'] .= $this->makeListItem( $key, $item, $options ); + // Add CSS class 'collapsible' to all links EXCEPT watchstar. + if ( + $key !== 'watch' && $key !== 'unwatch' && + isset( $options['vector-collapsible'] ) && $options['vector-collapsible'] ) { + if ( !isset( $item['class'] ) ) { + $item['class'] = ''; + } + $item['class'] = rtrim( 'collapsible ' . $item['class'], ' ' ); + } + $props['html-items'] .= $this->getSkin()->makeListItem( $key, $item, $options ); // Check the class of the item for a `selected` class and if so, propagate the items // label to the main label. @@ -518,7 +472,7 @@ class VectorTemplate extends BaseTemplate { // This code doesn't belong here, it belongs in the UniversalLanguageSelector // It is here to workaround the fact that it wants to be the first item in the personal menus. if ( array_key_exists( 'uls', $personalTools ) ) { - $uls = $this->makeListItem( 'uls', $personalTools[ 'uls' ] ); + $uls = $skin->makeListItem( 'uls', $personalTools[ 'uls' ] ); unset( $personalTools[ 'uls' ] ); } else { $uls = ''; diff --git a/skin.json b/skin.json index e0b0869..6823047 100644 --- a/skin.json +++ b/skin.json @@ -35,6 +35,7 @@ "BeforePageDisplayMobile": "Vector\\Hooks::onBeforePageDisplayMobile", "GetPreferences": "Vector\\Hooks::onGetPreferences", "PreferencesFormPreSave": "Vector\\Hooks::onPreferencesFormPreSave", + "SkinTemplateNavigation": "Vector\\Hooks::onSkinTemplateNavigation", "LocalUserCreated": "Vector\\Hooks::onLocalUserCreated" }, "@note": "When modifying skins.vector.styles definition, make sure the installer still works", diff --git a/tests/phpunit/integration/VectorHooksTest.php b/tests/phpunit/integration/VectorHooksTest.php index ee7ebfb..6d85da1 100644 --- a/tests/phpunit/integration/VectorHooksTest.php +++ b/tests/phpunit/integration/VectorHooksTest.php @@ -271,4 +271,41 @@ class VectorHooksTest extends \MediaWikiTestCase { $isAutoCreated = false; Hooks::onLocalUserCreated( $user, $isAutoCreated ); } + + /** + * @covers ::onSkinTemplateNavigation + */ + public function testOnSkinTemplateNavigation() { + $this->setMwGlobals( [ + 'wgVectorUseIconWatch' => true + ] ); + $skin = new SkinVector(); + $contentNavWatch = [ + 'actions' => [ + 'watch' => [ 'class' => 'watch' ], + ] + ]; + $contentNavUnWatch = [ + 'actions' => [ + 'move' => [ 'class' => 'move' ], + 'unwatch' => [], + ], + ]; + + Hooks::onSkinTemplateNavigation( $skin, $contentNavUnWatch ); + Hooks::onSkinTemplateNavigation( $skin, $contentNavWatch ); + + $this->assertTrue( + strpos( $contentNavWatch['views']['watch']['class'], 'icon' ) !== false, + 'Watch list items require an "icon" class' + ); + $this->assertTrue( + strpos( $contentNavUnWatch['views']['unwatch']['class'], 'icon' ) !== false, + 'Unwatch list items require an "icon" class' + ); + $this->assertFalse( + strpos( $contentNavUnWatch['actions']['move']['class'], 'icon' ) !== false, + 'List item other than watch or unwatch should not have an "icon" class' + ); + } } diff --git a/tests/phpunit/integration/VectorTemplateTest.php b/tests/phpunit/integration/VectorTemplateTest.php index 4d5e11f..20e2819 100644 --- a/tests/phpunit/integration/VectorTemplateTest.php +++ b/tests/phpunit/integration/VectorTemplateTest.php @@ -50,15 +50,32 @@ class VectorTemplateTest extends MediaWikiIntegrationTestCase { } /** - * @covers ::makeListItem + * @covers ::getMenuData */ public function testMakeListItemRespectsCollapsibleOption() { - $template = $this->provideVectorTemplateObject(); + $vectorTemplate = $this->provideVectorTemplateObject(); + $template = TestingAccessWrapper::newFromObject( $vectorTemplate ); $listItemClass = 'my_test_class'; $options = [ 'vector-collapsible' => true ]; $item = [ 'class' => $listItemClass ]; - $nonCollapsible = $template->makeListItem( 'key', $item, [] ); - $collapsible = $template->makeListItem( 'key', [], $options ); + $propsCollapsible = $template->getMenuData( + 'foo', + [ + 'bar' => $item, + ], + 0, + $options + ); + $propsNonCollapsible = $template->getMenuData( + 'foo', + [ + 'bar' => $item, + ], + 0, + [] + ); + $nonCollapsible = $propsNonCollapsible['html-items']; + $collapsible = $propsCollapsible['html-items']; $this->assertTrue( $this->expectNodeAttribute( $collapsible, 'li', 'class', 'collapsible' ), @@ -74,63 +91,6 @@ class VectorTemplateTest extends MediaWikiIntegrationTestCase { ); } - /** - * @covers ::makeListItem - */ - public function testWatcAndUnwatchHasIconClass() { - $template = $this->provideVectorTemplateObject(); - $this->setMwGlobals( [ - 'wgVectorUseIconWatch' => true - ] ); - $listItemClass = 'my_test_class'; - $options = []; - $item = [ 'class' => $listItemClass ]; - - $watchListItem = $template->makeListItem( 'watch', $item, [] ); - $unwatchListItem = $template->makeListItem( 'unwatch', [], $options ); - $regularListItem = $template->makeListItem( 'whatever', $item, $options ); - - $this->assertTrue( - $this->expectNodeAttribute( $watchListItem, 'li', 'class', 'icon' ), - 'Watch list items require an "icon" class' - ); - $this->assertTrue( - $this->expectNodeAttribute( $unwatchListItem, 'li', 'class', 'icon' ), - 'Unwatch list items require an "icon" class' - ); - $this->assertFalse( - $this->expectNodeAttribute( $regularListItem, 'li', 'class', 'icon' ), - 'List item other than watch or unwatch should not have an "icon" class' - ); - $this->assertTrue( - $this->expectNodeAttribute( $watchListItem, 'li', 'class', $listItemClass ), - 'Watch list items require an item class' - ); - } - - /** - * @covers ::makeListItem - */ - public function testWatchAndUnwatchHasIconClassOnlyIfVectorUseIconWatchIsSet() { - $template = $this->provideVectorTemplateObject(); - $this->setMwGlobals( [ - 'wgVectorUseIconWatch' => false - ] ); - $listItemClass = 'my_test_class'; - $item = [ 'class' => $listItemClass ]; - - $watchListItem = $template->makeListItem( 'watch', $item, [] ); - - $this->assertFalse( - $this->expectNodeAttribute( $watchListItem, 'li', 'class', 'icon' ), - 'Watch list should not have an "icon" class when VectorUserIconWatch is disabled' - ); - $this->assertTrue( - $this->expectNodeAttribute( $watchListItem, 'li', 'class', $listItemClass ), - 'Watch list items require an item class' - ); - } - /** * @covers ::getMenuProps */