Code

Opened 7 years ago

Closed 6 years ago

Last modified 5 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: master
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

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@…> 7 years ago.
4805-alt.diff (2.3 KB) - added by nick.lane.au@… 7 years ago.
Alternate implementation based on #2576
4805-alt.2.diff (5.6 KB) - added by nick.lane.au@… 7 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 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 7 years ago by nick.lane.au@…

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 Changed 7 years ago by SmileyChris

#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 Changed 7 years ago by Gary Wilson <gary.wilson@…>

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.

Changed 7 years ago by Gary Wilson <gary.wilson@…>

comment:5 Changed 7 years ago by Gary Wilson <gary.wilson@…>

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 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Has patch set
  • Patch needs improvement set

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

  • Needs documentation set
  • Needs tests set

Changed 7 years ago by nick.lane.au@…

Alternate implementation based on #2576

Changed 7 years ago by nick.lane.au@…

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

comment:9 Changed 6 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.