Bug fix: make config.cache indexation corresponding config.$tbodies

[VorontsovIE]

I've faced a bug with config.cache numeration. It's inconsistent with numeration of config.$tbodies when there're infoOnly tbodies. Namely, c.$tbodies doesn't include info blocks. while c.cache do include empty objects for such tbodies. It caused problems in many widgets (formatter, grouping, chart). I found it when runned formatter on a table with several info-tbody separators and it formatted only a half of a table.

Reason is that widgets typically iterate over config.$tbodies which doesn't include infoOnly blocks but use the same index for caching.

I found you've already done several bugfixes to workaround this behavior (see fixes for #568, #264 and commit 638d070) which became unnecessary now so were reverted in this commit.

$.tablesorter.css.info is now unused (it was used once in guard-condition (js/jquery.tablesorter.widgets.js:1180), check if it's ok to remove that usage) so was removed.

Some variables were renamed along the code, some unused variables were removed.

I've tried to find all occurences of cache and verify that this patch will not break anything, but I ask you to inspect whether I considered all the consequences. I still find my knowledge of tablesorter internals as superficial.

p.s. I recommend to check difference in code using git show --ignore-space-change because removal of unnecessary conditions caused shifting left without any changes of large code blocks inside those conditions.

[thezoggy]

fyi, you can ignore the whitespace on github as well by adding ?w=1
https://github.com/Mottie/tablesorter/pull/820/files?w=1

[VorontsovIE]

@thezoggy Thank you a lot! Didn't know that feature.

[Mottie]

Hi @prijutme4ty!

Thanks for contributing. I have a few issues with the changes so far:

Overall

• The purpose of this pull request was to make the c.$tbodies indexing match the internal cache, but the definition was never changed - it's actually defined twice in the code (line 236 & 1115).
• If the c.$tbodies were redefined, its change would pretty much break the majority of the currently available widgets - I haven't had time to specifically add an info-only tbody to the examples to test this, but I'm pretty sure there will be issues.
• The purpose of the c.$tbodies definition was to only target tbodies that are modified by sorting, and tbodies that contain useful information for widgets. If you need all tbodies, you can just as easily use c.$table.children('tbody'); maybe this variable should be renamed to avoid confusion?
• Another issue is that info-only tbodies have no limitations on colspan and rowspan because the information within them are ignored. Currently, this plugin does not properly support sorting cells with these settings and would likely break the formatting of the table if it attempted to sort, filter or obtain information from those rows within the info-only tbody.
• I know that the unit tests can be fickle and sometimes need to be run more than once to get a clean run, but these changes have broken a few tests.

Widget Specific

• Setting the pager to always use the first tbody will not work if the first tbody is for information only (such as in this demo; see issue dummy row in table sorter pager #800).
• Including info-only tbodies with the filter will hide info only tbodies if they don't match the filter.
• I'm sure there would be more issues with widgets, but hopefully I got my point across.

Honestly, I don't think it's a bad idea to make the indexing match the internal cache; but I would prefer to save big changes like this for Abelt.

[VorontsovIE]

Hi, @Mottie!
It looks I wrote a bad description of changes I've made. Let me try to explain it more concrete. All points you've listed were taken into account.
I made no changes in c.$tbodies definition. Quite the contrary, I've changed the definition of c.cache so now both variables do not include info only tbodies. Now c.cache[tbodyIndex] and c.$tbodies[tbodyIndex] both refer the same non-infoOnly tbody. So I don't get into issues you've mentioned.
There was no one place where cache for info only tbodies is used, so I think it's safe to remove unnecessary empty cache entries for those tbodies. Workaround which were used to access right cache element are now removed because we automatically get right cache indices. But even if they weren't removed, actually they would work anyway (typically they iterate through a list of all tbodies to find specific one non-infoOnly tbody; while index obtained this way can be different after patch, the tbody found is quite the same). So if somebody have written his own widget using approaches same as used in tablesorter itself, their widgets also don't need any changes. Also if one do not use c.cache, he also shouldn't bother.

The only situation which can be broken - if someone had manually iterated cache indices through all tbodies using
for(i = 0; i < c.$table.children('tbody').length; ++i) {...}
So yes, it can break compatibility in rare cases. But the library itself is consistent.

I've checked and fixed all the widgets in a project that they use c.$tbodies and c.cache in a proper way. Also I've tried to fix unit tests to check updated cache. Did you see the second commit in this pull request (I pushed it not immediately after the first commit)?

[VorontsovIE]

@Mottie,
If you think that change of cache definition is a bad idea, I can go through the code and fix all bugs which are present in tbody related code without changing cache -- in a way which was used for paging. While I was doing the bugfix, I found all the spots, so it is a simple task. I think it's worse than proposed solution because new bugs will appear again and again. But anyway, fixed bug is better than unfixed.
Just say me which method do you prefer?

[Mottie]

Hey! Yes, I think your solution is a good idea.

The reason I hesitate is because I was working on a widget which allows the sorting of tbodies (see #195). In order to make the widget work, it needs values stored in the cache from a target row in the tbody (read more about it in the linked issue). The problem appears when the tbody is a "info-only" tbody - the widget will still allow sorting it, but in that case it would need a slot within the cache to store the target row from the tbody. If we remove that slot, I'll need to find an alternative way of storing the row information for that widget.

I just haven't had the time to look into making that widget work, because when a table is full of "info-only" tbodies, the parsers don't get set. So now I need to make the parser detection code public so I can access it from the widget, or just copy over that entire block of code.

Here is an example of how the tables could be formatted... the included "sortTbody" widget was written before tablesorter v1.16, so the cache changes are not applied to it. Like I said, I haven't had much time to work on it.

[VorontsovIE]

I got your concerns. It sounds like a big obstacle.
I've looked original @emmerich's implementation of tbody sorting functionality and think, there is a good point in introducing another cache. I'm not sure specific cache structure was perfect, but I think it's right to create a separate cache for that purpose.
Info only tbodies usually have structure diferent from the one, ordinary tbodies have. It's possibly a bad idea to mix cached rows of different markup, it will complicate the code with many conditionals in a long run. Sorting of tbodies IMO should involve a separate cache which stores content to be compared. I hope it's possible to do with a widget as you propose, without an intervention in a core code.
I will try to write a comment in that issue.

[Mottie]

Well, that's the thing... the cache could be kept the same and the code to modify it adjusted to accommodate supporting colspan and rowspan anywhere in the table. I have been also thinking about this enhancement.

The output widget already supports colspan and rowspan output. That code could be instead used to populate the cache - essentially all cells within a colspan or rowspan get their content duplicated within the cache matrix. The problem arises when sorting is introduced. The grouped rows need to be kept together - easy if you make all the rows into childrows of a "main" row, but then when you sort, you need to move the rowspan to the top row to maintain the table formatting. It all makes sense conceptually... now I just need time to work on it. And, again... this is something I plan to put into Abelt; I really need to move away from the tablesorter name to avoid confusion.

So after all that rambling, what I am essentially saying is we don't necessarily need a separate cache.

[VorontsovIE]

I'm not sure I fully understand your decision. Do you mean that you will store infoOnly tbodies in cache? I don't think it will help sorting of tbodies, because the problem is not only in spans: meaning of cell content in infoOnly tbodies can be quite different. I'd say it's like sorting headers and content together.

Anyway a bug should be fixed, so I will make fixes which do not touch internal structure, just get correct indices all over the code. Stay tuned :)

[Mottie]

LOL, wait... I haven't made any final decisions. I appreciate your enthusiasm, but like I said, these are ideas I am thinking about putting into Abelt.

It is probably okay to merge these changes into this tablesorter fork... I haven't done it because I there are some conflicts with merging these changes into the build branch. And I was waiting on some feedback about that before I merged it.

So, if you would, make your changes in the build branch and I'll merge it.

[VorontsovIE]

Oh, sorry! I've misunderstood you.
So, I should reapply these changes on top of "working" branch and fix issues with tests if there are any?

[TheSin-]

I am currently in to process applying this to master for testing. I'll be testing it against many of the widgets to make sure things since work the same and I'll report back to @Mottie at that point I may attempt to apply it to working and test as well. So stay tuned.

[TheSin-]

• Applied cleanly to master and tested
• Applied cleanly to working and tested

widgets tested

• pager
• output
• chart
• columnSelecter
• cssStickyHeaders
• filter
• filter-formatter
• column
• zebra
• formatter
• editable

[TheSin-]

I didn't get through all modules but I have yet to find an issue, I'm going to accept this into working branch

I think only you can modify the request to make it against working, once you do I'll merge it.

[VorontsovIE]

@TheSin- made a new pull request, same as this one.

[TheSin-]

yup thank you, I'm just resolving the conflicts right now so I can merge it.

[Mottie]

Merged in #822. Thanks again!