Posted by ankur on May 11, 2005 at 9:55pm
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | search.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | David Lesieur |
| Status: | needs work |
| Issue tags: | Screenshot |
Issue Summary
Perhaps this request has been made before, but I was wondering: why don't we display the number of results (we're passing a SQL COUNT query to pager_query() anyway) as well as the range of results currently being displayed on the search results page.
Kind of like how google will say "Results 1 - 10 of about 76 for "blah blah".
-Ankur
Comments
#1
Seconded.
#2
The attached patch adds '[total] results found for [query], showing page [current] of [max]' to the top of the search pages.
I attempted to get it to show actual result counts (eg. 1-10, 11-20, etc) per page but couldn't figure out how to intercept that information from between the search and pager functions. If anyone knows how to get that info, please update the patch, as showing result numbers is probably more useful than page numbers.
#3
The patch is not escaping the search keys, leading to XSS issues.
You could argue that this is a theming issue though. The Civicspace theme changes theme_pager() so it already returns a summary like this patch.
#4
#5
subscribe. would be nice.
#6
Nice feature request. I agree with Steven that this is a pager issue. The best solution would be to come up with a theme pager function that gets that encapsulates this code:
+ if ($pager_total[0] > 1) {+ $searchsummary .= t(', showing page %page of %totalpages', array('%page' => $pager_page_array[0] + 1, '%totalpages' => $pager_total[0]));
+ }
And then recycles it in the search display.
#7
Subscribing.
#8
can't find file to patch at input line 3Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|--- search.module-47b4 2006-03-23 15:56:26.000000000 +0100
|+++ search.module 2006-03-27 14:14:03.000000000 +0200
--------------------------
File to patch:
#9
Here's a link to some code I created to get the 1-10,11-20...etc. for paged results: http://drupal.org/node/201969. Hope this helps.
#10
LIVE FROM THE MINNESOTA SEARCH SPRINT: Here's a first step... We'll probably want to add some default CSS for the pager information.
#11
Here's a second attempt, that I think is a little less invasive. It changes the name from pager_info to pager_range, calls it from the search result theming, rather than adding an argument to theme_pager, and lets the search results template print the pager wherever it wants.
#12
My opinion is that theme_pager_range isn't a theme function. It has global variables, math, and a PHP control structure. I would want to see that done in a normal function and have those values passed into a real theme function that just outputs HTML.
#13
I've added a new pager_describe_range() function that takes the logic away from the theme function.
I've also moved the pager range above the search results, which is closer to what Google does. ;)
#14
Re-rolled patch with unit tests.
#15
#16
If $pager_total_items[$element] <= 1, $from and $to are not used. You might want to assign them in the conditional where they are used.
#17
Oops, yes! Done.
#18
Removed the unneeded declaration of $pager_total from pager_describe_range().
#19
One more comment, FWIW,
if ($from == $to) {return t('Item @to of @total', array('@to' => $to, '@total' => $pager_total_items[$element]));
}
elseif ($pager_total_items[$element] <= $limit) {
return t('@total items', array('@total' => $pager_total_items[$element]));
}
else {
return t('Items @from - @to of @total', array('@from' => $from, '@to' => $to, '@total' => $pager_total_items[$element]));
}
... could be two lines shorter, but it may be clearer the way it is ... I'm not sure what the Drupal standard is here.
if ($from == $to) {return t('Item @to of @total', array('@to' => $to, '@total' => $pager_total_items[$element]));
}
elseif ($pager_total_items[$element] <= $limit) {
return t('@total items', array('@total' => $pager_total_items[$element]));
}
return t('Items @from - @to of @total', array('@from' => $from, '@to' => $to, '@total' => $pager_total_items[$element]));
#20
I prefer your shorter version. Here's the patch!
#21
I applied the patch. Because there's a new theme function, I had to force rebuilding the theme and code registries. I tested the display on the first couple pages, and on the last page of a large results set. I tested the display on an empty results set. I also ran the new pager tests.
I had a hard time finding the pager test on admin/build/testing because this test is now it's own section. Should we group the test with the search tests or system tests or something else. Other than this question with where to group the pager test on admin/build/testing, I think that this is RTBC.
#22
I have a question above about where the test should live, but that's possibly a question for Dries, so I'm marking this RTBC.
#23
I reviewed this patch and it all looks good. The only point I have is that it doesn't look attractive visually.
I'd recommend the following: (i) we remove the title 'Search results' and (ii) make a new dynamic title "Found x items".
I think that would remove some clutter and make it look a bit more visually appealing.
#24
I'm going to mark this 'code needs work' to see if we can improve the visuals a bit. If we can't, I'll commit it as is.
#25
@Dries: Do you see the dynamic title completely replacing the pager range description proposed in the patch?
I think a more useful title would include the search keywords, although it probably wouldn't fit very well with how the search results page is currently designed.
#26
The first attachment (A.patch) is mostly a re-roll of the patch from #20, with one small change. I removed the title text ("Search Results"), and moved the test to the simpletest directory.
The second attachment (B.patch) is based of the first one, but I changed the formatting a bit, and added the search sting. I wasn't sure of the right way to find the search string, so this one is more of a functional prototype as opposed to a ready to commit patch...
#27
CNR to test the patch.
#28
The last submitted patch failed testing.
#29
Sigh. Yet another issue that didn't make it into Drupal 7, and needs to be postponed to Drupal 8.
#30
Here's a solution (far from perfect) that's implemented at the theming layer.
Code for template.php:
function MYTHEME_preprocess_search_results(&$variables) {
//Get search terms
$keys = search_get_keys();
// define the number of results being shown on a page
$itemsPerPage = 10;
// get the current page
$currentPage = $_REQUEST['page']+1;
// get the total number of results from the $GLOBALS
$total = $GLOBALS['pager_total_items'][0];
// perform calculation
$start = 10*$currentPage-9;
$end = $itemsPerPage * $currentPage;
if ($end>$total) $end = $total;
// set this html to the $variables
$variables['MYTHEME_search_totals'] = "Displaying $start - $end of $total results for <b>$keys</b>";
}
Code for search-results.tpl.php:
<?php print $MYTHEME_search_totals; ?>Example Result: Displaying 1 - 3 of 3 results for "INSERT TERM HERE"
NOTE: When using Apache Solr for search and you let it take over taxonomy handling, this approach may produce undesired results (working on a solution).