r55685 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r55684‎ | r55685 | r55686 >
Date:21:50, 30 August 2009
Author:happy-melon
Status:reverted (Comments)
Tags:
Comment:
* Allow IndexPager->mLimitsShown to be an associative array of limit => text-to-display-in-limit-form pairs.
* Use this to add an "all" value to the limit form in Special:AllMessages. In reality, "all" is only 5000, but this is plenty more than most wikis should have.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/Pager.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialAllmessages.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/specials/SpecialAllmessages.php
——@@ -128,6 +128,7 @@
129129
130130 var $messages = null;
131131 var $talkPages = null;
 132+ public $mLimitsShown;
132133
133134 function __construct( $page, $conds, $langObj = null ) {
134135 parent::__construct();
——@@ -135,6 +136,10 @@
136137 $this->mPage = $page;
137138 $this->mConds = $conds;
138139 $this->mDefaultDirection = true; // always sort ascending
 140+ // We want to have an option for people to view *all* the messages,
 141+ // so they can use Ctrl+F to search them. 5000 is the maximum that
 142+ // will get through WebRequest::getLimitOffset().
 143+ $this->mLimitsShown = array( 20, 50, 100, 250, 500, 5000 => wfMsg('messagesall') );
139144
140145 global $wgLang, $wgContLang, $wgRequest;
141146
Index: trunk/phase3/includes/Pager.php
——@@ -879,9 +879,9 @@
880880 function getLimitSelect() {
881881 global $wgLang;
882882 $s = "<select name=\"limit\">";
883 - foreach ( $this->mLimitsShown as $limit ) {
 883+ foreach ( $this->mLimitsShown as $limit => $text ) {
884884 $selected = $limit == $this->mLimit ? 'selected="selected"' : '';
885 - $formattedLimit = $wgLang->formatNum( $limit );
 885+ $formattedLimit = $text ? $text : $wgLang->formatNum( $limit );
886886 $s .= "<option value=\"$limit\" $selected>$formattedLimit</option>\n";
887887 }
888888 $s .= "</select>";
Index: trunk/phase3/languages/messages/MessagesEn.php
——@@ -3814,6 +3814,7 @@
38153815 'watchlistall2' => 'all',
38163816 'namespacesall' => 'all',
38173817 'monthsall' => 'all',
 3818+'messagesall' => 'all',
38183819
38193820 # E-mail address confirmation
38203821 'confirmemail' => 'Confirm e-mail address',
Index: trunk/phase3/RELEASE-NOTES
——@@ -160,6 +160,8 @@
161161 * (bug 9691) Add type (signup or login) parameter to AuthPlugin::ModifyUITemplate()
162162 * (bug 14454) "Member of group(s)" in Special:Preferences causes language difficulties
163163 * (bug 16697) Unicode combining characters are difficult to edit in some browsers
 164+* IndexPager->mLimitsShown can now be an associative array of limit => text-to-
 165+ display-in-limit-form.
164166 * Parser test supports uploading results to remote CodeReview instance
165167 * (bug 20013) Added CSS class "mw-version-ext-version" is wrapped on the
166168 extension version in Special:Version

Follow-up revisions

RevisionCommit summaryAuthorDate
r55686Follow-up to r55685 - update messages.inc, and fix wierd positioning of comme...happy-melon22:06, 30 August 2009
r55688Revert r55685, r55686 and r55687 for now. Limits not working.nikerabbit06:54, 31 August 2009
r55692Reimplement r55685, r55686, r55687, with correct parsing of the associative a...happy-melon11:30, 31 August 2009

Comments

#Comment by Siebrand (talk | contribs)   21:53, 30 August 2009

maintenance/language/messages.inc needs updating when new messages are added

#Comment by Skizzerz (talk | contribs)   22:04, 30 August 2009

Then surely you can add it in? I realize you want other devs to do things properly, but marking things as fixme when you can easily fix them just to prove a point is counterproductive. Fixing it and then leaving a message here or on the talk page of the commiter, however, is not

#Comment by Happy-melon (talk | contribs)   22:08, 30 August 2009

I dunno, it's certainly helpful to point out things like this (I had *no idea* there was such a dependency). And if you comment on these commits it does drop an e-mail in the committer's inbox...

#Comment by Siebrand (talk | contribs)   22:18, 30 August 2009

Thanks. Status to new again for brion's review.

#Comment by Nikerabbit (talk | contribs)   07:07, 31 August 2009 
+foreach ( $this->mLimitsShown as $limit => $text ) {

Broken. Now $limit will be the index instead of the actual limit for all cases except where there is text. Selecting 100 sets limit to 2. 

+$formattedLimit = $text ? $text : $wgLang->formatNum( $limit );

Same here, you are formatting the text "all" too. Maybe there is confusion how foreach works for php arrays. 

+'messagesall'      => 'all',

Message documentation would be nice for translators.

There is also another instance of mLimitsShown in Pager.php, and since it is public it may be used by any extensions.

#Comment by Happy-melon (talk | contribs)   11:35, 31 August 2009

"Maybe there is confusion how foreach works for php arrays. "

Yes. I was expecting array( 20 ) to become 20 => null, but of course it actually becomes 0 => 20.

Reimplemented in r55692, also adding message documentation as you requested.

"There is also another instance of mLimitsShown in Pager.php, and since it is public it may be used by any extensions. "

Not sure what you mean here: could you clarify? The change was intended to be, and is now, fully B/C.

Status & tagging log