r69766 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r69765‎ | r69766 | r69767 >
Date:22:54, 22 July 2010
Author:mah
Status:reverted (Comments)
Tags:
Comment:
* Consolidate instances of LqtView::addJSandCSS() and eliminate one (apparently) spurious instance so that the JS and CSS is added only with when a thread is shown and the caller doesn't have to worry about also calling addJSandCSS()
* Add hook (LiquidThreadsShowThread) for integrating other extensions into the LQT viewer.
* Refactor WikiEditor/LQT integration so that WikiEditor uses a hook provided by LQT instead of the hackish LQT call to WikiEditor (r68787)
(Made sure commit doesn't have any pure WS changes since I *can* listen)
Modified paths:
  • /trunk/extensions/LiquidThreads/classes/View.php (modified) (history)
  • /trunk/extensions/LiquidThreads/pages/NewUserMessagesView.php (modified) (history)
  • /trunk/extensions/LiquidThreads/pages/SpecialHotTopics.php (modified) (history)
  • /trunk/extensions/LiquidThreads/pages/TalkpageHistoryView.php (modified) (history)
  • /trunk/extensions/LiquidThreads/pages/TalkpageView.php (modified) (history)
  • /trunk/extensions/LiquidThreads/pages/ThreadHistoryListingView.php (modified) (history)
  • /trunk/extensions/LiquidThreads/pages/ThreadPermalinkView.php (modified) (history)
  • /trunk/extensions/UsabilityInitiative/WikiEditor/WikiEditor.hooks.php (modified) (history)
  • /trunk/extensions/UsabilityInitiative/WikiEditor/WikiEditor.php (modified) (history)

Diff [purge]

Index: trunk/extensions/UsabilityInitiative/WikiEditor/WikiEditor.php
@@ -68,3 +68,4 @@
6969 // Register Hooks
7070 $wgHooks['EditPageBeforeEditToolbar'][] = 'WikiEditorHooks::addModules';
7171 $wgHooks['GetPreferences'][] = 'WikiEditorHooks::addPreferences';
 72+$wgHooks['LiquidThreadsShowThread'][] = 'WikiEditorHooks::addModulesForThread';
Index: trunk/extensions/UsabilityInitiative/WikiEditor/WikiEditor.hooks.php
@@ -473,6 +473,18 @@
474474 return true;
475475 }
476476
 477+ function addModulesForThread( $thread, $levelNum, $totalInLevel, $options ) {
 478+ static $firstRun = true;
 479+
 480+ if( $firstRun ) {
 481+ $temp = null;
 482+ WikiEditorHooks::addModules( $temp );
 483+ $firstRun = false;
 484+ }
 485+
 486+ return true;
 487+ }
 488+
477489 /**
478490 * GetPreferences hook
479491 * Add module-releated items to the preferences
Index: trunk/extensions/LiquidThreads/classes/View.php
@@ -1212,11 +1212,6 @@
12131213 $wgOut->addScriptFile( "$wgLiquidThreadsExtensionPath/js/lqt.toolbar.js" );
12141214 $wgOut->addExtensionStyle( "$wgLiquidThreadsExtensionPath/lqt.css?{$wgStyleVersion}" );
12151215
1216 - if ( class_exists( 'WikiEditorHooks' ) ) {
1217 - $temp = null;
1218 - WikiEditorHooks::addModules( $temp );
1219 - }
1220 -
12211216 self::$stylesAndScriptsDone = true;
12221217 }
12231218
@@ -1963,6 +1958,10 @@
19641959 )
19651960 );
19661961
 1962+ if ( wfRunHooks( 'LiquidThreadsShowThread', array( $thread, $levelNum, $totalInLevel, $options ) ) ) {
 1963+ self::addJSandCSS();
 1964+ }
 1965+
19671966 // Flush output to display thread
19681967 $this->output->addHTML( $html );
19691968 $this->output->addHTML( Xml::openElement( 'div',
Index: trunk/extensions/LiquidThreads/pages/TalkpageHistoryView.php
@@ -5,7 +5,6 @@
66 function show() {
77 global $wgUser;
88
9 - self::addJSandCSS();
109 wfLoadExtensionMessages( 'LiquidThreads' );
1110
1211 $sk = $wgUser->getSkin();
Index: trunk/extensions/LiquidThreads/pages/ThreadPermalinkView.php
@@ -232,7 +232,6 @@
233233 $this->output->addFeedLink( $format, $url );
234234 }
235235
236 - self::addJSandCSS();
237236 $this->output->setSubtitle( $this->getSubtitle() );
238237
239238 if ( $this->methodApplies( 'summarize' ) )
Index: trunk/extensions/LiquidThreads/pages/NewUserMessagesView.php
@@ -83,8 +83,6 @@
8484 }
8585
8686 function showOnce() {
87 - self::addJSandCSS();
88 -
8987 NewMessages::recacheMessageCount( $this->user->getId() );
9088
9189 static $scriptDone = false;
Index: trunk/extensions/LiquidThreads/pages/SpecialHotTopics.php
@@ -14,8 +14,6 @@
1515 $wgOut->setPageTitle( wfMsg( 'lqt-hot-topics' ) );
1616 $view = LqtView::getView();
1717
18 - LqtView::addJsAndCss();
19 -
2018 // Get hot topics
2119 $topics = LqtHotTopicsController::getHotThreads();
2220
Index: trunk/extensions/LiquidThreads/pages/ThreadHistoryListingView.php
@@ -7,7 +7,6 @@
88 $this->showMissingThreadPage();
99 return false;
1010 }
11 - self::addJSandCSS();
1211 wfLoadExtensionMessages( 'LiquidThreads' );
1312
1413 $this->thread->updateHistory();
Index: trunk/extensions/LiquidThreads/pages/TalkpageView.php
@@ -229,7 +229,6 @@
230230 wfLoadExtensionMessages( 'LiquidThreads' );
231231
232232 $this->output->setPageTitle( $this->title->getPrefixedText() );
233 - self::addJSandCSS();
234233
235234 // Expose feed links.
236235 global $wgFeedClasses, $wgScriptPath, $wgServer;

Follow-up revisions

RevisionCommit summaryAuthorDate
r69774Fix r69766: Fix strict error per r69766#c7900soxred9306:48, 23 July 2010
r69803re Catrope's comments on r69766 + the strict warnings:...mah17:30, 23 July 2010
r69804re r69803, r69766: remove where WikiEditor actually hooked into LQT.mah17:34, 23 July 2010
r70076LiquidThreads: revert r69766 and friends r69774, r69803, r69804. Broken, and ...werdna10:36, 28 July 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r68787Include WikiEditor resources if necessarywerdna20:48, 30 June 2010

Comments

#Comment by Raymond (talk | contribs)   06:43, 23 July 2010

Seen on translatewiki:

PHP Strict Standards: Non-static method WikiEditorHooks::addModulesForThread() cannot be called statically in /www/w/includes/Hooks.php on line 129
#Comment by MarkAHershberger (talk | contribs)   17:00, 23 July 2010

see r69774

#Comment by Catrope (talk | contribs)   06:45, 23 July 2010

This kind of intra-extension interaction freaks me out. LQT wishing to use WikiEditor and checking for it / invoking it is not too bad, but WikiEditor having to know about LQT is something I really don't like.

#Comment by MarkAHershberger (talk | contribs)   16:55, 23 July 2010

I don't see this as WikiEditor "having to know about LQT" -- but simply making its functionality available to anyone who calls the "LiquidThreadsShowThread" hook.

I did consider just having LQT call the 'EditPageBeforeEditToolbar' hook, which would accomplish the same thing. Would this be preferable?

#Comment by Catrope (talk | contribs)   17:00, 23 July 2010

WikiEditor hooking into a hook called "LiquidThreads...." counts as WikiEditor having to know about LQT for me.

The basic idea is that WikiEditor should expose some sort of generic interface that extensions can use, not one that's LQT-specific in either name or workings.

#Comment by Werdna (talk | contribs)   02:33, 24 July 2010

Well, in general either LQT has to know about WikiEditor or WikiEditor has to know about LQT.

Or we could add a generic hook like "EditBoxAddScripts" and call it from EditPage as well.

#Comment by Werdna (talk | contribs)   10:20, 28 July 2010

Another apparently untested revision. I get a MediaWiki exception and a PHP warning when viewing a LiquidThreads talk page.

#Comment by Werdna (talk | contribs)   10:22, 28 July 2010

Bug is in r69803.

Status & tagging log