Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sorting question - 2.13.3 not sorting <empty> items #419

Closed
thezoggy opened this issue Nov 15, 2013 · 26 comments
Closed

sorting question - 2.13.3 not sorting <empty> items #419

thezoggy opened this issue Nov 15, 2013 · 26 comments
Labels
Bug

Comments

@thezoggy
Copy link
Collaborator

@thezoggy thezoggy commented Nov 15, 2013

On 2.1.19, when sorting a date column where the values was empty (which would be at the bottom of the list by default), however the dataset of the empty values would be sorted.
Now on 2.13.13, the empty value dataset are no longer sorted... how do i return this back to how it was in 2.1.19?

Curious to know if there was an option that got changed / something got broken.

$(document).ready(function(){

    $("#showListTable:has(tbody tr)").tablesorter({
        sortList: [[5,1],[1,0]],
        textExtraction: {
            3: function(node) { return $(node).find("span").text().toLowerCase(); },
            4: function(node) { return $(node).find("span").text(); },
            5: function(node) { return $(node).find("img").attr("alt"); }
        },
        widgets: ['saveSort', 'zebra', 'stickyHeaders'],
        headers: {
            0: { sorter: 'isoDate' },
            1: { sorter: 'loadingNames' },
            3: { sorter: 'quality' },
            4: { sorter: 'eps' }
        }
    });

});
@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 15, 2013

here is an example of the issue: 2.13.13: http://jsfiddle.net/abkNM/1578/

sort the first column, notice the empty cells at the bottom (emptyTo).. look to the right.. notice that those values are NOT sorted.

example on 2.1.19: http://jsfiddle.net/abkNM/1579/ (sorry css is busted, but sorting works).

sort the first column, notice the empty cells at the bottom (emptyTo).. look to the right.. notice that those values are ARE sorted. (which I like)

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 15, 2013

Hey @thezoggy!

I think the change is from v2.12 where I restructured the textSorter and combined $.tablesorter.sortText() and $.tablesorter.sortTextDesc() into $.tablesorter.sortNatural(). I have a feeling it's just a misplaced negative sign somewhere.

Here is a demo using v2.11.1: http://jsfiddle.net/Mottie/abkNM/1582/

Oh, by the way, you can't link to raw github files any more... they get blocked, so I had to add all of the code directly into the demo to get it to work: http://jsfiddle.net/Mottie/abkNM/1581/

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 15, 2013

odd, the demos work for me.. they just show me a warning when linking to github.

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 15, 2013

I see this message in Chrome:

Refused to execute script from 'https://raw.github.com/Mottie/tablesorter/v2.1.19/js/jquery.tablesorter.js' because its MIME type ('text/plain') is not executable, and strict MIME type checking is enabled.

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 15, 2013

hmm, im using firefox.. so maybe thats the reason.. looks like they dont do the strict mime type check.
https://plus.google.com/+MikeWest/posts/28f8HS2M7cb

so looks like its a security feature for chrome.. basically dont try to load/run js served up as text/plain..
ha there is a site created just to 'fix' this... a proxy that basically puts the correct header on the linked files, http://rawgithub.com/

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 16, 2013

had a chance to look into this yet ?

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 16, 2013

I have been... I'm kind of stuck. I either fix it and break all the unit tests, or don't fix it and it sorts wrong. Maybe the easiest solution is to use sortAppend on the title column, or figure out how to make a stable sort.

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 16, 2013

can i have that only apply to the emptyTo set? since i wouldnt want column2's data sort to mess up the whole point of column1's data.. so would a 'default sort' be another way to solve this.. since if you notice in my init that ``sortList: [[5,1],[1,0]],` i actually do have the 1,0 set on init

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 16, 2013

looks like this is the commit that caused the problems: ff07d94#diff-4

specifically the change to:
ff07d94#diff-8378bdbbc48b4f6610fdb5627dac28c7L652

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 16, 2013

so found another quirk.. so if you sort the 2nd column then sort the first column (not multi column sort).. it works like it used to. if you click on the 3rd column then sort via the first column.. you will notice that the previous sort is whats basically used.. so its like there is just a sort order thats being reused instead of defaulted / something bleeding over.. where before it would just default to A->Z ordering.

so anyways i discovered I actually like the quirk of sorting the 2nd column then sorting the first column.. thus when i click on the first column it actually reverses the 2nd column emptyTo dataset.. so i can mimic this with 2.1.11 by using a multi-sort.. so that seems to be the key. the emptyTo data set should sort the next column as if that column is being used as a multi-sort.. but key off the sort order by the first columns ordering..

if this doesnt make sense hit me up on irc and i can show you a demo privately

also to note, so if i click on column 2 to sort, then column 1 to sort.. then click on column 1 to resort then click on column 1 to resort again.. the column 2 data doesnt sort. it then does resort on the next click on column 1. thus it seems like something is getting set on inital sort that normally doesnt.

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 16, 2013

Yeah, sadly, those are the quirks of an unstable sort.

I'm doing some other clean up, on and off, but I'll get back to this sorting issue soon.

I was going to include the hierarchical sorting that @steenreem shared this update, but I've got so much other stuff going on, I don't think I'll get around to it... so hopefully, I'll have this issue fixed (if it's possible), and a new update out by tomorrow.

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 16, 2013

so using saveSort... lets say i sort col 3 then col 1, have the jacked up data. reload the page, i see my saveSort loads with: "sortList":[[0,0]] and the data is sorted correctly. thus initial loading sort is actually good.. its just that when i sort on other columns that sort is bleeding over and messing up the emptyTo data.

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 16, 2013

ok i think i found the culprit.. if i remove the last default sort from the multisort routine the emptyTo data is sorted like before... that the whole 3rd click required to really reset the sort order is still there though (separate issue).

// fall back to natural sort
sort = ts.sortNatural(x[col], y[col])

so i went and compare the old natural sort code to now.. i notice the empty cell handling part isnt there anymore...
i see you pulled it out of that code but it appears that this is whats messing things up.

as a test I added the whole empty cell part back to the sortNatural section right after if (a === b) { return 0; } and now the emptyTo dataset will sort correctly after the 3rd click (only ASC).. but then of course other stuff breaks (cant click on certain column headers to sort now)

                e = c.string[ (c.empties[col] || c.emptyTo ) ]
                // sorting empty cells
                if (a === '' && e !== 0) { return typeof e === 'boolean' ? (e ? -1 : 1) : -e || -1; }
                if (b === '' && e !== 0) { return typeof e === 'boolean' ? (e ? 1 : -1) : e || 1; }

anyways hope this helps you narrow down wth is going on

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 16, 2013

Ok, I'm just going to add an new option named sortStable (stable sort), which when two exactly equal values are found, the original table row order is maintained.

So since that table's original row order has the second column in descending order, those values will be maintained. No matter how the rank column is sorted.


Edit: yeah I put all the empty string comparisons outside of the natural and text sort functions so that they always apply to sorts, no matter what external sorting function you use.

Basically all I did was add this for the new option:

col = sortList[i][0];
order = sortList[i][1];
// sort direction, true = asc, false = desc
dir = order === 0;

// stable sort
if (a[col] === b[col]) {
    return a[orgOrderCol] - b[orgOrderCol];
}
@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 16, 2013

what you just posted works great, made one tweak so the reverse sort works as well:

return dir ? a[orgOrderCol] - b[orgOrderCol] : b[orgOrderCol] - a[orgOrderCol];
@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 16, 2013

I was considering moving below where x and y are defined (which swaps a and b), then the order of the rating column would reverse (empty cells only) depending on the rank column sort direction... but then it's not really a "stable" sort anymore.

Here (demo), I added this code:

if (x[col] === y[col])
    return x[orgOrderCol] - y[orgOrderCol]
}
@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 16, 2013

hmm do notice that the multi sort now does not work though.. sort first column, then multi sort using 2nd column.. no change

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 16, 2013

Ugh, you're right... oh well, back to the drawing board.

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 16, 2013

Ok, try this (demo)

if (x[col] === y[col] && l === 1) {
    return x[orgOrderCol] - y[orgOrderCol];
}
@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 16, 2013

ok thats perfect but one bug still.. if you sort column 1 (which as empty data) then sort on column 2 (with shift) it works.. then while holding shift sort on column 2 again.. you notice that the multi sort applies to both the column 1 data and empty data. hold shift and click on column 1, now that its sorting DESC now shift click on column 2.. you will notice that only the non empty data in column 1 is getting sorted. thus the ASC/DESC multi-sorting is not the same with regards to emptyTo data

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 17, 2013

make any progress today? curious if you need me to test anything before I go out and try to be productive today around the house

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 17, 2013

LOL sorry no progress.. that's what I was trying to do too. Productive around the house and other Github projects. I'll see what I can do later tonight/tomorrow.

@Mottie Mottie closed this in c5a7109 Nov 20, 2013
@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 20, 2013

fyi your widgets file says:

/*! tableSorter 2.8+ widgets - updated 11/19/2013 */

thus its missing what its current version is.. i see it requires ts 2.8+ (t should be capitalized?)

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 20, 2013

I've never really had a version number for the widgets file... it's just easier for me to change the date.

When I forked the brand (should I call it that?) from tablesorter.com, the "t" wasn't capitalized, so I've been following that pattern. From looking at that, I should ask, why is the "s" capitalized?

@thezoggy
Copy link
Collaborator Author

@thezoggy thezoggy commented Nov 20, 2013

well if you look at what you do in the main .js you just do all lower case. honestly it seems like the widgets has been getting updated pretty much in sync with the core js.. so maybe just time to give it its own version number or at the very least.. tie it into the same as the core. why i mention this is because i went to upgrade a project and i couldnt tell if the widget file needed updating per the size/header. i had to diff the file to realize that in fact it had changes. thus i would welcome a version number being added into the file.

@Mottie
Copy link
Owner

@Mottie Mottie commented Nov 20, 2013

Ok, I'll start using the tablesorter core version number since I bump it anyway, even for minor changes to the widgets. I'll do the same for the pager.

When version 3 comes out, I'll have each widget in a separate file, so I'll add an individual version number to each.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.