Forum OpenACS Development: Re: Small feature enhancement for template::list
i am not a template::list expert, but nevertheless, here are my 2 cents:
a) it looks to me as if the proposed function recalculates something, which was computed before. The full number of rows must be know when the pagination happens. So, either this value is stored in the guts of the list template, or maybe it should be stored there in order to retrieve it from you function efficiently (without recalculation).
b) are you sure, the "uplevel 1" is correct? should it not be a "uplevel $list_properties(ulevel)"?
it seemed strange to me too that this value was not stored somewhere, but truth is it is not!
I think the reason is that the full count is not required to the paginator, as it really just needs to know how many records get to populate all the pages and then stop.
The original, untampered with, paginator query is never called: it is always wrapped into "offset" clauses that limit its results to number_of_pages * rows_per_page, and also overwritten, so I had to retrieve the original one with some code replication...
The uplevel is required so I can see filter vars in the caller scope. I can do better than that I've seen, because I have a reference to filter names and values in the list.
I think from a performance perspective, as the value is not anyway, it is ok to have a different proc doing this calculation. I could change the template::list::prepare so a reference to the original query is maintained and I can retrieve it, then remove the upvars by looking into list properties instead.
What do you think?
Storing the bound sql query to save on the uplevel is performance-wise not a big difference. sql-queries are (in good cases) in the range of milliseconds, tcl/xotcl statements in the range of micro-seconds. so, saving a sql query (especially an aggregating query) is certainly useful, since people use pagination on large tables, where the count query might be as well slow.
To my next proposal
on template::list::prepare, I save an entry for "count_query" in the list properties, then pass it to template::paginator::create together with "page_query_substed" as before.
Here, I check for a cache entry for the full_row_count, as happens for paginated row_ids. As happened before, if row_ids are not in the cache, I call paginator query into template::paginator::init. Into this proc I have added also the call to the count_query and the saving of the value in the cache.
This way now paginator has a new property called "full_row_count", which can be retrieved from template::paginator::get_full_row_count.
To have a common interface for getting the count in template::list, I have created a proc called template::list::get_rowcount. On a paginated list, it calls template::paginator::get_full_row_count, otherwise it just returns the size of the undelying multirow.
Its easier done than explained... Anyway, checks on both paginated and not paginated lists on my instance work. Is it ok if I commit the change?
On up-to-date systems, you will find the new proc
Giving the proper count of the number of records retrieved by a template::list
Thanks to Gustaf for his suggestions on this!
All the best