r86305 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r86304‎ | r86305 | r86306 >
Date:12:54, 18 April 2011
Author:diebuche
Status:ok (Comments)
Tags:todo 
Comment:
Followup r86088 per CR: Move month array builder into language; use mw.config.get(); Fix rowspans and some cleanup
Modified paths:
  • /trunk/phase3/includes/resourceloader/ResourceLoaderStartUpModule.php (modified) (history)
  • /trunk/phase3/languages/Language.php (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.tablesorter.js (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/resourceloader/ResourceLoaderStartUpModule.php
@@ -53,18 +53,7 @@
5454 implode( "\t", $digitTransTable ),
5555 );
5656 $mainPage = Title::newMainPage();
57 -
58 - #$localDateFormats = $wgContLang->getDateFormats();
59 - #$localPreferedFormat = $localDateFormats[$wgContLang->getDefaultDateFormat().' date'];
60 -
61 - $monthNames = array('');
62 - $monthNamesShort = array('');
63 - for ($i=1; $i < 13; $i++) {
64 - $monthNames[]=$wgContLang->getMonthName($i);
65 - $monthNamesShort[]=$wgContLang->getMonthAbbreviation($i);
66 - }
67 -
68 - #$localPreferedFormat = $localDateFormats['dmy date'];
 57+
6958 // Build list of variables
7059 $vars = array(
7160 'wgLoadScript' => $wgLoadScript,
@@ -85,8 +74,8 @@
8675 'wgEnableAPI' => $wgEnableAPI,
8776 'wgEnableWriteAPI' => $wgEnableWriteAPI,
8877 'wgDefaultDateFormat' => $wgContLang->getDefaultDateFormat(),
89 - 'wgMonthNames' => $monthNames,
90 - 'wgMonthNamesShort' => $monthNamesShort,
 78+ 'wgMonthNames' => $wgContLang->getMonthNamesArray(),
 79+ 'wgMonthNamesShort' => $wgContLang->getMonthAbbreviationsArray(),
9180 'wgSeparatorTransformTable' => $compactSeparatorTransTable,
9281 'wgDigitTransformTable' => $compactDigitTransTable,
9382 'wgMainPageTitle' => $mainPage ? $mainPage->getPrefixedText() : null,
Index: trunk/phase3/languages/Language.php
@@ -603,7 +603,15 @@
604604 function getMonthName( $key ) {
605605 return $this->getMessageFromDB( self::$mMonthMsgs[$key - 1] );
606606 }
607 -
 607+
 608+ function getMonthNamesArray() {
 609+ $monthNames = array( '' );
 610+ for ( $i=1; $i < 13; $i++ ) {
 611+ $monthNames[] = $this->getMonthName( $i );
 612+ }
 613+ return $monthNames;
 614+ }
 615+
608616 function getMonthNameGen( $key ) {
609617 return $this->getMessageFromDB( self::$mMonthGenMsgs[$key - 1] );
610618 }
@@ -611,7 +619,15 @@
612620 function getMonthAbbreviation( $key ) {
613621 return $this->getMessageFromDB( self::$mMonthAbbrevMsgs[$key - 1] );
614622 }
615 -
 623+
 624+ function getMonthAbbreviationsArray() {
 625+ $monthNames = array('');
 626+ for ( $i=1; $i < 13; $i++ ) {
 627+ $monthNames[] = $this->getMonthAbbreviation( $i );
 628+ }
 629+ return $monthNames;
 630+ }
 631+
616632 function getWeekdayName( $key ) {
617633 return $this->getMessageFromDB( self::$mWeekdayMsgs[$key - 1] );
618634 }
Index: trunk/phase3/resources/jquery/jquery.tablesorter.js
@@ -83,7 +83,7 @@
8484 /* debuging utils */
8585 //
8686 // function benchmark( s, d ) {
87 - // alert( s + "," + ( new Date().getTime() - d.getTime() ) + "ms" );
 87+ // console.log( s + " " + ( new Date().getTime() - d.getTime() ) + "ms" );
8888 // }
8989 //
9090 // this.benchmark = benchmark;
@@ -250,14 +250,24 @@
251251 }
252252
253253 function buildHeaders( table ) {
254 -
 254+ var maxSeen = 0;
 255+ var longest;
255256 // if ( table.config.debug ) {
256257 // var time = new Date();
257258 // }
258259 //var header_index = computeTableHeaderCellIndexes( table );
259260 var realCellIndex = 0;
260 -
261 - $tableHeaders = $( table.config.selectorHeaders, table ).each( function ( index ) {
 261+ $tableHeaders = $( "thead:eq(0) tr", table );
 262+ if ( $tableHeaders.length > 1 ) {
 263+ $tableHeaders.each(function() {
 264+ if (this.cells.length > maxSeen) {
 265+ maxSeen = this.cells.length;
 266+ longest = this;
 267+ }
 268+ });
 269+ $tableHeaders = $( longest );
 270+ }
 271+ $tableHeaders = $tableHeaders.find('th').each( function ( index ) {
262272 //var normalIndex = allCells.index( this );
263273 //var realCellIndex = 0;
264274 this.column = realCellIndex;
@@ -290,84 +300,6 @@
291301
292302 }
293303
294 - // // from:
295 - // // http://www.javascripttoolbox.com/lib/table/examples.php
296 - // // http://www.javascripttoolbox.com/temp/table_cellindex.html
297 - //
298 - // function computeTableHeaderCellIndexes(t) {
299 - // var matrix = [];
300 - // var lookup = {};
301 - // var thead = t.getElementsByTagName( 'THEAD' )[0];
302 - // var trs = thead.getElementsByTagName( 'TR' );
303 - //
304 - // for ( var i = 0; i < trs.length; i++ ) {
305 - // var cells = trs[i].cells;
306 - // for ( var j = 0; j < cells.length; j++ ) {
307 - // var c = cells[j];
308 - //
309 - // var rowIndex = c.parentNode.rowIndex;
310 - // var cellId = rowIndex + "-" + c.cellIndex;
311 - // var rowSpan = c.rowSpan || 1;
312 - // var colSpan = c.colSpan || 1;
313 - // var firstAvailCol;
314 - // if ( typeof( matrix[rowIndex] ) == "undefined" ) {
315 - // matrix[rowIndex] = [];
316 - // }
317 - // // Find first available column in the first row
318 - // for ( var k = 0; k < matrix[rowIndex].length + 1; k++ ) {
319 - // if ( typeof( matrix[rowIndex][k] ) == "undefined" ) {
320 - // firstAvailCol = k;
321 - // break;
322 - // }
323 - // }
324 - // lookup[cellId] = firstAvailCol;
325 - // for ( var k = rowIndex; k < rowIndex + rowSpan; k++ ) {
326 - // if ( typeof( matrix[k] ) == "undefined" ) {
327 - // matrix[k] = [];
328 - // }
329 - // var matrixrow = matrix[k];
330 - // for ( var l = firstAvailCol; l < firstAvailCol + colSpan; l++ ) {
331 - // matrixrow[l] = "x";
332 - // }
333 - // }
334 - // }
335 - // }
336 - // return lookup;
337 - // }
338 - // function checkCellColSpan( table, rows, row ) {
339 - // var arr = [],
340 - // r = table.tHead.rows,
341 - // c = r[row].cells;
342 - //
343 - // for ( var i = 0; i < c.length; i++ ) {
344 - // var cell = c[i];
345 - //
346 - // if ( cell.colSpan > 1 ) {
347 - // arr = arr.concat( checkCellColSpan( table, headerArr, row++ ) );
348 - // } else {
349 - // if ( table.tHead.length == 1 || ( cell.rowSpan > 1 || !r[row + 1] ) ) {
350 - // arr.push( cell );
351 - // }
352 - // // headerArr[row] = ( i+row );
353 - // }
354 - // }
355 - // return arr;
356 - // }
357 - //
358 - // function checkHeaderOptions( table, i ) {
359 - // if ( ( table.config.headers[i] ) && ( table.config.headers[i].sorter === false ) ) {
360 - // return true;
361 - // }
362 - // return false;
363 - // }
364 - // function formatSortingOrder(v) {
365 - // if ( typeof(v) != "Number" ) {
366 - // return ( v.toLowerCase() == "desc" ) ? 1 : 0;
367 - // } else {
368 - // return ( v == 1 ) ? 1 : 0;
369 - // }
370 - // }
371 -
372304 function isValueInArray( v, a ) {
373305 var l = a.length;
374306 for ( var i = 0; i < l; i++ ) {
@@ -397,7 +329,7 @@
398330
399331 function multisort( table, sortList, cache ) {
400332 // if ( table.config.debug ) {
401 - // var sortTime = new Date();
 333+ // var sortTime = new Date();
402334 // }
403335 var dynamicExp = "var sortWrapper = function(a,b) {",
404336 l = sortList.length;
@@ -410,15 +342,15 @@
411343 var s = "";
412344 if ( table.config.parsers[c].type == "text" ) {
413345 if ( order == 0 ) {
414 - s = makeSortFunction( "text", "asc", c );
 346+ s = makeSortText( c );
415347 } else {
416 - s = makeSortFunction( "text", "desc", c );
 348+ s = makeSortTextDesc( c );
417349 }
418350 } else {
419351 if ( order == 0 ) {
420 - s = makeSortFunction( "numeric", "asc", c );
 352+ s = makeSortNumeric( c );
421353 } else {
422 - s = makeSortFunction( "numeric", "desc", c );
 354+ s = makeSortNumericDesc( c );
423355 }
424356 }
425357 var e = "e" + i;
@@ -446,25 +378,11 @@
447379 cache.normalized.sort( sortWrapper );
448380
449381 // if ( table.config.debug ) {
450 - // benchmark( "Sorting on " + sortList.toString() + " and dir " + order + " time:", sortTime );
 382+ // benchmark( "Sorting in dir " + order + " time:", sortTime );
451383 // }
452384 return cache;
453385 }
454386
455 - function makeSortFunction( type, direction, index ) {
456 - var a = "a[" + index + "]",
457 - b = "b[" + index + "]";
458 - if (type == 'text' && direction == 'asc') {
459 - return "(" + a + " == " + b + " ? 0 : (" + a + " === null ? Number.POSITIVE_INFINITY : (" + b + " === null ? Number.NEGATIVE_INFINITY : (" + a + " < " + b + ") ? -1 : 1 )));";
460 - } else if (type == 'text' && direction == 'desc') {
461 - return "(" + a + " == " + b + " ? 0 : (" + a + " === null ? Number.POSITIVE_INFINITY : (" + b + " === null ? Number.NEGATIVE_INFINITY : (" + b + " < " + a + ") ? -1 : 1 )));";
462 - } else if (type == 'numeric' && direction == 'asc') {
463 - return "(" + a + " === null && " + b + " === null) ? 0 :(" + a + " === null ? Number.POSITIVE_INFINITY : (" + b + " === null ? Number.NEGATIVE_INFINITY : " + a + " - " + b + "));";
464 - } else if (type == 'numeric' && direction == 'desc') {
465 - return "(" + a + " === null && " + b + " === null) ? 0 :(" + a + " === null ? Number.POSITIVE_INFINITY : (" + b + " === null ? Number.NEGATIVE_INFINITY : " + b + " - " + a + "));";
466 - }
467 - }
468 -
469387 function makeSortText(i) {
470388 return "((a[" + i + "] < b[" + i + "]) ? -1 : ((a[" + i + "] > b[" + i + "]) ? 1 : 0));";
471389 }
@@ -482,12 +400,12 @@
483401 }
484402
485403 function sortText( a, b ) {
486 - if ( table.config.sortLocaleCompare ) return a.localeCompare(b);
 404+ //if ( table.config.sortLocaleCompare ) return a.localeCompare(b);
487405 return ((a < b) ? -1 : ((a > b) ? 1 : 0));
488406 }
489407
490408 function sortTextDesc( a, b ) {
491 - if ( table.config.sortLocaleCompare ) return b.localeCompare(a);
 409+ //if ( table.config.sortLocaleCompare ) return b.localeCompare(a);
492410 return ((b < a) ? -1 : ((b > a) ? 1 : 0));
493411 }
494412
@@ -556,13 +474,27 @@
557475
558476 }
559477
 478+ function explodeRowspans( $table ) {
 479+ // Split multi row cells into multiple cells with the same content
 480+ $table.find( '[rowspan]' ).each(function() {
 481+ var rowSpan = this.rowSpan;
 482+ this.rowSpan = 1;
 483+ var cell = $( this );
 484+ var next = cell.parent().nextAll();
 485+ for ( var i = 0; i < rowSpan - 1; i++ ) {
 486+ next.eq(0).find( 'td' ).eq( this.cellIndex ).before( cell.clone() );
 487+ };
 488+ });
 489+ };
 490+
560491 function buildCollationTable() {
561 - if ( typeof tableSorterCollation == "object" ) {
 492+ ts.collationTable = mw.config.get('tableSorterCollation');
 493+ if ( typeof ts.collationTable === "object" ) {
562494 ts.collationRegex = [];
563495
564496 //Build array of key names
565 - for ( var key in tableSorterCollation ) {
566 - if ( tableSorterCollation.hasOwnProperty(key) ) { //to be safe
 497+ for ( var key in ts.collationTable ) {
 498+ if ( ts.collationTable.hasOwnProperty(key) ) { //to be safe
567499 ts.collationRegex.push(key);
568500 }
569501 }
@@ -626,6 +558,7 @@
627559 // enabled.
628560 //$this.trigger( "sortStart" );
629561 // store exp, for speed
 562+ explodeRowspans( $this );
630563 var $cell = $( this );
631564 // get current column index
632565 var i = this.column;
@@ -782,7 +715,7 @@
783716 format: function (s) {
784717 s = $.trim( s.toLowerCase() );
785718 if ( ts.collationRegex ) {
786 - var tsc = tableSorterCollation;
 719+ var tsc = ts.collationTable;
787720 s = s.replace( ts.collationRegex, function ( match ) {
788721 var r = tsc[match] ? tsc[match] : tsc[match.toUpperCase()];
789722 return r.toLowerCase();
@@ -877,7 +810,7 @@
878811 return ( ts.dateRegex[0].test(s) || ts.dateRegex[1].test(s) || ts.dateRegex[2].test(s ));
879812 },
880813 format: function ( s, table ) {
881 - s = s.toLowerCase();
 814+ s = $.trim( s.toLowerCase() );
882815
883816 for ( i = 1, j = 0; i < 13 && j < 2; i++ ) {
884817 s = s.replace( ts.monthNames[j][i], i );

Follow-up revisions

RevisionCommit summaryAuthorDate
r90657Adding rowspan tests to tablesorting & make it more stablediebuche08:31, 23 June 2011
r95424Add comments and fix whitespace for r90657, r86305krinkle19:35, 24 August 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r86088Completely rewritten table sorting script....diebuche21:47, 14 April 2011

Comments

#Comment by Catrope (talk | contribs)   12:00, 20 April 2011
+	function getMonthNamesArray() {
+		$monthNames = array( '' );
+		for ( $i=1; $i < 13; $i++ ) { 
+			$monthNames[] = $this->getMonthName( $i );
+		}
+		return $monthNames;
+	}

This can be done much easier with e.g. return array_merge( array( ), self::$mMonthMsgs ); and similar for the other one.

#Comment by DieBuche (talk | contribs)   15:48, 20 April 2011

Sounds cool, but that would only return the english keys defined in L92:

array(
		'january', 'february', 'march', 'april', 'may_long', 'june',
		'july', 'august', 'september', 'october', 'november',
		'december'
	)

Everyone of those needs to be run through getMessageFromDB()

#Comment by Catrope (talk | contribs)   17:07, 20 April 2011

Right, I forgot. You can solve that with array_map, but that's not a whole lot clearer, so ignore me :)

#Comment by Brion VIBBER (talk | contribs)   20:08, 22 June 2011

Needs JS tests; will end up reverted along with r86088 and friends if regressions are not fixed.

#Comment by Brion VIBBER (talk | contribs)   22:07, 22 June 2011

(test case needed would be for the rowspans thing)

#Comment by DieBuche (talk | contribs)   12:41, 6 July 2011

Remarking as new; known regressions are fixed, rowspans got some tests.

#Comment by Catrope (talk | contribs)   13:54, 20 August 2011

PHP part looks OK. Tagging for Krinkle to review the JS part.

#Comment by Krinkle (talk | contribs)   19:24, 24 August 2011

Marking fixme. tableSorterCollation isn't set anywhere (other than in the unit test). A grep search didn't return any call to mw.config.set and/or an array property in PHP's config vars for "tableSorterCollation". Where is it coming from or should the function using tableSorterCollation be refactored ?

The rest of the revision is either fine or has been fixed in the mean time.

#Comment by Brion VIBBER (talk | contribs)   22:04, 24 August 2011

IIRC it's an interface allowing for user scripts / site scripts to set it to override the defaults. May need to be better defined, though.

#Comment by Krinkle (talk | contribs)   22:12, 24 August 2011

Seems like something content language specific. Reminds me of the category collation sorting. Perhaps that can be used ?

Regarding use script modification, theoretically that would be possible, but since the collation is build during construction and used from cache, it would be hard to use.

#Comment by DieBuche (talk | contribs)   08:51, 11 September 2011

Brion is right it's an site specific thing. Why would it be hard to use? You just put something like mw.config.set('tableSorterCollation', {'ä' : 'ae'}) into common.js

#Comment by MarkAHershberger (talk | contribs)   17:14, 13 September 2011

Changing to OK but tagged TODO re Krinkle's comments and our meeting on FIXMEs last night.

Status & tagging log