Bug report: Searchpanes+ajax reload breaks "clear pane" button

Bug report: Searchpanes+ajax reload breaks "clear pane" button

setwebmastersetwebmaster Posts: 78Questions: 5Answers: 0
edited March 2020 in SearchPanes

As can be seen in this sample at live.datatables.net/fucamoqa/5/, the "clear pane" buttons work with initial data, but when reloading via ajax, the panes data is refreshed but the "clear pane" buttons stop working.

Answers

  • setwebmastersetwebmaster Posts: 78Questions: 5Answers: 0
    edited March 2020

    @sandy It seems to be caused by the change that was made to prevent having a rise in event listeners for "detached" objects as now the listeners are only set once in setListeners

    A solution (my preferred one) I think could work would be to not redraw the buttons every time the _displayPane function is called as once initialized, there's nothing that changes their text/state/etc... What do you think about it? This could be applied for all buttons as I don't see the need to redraw them on each pane update, that would also improve performance a bit overall as it would be a couple buttons that don't need any refreshing every time displayPane is called, which happens pretty often.

    Another potential solution could be to add a single event listener that would be more generic and that would not be attached to every single button instance. It could then listen on any button click with a given control class like dtsp-clear-trigger and then find the related pane and clearing it.

  • setwebmastersetwebmaster Posts: 78Questions: 5Answers: 0

    Well I mixed up _diplayPane and _populatePane I guess, so it is not called that often. Yet the problem remains for the event listeners, they are only set once at first initialization and not updated when updating the table with ajax.reload(), the tricky part will be to re-add the event listeners, without causing their count rise as was the case before the _setListeners was implemented IIRC

  • setwebmastersetwebmaster Posts: 78Questions: 5Answers: 0

    I think I might have found another potential solution requiring minimal changes, simply reset the listSet flag to false when calling rebuildPane, which is itself called when reloading the table via an ajax call.

    That brings me to another quick question (related to it all). I saw in other threads that the way to use searchPanes with ajax reload was to chain the reload call with table.searchPanes.rebuildPane(), but now I just saw by reading more carefully the code, that there is already an event listener on the xhr event that will call the rebuildPane, so is adding table.searchPanes.rebuildPane() not redundant here?

  • setwebmastersetwebmaster Posts: 78Questions: 5Answers: 0

    @allan for my second point (the fact that we should call rebuildPane after an ajax reload, it is not mandatory if we simply add a little change to the searchPanes codebase.

    Effectively, it was first suggested as panes were not shown after an ajax.reload call, but this was only a small display bug where the this.classes.hide css class was added to the panes container if no pane were available, but it was never removed in case there were. I will provide a PR for this and would appreciate if you could take a look at it and merge if you think it is suitable.

    As for the other point here (the event listeners for the clear pane buttons), I have tested my proposed solution above (reset the listSet flag to false in the _rebuildPane function). I also added jQuery .off call before adding the listeners just to be 100% sure there are no leftovers that would creep up the event listeners count with usage. I will create a separate PR for this too if you are interested in applying the fix.

    Thanks for your consideration and I hope these changes will help improve the situation!

  • allanallan Posts: 63,498Questions: 1Answers: 10,471 Site admin

    so is adding table.searchPanes.rebuildPane() not redundant here?

    It is. Also, just chaining onto ajax.reload() wouldn't be enough since you'd need to wait for the reload of the data to complete (i.e. use a callback function). But as you say, all that is redundant and it should be handled internally.

    PRs to fixed these two points would be most welcome - thanks!

    Allan

  • setwebmastersetwebmaster Posts: 78Questions: 5Answers: 0
    edited March 2020

    For the chaining of the table.searchPanes.rebuildPane(), I thought passing it as first argument to the table.ajax.reload was already being considered as the callback to call after data was actually loaded, so it should have been working (and it effectively was), but it was not efficient as it caused double table/panes initialization.

    As for the PRs, they have already been proposed yesterday on the SearchPanes repository (with my personal account - Os1r1s110). I have 4 open PRs in fact. The one for searchPanes stateSave should be edited before being merged if it is of any interest to you as some changes were made to the codebase that render it non-working in its current state. The 3 others are those discussed here and in other recent threads about SearchPanes.

    Question for you, may these be moved to the "SearchPanes" category? Or are they stuck in "Free community support"? Not that I really bother, it would just be easier to track related requests/changes for searchpanes if they were categorized.

    Thanks in advance!

  • allanallan Posts: 63,498Questions: 1Answers: 10,471 Site admin

    That's the discussion moved. To be honest, I don't really look at the categories that much!

    Sandy is the one heading up the SearchPane dev - he currently is working one day a week with us, so he'll be looking at your PRs in the coming days.

    Allan

  • setwebmastersetwebmaster Posts: 78Questions: 5Answers: 0

    @Allan I don't know why but it's now been >2 months and I received 0 feedback on the 4 PRs that I submitted on the searchpanes repo (with my personal github account). I wouldn't mind if they get rejected or anything, but not having any single feedback is a little disappointing to be honest.

    I understand this is a big project and the person responsible for searchpanes is only working 1 day per week, but I think the PRs could have been merged/rejected in a really small amount of time and now that it's been 2 months that they lay there waiting, they might not be applicable/relevant/compatible anymore, which will make it harder to confirm if they can get merged (if there's any interest at all on your part).

    I think it would be nice if the team could be clear about what they expect from the community, is it only feedback and no contributions? If so, I think there is a way to disable PRs on the repositories.

    Thanks anyways for the wonderful library and hope the very best for the team, I know it's a lot of work, but for me, I will stop trying to improve it as there doesn't seem to be a need for community contributions.

  • allanallan Posts: 63,498Questions: 1Answers: 10,471 Site admin
    edited May 2020

    Apologies. No excuses from us - we simply lost track of these PRs. We don't use Github as our main tracker, so unless we act until things as they come in, they can sometimes get lost, which is what happened here. We are getting better (albeit slowly) at handling this - as you say there are a lot of things going on in the project.

    Sandy is on the case with them today - thank you for the nudge!

    Regarding what we would like from the community. Pull requests are indeed welcome. Generally for larger ones we'd like to discuss them before any works starts on them to make sure it aligns with what we are working on or planning, but small patches are welcome any time. As I say, we lost these ones in the pile. Sorry!

    Allan

  • setwebmastersetwebmaster Posts: 78Questions: 5Answers: 0
    edited May 2020

    @allan No worries, I do understand it's a big project with tons of moving parts and links between extensions and that it can be complicated for a small team to track all these sources of comments/questions/bug reports/PRs etc...

    One thing I might advise would be to take some time and try to communicate with the user base about the best way to approach the different scenarios such as:
    1. Bug report
    2. Feature request
    3. Comments/discussions

    *There is a page that already contains information about requiring help https://datatables.net/manual/tech-notes/10 so maybe just add some more details there would be fine

    As right now all of it seems to be mixed in the forum and it's really hard for people outside of the dev team to follow with the changes and what they are related to.

    For example, declaring a bug would be better done on the github repository in my opinion rather than in the forum, where it's easy to lose track of what is a bug, what is a feature request, etc... But again, I understand you are a small team and it might get hard to track multiple sources so if you prefer keeping it all together on the forum, well that's fine too, it's just harder to keep track of everything, especially bugs and their corresponding fixes in which we can't see issue specific details as you seem to track them in another issue trackerr (totally speculative, but based on the fact that Sandy and Colin often refer to "DD-xxxx" as being their internal reference) :)

  • allanallan Posts: 63,498Questions: 1Answers: 10,471 Site admin

    Yup - spot on. We use JIRA for the three of us to keep track of things. It might be that we should open our DD project issues, but when we've allowed bug reports via Github in the past it gets abused - at least 9/10 "bug reports" are actually support requests, which is why we are acting as gatekeepers to our bug tracker. We might be able to allow read only access to it - I'm not sure! We have some private company information on there as well so we'd need to look into how to handle that.

    Allan

  • setwebmastersetwebmaster Posts: 78Questions: 5Answers: 0

    I understand its a complex problem trying to balance the level of openness while still keeping control and avoid abuse.

    Maybe you could keep tracking the private part in Jira but document "accepted" bugs/PRs/fix commits on Github? This way, it would be way easier to read the "story" of the code and understand why some changes were made (for those who take time to investigate before proposing changes without having a single idea of the motovations/reasons behind the actual state of the code)

    I by no means want to make anyone think I have the only solution here I'm just legitimately trying to find a way for the user base to be able to contribute in a more efficient way when possible. No matter what, this is still a really great project and accomplishment for a small team!

  • allanallan Posts: 63,498Questions: 1Answers: 10,471 Site admin

    Thank you. And also for your feedback - this is exactly the sort of thing we need to be able to better work with the community. Open source is only as useful as the community around it and I'm so pleased that we have fantastic contributors such as yourself.

    All of our commit messages should explain why there has been a change, and reference either our internal tracker, a Github issue and or a forum thread, hopefully so there can be some knowledge of why things have changed.

    One thing I need to get better at is labelling actions in Github - e.g. Accepted as you say, Good first issue and so on.

    Thanks,
    Allan

This discussion has been closed.