After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 694943 - Add a shell search provider
Add a shell search provider
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
: 692549 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-03-01 16:56 UTC by Giovanni Campagna
Modified: 2014-09-28 22:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a shell search provider (56.46 KB, patch)
2013-03-01 16:56 UTC, Giovanni Campagna
none Details | Review
EphyShell: add support for starting without windows (4.78 KB, patch)
2013-08-18 14:00 UTC, Giovanni Campagna
committed Details | Review
Add a shell search provider (29.51 KB, patch)
2013-08-18 14:00 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2013-03-01 16:56:09 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.
Comment 1 Giovanni Campagna 2013-03-01 16:56:37 UTC
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)
Comment 2 Claudio Saavedra 2013-03-01 17:28:49 UTC
*** Bug 692549 has been marked as a duplicate of this bug. ***
Comment 3 Xan Lopez 2013-03-06 12:12:01 UTC
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!
Comment 4 Giovanni Campagna 2013-08-18 14:00:38 UTC
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.
Comment 5 Giovanni Campagna 2013-08-18 14:00:43 UTC
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)
Comment 6 Claudio Saavedra 2013-08-19 08:10:41 UTC
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.
Comment 7 Claudio Saavedra 2013-08-19 08:11:57 UTC
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.
Comment 8 Claudio Saavedra 2013-08-29 13:59:45 UTC
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.
Comment 9 Claudio Saavedra 2013-08-29 14:35:36 UTC
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.
Comment 10 Giovanni Campagna 2013-08-29 15:44:18 UTC
Attachment 252117 [details] pushed as 77df45f - EphyShell: add support for starting without windows
Attachment 252118 [details] pushed as f8f38cb - Add a shell search provider
Comment 11 chris 2013-08-30 07:55:02 UTC
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 :).
Comment 12 Claudio Saavedra 2013-08-30 07:58:03 UTC
I don't think it uses Google. Why do you say so?
Comment 13 chris 2013-08-30 14:50:55 UTC
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 :).
Comment 14 Giovanni Campagna 2013-08-30 15:03:06 UTC
(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.
Comment 16 Claudio Saavedra 2013-08-30 15:05:30 UTC
FWIW that is only used if the search engine setting has been cleared out.