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

Page size lost when table size is changed in background #1196

Closed
h3llrais3r opened this issue Apr 22, 2016 · 18 comments
Closed

Page size lost when table size is changed in background #1196

h3llrais3r opened this issue Apr 22, 2016 · 18 comments

Comments

@h3llrais3r
Copy link

h3llrais3r commented Apr 22, 2016

I'm having a problem with keeping the selected page size when using the tablesorter with the pager plugin. (I'm using version 2.25.3 of your plugin)

image

// Enable tablesorter and tablesorterPager for wanteditems table
$(document).ready(function () {
    $("#wanteditems")
        .tablesorter({
            // Enable widgets
            widgets: ['reflow', 'filter', 'zebra'],
            widgetOptions: {
                // No column filters
                filter_columnFilters: false,
                // External filter selector (will match any column)
                filter_external: '#wanteditemsfilter',
                // Search faster (default 300)
                filter_searchDelay: 50
            },
            // Use date format 'ddmmyyyy'
            dateFormat: 'ddmmyyyy',
            // Force text sorter in first column (this is needed due to img in table cell)
            // See https://github.com/Mottie/tablesorter/issues/1149
            headers: {
                0: {sorter: 'text'}
            },
            // Sort default by time desc
            sortList: [[8, 1]]
        })
        .tablesorterPager({
            container: $("#wanteditemspager"),
            output: '{startRow} to {endRow} ({filteredRows})'
        });
});

On each page load the table is reloaded with data from the server, meaning looping a list and creating a row for each item.
Since the content/size of the list can be changed in the meantime, it's possible that there are more or less items in the list after a page reload.
However, if my page size is set to 'all', and the size has been changed in the meantime, the 'all' option is not longer selected.

image

I've already debugged the code and it's because the 'lastPageSize' is still the old value.
Since the 'all' option is only set when the size equals the totalRows, it's not selected anymore.
Snapshot from your code:

// set to either set or get value
        parsePageSize = function( p, size, mode ) {
            var s = parseInt( size, 10 ) || p.size || p.settings.size || 10,
                // if select does not contain an "all" option, use size
                setAll = p.$size.find( 'option[value="all"]' ).length ? 'all' : p.totalRows;
            return /all/i.test( size ) || s === p.totalRows ?
                // "get" to get `p.size` or "set" to set `p.$size.val()`
                ( mode === 'get' ? p.totalRows : setAll ) :
                ( mode === 'get' ? s : p.size );
        },

Any idea if there is a solution for this? Or is this a bug when the number of rows of the table is changed after a page reload?

@Mottie
Copy link
Owner

Mottie commented Apr 24, 2016

Hi @h3llrais3r!

Sorry for the delay in responding.

Does the "all" option include a value set to "all" (<option value="all">All</option>)?

It's difficult for me to The "pagerLastSize" value is saved to jQuery data and does not persist on page reload, so please check the page size in local storage.

var values = $.tablesorter.storage(table, "tablesorter-pager");
// values = { page: #, size: # }

Or, maybe to fix this problem, you could update the page size immediately after ajax completes. In this example, the size is set to the total if the last size is plus or minus 9 of the total number of pages (untested code):

ajaxProcessing: function(result, table, xhr) {
  var c = table.config,
    last = $.tablesorter.storage(table, "tablesorter-pager") || {},
    totalPages = result.total;
  // process data
  // ....
  // set size to total pages if last page size is +/- 9 of total
  if (Math.abs((last.size || 0) - totalPages) < 10) {
    c.pager.size = totalPages;
  }
}

Hmm, or maybe I need to save "all" as the last size instead of an actual number...

@h3llrais3r
Copy link
Author

h3llrais3r commented Apr 24, 2016

Hi @Mottie.

No problem, thanks for replying in the first place.

Does the "all" option include a value set to "all" (All)?

Yes I have this all option.

            <div id="wanteditemspager" class="pager">
                <img src="/images/tablesorter/first.png" class="first"/>
                <img src="/images/tablesorter/prev.png" class="prev"/>
                <span class="pagedisplay"></span>
                <!-- this can be any element, including an input -->
                <img src="/images/tablesorter/next.png" class="next"/>
                <img src="/images/tablesorter/last.png" class="last"/>
                <select class="pagesize" title="Select page size">
                    <option value="10">10</option>
                    <option value="20">20</option>
                    <option value="30">30</option>
                    <option value="40">40</option>
                    <option value="50">50</option>
                    <option value="all">All</option>
                </select>
                <select class="gotoPage" title="Select page number"></select>
            </div>

so please check the page size in local storage.

I've checked this while debugging.
At first page load, the size is 41.
If I select the 'all' option, it shows all.
If I reload the page (submit, so no ajax processing), the size of my list is changed to 40 on the server, so only 40 items are still rendered.
The page size is still 41 in the local storage. Because the actual size doesn't match the size in the storage, it doesn't keep the 'all' option enabled.
However, if I should have selected the '10' option, it stays selected.

So I think your last remark will be the solution. If you select the 'all' option, you should save 'all' and not the actual size.

Thanks for having a look 😉

@Mottie
Copy link
Owner

Mottie commented Apr 30, 2016

DOH, I forgot to address this issue in the latest release. I'll try to spend some time on it this weekend.

@h3llrais3r
Copy link
Author

😄 No problem, thanks!

@Mottie Mottie closed this as completed in 5bc6425 May 1, 2016
@Mottie
Copy link
Owner

Mottie commented May 1, 2016

Version 2.26.0 was just released with this change. I made a minor version bump because ajax requests will now pass "all" as a page size, so it may break some current server side methods.

Additionally, I don't have any unit tests set up for the pager (neither the addon nor widget), so this update was not thoroughly tested with ajax methods. Please let me know if you encounter any issues.

Thanks!

@h3llrais3r
Copy link
Author

Thanks for the quick fix. I'll try to test it asap and I'll keep you posted. 👍

h3llrais3r added a commit to h3llrais3r/Auto-Subliminal that referenced this issue May 2, 2016
@h3llrais3r
Copy link
Author

@Mottie I confirm that the fix is working. 👍
Thanks again for your great plugin.

@lindonb
Copy link

lindonb commented Jul 9, 2016

I think this causes a pagination issue when using ajax where tablesorter thinks "all" rows have been retrieved when they haven't.

Let's say there are 100 rows in the database table and tablesorter is set to grab 5 rows at a time.

Line 175 in widget-pager.js sets p.totalRows to be equal to the rows retrieved (5):

            p.totalRows = c.$tbodies.eq( 0 )
                .children( 'tr' )
                .not( wo.pager_countChildRows ? '' : '.' + c.cssChildRow )
                .length;

And then line 1118 of the same file sets the p.size to "all" if p.totaRows equals the size:

            return /all/i.test( size ) || s === p.totalRows ?
                // "get" to set `p.size` or "set" to set `p.$size.val()`
                'all' : ( mode === 'get' ? s : p.size );

The result is that with no filters tablesorter always thinks it has returned all rows and so won't let the user advance the page since it thinks there is only one page. Is this a bug or do I need to change something?

@Mottie
Copy link
Owner

Mottie commented Jul 9, 2016

Hi @lindonb!

That definition at line 175 only sets the totalRows on initialization. If you're using ajax that number should be returned the ajax data. The snippet from line 1118 only deals with "all" values when setting the page select value. Internally, the totalRows should still be set to the value returned from the ajax data.

If you are encountering an issue, please modify this demo to show the problem.

@lindonb
Copy link

lindonb commented Jul 9, 2016

Hi @Mottie!

Difficult to translate what I've got to the demo but I'm not really seeing any functional differences. I do use ajax data to return the total. What about this: line 1210 of widget-pager.js is setting p.totalPages to 1 because p.size is set to "all":

p.totalPages = p.size === 'all' ? 1 : Math.ceil( tsp.getTotalPages( c, p ) / p.size );

@Mottie
Copy link
Owner

Mottie commented Jul 9, 2016

When all rows are showing, there should only be one page. This was meant to have the ajax retrieve all rows (hopefully there aren't thousands) when the "all" setting was used.

When you say "I think this causes a pagination issue" does that mean you encountered a problem? Please provide the steps to reproduce this issue.

@lindonb
Copy link

lindonb commented Jul 9, 2016

Hi @Mottie!

Yes, I do have an issue and was trying to guess the cause. Sorry but would have to provide you access to a site to show the problem, but need a little time to set that up. Not sure it will help you much but below is a portion of the tablesorter code I have (left out the pager_customAjaxUrl part`):

    $('table#adminusers1').tablesorter({
        headerTemplate: '{content} {icon}', 
        showProcessing: true, 
        widgets : ['stickyHeaders','filter','pager','columnSelector'], 
        serverSideSorting: true, 
        sortMultiSortKey : 'none', 
        sortList : [[1,0]], 
        widgetOptions : {
            stickyHeaders : 'ts-stickyHeader', 
            resizable : true, 
            filter_cssFilter : 'form-control', 
            filter_selectSourceSeparator : '|', 
            filter_serversideFiltering : true, 
            filter_reset : 'button#adminusers1-filterreset', 
            pager_size: 5, 
            pager_css: {
                container: 'ts-pager'
            }, 
            pager_selectors: {
                container : 'div#adminusers1-pager'
            }, 
            pager_output: '{startRow} - {endRow} / {filteredRows} ({totalRows})', 
            pager_processAjaxOnInit: false, 
            pager_initialRows: {total: 62, filtered: 62}, 
            pager_ajaxObject: {dataType: 'html'}, 
            pager_ajaxUrl : 'tiki-adminusers.php?{sort:sort}&{filter:filter}', 
            pager_savePages: false, 
            pager_ajaxProcessing: function(data, table){
                var parsedpage = $.parseHTML(data), table = $(parsedpage).find('table#adminusers1'), r = {};
                r.rows = $(table).find('tbody tr');

                r.total = '62';
                r.filteredRows = $(table).data('count');
                r.headers = null;
                return r;
            }, 
(left out the customAjaxUrl part)

@Mottie
Copy link
Owner

Mottie commented Jul 9, 2016

Ok, that all looks good. So what are you expecting/doing when the user sets the page size to "all"? You know you don't have to provide that option, right?

@lindonb
Copy link

lindonb commented Jul 9, 2016

I don't have an "all" setting, but the size variable seems to get set to "all" in the code. In the size drop down I just have 5, 10, 15, 20, but none of them are selected when the table is loaded.

@Mottie
Copy link
Owner

Mottie commented Jul 9, 2016

Hmm, try clearing out your localStorage. Enter this into the console:

$.tablesorter.storage($('#adminusers1'), 'tablesorter-pager', '')

@lindonb
Copy link

lindonb commented Jul 9, 2016

That didn't seem to work but I went to local storage through the browser and deleted tablesorter-pager, but I still have the same issue. After the page loads and I select a value, like 5, then everything is okay, ie the page dropdown has the right number of pages. But unless I select a size it thinks there's only one page.

Mottie added a commit that referenced this issue Jul 9, 2016
When `processAjaxOnInit` is false
@Mottie
Copy link
Owner

Mottie commented Jul 9, 2016

There was a problem with the pager widget code being executed twice on initialization, the second time a call would go out to the server to get new data even when the processAjaxOnInit was false. I just patched that in the master branch.

I also found the problem, upon initialization, the parsePageSize function was seeing the number of rows & size setting as the same and setting the size to "all". Once it got set to "all" it didn't revert after the totalRows was change to the pager_initialRows setting. It's all fixed in the master branch now (demo).

I plan to sort out the issues with the select2 filter formatter before the next release. If I can't get it figured out by tomorrow, I'll release the next version anyway.

@lindonb
Copy link

lindonb commented Jul 10, 2016

Did a quick test - looks like that does it! Many thanks for the quick work.

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

3 participants