Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

About search #17404

Closed
BernhardPosselt opened this issue Jul 6, 2015 · 18 comments
Closed

About search #17404

BernhardPosselt opened this issue Jul 6, 2015 · 18 comments

Comments

@BernhardPosselt
Copy link
Contributor

This PR seems reasonable: #16810 however it "breaks" apps that don't use the built in search. Reasons to not use the built in search are plenty:

  • Undocumented
  • It renders into div#searchresults which forces you to duplicate styling, events and keyboard bindings
  • Any JavaScript framework that renders the content based on JSON objects won't work (react, angular, ember, backbone, every other js framework that does MV*)
  • Search string is not reflected in the URL which does not allow you to bookmark searches or use those URLs in your app
  • It searches automatically when you type. This is problematic if searching takes a very long time or if you've optimized your app for keyboard usage (the news app focuses the content div after content was loaded in so you can automatically start to scroll down in the content area)

So my current solution is ATM to just show the input field using an important rule, which is probably fragile because the js solution renders with a higher priority by using the style attribute:

#searchbox {
    display: block !important;
}

A better approach would probably be to explicitely mark your app as supporting search in addition to the current approach, e.g.

info.xml

<search type="built-in|custom|unsupported" /> 
  • unsupported: default behavior, does not render a search box
  • built-in: renders a search box and wires up the internal js stuff
  • custom: only renders the search box

@oparoz @jancborchardt @nickvergessen @PVince81 @butonic @MorrisJobke @LukasReschke @DeepDiver1975

@DeepDiver1975
Copy link
Member

I had the honor to integrate search into mail and app management (or at least contributed) and I'm about to make it work again for contacts.

TBH: it's a nightmare and almost unusable - basically all I need it a callback as soon as the user types into the search field. All the provider and search result kung-fu is pointless - in non of these apps I do care about search results of other apps.

I vote for dropping the concept and let app developers hook up easily to the search box.

@DeepDiver1975 DeepDiver1975 added this to the 8.2-next milestone Jul 6, 2015
@oparoz
Copy link
Contributor

oparoz commented Jul 6, 2015

I want to use the search box for Gallery, but haven't looked at the details. Here is how I would expect it to work.

  • As I type requests to a backend
  • JSON results that I can use to format the data however I want to.

If that's not how it will work in 8.2, then I would appreciate having the opportunity to use the search box to add my own custom sauce, although it will probably never happen. I don't see the point in having each app dev create their own search mechanism which does 99% the same thing.

So, 👍 for letting devs pick how search should behave in their apps.

@jancborchardt
Copy link
Member

Yeah. But we should aim to make it as type-to-search as possible. Like in Files (and Mail), just reduce the list of results. Same can be done for Gallery and News. If it takes longer just show a feedback spinner.

@MorrisJobke
Copy link
Contributor

cc @libasys because you also had some thoughts about the search stuff recently.

@BernhardPosselt
Copy link
Contributor Author

@oparoz that's 2 lines of js :)

@libasys
Copy link
Contributor

libasys commented Jul 6, 2015

#17339 with this PR only the needed search provider will loaded!

@DeepDiver1975
Copy link
Member

TBH: there is no need for the search providers - generally speaking - if you search/filter with one app: who is interested in results from another app?

@libasys
Copy link
Contributor

libasys commented Jul 6, 2015

for example the music app, show the albums and titles and also where the file stays on the data dir!

@oparoz
Copy link
Contributor

oparoz commented Jul 6, 2015

TBH: there is no need for the search providers - generally speaking - if you search/filter with one app: who is interested in results from another app?

It depends. With everything being an app. How does that work with files_external? What about if someone creates custom cloud connector? How does he include the results in the list?

@oparoz that's 2 lines of js :)

Good :) now Gallery needs a special view to present the results...

@DeepDiver1975
Copy link
Member

It depends. With everything being an app. How does that work with files_external? What about if someone creates custom cloud connector? How does he include the results in the list?

file related stuff is anyhow a topic of its own - any file information is being kept in the db (file cache table) and the search is operating on the cache.

@BernhardPosselt
Copy link
Contributor Author

for example the music app, show the albums and titles and also where the file stays on the data dir!

Can also be provided by the music app, should not be a problem :)

Good :) now Gallery needs a special view to present the results...

Just render it like you render it normally. I suppose you generate it using JavaScript? Otherwise another 1 line of code with jQuery :)

@oparoz
Copy link
Contributor

oparoz commented Jul 6, 2015

Just render it like you render it normally. I suppose you generate it using JavaScript? Otherwise another 1 line of code with jQuery :)

Can't do that unfortunately, because it works with rows. Can't render one until it's full. Works when you know in advance how many thumbnails you're expecting, but not when you change that number on the fly.

@BernhardPosselt
Copy link
Contributor Author

@oparoz you probably need to change your architecture then, don't rely on eventsource for everything but just return a list of json objects and use pagination, works like a charm.

@jancborchardt how should search on keyup work if you want to focus the content area to be able to scroll down?

@jancborchardt
Copy link
Member

@jancborchardt how should search on keyup work if you want to focus the content area to be able to scroll down?

@Raydiation I don’t really get the problem. The search box is not focused by default.

@butonic
Copy link
Member

butonic commented Jul 6, 2015

I think we were on the right track with introducing "filter & find" as a general approach in files. Filter to reduce the visible content, find to add other results from the same app. In the files app that are the results from other directories. Or even full text search results from search_lucene. This serves just as example that other apps might want to add results to your app, so swallow your criticism of the state of search_lucene for later.

The search box should only be made visible by apps that implement some kind of search, nobody forces you to use the search providers. That being said it has the nice benefit of letting someone write a dashboard that queries all search providers.

Furthermore, after your first implementation of search queries you will realize that your users want to add negation (prefix with -) or metadata search (e.g. author:bob, artist:"daft punk") to the query terms. So you start implementing a query language...

Finally, you need to implement paging your json results or otherwise your app will sooner or later become a performance hog. The data in you app is only going to grow. We have yet to observe otherwise.

No, there is good reason to have a search API in core that provides a well thought out mechanism for simple queries, a query language and paging. We are not there yet 100%, but I think it makes sense to go the last mile, clean up or better get rid of deprecated cruft and provide a clean restful API. The elasticsearch API and send search lucenes query language are a good starting point.

@libasys
Copy link
Contributor

libasys commented Jul 7, 2015

@butonic please review this pr: #17339

@DeepDiver1975 DeepDiver1975 modified the milestones: backlog, 8.2-current Sep 21, 2015
@PVince81
Copy link
Contributor

PVince81 commented Oct 7, 2016

the PR was merged a long time ago.

I guess there is still more work to be done so keeping open ?

@lock
Copy link

lock bot commented Aug 3, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants