Page MenuHomePhabricator

ProofreadPage source tab not shown in the Minerva skin
Closed, ResolvedPublic

Description

The "Source" tab isn't shown in the Minerva skin at WIkisource for a page using the <pages/> tag.

There is a proofread status progress bar, but no link.

This means mobile users have no way to access the index page.

2021-02-10_111810_622x196_screenshot.png (196×622 px, 19 KB)

Event Timeline

Jdlrobson subscribed.

ProofReadPageExtension should be able to do this by adding to the namespaces key like so:

$wgHooks['SkinTemplateNavigation::Universal'] = function ( SkinTemplate $sktemplate, array &$links ) {
	if ($sktemplate->getSkin()->getSkinName() === 'minerva' ) {

		$links['namespaces']['link'] = [
			'href' => '$',
			'text' => 'yo'
		];
	}
};

Let me know if you need any help.

@Jdlrobson where should this be done? No other code seems to reference $wgHooks['SkinTemplateNavigation::Universal'] in this way? Also, we need to only do this when a page uses the <pages /> tag.

@Jdlrobson where should this be done?

Inside Extension:ProofReadPage

No other code seems to reference $wgHooks['SkinTemplateNavigation::Universal'] in this way?

That's because this is new, specifically for purposes like this. :)

Also, we need to only do this when a page uses the <pages /> tag.

The hook https://www.mediawiki.org/wiki/Manual:Hooks/SkinTemplateNavigation::Universal has access to SkinTemplate. Can you check whether a pages tag is present based on this instance?

Try it out, and let me know if you run into any issues. Happy to help debug any patches that are not quite working as expected.

Inside Extension:ProofReadPage

Sorry I'm being thick: in what function? There's already a mapping in extension.json: "SkinTemplateNavigation": "ProofreadPage\\ProofreadPage::onSkinTemplateNavigation",

Also, this should be for all skins: currently the link is added client side by JS and then enWS "polyfills" one for mobile. Ideally this should be server side all the time.

How would you check for the presence of a tag based on a SkinTemplate?

Inside Extension:ProofReadPage

Sorry I'm being thick: in what function? There's already a mapping in extension.json: "SkinTemplateNavigation": "ProofreadPage\\ProofreadPage::onSkinTemplateNavigation",

The onSkinTemplateNavigation is being deprecated for two separate hooks, (SkinTemplateNavigation::SpecialPage (for SpecialPages) and SkinTemplateNavigation::Universal (everything else) per T255319).

How would you check for the presence of a tag based on a SkinTemplate?

AFAIK if we obtain the article ID (from $skin->getTitle()->getArticleID()), we can just use ProofreadPageDbConnector::getIndexTitleForPageId( $id ); to get the associated Index: page for the particular page.

That being said, I'm not sure, but I personally think we should probably implement this using $skin->getOutput()->getExtensionData() (setting the required extensionData during the <pages /> parsing operation using $parserOutput->setExtensionData()) since ProofreadPageDbConnector::getIndexTitleForPageId( $id ); internally queries the templatelinks table which might be expensive to do for every page load. (cc @Tpt)

The "Source" tab is currently inserted using javascript ("modules/article/ext.proofreadpage.article.js"). It might be possible to use the same trick for Minerva. But migrating everything to PHP would be nicer.

That being said, I'm not sure, but I personally think we should probably implement this using $skin->getOutput()->getExtensionData() (setting the required extensionData during the <pages /> parsing operation using $parserOutput->setExtensionData()) since ProofreadPageDbConnector::getIndexTitleForPageId( $id ); internally queries the templatelinks table which might be expensive to do for every page load. (cc @Tpt)

ProofreadPageDbConnector::getIndexTitleForPageId( $id ); has the advantage to work not only with the <pages> tag but with also plain inclusion syntax {{Page:XXX}}. Dropping support for this will remove the "Source" tab for this tab.
I wrote a work around as part of this change.

See also T239033 which is about moving to be server-side in all skins.

The onSkinTemplateNavigation is being deprecated for two separate hooks, (SkinTemplateNavigation::SpecialPage (for SpecialPages) and SkinTemplateNavigation::Universal (everything else) per T255319).

Not quite! The SkinTemplateNavigation::Universal runs for special pages too and that's the one I'd recommend using. SkinTemplateNavigation::SpecialPage will also be deprecated eventually.

However, I wouldn't worry too much. Deprecation of these is going to be pretty slow given the many usages and it's not a priority of mine right now.

Soda assigned this task to Jdlrobson.