r86337 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r86336‎ | r86337 | r86338 >
Date:19:20, 18 April 2011
Author:diebuche
Status:resolved (Comments)
Tags:
Comment:
r86088: Get rid of eval by implemting a MergeSort algorithm. It's a few ms slower on very large tables, but is stable-sorting in all browsers
Modified paths:
  • /trunk/phase3/resources/jquery/jquery.tablesorter.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.tablesorter.js
@@ -83,12 +83,11 @@
8484 /* debuging utils */
8585 //
8686 // function benchmark( s, d ) {
87 - // console.log( s + " " + ( new Date().getTime() - d.getTime() ) + "ms" );
 87+ // console.log( s + " " + ( new Date().getTime() - d.getTime() ) + "ms" );
8888 // }
8989 //
9090 // this.benchmark = benchmark;
9191 //
92 - //
9392 /* parsers utils */
9493
9594 function buildParserCache( table, $headers ) {
@@ -167,7 +166,6 @@
168167 /* utils */
169168
170169 function buildCache( table ) {
171 -
172170 // if ( table.config.debug ) {
173171 // var cacheTime = new Date();
174172 // }
@@ -221,10 +219,9 @@
222220 }
223221
224222 function appendToTable( table, cache ) {
225 -
226223 // if ( table.config.debug ) {
227 - // var appendTime = new Date()
228 - // }
 224+ // var appendTime = new Date()
 225+ // }
229226 var c = cache,
230227 r = c.row,
231228 n = c.normalized,
@@ -245,8 +242,8 @@
246243 }
247244 tableBody[0].appendChild( fragment );
248245 // if ( table.config.debug ) {
249 - // benchmark( "Rebuilt table:", appendTime );
250 - // }
 246+ // benchmark( "Rebuilt table:", appendTime );
 247+ // }
251248 }
252249
253250 function buildHeaders( table ) {
@@ -327,96 +324,76 @@
328325 }
329326 }
330327
331 - function multisort( table, sortList, cache ) {
332 - // if ( table.config.debug ) {
333 - // var sortTime = new Date();
334 - // }
335 - var dynamicExp = "var sortWrapper = function(a,b) {",
336 - l = sortList.length;
 328+ function checkSorting (array1, array2, sortList) {
 329+ var col, fn, ret;
 330+ for ( var i = 0, len = sortList.length; i < len; i++ ) {
 331+ col = sortList[i][0];
 332+ fn = ( sortList[i][1] ) ? sortTextDesc : sortText;
 333+ ret = fn.call( this, array1[col], array2[col] );
 334+ if ( ret !== 0 ) {
 335+ return ret;
 336+ }
 337+ }
 338+ return ret;
 339+ }
337340
338 - // TODO: inline functions.
339 - for ( var i = 0; i < l; i++ ) {
340 -
341 - var c = sortList[i][0];
342 - var order = sortList[i][1];
343 - var s = "";
344 - if ( table.config.parsers[c].type == "text" ) {
345 - if ( order == 0 ) {
346 - s = makeSortText( c );
347 - } else {
348 - s = makeSortTextDesc( c );
 341+ // Merge sort algorithm
 342+ // Based on http://en.literateprograms.org/Merge_sort_(JavaScript)
 343+ function mergeSortHelper(array, begin, beginRight, end, sortList) {
 344+ for (; begin < beginRight; ++begin) {
 345+ if (checkSorting( array[begin], array[beginRight], sortList )) {
 346+ var v = array[begin];
 347+ array[begin] = array[beginRight];
 348+ var begin2 = beginRight;
 349+ while ( begin2 + 1 < end && checkSorting( v, array[begin2 + 1], sortList ) ) {
 350+ var tmp = array[begin2];
 351+ array[begin2] = array[begin2 + 1];
 352+ array[begin2 + 1] = tmp;
 353+ ++begin2;
349354 }
350 - } else {
351 - if ( order == 0 ) {
352 - s = makeSortNumeric( c );
353 - } else {
354 - s = makeSortNumericDesc( c );
355 - }
 355+ array[begin2] = v;
356356 }
357 - var e = "e" + i;
358 -
359 - dynamicExp += "var " + e + " = " + s; // + "(a[" + c + "],b[" + c + "]); ";
360 - dynamicExp += "if(" + e + ") { return " + e + "; } ";
361 - dynamicExp += "else { ";
362357 }
 358+ }
363359
364 - // if value is the same keep original order
365 - var orgOrderCol = cache.normalized[0].length - 1;
366 - dynamicExp += "return a[" + orgOrderCol + "]-b[" + orgOrderCol + "];";
 360+ function mergeSort(array, begin, end, sortList) {
 361+ var size = end - begin;
 362+ if (size < 2) return;
367363
368 - for ( var i = 0; i < l; i++ ) {
369 - dynamicExp += "}; ";
370 - }
 364+ var beginRight = begin + Math.floor(size / 2);
371365
372 - dynamicExp += "return 0; ";
373 - dynamicExp += "}; ";
 366+ mergeSort(array, begin, beginRight, sortList);
 367+ mergeSort(array, beginRight, end, sortList);
 368+ mergeSortHelper(array, begin, beginRight, end, sortList);
 369+ }
374370
375 - // if ( table.config.debug ) {
376 - // benchmark( "Evaling expression:" + dynamicExp, new Date() );
377 - // }
378 - eval( dynamicExp );
379 - cache.normalized.sort( sortWrapper );
 371+ var lastSort = '';
 372+
 373+ function multisort( table, sortList, cache ) {
 374+ //var sortTime = new Date();
 375+
 376+ var i = sortList.length;
 377+ if ( i == 1 && sortList[0][0] === lastSort) {
 378+ // Special case a simple reverse
 379+ cache.normalized.reverse();
 380+ } else {
 381+ mergeSort(cache.normalized, 0, cache.normalized.length, sortList);
 382+ }
 383+ lastSort = ( sortList.length == 1 ) ? sortList[0][0] : '';
380384
381 - // if ( table.config.debug ) {
382 - // benchmark( "Sorting in dir " + order + " time:", sortTime );
383 - // }
 385+ //benchmark( "Sorting in dir " + order + " time:", sortTime );
 386+
384387 return cache;
385388 }
386389
387 - function makeSortText(i) {
388 - return "((a[" + i + "] < b[" + i + "]) ? -1 : ((a[" + i + "] > b[" + i + "]) ? 1 : 0));";
389 - }
390 -
391 - function makeSortTextDesc(i) {
392 - return "((b[" + i + "] < a[" + i + "]) ? -1 : ((b[" + i + "] > a[" + i + "]) ? 1 : 0));";
393 - }
394 -
395 - function makeSortNumeric(i) {
396 - return "a[" + i + "]-b[" + i + "];";
397 - }
398 -
399 - function makeSortNumericDesc(i) {
400 - return "b[" + i + "]-a[" + i + "];";
401 - }
402 -
403390 function sortText( a, b ) {
404 - //if ( table.config.sortLocaleCompare ) return a.localeCompare(b);
405 - return ((a < b) ? -1 : ((a > b) ? 1 : 0));
 391+ return ((a < b) ? false : ((a > b) ? true : 0));
406392 }
407393
408394 function sortTextDesc( a, b ) {
409 - //if ( table.config.sortLocaleCompare ) return b.localeCompare(a);
410 - return ((b < a) ? -1 : ((b > a) ? 1 : 0));
 395+ return ((b < a) ? false : ((b > a) ? true : 0));
411396 }
412397
413 - function sortNumeric( a, b ) {
414 - return a - b;
415 - }
416 -
417 - function sortNumericDesc( a, b ) {
418 - return b - a;
419 - }
420 -
421398 function buildTransformTable() {
422399 var digits = '0123456789,.'.split('');
423400
@@ -483,9 +460,9 @@
484461 var next = cell.parent().nextAll();
485462 for ( var i = 0; i < rowSpan - 1; i++ ) {
486463 next.eq(0).find( 'td' ).eq( this.cellIndex ).before( cell.clone() );
487 - };
 464+ }
488465 });
489 - };
 466+ }
490467
491468 function buildCollationTable() {
492469 ts.collationTable = mw.config.get('tableSorterCollation');
@@ -518,8 +495,7 @@
519496 if ( !this.tHead || !this.tBodies ) return;
520497 // declare
521498 var $this, $document, $headers, cache, config, shiftDown = 0,
522 - sortOrder;
523 -
 499+ sortOrder, firstTime = true, that = this;
524500 // new blank config object
525501 this.config = {};
526502 // merge and extend.
@@ -540,10 +516,6 @@
541517 //performance improvements in some browsers
542518 cacheRegexs();
543519
544 - // try to auto detect column type, and store in tables config
545 - this.config.parsers = buildParserCache( this, $headers );
546 - // build the cache for the tbody cells
547 - cache = buildCache( this );
548520 // get the css class names, could be done else where.
549521 var sortCSS = [config.cssDesc, config.cssAsc];
550522 // apply event handling to headers
@@ -552,13 +524,21 @@
553525
554526 function (e) {
555527 //var clickTime= new Date();
 528+ if (firstTime) {
 529+ firstTime = false;
 530+ explodeRowspans( $this );
 531+ // try to auto detect column type, and store in tables config
 532+ that.config.parsers = buildParserCache( that, $headers );
 533+ // build the cache for the tbody cells
 534+ cache = buildCache( that );
 535+ }
556536 var totalRows = ( $this[0].tBodies[0] && $this[0].tBodies[0].rows.length ) || 0;
557537 if ( !this.sortDisabled && totalRows > 0 ) {
558538 // Only call sortStart if sorting is
559539 // enabled.
560540 //$this.trigger( "sortStart" );
 541+
561542 // store exp, for speed
562 - explodeRowspans( $this );
563543 var $cell = $( this );
564544 // get current column index
565545 var i = this.column;
@@ -574,8 +554,7 @@
575555 config.sortList.push( [i, this.order] );
576556 // multi column sorting
577557 } else {
578 - // the user has clicked on an all
579 - // ready sortet column.
 558+ // the user has clicked on an already sorted column.
580559 if ( isValueInArray( i, config.sortList ) ) {
581560 // revers the sorting direction
582561 // for all tables.

Follow-up revisions

RevisionCommit summaryAuthorDate
r90612Fix tablesorting bug that caused weird interferences between two tables; Make...diebuche21:54, 22 June 2011
r90613Followup r90595: Tweak tablesorter tests so they run on r86088 version of jqu...brion21:56, 22 June 2011

Past revisions this follows-up on

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

Comments

#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)   21:57, 22 June 2011

This rev actually seems to have caused the main regressions I found -- running the basic tests against the r86088 or r86305 versions work fine.

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

... and fixed in r90612. \o/

Status & tagging log