I believe the audio.module should invoke hook_search so that audio node metadata can be indexed more thoroughly and would allow for other modules, namely the playlist module, to do search queries on audio nodes only. It would also add an 'audio' tab to the search page so users could search only audio files.

CommentFileSizeAuthor
#12 audio.module.search_1.patch2.01 KBdrewish
#6 audio.module.search_0.patch2.2 KBzirafa
#3 audio.module.search2.patch2.73 KBzirafa
#2 audio.module.search.patch2.2 KBzirafa

Comments

drewish’s picture

drewish commented

right you are. i'd kind of toyed around with doing this but the hook search documentation sucks. i never got around to finding a contrib module that implemented the hook to copy.

zirafa’s picture

zirafa commented
Assigned: Unassigned » zirafa
Status: Active » Reviewed & tested by the community
StatusFileSize
new audio.module.search.patch2.2 KB

Here is a patch which will implement hook_search for audio.

zirafa’s picture

zirafa commented
StatusFileSize
new audio.module.search2.patch2.73 KB

This updated patch includes a function for "search adding" for the playlist.module. This hook can/should be applied to both 4.6 and 4.7 versions of the audio.module.

drewish’s picture

drewish commented

[i'm changing the status back to needs review until we can get some feedback on the patch]

just a clarification, there's actually not a 4.7 version of the module at this point. there is, however, a 4.7 development fork in my sandbox (here's the issue for it). if you want to roll a patch for that i'd love the help.

drewish’s picture

drewish commented
Status: Reviewed & tested by the community » Needs review

forgot to actually change the status...

zirafa’s picture

zirafa commented
StatusFileSize
new audio.module.search_0.patch2.2 KB

Actually I am rolling back to the first patch I submitted. I thought about it and I don't want to have the playlist.module depend on code from other modules. I'm reposting the first patch, which simply adds hook_search. Works on 4.6 to my knowledge. Also drewish thanks for explaining that - I thought the audio.module had been converted already.

Farsheed

zirafa’s picture

zirafa commented

I am voting we commit this and bug fix it later if there are problems....which so far I see none after testing it a ton.

drewish’s picture

drewish commented

I haven't had a chance to look into the way that the search works in 4.6, but in 4.7 I don't think this is the "right" right way to do the search. It seems like rather than adding a separate search process and tab, we should either put the metadata into the teaser/body so it gets indexed automatically or implement hook_update_index().

zirafa’s picture

zirafa commented

1) The metadata is already published in the node body and will get indexed by the general node search mechanism, even if hook_search wasn't implemented. But that isn't anything special, it happens automatically. The point is to provide a special search form to search *only* metadata and *only* audio nodes.
2) This isn't a "special" method for searching. This is just how the hook_search function works. Node.module has a hook_search function, user.module has a search function, and there is nothing extraordinary about adding a hook_search for your custom node type to allow searching within those node types.
3) This is aimed at the 4.6 branch of the audio module. We can make changes to the 4.7 branch, but I'm pretty sure this won't break in 4.7 either.
4) Yes, 4.7 has the ability in advanced search to only search specific node types. But which is easier, telling your users to click the advanced search button, or a tab that will search only audio files?
5) Some users will modify or override the audio module's theme function and thus may choose to NOT have the metadata displayed in the teaser/body. If you make the search dependent on the node body text and not the actual metadata, this will break the search of audio files. This is the problem with relying on the default node search system.

I think it would be a good idea to get this in before the 4.6 branch and then we can play with it more in the 4.7 branch.

drewish’s picture

drewish commented

I understand how the search works but I'm not convinced it makes sense to add a separate search. It makes sense to have additional searches for non-node content, like users or CVS commits, but audio nodes are content, and already indexed.

I think the focus should be on improving the default search, i.e. hook_update_index or hook_nodeapi(op="update index"), that way if people re-theme the node the data will still be indexed.

zirafa’s picture

zirafa commented

That is a valid concern...using hook_update_index makes more sense in 4.7, but like I mentioned this patch is really for 4.6. You can't search nodes by type in 4.6.

I'll be happy to write a hook_update_index function for 4.7 which makes use of the search_index function, but I would rather do that after there is a stable 4.7 version of the audio.module.

drewish’s picture

drewish commented
StatusFileSize
new audio.module.search_1.patch2.01 KB

i cleaned up some of the formatting and re-rolled this patch. anyone want to review it?

moshe weitzman’s picture

moshe weitzman commented

Views.module provides interesting built in search features once you let it know about your node metadata. Drewish - lets discuss. I think thats a more fruitful avenue than hook_search(). need some exploring. See the 'exposed filters' feature of Views.

Also, bryght needs Views integration anyways for same client.

drewish’s picture

drewish commented

moshe, this is for 4.6. the version knob is broken on this project.

drewish’s picture

drewish commented
Version: 5.x-1.x-dev » 4.6.x-1.x-dev

hey zirafa, are you still interested in seeing this committed?

zirafa’s picture

zirafa commented

I am still interested in this. I'll have to do a bit more research on the new search mechanism...my guess is that hook_search may not even be necessary.

zirafa’s picture

zirafa commented

Sorry, I think I misunderstood. For the record, I'm not too interested in this for 4.6 anymore. The 4.6 playlist module is pretty broken at the moment anyway.

drewish’s picture

drewish commented
Status: Needs review » Closed (won't fix)

okay, my understanding is that in 4.7 it's taken care of. i'm going to mark it as won't fix, feel free to reopen.