[SearchBuilder-devel] [RFC][PATCH] Pages fixes and tests for it functionality

[Ruslan Zakirov]

On 7/28/05, David Glasser <glasser at bestpractical.com> wrote:
> On Jul 28, 2005, at 10:27 AM, Jesse Vincent wrote:
> 
> > Dave,
> >
> > How much code is there in "Internal Project" that we could harvest and
> > hand to ruslan for the paging work?
> >
> > Jesse
> 
> The code is just:
> 
> =head2 pager
> 
> Returns a L<Data::Page> object associated with this collection.  This
> object defaults to 10 entries per page.  You should use only use
> L<Data::Page>  methods on this object to B<get> information about
> paging,
> not to B<set> it; use C<set_page_info> to set paging information.
> 
> =cut
> 
> __PACKAGE__->mk_accessors(qw(pager));
> 
> =head2 set_page_info [per_page => NUMBER,] [current_page => NUMBER]
> 
> Sets the current page (one-based) and number of items per page on the
> pager object, and pulls the number of elements from the collection.
> This both sets up the collection's L<Data::Page> object so that you
> can use
> its calculations, and sets the L<DBIx::SearchBuilder> C<first_row> and
> C<rows_per_page> so that queries return values from the selected page.
> 
> =cut
> 
> sub set_page_info {
>    my $self = shift;
>    my %args = (
>      per_page => undef,
>      current_page => undef, # 1-based
>      @_
>    );
> 
>    $self->pager->total_entries($self->count_all)
>                ->entries_per_page($args{'per_page'})
>                ->current_page($args{'current_page'});
> 
>    $self->rows_per_page($args{'per_page'});
>    $self->first_row($self->pager->first);
> 
> }
I saw this block of code and see other problems in it:
* you have to call set_page_info only when you end up with limits, but
this should work any time you want.
* you run count_all immediately, I like other solution: - "do query DB
only when you couldn't do anything else"
* I don't see how to turn off paging if you don't need it any more.

If we look on pages support from this point of view then we would see
that we have two solutions no matter use we Data::Page or don't:
1) when we really need page info touch count_all and calculate info as
Data::Page does.
2) when we need page info fetch page data(like my patch for SB does)

> 
> Some notes though:
> 
>    * This code expects users to read paging information not through
> an SB call, but through calls like
>         $collection->pager->first
>      This is good in that it lets us just say "read the Data::Page
> docs for what we can do" and we don't have to write ten methods that
> do nothing but delegate, but on the other hand it means that if we
> want to put our own code wrapping one of the methods we lose.
pager method that returns pager object to control pages is good idea,
we can return own object if we want to.

> 
>    * I don't know if my decision to make the only legal *set*
> operation on pagination be a single set-per_page-and-current_page-at-
> the-same-time operation was a good idea.  (And the name
> "set_page_info" totally sucks.)  Probably a better API would be to
> put this logic in "per_page" and "current_page" calls themselves,
> which both shell out to an internal method that does the logic
> above.  And not actually expose "first_row" as part of the public
> API, or at least say "don't use first_row if you're using our
> pagination support"
+1 for removing first_row method from public API.

> 
>    * the class here is a Class::Accessor.  We might want to do that
> for Jifty::DBI too, although we might not like having methods named
> get and set show up.
-1 for Class::Accessor, just don't like this solution.

> 
> So I wouldn't recommend just throwing this code into Jifty::DBI
> without thinking.
> 
> --dave
>    Code Monkey, Best Practical Solutions
> --
> David Glasser | glasser at bestpractical.com
> 
> 


-- 
Best regards, Ruslan.