Opened 4 years ago
Closed 7 months ago
#14 closed enhancement (fixed)
Allow a variable number of items per page
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | Future Release |
| Component: | interface | Version: | |
| Keywords: | has-patch needs-review dev-feedback | Cc: | joostdevalk, nbachiyski, juanramon.diaz@… |
Description (last modified by nbachiyski)
GlotPress should let the user decide how many items to display per page.
For example: 5-20-50-100 etc...
Attachments (6)
Change History (24)
comment:1
nbachiyski — 3 years ago
- Description modified (diff)
comment:2
nbachiyski — 3 years ago
- Milestone set to Future Release
- Priority changed from major to normal
joostdevalk — 20 months ago
comment:3
joostdevalk — 20 months ago
comment:4
joostdevalk — 20 months ago
Biggest issue I have with the patch myself is that it's stored in a cookie. In WP core I'd a user_meta, but I didn't see an API for that in GP yet, but I think that'd be the way to go. If we had that we could also store a default sort.
comment:5
vanillalounge — 20 months ago
- Keywords has-patch needs-review added
comment:6
nbachiyski — 20 months ago
See GP_User::get_meta() and its friends.
comment:7
joostdevalk — 20 months ago
Ok found that indeed, couple of issues that needed fixing to get this to work:
- get_meta, set_meta and delete_meta all assumed $this->id was filled, when it wasn't in this case, so switched them over to first using the current method of GP_User.
- gp_update_meta always returned false when $global wasn't set, which it isn't when called from GP_User, so I removed that check.
- GP_Thing's sql_limit_for_paging method first correctly set its internal $per_page variable to then use $this->per_page for the query, fixed.
- GP_Translation now checks whether there's user setting per_page and uses that value, otherwise it reverts to its default.
comment:8
joostdevalk — 20 months ago
- Cc joostdevalk added
comment:9
joostdevalk — 20 months ago
Just found a bug in pagination, tracking down, don't commit patch yet.
comment:10
joostdevalk — 20 months ago
Pagination issue found and fixed, actually fixed it by implementing the $per_page fix cleaner and earlier, before getting the translations.
comment:11
joostdevalk — 20 months ago
This last patch adds a redirect for non-logged in users to the login page when they try to open the /profile page.
comment:12
nacin — 14 months ago
- Keywords changed from has-patch, needs-review to has-patch needs-review
Figured I would review this. It looks good, but a few thoughts:
get_meta() et al. store raw usermeta keys into the DB. In WP.org's case, we share user tables across literally everything -- WP, bbPress, SVN, GP, etc. Even without our complex setup, since GP does not have an interface, then it is almost assuredly going to be sharing the user table with a WP install nearby. What we may need here is a get_option() method that keys things by the table prefix. This is exactly how get_user_option() works in WP.
Or, really, we do want it to be *global* but global to GlotPress. (We may have multiple installs of GlotPress in the future, for example, and those settings should span installs.) So perhaps the meta keys could start with 'glotpress_' or 'gp_'.
Also, since the default sort is stored in an array, gp_array_get() will probably enable some of those pieces to appear a bit cleaner (especially since it also takes a default).
comment:13
nbachiyski — 13 months ago
A couple of things on the patch:
- It's not OK to always use the current user in GP_User:: get_meta(). What if I have an instance of GP_User and I want to get its meta? I will get the current user's one instead. Please use GP::$user->current->get_meta().
- The default sort logic is duplicated, it's worth it to have it somewhere.
- In a couple of places you're using double quotes, where single would be enough
And a general thing: please run the tests with your changes. For example your user meta changes prevented user meta to be set and get correctly and it showed in the tests.
Nacin, if you think the prefixing is needed, we should implement it deeper in GP_User::get/set_meta() – it could transparently add gp_ to the meta key and we won't have to prefix it by ourselves.
comment:14
nbachiyski — 13 months ago
- Cc nbachiyski added
comment:15
juanramondiaz — 13 months ago
- Cc juanramon.diaz@… added
comment:16
nbachiyski — 7 months ago
- Resolution set to fixed
- Status changed from new to closed
comment:17
vanillalounge — 7 months ago
- Keywords dev-feedback added
- Resolution fixed deleted
- Status changed from closed to reopened
Both my local install (bleeding edge) and http://translate.wordpress.org/profile return a 500. Local log says:
PHP Fatal error: Class 'GP_Route_Profile' not found in /Applications/MAMP/htdocs/trunkgp.dev/gp-includes/router.php on line 119
comment:18
nacin — 7 months ago
- Resolution set to fixed
- Status changed from reopened to closed

I came up with a patch for this today as the default 15 annoyed the heck out of me and a lot of people. Attached patch adds a /profile page where people can set how many items they want displayed on a page, currently stored in a cookie.