Forum: Pager links in Forum topics list

Oops!

That's an anti-patch... or something.

The corrected version is attached to THIS post.

For a 4.6 version of this patch, see this thread:

http://drupal.org/node/44876

New in this version of patch:

• Optimization: Never displays more than four links

Has anyone else tested this patch?

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.

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.

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.

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.

Cool, I'll look into it.

Do you have any recomendations on how this should be done?

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.

Wait... so if I change a theme fuction, wouldn't that break all existing themes?

Maybe my dirty hack was a better idea...

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?

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.

Moving to 6.x-dev queue.

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.

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!!

This time without tabs.

"+  //print '<pre>'; print_r($items); print '</pre>';"

Uh

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?

bump

Hope this still applies..

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

Nor did I ever know it was there(in advanced forums). Hehehehe.