Needs work
Project:
Forum
Version:
2.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
18 Jan 2006 at 00:48 UTC
Updated:
4 May 2024 at 07:33 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
MrEricSir commentedOops!
That's an anti-patch... or something.
The corrected version is attached to THIS post.
Comment #2
MrEricSir commentedFor a 4.6 version of this patch, see this thread:
http://drupal.org/node/44876
Comment #3
MrEricSir commentedNew in this version of patch:
Has anyone else tested this patch?
Comment #4
MrEricSir commentedThis 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.
Comment #5
chx commentedthanks 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.
Comment #6
MrEricSir commentedYou'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.
Comment #7
chx commentedYou 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.
Comment #8
MrEricSir commentedCool, I'll look into it.
Do you have any recomendations on how this should be done?
Comment #9
MrEricSir commentedOkay, 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.
Comment #10
MrEricSir commentedWait... so if I change a theme fuction, wouldn't that break all existing themes?
Maybe my dirty hack was a better idea...
Comment #11
MrEricSir commentedI 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?
Comment #12
chx commentedlooks 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.
Comment #13
Jaza commentedMoving to 6.x-dev queue.
Comment #14
catchWe'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.
Comment #15
Gurpartap Singh commentedScreenshot 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!!
Comment #16
Gurpartap Singh commentedThis time without tabs.
Comment #17
dmitrig01 commented"+ //print '<pre>'; print_r($items); print '</pre>';"Uh
Comment #18
Gurpartap Singh commentedI'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?
Comment #19
Gurpartap Singh commentedbump
Comment #20
Gurpartap Singh commentedHope this still applies..
Comment #21
michelleHuh... 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
Comment #22
Gurpartap Singh commentedNor did I ever know it was there(in advanced forums). Hehehehe.
Comment #23
v8powerage commentedI'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.
Comment #24
catchLooks like it needs work.
Comment #25
ton_cut345 commentedhow about for drupal 6???
Comment #26
michelleD6 is feature frozen and this isn't a bug so it won't get backported.
Michelle
Comment #27
mitchell commentedDuplicate of #33809: Make pagers not suck, and more so #286665: SEE ISSUE #1805996 -- Moving Views into Drupal Core,.
I started #449236: Move Forum to CCK + Views to coordinate Forum development.
Comment #28
Gurpartap Singh commentedViews isn't in core *yet*.
Comment #29
jody lynnThis should be using theme_pager..
Comment #42
quietone commentedForum is approved for removal. See #1898812: [policy] Deprecate forum module for removal in Drupal 11
This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.
It will be moved to the contributed extension once the Drupal 11 branch is open.
Comment #44
quietone commentedComment #45
larowlan