Fundamentally broken internal data key mixup?

[DaneelTrevize]

tablesorter/dist/js/jquery.tablesorter.combined.js

Line 703 in 8703f25

parser = ts.getParserById( ts.getData( header, configHeaders, 'sorter' ) );

Surely this should be using the key 'parser', not 'sorter'?

I have a simple instance where I've $.tablesorter.addParser()'d 2 new parsers, then

headers: {
	0: { parser: "groupIDParser", sorter: "digit" },
...

and it fails to use this parser, rather debug: true shows me it's auto-detecting on all headers/columns.
I change the getData() key string in line703 of /dist/js/jquery.tablesorter.combined.js and it all works.

I can't quite believe it's as simple as it seems to me, because surely this would have been noticed before & caused many issues throughout..?

[Mottie]

Hi @DaneelTrevize!

I think it's actually an issue of bad documentation since the parser setting was added after the sorter setting.

The sorter key should be used to set custom parsers (see demo).

The parser key is used when you have an element like a select in the table cell and you want to sort the selected value by ip Address... the setting would then be:

headers: {
	0: { parser: "select", sorter: "ipv4Address" }
  ...
}

[DaneelTrevize]

According to this doc page & my recollection of reviewing the code earlier, the parser value is indeed to save on having the nature of the data be auto-detected, and then the values that are generated by this choice of parser are naturally independant of how they are then sorted by the chosen sorter.
To use "sorter" to also determine the "parser" seems to overly conflate the purposes of the two.

Obviously you know how it was intended to be, yet this key name change fixed my problem (how else to efficiently define a cell's data as coming from an arbitrary attribute/data extension of the parent row, at the parsing & caching stage?), is consistent with the actual names of the parse->sort stages, and didn't break anything that I know of so far.
It's my impression that were I to only define a sorter per header, that there'd still be a slower process of auto-detecting the text/numeric nature of the extracted & cached data values. And then the potential of mismatching between this nature/type and any defined sorter/parser (e.g. seemingly textual data being fed to a parser that's defined as type: 'numeric'.
P.S. is there a doc clarifying how text vs numeric should map to digit, etc, or is that left to reading their definitions?

I have several different use-cases where the value to sort a given column by is found via lookup into another data structure (e.g. of the arbitrary ranking of custom images/icons representing threat), and it seemed by design that the parser stage would be supplied with the custom function(s) to do this so that the values are cached, rather than regenerated every time every cell is compared & sorted against every other (i.e. the lookup performed by the sorting/comparator definition).

[Mottie]

Setting parser: false is a special case in which the column data isn't processed to save on initialization time.

Yes, I know the naming is confusing (I blame @TheSin- 😜), but it was necessary to keep sorter as the main method to set the parser on a column in order to maintain backward compatibility with the original tablesorter (v2.0.5). I wanted to make switching from the old tablesorter to this fork as painless as possible.

Do you still have a situation that can't be resolved by the methods available in this fork? I'd appreciate a demo showing the issue.

[DaneelTrevize]

Here's a fiddle for this issue and another that I don't have time left to fully explain today, but it's using the single key name modification as hosted on spectrefleet.com.
If you use standard TableSorter & Widgets .js resources instead, it will use the 'digit' "parser" according to the debug output, which won't then sort "3k" as though it were 3000. I think this matches what you're describing, but:

If you keep the key mod (and it's for you to test the unforked version has the same behaviour as I was going to raise this as another issue but can't clearly explain it atm), you'll find TableSorter's miscounting the header indexes across colspans (the 3rd TH spans 2 TRs) according to its own debug: output, because commenting out the 3 or 4 headers:{} will also show it polling the column value for indexes 4 and 5 (i.e. not 3). Leaving in all the declarations will result in no auto-detection as intended (3k sorts wrong, remembering here sorters sort and parser parse), but then there's 1 too many declarations any which way you look at it.
Hopefully this makes more sense tomorrow.

[Mottie]

Sorry for taking so long to get back to you.

When I ran a diff on the spectrefleet.com tablesorter combined version and found this change:

+ parser = ts.getParserById( ts.getData( header, configHeaders, 'parser' ) );
- parser = ts.getParserById( ts.getData( header, configHeaders, 'sorter' ) );
  noParser = ts.getData( header, configHeaders, 'parser' ) === 'false';

So it appears that the use of parser within the headers (which would also effect the header class name, so parser-distanceParser would need to be used) in the demo provided will only work with that modified version of tablesorter

4: {
  parser: "distanceParser",
  sorter: "digit",
  sortInitialOrder: "asc"
},

I'm not sure why sorter is being set to digit since the type is actually set within the $.tablesorter.addParser by the type value.

Anyway, I've modified the demo to remove parser and change the sorter value to indicate the custom parser id

4: {
  sorter: "distanceParser",
  sortInitialOrder: "asc"
},

You can also minimize the code by setting a sorter-distanceParser (and sortInitialOrder-asc) class name in the three headers and completely remove the sorter settings in the headers option (demo).

<thead>
  <tr>
    <th class="parser-false">Group</th>
    <th>Σ</th>
    <th colspan="2" class="sorter-typeParser sortInitialOrder-asc">Type</th>
    <th>Σ</th>
    <th class="sorter-distanceParser sortInitialOrder-asc">Closest</th>
    <th class="sorter-distanceParser sortInitialOrder-asc">Median</th>
    <th class="sorter-distanceParser sortInitialOrder-asc">Furthest</th>
  </tr>
</thead>

$("#test_table").tablesorter({
  debug: true,
  theme: "bootstrap",
  widgets: ["uitheme"],
  headerTemplate: "{content} {icon}",
  sortInitialOrder: "desc",
  sortRestart: true,
  sortList: [[4, 1]]
});

[Mottie]

I'm guessing this issue has been resolved, so I'm going to close it. If you continue to have problems, please feel free to continue the discussion in this thread.

[DaneelTrevize]

Allow me to equally apologise for basically abandoning this once it was working "well-enough".

It seems your alterations to the demo fiddles have broken sorting for the last column.

I've just tried using the latest 2.30.2 as-is (simplifying my headers back to trying sorter: rather than parser:, removing duplication where it shouldnt be required), but I'm still facing the problems of:
colspan not being counted correctly;
leading to autodetecting happening for column values, which gets it wrong w.r.t. text vs numbers;
and a desire to minimise work by reusing a few headers arrays rather than setting data attributes for potentially 13 category tables multiplied by 3-4 views on one page (and/or having them autodetect the data type).
You can see an example of this table count scaling here or here.
That's why I want to manually define the data type per column, as well as the sorting function, which leads back to the strange colspan issue.

[Mottie]

Oh, I looked at the code again, and I think I found the issue... I'll update it in the master branch, so it won't be available until the next release.

Updated demo - https://jsfiddle.net/Mottie/o7jctbmr/54/ (using master branch; rawgit may take a few minutes to update)

[Mottie]

Oops, had an issue with the docs... the next version is now available.

[DaneelTrevize]

I appreciate the very rapid response. I've had a chance to do some testing with 2.30.3, and while I've put it live, we're still seeing an issue with column header indexing. You too can now hopefully reproduce this result for that /d/1ds URL, by choosing "On & Off Grid Views", and considering the first (DPS) table being initialised. To test, the correct "Furthest" sorting should be 4, 15, 518, 1k.

If we don't define a 7-indexed headers entry, we get this autodetection for the 6th https://i.imgur.com/SY9yVTD.png as text which sorts wrong.
And without a 3-indexed header entry, we get similar autodetecting for the data-column 4.

I tried to enable debugging just for core, but it seems to also log the uitheme widget ...which also might be indicating that widgets are being applied twice? https://i.imgur.com/9o2LLTR.png

I'll try put together a jsfiddle of that DPS table from /d/1ds to simplify things...

[DaneelTrevize]

Ok, remove 1 of the 4 distanceParser assignments without breaking any of the 3 distance column sorts ;-)

https://jsfiddle.net/o7jctbmr/56/

I'm beginning to suspect a mistake in the object literal syntax of

headers: {
    	0: { sorter: "typeParser", sortInitialOrder: "asc" },
    	1: { sorter: "digit" },
    	2: { sorter: "distanceParser", sortInitialOrder: "asc" },	/* indexing still not fixed */
    	3: { sorter: "distanceParser", sortInitialOrder: "asc" },
    	4: { sorter: "distanceParser", sortInitialOrder: "asc" },
    	5: { sorter: "distanceParser", sortInitialOrder: "asc" }
}

but that could be the flashbacks of PHP associative arrays & type coercing of numeric strings to integers talking...

[Mottie]

Ahh ok I see what you're saying now.

The problem is the headers option is maintaining the behavior of the original tablesorter. See this note from the docs:

the headers index values corresponds to the table header cells index (zero-based) and not the actual columns. For example, given the following table thead markup, the header-index counts the header th cells and does not actually match the data-column index when extra rows and/or colspan or rowspan are included in any of the header cells

To get around this, don't use the headers option (demo):

<thead>
  <tr>
    <th colspan="2" class="sorter-typeParser sortInitialOrder-asc">Type</th>
    <th class="sorter-digit">Σ</th>
    <th class="sorter-distanceParser sortInitialOrder-asc">Closest<br>km</th>
    <th class="sorter-distanceParser sortInitialOrder-asc">Median<br>km</th>
    <th class="sorter-distanceParser sortInitialOrder-asc">Furthest<br>km</th>
  </tr>
</thead>

I noticed that the HTML didn't include the wrapping tr inside the thead; not a problem really, but it's not valid HTML.

[DaneelTrevize]

I was aware of the header index vs data-column difference, that's what I've been trying to work with. Wouldn't your theory fail to explain why the 6th column 5:... needs to be defined to work? I couldn't keep all 3 columns functioning without 4 lines, regardless of which was removed/renumbered.

My concern is that tablesorter appears to be inconsistently handling counting/indexing headers/columns, and that no documentation makes this clear.

Thanks for the markup note.

[Mottie]

Yes, the headers option isn't working as expected. There is an issue getting the header cell's internal cellIndex (which doesn't account for colspans) versus the calculated colIndex.

This issue is related to #1362, and could be fixed, but then it would break all backwards compatibility. I already have this fix on my to-do list for the next major version (Abelt).