GNOME Bugzilla – Bug 753470
Add a search provider
Last modified: 2018-05-22 13:04:34 UTC
As the search feature has been improved significantly, it would be good to add a search provider. Speed could be a problem, especially for rotating storage, but getting back the first result should be pretty quick.
Created attachment 333472 [details] [review] Add initial implementation of search provider All the files needed for search provider to function are created in this commit.We implement the following three methods of the shell search provider interface in this commit: * handle-get-initial-result-set * handle-get-subsearch-result-set * handle-get-result-metas The user entered search text in gnome shell overview is searched in all the available journal fields of each journal entry.In the search provider results we show the journal entry message as the title of each result and the process name as a description of each result.We use the 'text-x-generic' icon for the search results.
Created attachment 333473 [details] [review] Implement handle-activate-result method We implement the "handle-activate-result" shell search provider interface method in this commit. In the gnome overview search provider results, if we click on a search result, a new window opens which shows the details of the selected result.
Created attachment 333474 [details] [review] Implement handle-launch-search method We implement the handle-launch-search method of the shell search provider interface in this commit. When the user clicks on the GNOME Logs icon besides the search provider results, a new window will open showing all the results for the user entered search term in GNOME Shell Overview search bar. The search results in the window are shown from "All" category so that user can see all possible results for his search term.
Review of attachment 333472 [details] [review]: ::: Makefile.am @@ +19,3 @@ + --generate-c-code src/gl-search-provider-generated \ + $(top_srcdir)/data/shell-search-provider-dbus-interfaces.xml \ + $(NULL) There is no need for the NULL. Additionally, the indentation should be with spaces. ::: data/gnome-logs-search-provider.ini @@ +1,1 @@ +[Shell Search Provider] This file needs to be installed. ::: src/gl-journal-model.c @@ +135,3 @@ +GPtrArray * +gl_journal_model_get_hits (GlJournalModel *model) This does not need a new API. You should be using the same interface as the event view list to retrieve entries from the model ::: src/gl-search-provider.c @@ +76,3 @@ + GlQuery *query; + + // Clear the earlier searches No C99 comments. @@ +84,3 @@ + search_text = g_strjoinv (" ", terms); + + search_provider->model = gl_journal_model_new (); Creating a new journal model for each search will become expensive. Why can you not set a new query on the existing model? @@ +147,3 @@ + const gchar *process_name; + GHashTable *metas_cache; + GVariantBuilder meta; Move the variables that are only used inside the for loop there to reduce their scope. @@ +160,3 @@ + result_icon = g_themed_icon_new_with_default_fallbacks ("text-x-generic"); + + for (idx = 0; idx < search_provider->hits->len; idx++) len (in GPtrArray) is a guint, not a gint. Also, just use 'i'.
Review of attachment 333473 [details] [review]: ::: src/gl-application.h @@ +37,2 @@ GtkApplication * gl_application_new (void); +void gl_application_open_detail_entry (GApplication *self, GlJournalEntry *entry); Should be a GlApplication, not a GApplication. ::: src/gl-window.c @@ +387,3 @@ + "view-mode"); + /* Switch to detail view */ + g_action_change_state (view_mode, g_variant_new_string ("detail")); The two parallel ways of setting a detailed entry view is pretty ugly. You should find a way to combine the two methods.
Review of attachment 333474 [details] [review]: ::: src/gl-application.h @@ +37,3 @@ GtkApplication * gl_application_new (void); void gl_application_open_detail_entry (GApplication *self, GlJournalEntry *entry); +void gl_application_search (GApplication *self, const gchar *text); Should be a GlApplication, not a GApplication. ::: src/gl-eventviewlist.c @@ +1022,3 @@ + /* Set text on search entry */ + gtk_entry_set_text (GTK_ENTRY (priv->search_entry), text); How does this affect the existing search behvaiour? ::: src/gl-eventviewlist.h @@ +32,2 @@ GtkWidget * gl_event_view_list_new (void); +void gl_event_view_list_search (GlEventViewList *view, const gchar *text); Do not change argument names unless there is a good reason to do so.
As we are changing the GlEventView type from GtkStack to GtkPopover in https://bugzilla.gnome.org/show_bug.cgi?id=709294, any suggestions on how handle-activate-result signal callback in GlShellSearchProvider should present the detailed event information to the user ?
(In reply to Pranav Ganorkar from comment #7) > As we are changing the GlEventView type from GtkStack to GtkPopover in > https://bugzilla.gnome.org/show_bug.cgi?id=709294, any suggestions on how > handle-activate-result signal callback in GlShellSearchProvider should > present the detailed event information to the user ? Is it possible to emit "row-activated" signal on GlEventViewList in handle-activate-result signal callback? If it is, this might be the solution.
Created attachment 355291 [details] [review] Add initial implementation of shell search provider * Use the same instance of GlJournalModel for every search invocation. * "Cursor" journal field used as the result-id for the search results.
Created attachment 355620 [details] [review] Implement handle-launch-search method The "handle-launch-search" method of the shell search provider interface is implemented in this commit. When the user clicks on the GNOME Logs icon besides the search provider results, a new window opens showing all the results for the user entered search term in GNOME Shell Overview. The search results in the window are shown from "All" category, by default, so that user can see all possible results for his search term.
(In reply to Jonathan Kang from comment #8) > (In reply to Pranav Ganorkar from comment #7) > > As we are changing the GlEventView type from GtkStack to GtkPopover in > > https://bugzilla.gnome.org/show_bug.cgi?id=709294, any suggestions on how > > handle-activate-result signal callback in GlShellSearchProvider should > > present the detailed event information to the user ? > > Is it possible to emit "row-activated" signal on GlEventViewList in > handle-activate-result signal callback? If it is, this might be the solution. It is possible. As we return the "__CURSOR" journal field to handle-activate-result signal as the unique result-id for the activated journal entry, we maybe able to perform an exact search for the GtkListBoxRow matching the activated journal entry's "__CURSOR" journal field and further emit the "row-activated" signal on this GtkListBoxRow. We can even hide the GlCategory list as we need to show the details of just a single entry. Though, for doing this, we will need to add explicit support to GlQuery for performing the exact search by "__CURSOR" journal field as it is not provided by sd_journal_add_match() (The reason being "__CURSOR" journal field is not a Trusted field). But we use it as result-id in GlShellSearchProvider as it uniquely defines the position of a journal entry in the journal. I would try implementing this and let you know the results.
Created attachment 355876 [details] [review] Add initial implementation of shell search provider * Fixed search invocation crashes * Show logs from current boot * Handle messages containing multiple lines * Show process and time in description of shell search provider results * Use a integer counter as result id instead of the "cursor" journal field
Created attachment 355877 [details] [review] Implement handle-launch-search method * Bring the newly opened window always to the front using gtk_window_present_with_time()
Created attachment 355878 [details] [review] Implement handle-activate-result method The "handle-activate-result" shell search provider interface method is implemented in this commit. In the GNOME Shell overview search provider results, if a search result is clicked , a new window opens which displays only that entry in the events list with a detailed GtkPopover pointing to that result in the events list.
Created attachment 357186 [details] [review] Add initial implementation of shell search provider * Add "gnome-logs-search-provider.ini" to Makefile.am * Rebased on current master
Created attachment 357187 [details] [review] Implement handle-launch-search method * Reuse existing window, if present, to show all the search results.
Created attachment 357188 [details] [review] Implement handle-activate-result method * Hide event-toolbar and category-list in the detailed window * Rebase on current master
Review of attachment 357186 [details] [review]: This patch doesn't work for me. After installing it, restarting gnome-shell and search in the overview, I got the following error: > gnome-shell[1381]: JS LOG: Received error from DBus search provider org.gnome.Logs.desktop: Gio.DBusError: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface 'org.gnome.Shell.SearchProvider2' on object at path /org/gnome/Logs/SearchProvider ::: src/gl-shell-search-provider.h @@ +23,3 @@ +#include <gio/gio.h> + +#include <gl-journal-model.h> It should be: #include "gl-journal-model.h" instead.
(In reply to Jonathan Kang from comment #18) > Review of attachment 357186 [details] [review] [review]: > > This patch doesn't work for me. After installing it, restarting gnome-shell > and search in the overview, I got the following error: > > > gnome-shell[1381]: JS LOG: Received error from DBus search provider org.gnome.Logs.desktop: Gio.DBusError: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface 'org.gnome.Shell.SearchProvider2' on object at path /org/gnome/Logs/SearchProvider > Seems some kind of error during install. Can you check the following things: 1) 'gnome-logs-search-provider.ini' exists at /usr/local/share/gnome-shell/search-providers 2) 'org.gnome.Logs.service' exists at /usr/local/share/dbus-1/services 3) Whether the Logs D-Bus service is running or not using 'ps aux | grep "gnome-logs"'. It should list '/usr/local/bin/gnome-logs --gapplication-service' as the running process. 4) Logs search provider is turned on from the system settings.
(In reply to Pranav Ganorkar from comment #19) > (In reply to Jonathan Kang from comment #18) > > Review of attachment 357186 [details] [review] [review] [review]: > > > > This patch doesn't work for me. After installing it, restarting gnome-shell > > and search in the overview, I got the following error: > > > > > gnome-shell[1381]: JS LOG: Received error from DBus search provider org.gnome.Logs.desktop: Gio.DBusError: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface 'org.gnome.Shell.SearchProvider2' on object at path /org/gnome/Logs/SearchProvider > > > > Seems some kind of error during install. Can you check the following things: > > 1) 'gnome-logs-search-provider.ini' exists at > /usr/local/share/gnome-shell/search-providers > > 2) 'org.gnome.Logs.service' exists at /usr/local/share/dbus-1/services > > 3) Whether the Logs D-Bus service is running or not using 'ps aux | grep > "gnome-logs"'. > > It should list '/usr/local/bin/gnome-logs --gapplication-service' as the > running process. > > 4) Logs search provider is turned on from the system settings. Ah. It works right now somehow. :-) I'll get these patches reviewed sooooooon.
Review of attachment 357186 [details] [review]: Looks good. One thing to mention is that the latest search result is in the last place, maybe you should reverse the order.
Review of attachment 357187 [details] [review]: Apart from David'd previous review, it looks good to me.
Review of attachment 357188 [details] [review]: Apart from David'd previous review and the following comment, it looks good to me. ::: src/gl-application.h @@ +22,2 @@ #include <gtk/gtk.h> +#include <gl-journal.h> gl-journal.h is a local header, so it should be #include "gl-journal.h"
Created attachment 357753 [details] [review] Add initial implementation of shell search provider * Show latest logs first
Created attachment 357755 [details] [review] Implement handle-launch-search method * Rebase on new changes in the previous patch
Created attachment 357757 [details] [review] Implement handle-activate-result method * Use double quote for local header file in gl-application.h * Rebase on changes in previous patch
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-logs/issues/8.