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

Missing Events in PageEvents list #854

Closed
omnilord opened this issue Mar 25, 2015 · 9 comments
Closed

Missing Events in PageEvents list #854

omnilord opened this issue Mar 25, 2015 · 9 comments

Comments

@omnilord
Copy link

There is a list of events here: TableSorter Pager Events

That are not included in the pager events list here: Line 117 - jquery.tablesorter.pager.js

And this causes problems when destroying tablesorter or the pager plugin because these events are not included in the cleanup activities/unbind calls internal to tablesorter.

Is this a defect or an omission by design? If by design, I'd like to understand why.

@Mottie
Copy link
Owner

Mottie commented Mar 25, 2015

Hi @omnilord!

Everything in that list is removed in the destroy method (see line 797); and again upon initialization.

$(table).unbind( pagerEvents.split(' ').join('.pager ').replace(/\s+/g, ' ') );

I just reviewed all the binds in the pager plugin and noticed that "refreshComplete" wasn't included in the list - I'll fix that ASAP.

I just created this demo and:

  • When I destroy the pager, no events starting with "pager" are in the events list (look at the console).
  • When I destroy everything, only "refreshComplete" remains - which I'll have fixed soon.

So, what specifically is the problem that is occurring?

@omnilord
Copy link
Author

pagerComplete is also missing. Also, pagerInitialize

I have two modes for a table, each with a unique set of columns and additional functionality that needs to get detached from the pagerComplete callback when switching between modes. I was having a lot of trouble just adding/removing columns on the fly, so opted for just destroying and re-initializing the tables (low cost because all the data is AJAX-fetched). The problem was pagerComplete was not being unset which resulted in pagerComplete callbacks being called once for each time the modes were changed. Adding pagerComplete to the list at Line 117 works fine for me.

I really should learn the "Pull Request" thing here and just make a patch next time. :)

@Mottie
Copy link
Owner

Mottie commented Mar 25, 2015

Huh?

The pagerInitialized (with a "d" on the end) and pagerComplete are events that are triggered on the table. No where in the pager plugin/widget is there a bind to those events.

The only widgets that do bind to "pagerComplete" are:

  • editable
  • formatter
  • stickyHeaders

And those widgets namespace the event bindings to the widget name, so that shouldn't be the problem.

I wonder if after triggering a destroy in the pager, any event bindings within the code on that page also need to be removed.

I might be able to better help troubleshoot the problem if some of the code from that page were shared.

@omnilord
Copy link
Author

I updated Your example with a third button (Lines 24 to 33) and a callback for pagerInitalized pagerComplete (on line 40). Click the destroy 'All' button, then click the 'Restart' button to re-initialize tablesorter and pager without the callback. Then page through the data, you should get an alert every page change even though the tablesorter with the pagerComplete event was "destroyed" previously.

Also, I may have made a typo with the pagerInitialized because I don't see an alert when the example initializes.

I also create a Two Modes Toggle Example where one state has pagerInitialized pagerComplete events a little more in line with what I have. Just click back and forth between the two mode buttons a couple of times and you should see the compounding effect of calls to the callback function (pagerInitialized seems to be working here even though I didn't change that code from the previous iteration).

@Mottie
Copy link
Owner

Mottie commented Mar 26, 2015

Maybe I have the wrong idea, but I don't think the plugin is "responsible" for bindings that are made outside of the plugin.

There is no way for it to determine when a binding should or shouldn't be removed, or reason out why and prevent an event from being bound multiple times. What I mean is, if a developer binds to an event, then they should remove that bind when they think it should be removed.

In the case of those demos, what is likely missing is an unbind to the "pagerInitialized" and "pagerComplete" events:

$('table').trigger('destroy').off('pagerInitialized pagerComplete');

Updated:

@omnilord
Copy link
Author

Any drawbacks to doing the .off() call in a callback attached to the destroy event like This?

@Mottie
Copy link
Owner

Mottie commented Mar 26, 2015

None that I can see.

@omnilord
Copy link
Author

I'll pass this along to my team. Thanks for the patience, I know these kinds of "not-a-bug" investigations can be bothersome, but they can be educational for everyone else.

@Mottie
Copy link
Owner

Mottie commented Mar 26, 2015

Not a problem :)

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

No branches or pull requests

2 participants