Advanced search rebased

[abrown]

Adds the required search providers and search results for the new functionality in the Advanced Search app. To test, checkout the advanced_search_rebased branch on the 3rdparty, core, and app repositories. I have submitted parallel pull requests in 3rdparty pull #7 and apps pull #520.

This work overlaps some of the work @butonic did with the Lucene database but also adds the ability to search all types of search results in one pane. @jancborchardt see as well as these changes could eventually replace the AJAX search dropdown; for this to happen, I think the app would have to be included in core, since it would be a required app.

[LukasReschke]

Same for this.

[LukasReschke]

Rebase needed

[DeepDiver1975]

We closed OC5 for new features last week.
I'll close these PRs for now and once we are back on track for OC6/OC55 we can reopen them.

THX for your work

[butonic]

Hey @andrewsbrown, sorry if the comments seemed to be all negative, you did some great work here and now that 5.0 is out we can have a look at this again. Could you rebase your code on current master? Then I'll have another look and maybe we can polish this for OC6!

[abrown]

Ok, give me a few days and I will rebase...

[abrown]

Ok, so I rebased and pushed, but had to use --force.

[MorrisJobke]

@DeepDiver1975 is this a issue with the pull request or the CI?

[LukasReschke]

I see some potential XSS holes in this code. Please don't merge this until I've reviewed the code.

[DeepDiver1975]

@owncloud-bot retest this please

[MorrisJobke]

@andrewsbrown I've removed the owncloud-apps submodule, because there wasn't any URL specified in .gitmodules

[abrown]

@kabum, sounds good.

[bartv2]

@andrewsbrown i see some great things, but i think this PR is a bit to big to review properly. For example the app search providers should be in the apps themself. I'm also missing where fillFromId and formatToHtml are called. Can you do a new PR with only the provider for File, which is in core?

[abrown]

@bartv2 Give me a week or two to look at this again... I didn't really think this would ever get merged so I forgot about it. I'll re-open, though, if you're interested in it.

[MorrisJobke]

@andrewsbrown A better approach is to add many small PRs instead of one really big ... prepare your change step by step and the review will be a lot easier. ;) And don't forget to crosslink between them and mention people you think that they can review it - i.e. current maintainer of this code base or people showed up in this PR;)

[bartv2]

@andrewsbrown the search code can have some love, but in small steps or with a clear proposal. Otherwise there will be a lot of wasted time and code.

[abrown]

@bartv2 and @kabum: look, I don't know how this project is being run and it was near impossible to figure out when I first tried to use ownCloud. So I built what I wanted to build, which was an app that was listed search results from all searchable apps. To do that, I created or improved search providers for about 10 apps, integrated the Lucene search @butonic implemented, and added the ability to perform actions (edit, delete, etc.) on all types of search results. I get it, it was too many changes and no one understood the scope of the project, especially while it was still in progress (see comments above). I still don't understand who to talk to about this, but I do know that if search needs improving, OC_Search, OC_Search_Provider, and OC_Search_Result need to change.

[karlitschek]

@andrewsbrown Thanks a lot for your awesome work. I think @butonic and I are the guys to talk to about your plans. It would be great if you could explain your plans a bit more here. Beside that just go ahead but please with smaller pull requests that are easier to review as @bartv2 suggested. :-)

[abrown]

@karlitschek, @butonic: So the first thing I think I need is good search results. Right now, OC_Search_Result is a pretty bare affair and if I need search results with as much data as possible. In my implementation, each search result type (e.g. OC_Search_Result_File or OC_Search_Result_Event) has its inherited properties--name, id, etc.--but is extended with applicable properties for that type--created, modified, etc. That is one way to do that, but perhaps a more extensible way is to have a separate Column type and add column data as an object; either way, the goal is to have as much data as possible in the search result so that results can be sorted or queried further.

So, besides more data, the other thing they need are Actions. Right now I am creating those the hard way inside a formatToHtml() method; I think this definitely needs to have it's own separate class so I don't have to keep mixing in HTML with the actual code. An action class would have a title, a URL, and permissions; the provider would add Actions to each search result, e.g. an OC_Search_Result_File would have Actions like 'Download', 'Edit', 'Delete', etc.

If I had those two things, columns and actions, it would be a good start. The problem is that all of the apps are entrenched in the current search provider/result system and require significant tweaking to change. That's why I went ahead and wrote new providers and new search results for all of the apps included in the 'apps' repository; and that's why this pull request looks so massive. In other words, to make search better/more extensible/more useful in the future, I think we need to significantly modify the current search pattern. But, again, it's a lot of change.

[karlitschek]

I think this makes sense. @butonic What do you think?

[butonic]

In general I like the current solution that returns the search results as JSON. That also works when subclassing the 'abstract' or generic search result. IMO we should change OC_Search_Result into an adapter for the new \OC\Search\Result\Generic class that implements the \OC\Search\Result interface with the four currently used members $name, $text $link and $type. $type currently is documented as the type of result as human readable string ('File', 'Music', etc) I would rather use it to coose the JavaScript renderer that creates the HTML for this type of result. That way subclasses like \OC\Search\Result\Contact can set $type to contact and add an $image. On the client side the Contacts app can register a handler for this type of search results and show the contact $image in front of the $name.

@andrewsbrown could you create a PR that contains the new \OC\Search\Result, \OC\Search\Result\Generic and converts OC_Search_Result into a legacy adapter? That should give us a start for a cleaner and more flexible internal search code. I guess the fill and format methods are not yet necessary (and I prefer that kind of code on the client)?

After that we can work on the client side solution. There is already a customization approach but only the text editor, the deprecated imageviewer, bookmarks and the media app use it. grep -R OC.search.customResults * should give you the files. Instead of just customizing the result I would hand over the complete HTML generation for the search result to them. If no special renderer is found we can use the generic renderer (which dedicated renderers can use to get the basic HTML for a search result). The changes should not be that big.

One step after another, and thx for all the effort you already put into this!

[abrown]

@butonic, I understand you want to keep the HTML generation client-side and I will make the changes as soon as possible. Give me about a week since I am currently out of the country and my Internet access is minimal.

[abrown]

@butonic, I've been coding this for a bit and I ran into a few questions:

1. Should I convert all search classes to the new namespace system (e.g. OC\Search, OC\Search\Provider, etc)? That breaks a couple of things but I can add commits to fix them...
2. Can I add extra fields to stuff like OC\Search\Result\File, like modified time, created time, etc. These won't be useful for the AJAX search box but I think they will be useful later.
3. I was thinking of dropping the $type property and just using the class name to identify the type of search results. What do you think?

[butonic]

@andrewsbrown awesome!

1. I was thinking of replacing the old OC_Search etc with a wrapper to OC\Search. Have a look at https://github.com/owncloud/core/tree/master/lib/legacy for examples. This ensures backward compatibility, shouldn't it?
2. Additional fields should not be a problem. And yes, future search apps might need it.
3. I would like to see it because it cleans up the code. But I see a problem where developers might want to serialize a search result to json, only using json_encode. That would lack the $type if we remove it from the Classes. Hm, since we will be using OC\Search to render the list of results we can take care of that and add the type based on the class name. Or just use the class name ... then again the type is used to treat similar types of results in the same way. core search and search_lucene both produce type='file' results. And I can imagine different Contact/Addressbook app backends to all produce type='contact' results regardless if the search result proveider was \OCA\Contacts\Search\Provider or \OC\Users_Ldap\Search\Provider (neither of them exists with that name, but they would make sense IMHO). For now, keep it.