Opened 17 years ago

Closed 17 years ago

Last modified 15 years ago

#4805 closed (fixed)

Pass paginator object in generic view's context instead of several variables related to the paginator

Reported by: Gary Wilson <gary.wilson@…> Owned by: nobody
Component: Core (Other) Version: dev
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Currently, there is only one generic view with pagination capabilities (object_list), but it is being asked for in the date-based generic views (#2367). I propose that instead of passing 11 different context variables related to pagination, that we pass just the paginator object and let the templates use what they need.

This would provide the following benefits:

  • Paginator methods would only be called when needed instead of called to fill the 11 different context variables that might not get used in the template anyway.
  • We are not repeating ourselves in the generic views. Instead of 11 context variables for each of the generic views and documentation of each, we have just one variable and have a Paginator documentation page that the generic view documentation sections could link to.
  • Future enhancements would not require context variable and documentation changes for each generic view. They keep passing the paginator object as a whole.

Attachments (3)

4805.diff (9.0 KB ) - added by Gary Wilson <gary.wilson@…> 17 years ago.
4805-alt.diff (2.3 KB ) - added by nick.lane.au@… 17 years ago.
Alternate implementation based on #2576
4805-alt.2.diff (5.6 KB ) - added by nick.lane.au@… 17 years ago.
Alternate implementation based on #2576... that isn't completely broken like the last one I added

Download all attachments as: .zip

Change History (12)

comment:1 by Gary Wilson <gary.wilson@…>, 17 years ago

Triage Stage: UnreviewedDesign decision needed

comment:2 by nick.lane.au@…, 17 years ago

I've thought about this before too, and I'm +1.

It would also make it easier to add pagination to other generic views (#2367), and custom pagination template tags wouldn't depend on those 11 context variables. It would be a lot cleaner to just add the paginator object to the template context in a custom view, than to have to add the 11 variables.

comment:3 by Chris Beaven, 17 years ago

#2576 which suggested introducing this was closed as wontfix but maybe I wasn't very clear about why it would be useful and I got a bit side-tracked with my base-0 or base-1 options.

comment:4 by Gary Wilson <gary.wilson@…>, 17 years ago

Yes, I actually saw the related django-dev thread and your ticket (#2576). The passing of all these variables just bothers me to no end, it must be fixed!

In that thread, Adrian was a fan of the separate class idea, which is what #2576 was. I think that a separate class might be a little overkill, and favor just a single, page-aware paginator class. I was going to open a separate "paginator overhaul" ticket, but then your comment made me realize that this ticket would not be possible without some page-aware class since, for the templates, we need methods that don't have to take parameters. So I guess will just attach the code to this ticket.

We should really start a new django-dev thread to discuss all this.

by Gary Wilson <gary.wilson@…>, 17 years ago

Attachment: 4805.diff added

comment:5 by Gary Wilson <gary.wilson@…>, 17 years ago

I'm not sure if we would want to create a new Paginator class or change the existing ObjectPaginator class. The patch above I think is only backwards incompatible with a few of the functions that I changed to be 1-based. I think 1-based page numbers are more natural, and is the way it is currently coming from the URL. So we need some design decisions here.

The patch allows page to be set in the constructor, but it can also be set after the instance is created. Things like num_per_page and the object sequence can also be changed on the fly. Once a page is set you can call methods such as .next_page() or .first_on_page() without passing a page number, perfect for use in templates.

comment:6 by Gary Wilson <gary.wilson@…>, 17 years ago

Has patch: set
Patch needs improvement: set

comment:7 by Gary Wilson <gary.wilson@…>, 17 years ago

Needs documentation: set
Needs tests: set

by nick.lane.au@…, 17 years ago

Attachment: 4805-alt.diff added

Alternate implementation based on #2576

by nick.lane.au@…, 17 years ago

Attachment: 4805-alt.2.diff added

Alternate implementation based on #2576... that isn't completely broken like the last one I added

comment:9 by Adrian Holovaty, 17 years ago

Resolution: fixed
Status: newclosed

Fixed in [7306] (Paginator changes) and [7307] (generic views).

Note: See TracTickets for help on using tickets.
Back to Top