Paginator revisited

[Harry-Anton Talvik]

Hi,

I looked around Lift sources to find any Paginator related tests that
would confirm or reject my possible misunderstandings, could not find
any; it seems that the border case of zero items is not tested,
really.

So, I'm going to mention here some things that seems fishy to me and
could use some attention to provide more robust Paginator out of the
box.

a) When there is empty collection (0 items, nothing to iterate over)
then following [1]
def currentXml: NodeSeq = Text("Displaying records
"+(first+1)+"-"+(first+itemsPerPage min count)+" of "+count)
calculates to
"Displaying records 1-0 of 0"

Does not look right to me, I would expect "Displaying records 0-0 of 0"

b) I'd suggest to split calculations away from
Text("Displaying records "+(first+1)+"-"+(first+itemsPerPage min
count)+" of "+count)
so that it could be used via paginate() binding like
<nav:recordsFrom /> - <nav:recordsTo /> of <nav:recordsCount />

c) Calculation for "next" link [2]
"next" -> pageXml(first+itemsPerPage min
itemsPerPage*(numPages-1), nextXml),
calculates negative value when 'numPages' is zero resulting
"next"-link to contain
?offset=-<itemsPerPage>
Expected: No link in case of empty collection (as there is fro "prev")

d) Same problem for the "last" (as for "next", described previously)

e) More of a debatable robustness issue: I would expect the Paginator
offered by the framework to be smart enough that it could not result
in an error like
Message: org.postgresql.util.PSQLException: ERROR: OFFSET must
not be negative
(even when user plays with parameters on address bar)

[1] Line 102 at
http://github.com/lift/lift/blob/2.x-2.8_devel/framework/lift-base/lift-webkit/src/main/scala/net/liftweb/http/Paginator.scala
[2] Line 142 at
http://github.com/lift/lift/blob/2.x-2.8_devel/framework/lift-base/lift-webkit/src/main/scala/net/liftweb/http/Paginator.scala

All the best,
Harry-A

[Naftoli Gugenheim]

Thanks for pointing this out. I'll try to take care of this today.

How would you suggest fixing (e)?

--
You received this message because you are subscribed to the Google Groups "Lift" group.
To post to this group, send email to lif...@googlegroups.com.
To unsubscribe from this group, send email to liftweb+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

[Harry-Anton Talvik]

Hi,

On Tue, Aug 3, 2010 at 02:09, Naftoli Gugenheim <nafto...@gmail.com> wrote:
> Thanks for pointing this out. I'll try to take care of this today.
> How would you suggest fixing (e)?

I'd suggest to handle any negative offset as zero, could be replaced
when getting/setting offsetParam.

All the best,
Harry-A

[Harry-Anton Talvik]

Hi,

Seems that any possible fixes didn't made into 2.1-M1, nor could I
find a specific ticket under 2.1-M2 [1]. Shall I open one myself or
what's the standard procedure here?

[1] http://www.assembla.com/spaces/liftweb/milestones/236853-2-1-m2

All the best,
Harry-A

[David Pollak]

On Thu, Aug 5, 2010 at 3:54 AM, Harry-Anton Talvik <harry...@gmail.com> wrote:

Hi,

Seems that any possible fixes didn't made into 2.1-M1,

Naftoli isn't a committer, so he shouldn't be making implied promises to fix things.

You made your original post on Monday which was after the M1 code slush.  During th code slush, we only fix important bugs that are identified just prior to or during the code slush.  While I agree that the issues you've identified with the paginator should get fixed, they have been in the code for a long time and we would not hold up a release for them.

Finally, Tim Perrett currently maintains the paginator.  He's off-grid this week.

So, open a ticket at http://ticket.liftweb.net (you must be a watcher of the Liftweb space on Assembla to open tickets).  Tim will address the ticket when he has the bandwidth.

Thanks,

David
 

--

You received this message because you are subscribed to the Google Groups "Lift" group.
To post to this group, send email to lif...@googlegroups.com.
To unsubscribe from this group, send email to liftweb+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

--
Lift, the simply functional web framework http://liftweb.net
Beginning Scala http://www.apress.com/book/view/1430219890
Follow me: http://twitter.com/dpp
Blog: http://goodstuff.im
Surf the harmonics

[Harry-Anton Talvik]

Hi,

Thank you for the feedback and clarifications.

The ticket is here:
https://liftweb.assembla.com/spaces/liftweb/tickets/611-paginator---empty-collection--negative-offset

Thanks,
Harry-A