Skip to:
Content

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#1274 closed defect (fixed)

Search Improvements

Reported by: GautamGupta Owned by:
Milestone: 1.0.3 Priority: high
Severity: critical Version: 1.1-alpha
Component: Front-end Keywords: has-patch tested
Cc:

Description

[2429] (#1222) generated the problem. Now every search made returns every post. If a forum is selected, then posts from only that forum are displated.

Attachments (20)

kakumei-search.diff (1.9 KB) - added by mr_pelle 4 years ago.
Correct patch: I forgot to revert search.php to [2377]
recent-posts-last-poster.diff (1.6 KB) - added by mr_pelle 4 years ago.
Little add to display last poster next to each Recent Post
search-results-last-poster.diff (1.7 KB) - added by mr_pelle 4 years ago.
More details on search results
1274.diff (9.5 KB) - added by GautamGupta 4 years ago.
1274.2.diff (9.3 KB) - added by GautamGupta 4 years ago.
datetime-title.diff (1.1 KB) - added by mr_pelle 4 years ago.
datetime format topic_time title on links
datetime-title.2.diff (1.2 KB) - added by mr_pelle 4 years ago.
better patch for datetime title
with1280.diff (1.1 KB) - added by mr_pelle 4 years ago.
if #1280 will be committed, just use this to display datetime title
search.php (2.0 KB) - added by chrishajer 4 years ago.
search.php file after trying to apply 1274.2.diff
search.php.orig (2.0 KB) - added by chrishajer 4 years ago.
search.php.orig after trying to apply 1274.2.diff
search.php.rej (1.5 KB) - added by chrishajer 4 years ago.
search.php.rej after trying to apply 1274.2.diff
1274-results.png (69.5 KB) - added by GautamGupta 4 years ago.
Final Outcome after applying the patch
1274-final.diff (9.2 KB) - added by GautamGupta 4 years ago.
Final Patch - Changes the way time is displayed in Relevant Topics
posted-by-on.diff (2.1 KB) - added by mr_pelle 4 years ago.
fixes bugs. Relevant freshness is the same as Recent's
last-post-ago-with-datetime-title.diff (2.1 KB) - added by mr_pelle 4 years ago.
fixes bugs. Relevant freshness is the same as front page last posts'
2436-fixes.diff (2.8 KB) - added by GautamGupta 4 years ago.
Fixes the bugs that went into [2436]
2436-fixes.2.diff (5.6 KB) - added by mr_pelle 4 years ago.
updated with page navigation link. Does NOT fix everything yet!
2436-fixes.3.diff (6.3 KB) - added by mr_pelle 4 years ago.
further update. Restores 5 item per page. Does NOT fix everything yet!
2436-and-per-page-fixes.patch (6.4 KB) - added by GautamGupta 4 years ago.
Includes 2436-fixes.diff, bb_search_pages fix and per_page fix (checks for items per page)
search.php.diff (2.7 KB) - added by GautamGupta 4 years ago.
Better way to do pagination

Download all attachments as: .zip

Change History (80)

comment:2 mr_pelle4 years ago

Actually it was not [2429]'s fault, but [2414]'s.

Reverting to [2377] fixes the search but (obviously) breaks Relevant Posts' links (both post and author's) again.

comment:3 follow-up: GautamGupta4 years ago

I think it was actually Relevant Topics, not Relevant Posts

comment:4 in reply to: ↑ 3 mr_pelle4 years ago

Replying to GautamGupta:

I think it was actually Relevant Topics, not Relevant Posts

You're right! Relevant query returns topic objects if its type is set to 'topic' so Kakumei search.php must use

foreach ( $relevant as $topic )

instead of

foreach ( $relevant as $bb_post )

Of course post_link(), bb_get_post_time() and post_text() must be changed as well.

I've just completed a simple patch, I will submit it within minutes.

comment:5 mr_pelle4 years ago

  • Keywords has-patch tested added; needs-patch removed

mr_pelle4 years ago

Correct patch: I forgot to revert search.php to [2377]

comment:6 mr_pelle4 years ago

Correct patch submitted. I forgot to revert search.php to [2377]

mr_pelle4 years ago

Little add to display last poster next to each Recent Post

comment:7 mr_pelle4 years ago

I've added another (optional) patch to display last poster next to each Recent Post. It comes from a theme of mine, hope you like it. :)

comment:8 GautamGupta4 years ago

I have made something better, will post after a few minutes.

mr_pelle4 years ago

More details on search results

comment:9 mr_pelle4 years ago

Last patch I added is what I currently use in my theme. Maybe is too much for a default theme...

GautamGupta4 years ago

comment:10 GautamGupta4 years ago

The patch does the following:

  • Styles the Search
  • Also highlights the keywords in the title
  • Adds a title
  • Adds pagination
  • Fixes some translation strings
  • Also adds "Posted By" and "Last Post by" (which was submitted in a patch by mr_pelle
  • Adds a nice message with suggestions if no result was found

GautamGupta4 years ago

comment:11 GautamGupta4 years ago

Updated the patch.

comment:12 mr_pelle4 years ago

Wow, great work Gautam! Can't wait for 1.0.3 to be released!

mr_pelle4 years ago

datetime format topic_time title on links

comment:13 mr_pelle4 years ago

Consistency patch to give search results links the title attribute with datetime format topic_time like last post links in v1.1-alpha's front page, forum, etc.

comment:14 mr_pelle4 years ago

Important note: my last patch makes use of 'datetime' format defined in _bb_time_function_return() (in v1.1-alpha) for Relevant Topics links. Unfortunately that function does not check bbPress time zone, like Recent Posts seems to do, making results times differ of your GMT offset...

Any way to fix this? Use bb_get_last_post() and retrieve its posting time instead of just topic_time()?

comment:15 GautamGupta4 years ago

Hmm.. that function is also used elsewhere (in front-page etc.).

mr_pelle4 years ago

better patch for datetime title

comment:16 mr_pelle4 years ago

Fixed in my latest patch.

I've also tried to set 'localize' in the query args (see functions.bb-template.php, line 3558), but it didn't work...

comment:17 follow-up: mr_pelle4 years ago

According to this page, _bb_time_function_return() is referenced 7 times in bbPress source (2 in Kakumei's search.php): are we going to replace all those instances with bb_datetime_format_i18n?

comment:18 in reply to: ↑ 17 ; follow-up: GautamGupta4 years ago

Replying to mr_pelle:

According to this page, _bb_time_function_return() is referenced 7 times in bbPress source (2 in Kakumei's search.php): are we going to replace all those instances with bb_datetime_format_i18n?

Please open a new ticket for that.

comment:19 in reply to: ↑ 18 mr_pelle4 years ago

Replying to GautamGupta:

Replying to mr_pelle:

According to this page, _bb_time_function_return() is referenced 7 times in bbPress source (2 in Kakumei's search.php): are we going to replace all those instances with bb_datetime_format_i18n?

Please open a new ticket for that.

Done: #1280.

What about this ticket, now?

comment:20 GautamGupta4 years ago

I'll tell chrishajer to test and commit the patch.

mr_pelle4 years ago

if #1280 will be committed, just use this to display datetime title

comment:21 mr_pelle4 years ago

Forget about my last reply (see #1280), is better to use i18n functions to filter dates and times...

comment:22 chrishajer4 years ago

I suspect this is MORE broken now.

I fixed #1280.

I patched with with1280.diff then I tried 1274.2.diff and got a bunch of errors:

(Stripping trailing CRs from patch.)
patching file bb-includes/functions.bb-formatting.php
(Stripping trailing CRs from patch.)
patching file bb-includes/functions.bb-template.php
(Stripping trailing CRs from patch.)
patching file bb-templates/kakumei/search.php
Hunk #2 FAILED at 24.
Hunk #3 succeeded at 37 (offset -1 lines).
1 out of 3 hunks FAILED -- saving rejects to file bb-templates/kakumei/search.php.rej
(Stripping trailing CRs from patch.)
patching file bb-templates/kakumei/style.css
(Stripping trailing CRs from patch.)
patching file search.php

So I reverted.

I will attach the search files that were unpatched (orig, rej and php)

chrishajer4 years ago

search.php file after trying to apply 1274.2.diff

chrishajer4 years ago

search.php.orig after trying to apply 1274.2.diff

chrishajer4 years ago

search.php.rej after trying to apply 1274.2.diff

comment:23 chrishajer4 years ago

I reverted my local copy until someone can tell me the correct order to apply these patches.

comment:24 sambauers4 years ago

Easier if people submit one big patch on the root directory instead of lot's of little ones on each file.

comment:25 follow-up: GautamGupta4 years ago

Actually the only patch needed to be applied was 1274.2.diff, and I had sent a mail to chris to test it with with1280.diff before mr_pelle's 21st comment, which created the problem.

So, you just need to apply 1274.2.diff. It works fine for me, I'll post a screenshot of it's final outcome.

GautamGupta4 years ago

Final Outcome after applying the patch

comment:26 GautamGupta4 years ago

  • Summary changed from Search results in all posts being displayed to Search Improvements
  • Type changed from defect to enhancement

comment:27 in reply to: ↑ 25 mr_pelle4 years ago

Replying to GautamGupta:

Actually the only patch needed to be applied was 1274.2.diff, and I had sent a mail to chris to test it with with1280.diff before mr_pelle's 21st comment, which created the problem.

Actually I replied again in #1280 saying that my patch was wrecking all other last post links before chris submitted it...

Anyway, sorry about that...

comment:28 mr_pelle4 years ago

What about including part/all of #677?

GautamGupta4 years ago

Final Patch - Changes the way time is displayed in Relevant Topics

comment:29 GautamGupta4 years ago

Added the final patch which makes the relevant topics' time style look like one of recent posts.

comment:30 chrishajer4 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [2436]) Search enhancements. Maybe fixes #1274. Props GautamGupta, mr_pelle

comment:31 follow-up: mr_pelle4 years ago

  • Keywords has-patch tested removed
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from enhancement to defect

I'm really sorry to say that we still have problems... probably something of Gautam's patch got lost in [2436]:

1) All Relevant Topics links link to last Recent Posts result.

2) All Relevant Topics author links are broken.

3) At line 31 of search.php, I'm pretty sure it should be

if ( $relevant ) {

4) Relevant Topics freshness span text is different from the one you can see here.

5) Relevant Topics context is not retrieved because bb_show_context( $q, $topic->post_text ); does not work. We should first retrieve topic first post id and then call "->post_text"


Regarding point 4, do you think Relevant Topics links must link to the topic itself or to topic last post?

I'd prefer the former. Thoughts?

I'll start writing 2 different patches, you'll decide which one to use.

comment:32 in reply to: ↑ 31 ; follow-up: GautamGupta4 years ago

Replying to mr_pelle:

4) Relevant Topics freshness span text is different from the one you can see here.

I explained that in this reply, the format in which time is displayed should be same (in recent and relevant).

And sorry for the bugs in the patch, I don't know how they crept in.

comment:33 in reply to: ↑ 32 mr_pelle4 years ago

Replying to GautamGupta:

Replying to mr_pelle:

4) Relevant Topics freshness span text is different from the one you can see here.

I explained that in this reply, the format in which time is displayed should be same (in recent and relevant).

Like this?

Recent Posts

link to post, "Posted by ... on ..."

Relevant Topics

link to topic last post, "Posted by ... on ..."


My patch is almost done...

comment:34 mr_pelle4 years ago

Imho, if Relevant Topics link to topic last post, their freshness should be "Last post ... ago by ...", with datetime <title/> as front page last posts links.

Don't you think "Posted by ... on ..." on a topic is a little confusing? I mean, does it refer to topic first or last post? It's not that clear...

mr_pelle4 years ago

fixes bugs. Relevant freshness is the same as Recent's

mr_pelle4 years ago

fixes bugs. Relevant freshness is the same as front page last posts'

comment:35 mr_pelle4 years ago

  • Keywords has-patch tested added

Choose your favorite, not both! ;)

comment:36 GautamGupta4 years ago

We are actually linking to the first post of the topic, not to the last post. Attaching another patch which fixes the bugs and also replaces "Posted by" with "By" ("Posted" takes up lot of space and it doesn't look nice).

GautamGupta4 years ago

Fixes the bugs that went into [2436]

comment:37 mr_pelle4 years ago

Replying to GautamGupta:

We are actually linking to the first post of the topic, not to the last post. Attaching another patch which fixes the bugs and also replaces "Posted by" with "By" ("Posted" takes up lot of space and it doesn't look nice).

Ok, that's right.

From a mere spacing point of view, don't you think <li/> elements inside both Recent and Relevant loops are 1 tab too much on the right?
Also <?php $bb_post = bb_get_first_post( $topic ); ?> should be aligned correctly, being it inside the loop too.

comment:38 mr_pelle4 years ago

Ignore my last reply: I understood <li/>s indentation refers to <ol/> tag and not to any php...

comment:39 follow-up: mr_pelle4 years ago

Uff... bb_search_pages() does not return pages navigation link. I set bbPress "items per page" to 2 and I get only 2 Recents and 2 Relevants.

comment:40 in reply to: ↑ 39 ; follow-up: GautamGupta4 years ago

Replying to mr_pelle:

Uff... bb_search_pages() does not return pages navigation link. I set bbPress "items per page" to 2 and I get only 2 Recents and 2 Relevants.

You need to change 5 everywhere, in bb_search_pages function, and $search_start, $search_stop and per_page in search.php which exists in root folder.

comment:41 in reply to: ↑ 40 mr_pelle4 years ago

Replying to GautamGupta:

Replying to mr_pelle:

Uff... bb_search_pages() does not return pages navigation link. I set bbPress "items per page" to 2 and I get only 2 Recents and 2 Relevants.

You need to change 5 everywhere, in bb_search_pages function, and $search_start, $search_stop and per_page in search.php which exists in root folder.

Sorry, change 5 into what? bb_get_option('page_topics')?

comment:42 mr_pelle4 years ago

I managed to get pages navigation link displayed, but it disappears on page 2...

I also added $args to bb_search_pages(), since it was missing (usually used for adding div.nav).

comment:43 mr_pelle4 years ago

I think there is a problem with $search_count: at the moment it is different from results total, thus it cannot be used as "total" in get_page_number_links().

Example: if we have 3 results, 2 items per page (bbPress option) and we browse to page 2, $search_count will be 1 (thus not showing page navigation link), while it should be 3 in all the pages, allowing bbPress to correctly calculate ceil( $total/$per_page ) (line 1360, functions.bb-template.php).

Pagination should be controlled via bbPress "item per page" option and not hardcoded, imho.

Anyway, at the moment I've no idea how to fix this, but I'll update last patch with some little additions.

mr_pelle4 years ago

updated with page navigation link. Does NOT fix everything yet!

comment:44 mr_pelle4 years ago

If we could set get_page_number_links()'s "total" (line 1651, functions.bb-template.php) to the correct value (= results total), all problems would solve. I've indeed tried setting it manually and works.

mr_pelle4 years ago

further update. Restores 5 item per page. Does NOT fix everything yet!

comment:45 mr_pelle4 years ago

Do the 2 new filters make search ignore posts in the same topic??

GautamGupta4 years ago

Includes 2436-fixes.diff, bb_search_pages fix and per_page fix (checks for items per page)

comment:46 follow-up: GautamGupta4 years ago

  • Keywords needs-testing added; tested removed

Can you please test 2436-and-per-page-fixes.patch? It includes the fixes in 2436-fixes.diff, bb_search_pages function fixes by Mr. Pelle and items per page fixes.

comment:47 in reply to: ↑ 46 mr_pelle4 years ago

Replying to GautamGupta:

Can you please test 2436-and-per-page-fixes.patch? It includes the fixes in 2436-fixes.diff, bb_search_pages function fixes by Mr. Pelle and items per page fixes.

I'm on it! :)

comment:48 follow-up: mr_pelle4 years ago

Works great except for the 2 new filters that make search ignore posts in the same topic, as I said before. I had to remove them both to get complete results...

If your search string contains exclamation points or question marks, these chars are escaped in the post context (\! \?). This does not happen with ticks, backticks, slashes or backslashes.


Nit-picking, there are some superfluous tabs (lines 1647, 1650 of functions.bb-template.php; lines 16, 28, 38, 40 of search.php) and 2 pairs of braces that could be removed (lines 25, 27, 35, 37 of search.php).

Can you also explain me why you added tabs to lines 46, 47, 48 of kakumei/search.php, please?

comment:49 in reply to: ↑ 48 ; follow-ups: GautamGupta4 years ago

Replying to mr_pelle:

Works great except for the 2 new filters that make search ignore posts in the same topic, as I said before. I had to remove them both to get complete results...
If your search string contains exclamation points or question marks, these chars are escaped in the post context (\! \?). This does not happen with ticks, backticks, slashes or backslashes.

Hmm.. would have to look into that.

Nit-picking, there are some superfluous tabs (lines 1647, 1650 of functions.bb-template.php; lines 16, 28, 38, 40 of search.php)

They're fine for me - in my code editor (Komodo).

2 pairs of braces that could be removed (lines 25, 27, 35, 37 of search.php).

I don't like if else statements in which one section has braces and the other doesn't.
Please check http://codex.wordpress.org/WordPress_Coding_Standards

Can you also explain me why you added tabs to lines 46, 47, 48 of kakumei/search.php, please?

Those are 8 character space tabs (which are also there in rest of the file). Previous were 4.

comment:50 in reply to: ↑ 49 ; follow-up: mr_pelle4 years ago

Replying to GautamGupta:

Replying to mr_pelle:

Nit-picking, there are some superfluous tabs (lines 1647, 1650 of functions.bb-template.php; lines 16, 28, 38, 40 of search.php)

They're fine for me - in my code editor (Komodo).

I use Komodo (Edit) too. I've always believed that if a line contains no text, spaces and tabs should be removed too.

2 pairs of braces that could be removed (lines 25, 27, 35, 37 of search.php).

I don't like if else statements in which one section has braces and the other doesn't.
Please check http://codex.wordpress.org/WordPress_Coding_Standards

I already knew the page, but I forgot that rule, sorry.

Can you also explain me why you added tabs to lines 46, 47, 48 of kakumei/search.php, please?

Those are 8 character space tabs (which are also there in rest of the file). Previous were 4.

If you mean the <li/>s at lines 15-19 and 31-35, their indentation is one 4-spaces-tab on their parent <ol/>'s right, while at lines 46-48 the <ul/> is not indented, so <li/>'s indentation should be just one 4-spaces-tab, not 2. Right?

comment:51 in reply to: ↑ 50 ; follow-up: GautamGupta4 years ago

  • Keywords tested added; needs-testing removed

Replying to mr_pelle:

I use Komodo (Edit) too. I've always believed that if a line contains no text, spaces and tabs should be removed too.

Not necessary, I like them to be there.

If you mean the <li/>s at lines 15-19 and 31-35, their indentation is one 4-spaces-tab on their parent <ol/>'s right, while at lines 46-48 the <ul/> is not indented, so <li/>'s indentation should be just one 4-spaces-tab, not 2. Right?

You can check the patch, 1 indent = 8 spaces. At lines 15, 19 and 31, 35 - there are a total of 2 tabs = 16 spaces and the li I edited, is only 1 tab = 8 spaces. Hope that clears your doubt.

comment:52 in reply to: ↑ 51 mr_pelle4 years ago

Replying to GautamGupta:

Replying to mr_pelle:

If you mean the <li/>s at lines 15-19 and 31-35, their indentation is one 4-spaces-tab on their parent <ol/>'s right, while at lines 46-48 the <ul/> is not indented, so <li/>'s indentation should be just one 4-spaces-tab, not 2. Right?

You can check the patch, 1 indent = 8 spaces. At lines 15, 19 and 31, 35 - there are a total of 2 tabs = 16 spaces and the li I edited, is only 1 tab = 8 spaces. Hope that clears your doubt.

Yep, thank you!

I hope we're finally done with this ticket!

comment:53 mr_pelle4 years ago

Oh, what about the filters?

comment:54 chrishajer4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [2440]) Trying again to fix pagination in search. Fixes #1274. Props GautamGupta

comment:55 in reply to: ↑ 49 ; follow-up: mr_pelle4 years ago

Replying to GautamGupta:

Replying to mr_pelle:

Works great except for the 2 new filters that make search ignore posts in the same topic, as I said before. I had to remove them both to get complete results...
If your search string contains exclamation points or question marks, these chars are escaped in the post context (\! \?). This does not happen with ticks, backticks, slashes or backslashes.

Hmm.. would have to look into that.

Thank you Chris, but... Gautam, have you checked the filters behavior?

comment:56 in reply to: ↑ 55 ; follow-up: GautamGupta4 years ago

Replying to mr_pelle:

Thank you Chris, but... Gautam, have you checked the filters behavior?

Those have been there for long time. Check [907] #694. Also http://trac.bbpress.org/log/trunk/search.php

comment:57 in reply to: ↑ 56 mr_pelle4 years ago

Replying to GautamGupta:

Replying to mr_pelle:

Thank you Chris, but... Gautam, have you checked the filters behavior?

Those have been there for long time. Check [907] #694. Also http://trac.bbpress.org/log/trunk/search.php

Oh, ok...

comment:58 chrishajer4 years ago

Are the incorrect search results on this page due to this bug or another bug that's been reported?

http://bbpress.org/plugins/search.php?q=approve

None of the links work, and the date and time are the date and time the search was performed (in GMT I think.)

GautamGupta4 years ago

Better way to do pagination

comment:59 GautamGupta4 years ago

Added another patch which does pagination the better way.

comment:60 chrishajer4 years ago

Committed that patch in r2449

Note: See TracTickets for help on using tickets.