GNOME Bugzilla – Bug 694943
Add a shell search provider
Last modified: 2014-09-28 22:03:56 UTC
The gnome-shell design calls for epiphany to provide results in the overview, from the history and bookmarks. I know we're in feature freeze for 3.8, but I got bored last night and implemented it. It uses EphyCompletionModel, which might or might not be the best choice: it makes the search provider stateful, which works right now, but might break in the future; also, I had to move it from src/ to lib/, together with the whole libephybookmarks, which messes the patch a bit. PS: favicons don't work, they always come NULL from the model.
Created attachment 237725 [details] [review] Add a shell search provider The search provider offers the same results as the inline completion in the location bar, gathering results from the history and from bookmarks. Also, it provides a special "Search the web" item, that continues search with the configured search engine (Google by default)
*** Bug 692549 has been marked as a duplicate of this bug. ***
I wonder if it would not be better to just add a "headless" mode for Epiphany and make the search provider be that, we can even probably reuse the mode we use for testing. Would allow us to not move stuff into lib/ just to have it in the search provider binary, and (I think) it makes sense to just use the same code paths all the time. Other than that this looks awesome, thanks for doing it!
Created attachment 252117 [details] [review] EphyShell: add support for starting without windows To launch epiphany as a DBus service, we need to be able to launch without opening windows, and then use the GApplication inactivity timeout to manage our lifetime.
Created attachment 252118 [details] [review] Add a shell search provider The search provider offers the same results as the inline completion in the location bar, gathering results from the history and from bookmarks. Also, it provides a special "Search the web" item, that continues search with the configured search engine (Google by default)
Review of attachment 252117 [details] [review]: ::: tests/Makefile.am @@ +100,3 @@ -I$(top_srcdir)/src/bookmarks +CPPFLAGS = \ How is this related? If there's anything to fix here please file a separate bug.
Review of attachment 252118 [details] [review]: I just want to say that this looks awesome. I need a bit of time to review the patch and test it, but will try to make sure we get it in in this cycle.
Review of attachment 252117 [details] [review]: Just remove the CPPFLAGS stuff from this patch, otherwise looks good. ::: data/org.gnome.Epiphany.service.in @@ -1,3 @@ [D-BUS Service] Name=org.gnome.Epiphany -Exec=@bindir@/epiphany I wonder what was the use of this file in the first place.
Review of attachment 252118 [details] [review]: Looks good in general. Please fix what's commented, and commit afterwards. Thanks a lot for this! ::: src/ephy-search-provider.c @@ +35,3 @@ +#include "ephy-profile-utils.h" +#include "ephy-search-provider.h" +#include "ephy-shell.h" Check HACKING to see how to sort the included headers. @@ +56,3 @@ + +static void +on_model_updated(EphyHistoryService *service, Nit: space before the opening parenthesis. @@ +196,3 @@ + "id", g_variant_new_string ("special:search")); + g_variant_builder_add (&builder, "{sv}", + "name", g_variant_new_string (_("Search the Web"))); Maybe 'Search the Web for %s' would be more informative? (this needs to be fixed in ephy as well). @@ +246,3 @@ + + g_bytes_unref (favicon_bytes); + g_object_unref (favicon); May I suggest splitting the construction of the favicon variant into a new method? I am not particularly fond of large functions. @@ +304,3 @@ +#else + WebKitNetworkRequest *request = webkit_network_request_new (uri); +#endif We have removed support for WK1, so you can safely assume WK2 only. @@ +308,3 @@ + ephy_shell_new_tab_full (ephy_shell_get_default (), NULL, NULL, request, + EPHY_NEW_TAB_IN_NEW_WINDOW | EPHY_NEW_TAB_OPEN_PAGE, + EPHY_WEB_VIEW_CHROME_ALL, FALSE, timestamp); I think we should follow user preferences here and not just open always in a new window. ephy_shell_open_uris() will probably do that. @@ +323,3 @@ + g_free (url_search); + + url_search = g_strdup (_("http://www.google.com/search?q=%s&ie=UTF-8&oe=UTF-8")); Please use DuckDuckGo here. The localizable string is available in data/default-bookmarks.rdf.in, you can reuse that. ::: src/ephy-search-provider.h @@ +23,3 @@ +#include <glib-object.h> +#include <gio/gio.h> +#include "ephy-shell-search-provider-generated.h" Please follow HACKING to decide the order of headers. @@ +48,3 @@ + (G_TYPE_INSTANCE_GET_CLASS ((obj), \ + EPHY_TYPE_SEARCH_PROVIDER, EphySearchProviderClass)) + We usually write each of these macros in their one line. Please check other headers.
Attachment 252117 [details] pushed as 77df45f - EphyShell: add support for starting without windows Attachment 252118 [details] pushed as f8f38cb - Add a shell search provider
Hi there :), This is a cool thing! YAY. But i have an question about it. Whty does it search by default in google not in the new default search enginge of epiphany duckduckgo.com? I realy enjoy your feture :).
I don't think it uses Google. Why do you say so?
I Looked into the patch and read the follow comment: <Comment in Attachment 252118 [details]> The search provider offers the same results as the inline completion in the location bar, gathering results from the history and from bookmarks. Also, it provides a special "Search the web" item, that continues search with the configured search engine (Google by default) </Comment in Attachment 252118 [details]> and i see the follow Code: <Code in Attachment 252118 [details]> + if (url_search == NULL || url_search[0] == '\0') { + g_free (url_search); + + url_search = g_strdup (_("http://www.google.com/search?q=%s&ie=UTF-8&oe=UTF-8")); + } </Code in Attachment 252118 [details]> So its looks to me like google is the default not duckduckgo :).
(In reply to comment #13) > I Looked into the patch and read the follow comment: > <Comment in Attachment 252118 [details]> > The search provider offers the same results as the inline completion > in the location bar, gathering results from the history and from > bookmarks. Also, it provides a special "Search the web" item, that > continues search with the configured search engine (Google by default) > </Comment in Attachment 252118 [details]> Ops, yes, the commit message was wrong. > and i see the follow Code: > > <Code in Attachment 252118 [details]> > + if (url_search == NULL || url_search[0] == '\0') { > + g_free (url_search); > + > + url_search = g_strdup > (_("http://www.google.com/search?q=%s&ie=UTF-8&oe=UTF-8")); > + } > </Code in Attachment 252118 [details]> > > So its looks to me like google is the default not duckduckgo :). It was fixed before I committed, if you look at the code in git it uses DDG.
https://git.gnome.org/browse/epiphany/tree/src/ephy-search-provider.c?#n290
FWIW that is only used if the search engine setting has been cleared out.