Posted by MrEricSir on January 18, 2006 at 12:48am
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | forum.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Gurpartap Singh |
| Status: | needs work |
Issue Summary
This adds page links to the list of forum topics, much like phpBB and other popular software.
Example:
Before: "This thread rules!!!!"
After: "This thread rules!!!! [ 1, 2, 3, 4]"
Where 1, 2, 3, 4 are links to those pages in the topic.
Benefits:
- Users of popular forums have less clicking to do.
- Saves a few page loads, especially if tracker disabled.
Drawbacks:
- Had to impliment my own pager for this.
- Should be tested further.
Note: this version will NOT WORK with 4.6.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| dif.txt | 2.04 KB | Ignored: Check issue status. | None | None |
Comments
#1
Oops!
That's an anti-patch... or something.
The corrected version is attached to THIS post.
#2
For a 4.6 version of this patch, see this thread:
http://drupal.org/node/44876
#3
New in this version of patch:
Has anyone else tested this patch?
#4
This version of the patch should work easily with the forum.module cvs. It's been cleaned up a little and afaik complies fully with the Drupal coding standards.
I guess the code could be made cleaner, but I've tested it thoroughly as-is and don't want to upset the delicate balance of the force. I doubt it could be made significantly more efficient anyway.
#5
thanks for the hard work, but you should let others review your patch before setting ready to commit.
Also... Drupal already have a pager system. I doubt this patch will accepted as it's essentially code doubling.
#6
You're probably right that it needs review.
But as far as I can tell there's no way to create a pager using existing functions without calling pager_query().
Maybe I'm wrong, but calling pager_query() for each topic seems excessively database intensive. That's why I felt the need for a custom pager.
#7
You are right in that currently there is other way but then you should extend pager.inc so that it can? You only need a function which would initialize some vars.
#8
Cool, I'll look into it.
Do you have any recomendations on how this should be done?
#9
Okay, I looked into it myself. Sadly this cannot be done using the existing theme_pager_link(). I'll have to modify it to accept a new parameter.
The offending line is this one:
return l($text, $_GET['q'], array(), count($query) ? implode('&', $query) : NULL);
I could just set $_GET['q'] = "node/$topic->nid"; but that would break other pagers, and it's a lousy hack in the first place.
#10
Wait... so if I change a theme fuction, wouldn't that break all existing themes?
Maybe my dirty hack was a better idea...
#11
I discovered a workaround, and after some testing got it running. I'm not sure how "legal" something like this is:
$_GET['q'] = "node/$topic->nid";
This is due to the following line in theme_pager_link():
return l($text, $_GET['q'], $attributes, count($query) ? implode('&', $query) : NULL);
So to change the link, I either have to set $_GET['q'] or modify ALL the pager theme functions! Another possibility is to add some kind of global variable containing the link that only theme_pager_link() would use.
Any comments on the perferred way to implement this functionality?
#12
looks good to my eyes -- but Dries may think otherwise. However, I would $on_page = $_GET['q']; // store current url just above the $_GET['q'] statement.
#13
Moving to 6.x-dev queue.
#14
We've done some template hacks to get this into forum listings (and hacked views.module slightly to get the same on the tracker). Will try to post up some of that code later if it takes a different approach. For drupal 7 though.
#15
Screenshot is attached to this comment. Ignore the date created thingy.. that's going into another issue.
If this gets good reviews and code improvement suggestions, I'll be taking up with more forum UI issues!!
#16
This time without tabs.
#17
"+ //print '<pre>'; print_r($items); print '</pre>';"Uh
#18
I'm wondering why drupal_add_css() for forum.css in forum_init() is not adding forum.css........... In the patch I re call drupal_add_css for that. Is that a bug or I'm missing something?
#19
bump
#20
Hope this still applies..
#21
Huh... Never knew this issue was here. I do this in advforum. Will take a look at this when I have a chance to see which method is better :)
Michelle
#22
Nor did I ever know it was there(in advanced forums). Hehehehe.
#23
I've noticed bug in this patch, after nearly year of using, because before I haven't got such a long topics.
When navigate to a second page of topic listings, all thread pagers also showing 2 page active, it's true for next pages as well of course.
Screenshot as an attachment.
#24
Looks like it needs work.
#25
how about for drupal 6???
#26
D6 is feature frozen and this isn't a bug so it won't get backported.
Michelle
#27
Duplicate of #33809: Make pagers not suck, and more so #286665: Moving Views into Drupal Core,.
I started #449236: Move Forum to CCK + Views to coordinate Forum development.
#28
Views isn't in core *yet*.
#29
This should be using theme_pager..