-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Completion for search terms #6049
Comments
@samyak-jain I don't really have a clear plan on how to go about this. Maybe @rcorre can say more, but my gut feeling would be that it'd go something like this:
Let me note again this is quite at the bottom of my priorities at the moment, so any review/help will likely take a lot of time - and I still reserve the right to reject PRs if I feel like this isn't worth the incurred complexity (or other costs). Not trying to discourage you if you want to move forward with this, just making clear what to expect. 🙂 @andrewcarlotti That seems mostly unrelated. The sqlite completion already does lazy loading - the problem is that there doesn't seem to be a good way to query it asynchronously, and it's very unclear (both from Qt's and sqlite's side) what's allowed to be in a separate (non-main) thread and what isn't. There might be variety of other solutions in that direction as well (see e.g. #1099), and finally there's #3989 of course. 😉 |
@The-Compiler Thanks for looking into this! Regarding #5537, I would gladly work on that issue! Example of how duckduckgo's opensearch xml looks like
Other search engines are similar. Regarding requests, I understand if this is not a priority, you can review this issue at your own pace. Thanks! |
Great! Could you please leave a comment over there? Then I can assign the issue to you (GitHub only shows commenters for assignees). Let's perhaps also wait for an answer from @rcorre since he mentioned he wants to pick it up, so I'm not sure if he already started work on it at some point.
Ah, perfect! One thing I don't understand yet: How to find the opensearch file from a website? I thought there would be something like a
Exactly! It does the request in Qt's main loop asynchronously and then triggers the |
Regarding how to get the opensearch file from the website, there doesn't seem to be a perfect solution to this unless I'm missing something. According to the OpenSearch spec, the website is supposed to have the following inside the
The problem is not all search engines seem to comply with this. Notable google doesn't. Duckduckgo however does. It looks like the approach most browsers are taking here is:
I think implementing these 3 approaches should be enough. Regarding xml parsing, your concerns are fair. The official python docs seem to recommend https://pypi.org/project/defusedxml/. Not sure why this is not figured out in the stdlib, smh. |
Qt Core appears to have an XML parser: https://doc.qt.io/qt-5/qxmlstreamreader.html. I couldn't easily find if it suffers from the same vulnerabilities as the stdlib. I'm also not sure how scared we should be of such vulnerabilities. Based on https://docs.python.org/3/library/xml.html#xml-vulnerabilities, the stdlib is safe against DTD/external entity expansion, which I think are the scary ones on a local machine. Billion laughs/exponential blowup are scary for a server, but probably just annoying for a local machine. If we're fetching XML from a limited set of trusted sources, the only risk comes from an attacker either taking over the trusted domain or redirecting you from it (e.g. via DNS poisoning), and an attacker with that capability can do far worse things through your browser than burn some CPU :) |
Agreed! But let's take this one over to #717 to keep things organized a bit 🙂 |
Another idea from IRC: Simply saving and suggesting prior search terms after specifying a search engine in As for the completions, over in #6066, @leo848 suggested looking at SearX completion implementations, which might be a good starting point for the most common engines. |
Splitting this off from #32 as there's quite some discussion in there about this - cc @rcorre, @samyak-jain, @andrewcarlotti.
@samyak-jain answered to my comment:
and @andrewcarlotti said:
The text was updated successfully, but these errors were encountered: