GNOME Bugzilla – Bug 711075
Include a gnome-shell search provider
Last modified: 2014-04-14 08:55:46 UTC
It would be pretty awesome to be able to search for a terminal from the Activities Overview. Search could match against: * user * location * currently running command * last run command Search would return terminals (identified by the prompt and currently running / last run command). Selecting a search result would switch to the relevant window/tab.
Created attachment 263685 [details] [review] screen: Allow getting the name of the current foreground process
Created attachment 263686 [details] [review] server: Add a search provider
Comment on attachment 263685 [details] [review] screen: Allow getting the name of the current foreground process if (process_name) - *process_name = name; + *process_name = g_strdup (name); Use gs_transfer_out_value() here instead.
Review of attachment 263686 [details] [review]: What's the opinion of gnome-shell developers on having g-t provide this? ::: src/gnome-terminal-search-provider.ini @@ +1,1 @@ +[Shell Search Provider] This file is missing a copyright and licence header comment. @@ +3,3 @@ +BusName=org.gnome.Terminal +ObjectPath=/org/gnome/Terminal +Version=2 No newline at end of file Fix the missing newline. ::: src/org.gnome.ShellSearchProvider2.xml @@ +1,1 @@ +<!DOCTYPE node PUBLIC This file looks like it's copypasted from somehwere else (gnome-shell)? Should build-depend on the other project instead of copying. Plus, it's not usable since it has no copyright and licence info.
Created attachment 263717 [details] [review] screen: Allow getting the name of the current foreground process Use gs_transfer_out_value as suggested.
Created attachment 263930 [details] [review] screen: Allow getting the cmdline of the current foreground process
(In reply to comment #4) > Review of attachment 263686 [details] [review]: > > What's the opinion of gnome-shell developers on having g-t provide this? I am not sure why they would have an opinion. :-) The search providers (except the one that searches for applicatins) are all offered by the respective applications which will handle the activation of a particular result. Other than that, I spoke about this to Jasper & fmuellner and we have the blessings of aday. So we should be OK. > ::: src/gnome-terminal-search-provider.ini > @@ +1,1 @@ > +[Shell Search Provider] > > This file is missing a copyright and licence header comment. > > @@ +3,3 @@ > +BusName=org.gnome.Terminal > +ObjectPath=/org/gnome/Terminal > +Version=2 > No newline at end of file > > Fix the missing newline. Fixed both. > ::: src/org.gnome.ShellSearchProvider2.xml > @@ +1,1 @@ > +<!DOCTYPE node PUBLIC > > This file looks like it's copypasted from somehwere else (gnome-shell)? Should > build-depend on the other project instead of copying. Plus, it's not usable > since it has no copyright and licence info. Yes, it is copied from gnome-shell. I will change it to pick it up from ${datadir}/dbus-1/interfaces instead of copying it.
Created attachment 263931 [details] [review] server: Add a search provider This is still a work in progress. 1) We now clear the GHashTable every time a new search is started. 2) We try to match the terms against the full cmdline of a foreground process, and not just the basename of argv[0]. 3) If there is a foreground process, we try to show show a few lines around the current cursor position while displaying the search results.
The new patch still has the xml copypasted? Need to make a configure check for that instead. Also I'd like to make this optional, so please also add a --disable-search-provider configure switch and corresponding ifdefs around the code.
Created attachment 265229 [details] [review] server: Add a search provider
(In reply to comment #9) > The new patch still has the xml copypasted? Need to make a configure check for > that instead. The XML is no longer copy pasted. > Also I'd like to make this optional, so please also add a > --disable-search-provider configure switch and corresponding ifdefs around the > code. Done.
For those willing to test it, you need the gnome-shell patch from bug 719965
Review of attachment 265229 [details] [review]: Some minor issues, plus the big problem of returning bytestring arrays as "as". ::: configure.ac @@ +95,3 @@ # **** +PKG_CHECK_VAR([DBUS_INTERFACES_DIR], [dbus-1], [interfaces_dir]) Unfortunately PKG_CHECK_VAR requires the very latest pkg-config version 0.28 (which e.g. F19 doesn't have). ::: src/Makefile.am @@ +313,3 @@ + +searchproviderdir = $(datadir)/gnome-shell/search-providers +searchprovider_DATA = gnome-terminal-search-provider.ini dist_searchprovider_DATA, which obsoletes the EXTRA_DIST below? @@ +345,3 @@ $(BUILT_SOURCES) +EXTRA_gnome_terminal_server_SOURCES = \ What's that? Why is the gnome_terminal_server_SOURCES += ... above not sufficient? Also, EXTRA only exists for primaries, not suffixes like SOURCES etc. @@ +352,2 @@ EXTRA_DIST = \ + gnome-terminal-search-provider.ini \ See above. ::: src/terminal-app.c @@ +430,3 @@ +#ifdef ENABLE_SEARCH_PROVIDER + if (!terminal_search_provider_dbus_register (app->search_provider, connection, object_path, error)) + return FALSE; Why register this on the object path of the factory, instead of e.g. /org/gnome/terminal/search-provider2/ or sth like that? ::: src/terminal-search-provider.c @@ +59,3 @@ + { + if (strstr (str, terms[i]) == NULL) + { strstr seems a bit simplistic. What about e.g. accented chars, should searching for 'e' not return results for 'é' etc? Note however that this function is used in places where @str is a bytestring, not UTF-8 text… split into 2 funcs one for bytestrings (strstr problably ok) and one for UTF-8 (to use when matching the title) ? @@ +72,3 @@ +handle_get_initial_result_set_cb (TerminalSearchProvider2 *skeleton, + GDBusMethodInvocation *invocation, + char **terms, Shouldn't that be const char * const * ? @@ +100,3 @@ + } + + results = g_ptr_array_new (); g_ptr_array_new_with_free_func (g_free), and declare add the gs_unref_ptrarray attribute to the variable declaration. @@ +120,3 @@ + + key = g_strdup_printf ("%p", screen); + g_hash_table_insert (provider->hash, g_strdup (key), screen); This is beyond ugly :-) TerminalScreen has a UUID (terminal_screen_get_uuid()), use that instead? Or just add API to TerminalApp to go from a UUID to a TerminalScreen ? @@ +130,3 @@ + terminal_search_provider2_complete_get_initial_result_set (skeleton, + invocation, + (const char *const *) results->pdata); There's a *big* problem here. This method's return type "as"; however, the information above (cmdline, cwd, process) are bytestrings, not UTF-8. @@ +135,3 @@ + + g_strfreev ((gchar **) results->pdata); + g_ptr_array_free (results, FALSE); Can remove these 2 lines as per remark above. @@ +144,3 @@ + char **previous_results, + char **terms, + gpointer user_data) Shouldn't these be const char * const * ? @@ +152,3 @@ + _terminal_debug_print (TERMINAL_DEBUG_SEARCH, "GetSubsearchResultSet started\n"); + + results = g_ptr_array_new (); new_with_free_func, use the gs_ attribute, and and lose the freeing at the end. @@ +186,3 @@ + invocation, + (const char *const *) results->pdata); + Same problem as in GetInitialResultSet above. @@ +211,3 @@ +handle_get_result_metas_cb (TerminalSearchProvider2 *skeleton, + GDBusMethodInvocation *invocation, + char **results, const char * const * ? @@ +241,3 @@ + if (terminal_screen_has_foreground_process (screen, NULL, NULL)) + text = vte_terminal_get_text (VTE_TERMINAL (screen), text_is_selected_cb, NULL, NULL); + What's the purpose of this? @@ +265,3 @@ +handle_activate_result_cb (TerminalSearchProvider2 *skeleton, + GDBusMethodInvocation *invocation, + char *identifier, const ? @@ +266,3 @@ + GDBusMethodInvocation *invocation, + char *identifier, + char **terms, const char * const * ?
Hmm actually seems I was confused, it's just comparing the bytestrings, but returning only he "%p" ID. So that seems fine, even though I'd rather use the screens' UUID as ID.
(In reply to comment #13) > Review of attachment 265229 [details] [review]: Thanks for the review. I addressed the Autotools issues. > ::: configure.ac > @@ +95,3 @@ > # **** > > +PKG_CHECK_VAR([DBUS_INTERFACES_DIR], [dbus-1], [interfaces_dir]) > > Unfortunately PKG_CHECK_VAR requires the very latest pkg-config version 0.28 > (which e.g. F19 doesn't have). Ok. I changed it to use AC_ARG_WITH and $PKG_CONFIG --variable. Since we require dconf-devel, I have assumed the presence of dbus-1.pc. > ::: src/Makefile.am > @@ +313,3 @@ > + > +searchproviderdir = $(datadir)/gnome-shell/search-providers > +searchprovider_DATA = gnome-terminal-search-provider.ini > > dist_searchprovider_DATA, which obsoletes the EXTRA_DIST below? I did it to support: $ ./autogen.sh --disable-search-provider $ make $ make distcheck Otherwise, when ENABLE_SEARCH_PROVIDER is false, gnome-terminal-search-provider.ini is not part of the generated tarball. > @@ +345,3 @@ > $(BUILT_SOURCES) > > +EXTRA_gnome_terminal_server_SOURCES = \ > > What's that? Why is the gnome_terminal_server_SOURCES += ... above not > sufficient? I did it for the same reason, but it looks like irrespective of ENABLE_SEARCH_PROVIDER the "gnome_terminal_server_SOURCES +=" is respected when generating the tarball. I have removed it now. > Also, EXTRA only exists for primaries, not suffixes like SOURCES > etc. I took it from http://git.savannah.gnu.org/cgit/parted.git/tree/libparted/Makefile.am#n47
Created attachment 265432 [details] [review] server: Add a search provider
(In reply to comment #13) > @@ +241,3 @@ > + if (terminal_screen_has_foreground_process (screen, NULL, NULL)) > + text = vte_terminal_get_text (VTE_TERMINAL (screen), > text_is_selected_cb, NULL, NULL); > + > > What's the purpose of this? If there is a foreground process in the terminal, I want to return some text around the current cursor position. This could be useful if the user has lots of terminals with text editors in them. I took it from https://wiki.gnome.org/Design/Whiteboards/TerminalBrainstorm#Allow_searching_for_open_terminals_from_the_Activities_Overview I have not looked into how vte_terminal_get_text would perform if there is a huge scrollback. The documentation does say "visible part of the terminal".
(In reply to comment #13) > Review of attachment 265229 [details] [review]: > ::: src/terminal-app.c > @@ +430,3 @@ > +#ifdef ENABLE_SEARCH_PROVIDER > + if (!terminal_search_provider_dbus_register (app->search_provider, > connection, object_path, error)) > + return FALSE; > > Why register this on the object path of the factory, instead of e.g. > /org/gnome/terminal/search-provider2/ or sth like that? I copied what Epiphany was doing. I have now moved it to /org/gnome/Terminal/SearchProvider, something which some other applications (eg., gnome-documents, gnome-control-center) are doing. > ::: src/terminal-search-provider.c > @@ +59,3 @@ > + { > + if (strstr (str, terms[i]) == NULL) > + { > > strstr seems a bit simplistic. What about e.g. accented chars, should searching > for 'e' not return results for 'é' etc? Fixed.. I punted on this because I noticed that some applications (eg., gnome-documents) are not taking care of accented characters and such, but now that you say so, I think we should fix those applications. > @@ +72,3 @@ > +handle_get_initial_result_set_cb (TerminalSearchProvider2 *skeleton, > + GDBusMethodInvocation *invocation, > + char **terms, > > Shouldn't that be const char * const * ? Yes. I had used char** because char ** needs an explicit cast to const char * const * as opposed to, say, char * to const char*. I have added the const now. > @@ +100,3 @@ > + } > + > + results = g_ptr_array_new (); > > g_ptr_array_new_with_free_func (g_free), and declare add the gs_unref_ptrarray > attribute to the variable declaration. Fixed. > @@ +120,3 @@ > + > + key = g_strdup_printf ("%p", screen); > + g_hash_table_insert (provider->hash, g_strdup (key), screen); > > This is beyond ugly :-) > > TerminalScreen has a UUID (terminal_screen_get_uuid()), use that instead? Or > just add API to TerminalApp to go from a UUID to a TerminalScreen ? Ah, yes, I missed that it already has a UUID. I will change this. > @@ +130,3 @@ > + terminal_search_provider2_complete_get_initial_result_set (skeleton, > + invocation, > + (const char > *const *) results->pdata); > > There's a *big* problem here. This method's return type "as"; however, the > information above (cmdline, cwd, process) are bytestrings, not UTF-8. terminal_screen_has_foreground_process uses g_filename_to_utf8, but I don't know if that is enough. > @@ +135,3 @@ > + > + g_strfreev ((gchar **) results->pdata); > + g_ptr_array_free (results, FALSE); > > Can remove these 2 lines as per remark above. Removed. > @@ +152,3 @@ > + _terminal_debug_print (TERMINAL_DEBUG_SEARCH, "GetSubsearchResultSet > started\n"); > + > + results = g_ptr_array_new (); > > new_with_free_func, use the gs_ attribute, and and lose the freeing at the end. Done.
Created attachment 265461 [details] [review] server: Add a search provider
Created attachment 265463 [details] [review] server: Add a search provider - Use the UUID as the hashtable key - Use dist_searchprovider_DATA and remove the entry from EXTRA_DIST
I wonder if g_str_match_string() is good enough as a match func here.
(In reply to comment #21) > I wonder if g_str_match_string() is good enough as a match func here. From the documentation: As some examples, searching for "fred" would match the potential hit "Smith, Fred" and also "Frédéric". Searching for "Fréd" would match "Frédéric" but not "Frederic" (due to the one-directional nature of accent matching). Searching "fo" would match "Foo" and "Bar Foo Baz", but not "SFO" (because no word as "fo" as a prefix). Currently, if you search for "líně" in gnome-shell gnome-control-center's search provider will match it against "Online Accounts", which seems to be the opposite of the aforementioned behaviour. ie. "líně" should not match "Online Accounts" due to the one-directional nature of accent matching and it not being a prefix. So, I guess, we can't simply use g_str_match_string as it is. Other than that, I chose the current code for consistency because it is already used by gnome-shell itself to search for applications, and gnome-control-center uses it in its own search provider.
Created attachment 266282 [details] [review] server: Add a search provider Oops, I forgot to switch tabs when a search result is selected.
Review of attachment 266282 [details] [review]: Just a few problems; with these fixed this is good to go. ::: configure.ac @@ +89,3 @@ + [AS_HELP_STRING([--with-dbus-interface-dir=PATH],[dbus interace file directory])], + [dbusinterfacedir="$withval"], + [dbusinterfacedir="$($PKG_CONFIG --variable interfaces_dir dbus-1)"]) Use [dbusinterfacedir='${datadir}/dbus-1/interfaces']) instead of pkg-config. ::: src/terminal-search-provider.c @@ +32,3 @@ + GObject parent; + + GHashTable *hash; So this hash stores UUID -> TerminalScreen ? I think that should done as API on TerminalApp instead: TerminalScreen *terminal_app_get_screen_by_uuid (TerminalApp *app, const char *uuid); (which may return NULL). @@ +212,3 @@ + const char *title; + + cwd = terminal_screen_get_current_dir (screen); This makes @cwd be in filename encoding, not UTF-8. @@ +215,3 @@ + title = terminal_screen_get_title (screen); + terminal_screen_has_foreground_process (screen, &process, &cmdline); + if (match_terms (cwd, (const char *const *) casefolded_terms) || ... but here it assumes UTF-8. Also terminal_screen_get_current_dir() has some fallback that isn't right to use here IMHO, so probably better to use vte_terminal_get_current_directory_uri() directly.
(In reply to comment #24) > This makes @cwd be in filename encoding, not UTF-8. bytestring, actually.
(In reply to comment #24) > Review of attachment 266282 [details] [review]: > > Just a few problems; with these fixed this is good to go. > > ::: configure.ac > @@ +89,3 @@ > + [AS_HELP_STRING([--with-dbus-interface-dir=PATH],[dbus interace file > directory])], > + [dbusinterfacedir="$withval"], > + [dbusinterfacedir="$($PKG_CONFIG --variable interfaces_dir dbus-1)"]) > > Use > [dbusinterfacedir='${datadir}/dbus-1/interfaces']) > instead of pkg-config. Done. > ::: src/terminal-search-provider.c > @@ +32,3 @@ > + GObject parent; > + > + GHashTable *hash; > > So this hash stores UUID -> TerminalScreen ? I think that should done as API on > TerminalApp instead: > > TerminalScreen *terminal_app_get_screen_by_uuid (TerminalApp *app, const char > *uuid); > > (which may return NULL). Done.
Created attachment 266977 [details] [review] app: screen: Add API to map UUIDs to screens
Created attachment 266978 [details] [review] server: Add a search provider
Created attachment 266979 [details] [review] server: Add a search provider
(In reply to comment #24) > Review of attachment 266282 [details] [review]: > @@ +212,3 @@ > + const char *title; > + > + cwd = terminal_screen_get_current_dir (screen); > > This makes @cwd be in filename encoding, not UTF-8. > > @@ +215,3 @@ > + title = terminal_screen_get_title (screen); > + terminal_screen_has_foreground_process (screen, &process, &cmdline); > + if (match_terms (cwd, (const char *const *) casefolded_terms) || > > ... but here it assumes UTF-8. > > Also terminal_screen_get_current_dir() has some fallback that isn't right to > use here IMHO, so probably better to use > vte_terminal_get_current_directory_uri() directly. Changed to use vte_terminal_get_current_directory_uri.
Review of attachment 263930 [details] [review]: ::: src/terminal-screen.c @@ +1982,2 @@ return FALSE; Let's add a return here if both process_name and cmdline are NULL. @@ +2002,3 @@ + data[i] = ' '; + } + Hmm. So cmdline really is a string array, with \0 as delimiter. Do we need to access the args beyond argv[0] ? If not, just return g_strdup(cmdline) ? Otherwise, make it a strv ? @@ +2004,3 @@ + + command = g_filename_to_utf8 (data, -1, NULL, NULL, NULL); + if (!command) It's not guaranteed that this will succeed... URL encode maybe?
Review of attachment 266977 [details] [review]: With these fixed, ok to commit. ::: src/terminal-app.c @@ +549,3 @@ + + uuid = terminal_screen_get_uuid (screen); + g_hash_table_remove (app->screen_map, uuid); g_assert that remove returned TRUE. ::: src/terminal-app.h @@ +83,3 @@ +void terminal_app_add_screen (TerminalApp *app, + TerminalScreen *screen); + Nit: I'd call this register/unregister instead of add/remove. @@ +86,3 @@ +TerminalScreen *terminal_app_get_screen (TerminalApp *app, + const char *uuid); + ..._get_screen_by_uuid
Review of attachment 266979 [details] [review]: ::: src/terminal-search-provider.c @@ +47,3 @@ + * Originally written by Aleksander Morgado <aleksander@gnu.org> + */ + I don't really like to put copypasted code like this, so please at least add a FIXME and file a bug so that something like it is added to glib.
(In reply to comment #32) > Review of attachment 266977 [details] [review]: > > With these fixed, ok to commit. > > ::: src/terminal-app.c > @@ +549,3 @@ > + > + uuid = terminal_screen_get_uuid (screen); > + g_hash_table_remove (app->screen_map, uuid); > > g_assert that remove returned TRUE. Done. > ::: src/terminal-app.h > @@ +83,3 @@ > +void terminal_app_add_screen (TerminalApp *app, > + TerminalScreen *screen); > + > > Nit: I'd call this register/unregister instead of add/remove. Changed. > @@ +86,3 @@ > +TerminalScreen *terminal_app_get_screen (TerminalApp *app, > + const char *uuid); > + > > ..._get_screen_by_uuid Done.
(In reply to comment #31) > Review of attachment 263930 [details] [review]: > > ::: src/terminal-screen.c > @@ +1982,2 @@ > return FALSE; > > > Let's add a return here if both process_name and cmdline are NULL. Done. > @@ +2002,3 @@ > + data[i] = ' '; > + } > + > > Hmm. So cmdline really is a string array, with \0 as delimiter. Do we need to > access the args beyond argv[0] ? If not, just return g_strdup(cmdline) ? > Otherwise, make it a strv ? We need to go beyond argv[0] (otherwise process_name would have been enough) because it helps to search based on, say the hostname, when the foreground process is ssh (eg., ssh master.gnome.org). Similarly for editors (eg., emacs -nw /path/to/a/particular-file-i-am-editing). We could still use an array of strings, if you want, but the way in which we are using this API in terminal-search-provider a string was more convenient.
(In reply to comment #33) > Review of attachment 266979 [details] [review]: > > ::: src/terminal-search-provider.c > @@ +47,3 @@ > + * Originally written by Aleksander Morgado <aleksander@gnu.org> > + */ > + > > I don't really like to put copypasted code like this, so please at least add a > FIXME and file a bug so that something like it is added to glib. It would be possible to re-implement it in terms of g_str_tokenize_and_fold. It would be a bit roundabout because we are only interested in the unaccenting logic. I filed bug 723106
(In reply to comment #31) > Review of attachment 263930 [details] [review]: > @@ +2004,3 @@ > + > + command = g_filename_to_utf8 (data, -1, NULL, NULL, NULL); > + if (!command) > > It's not guaranteed that this will succeed... URL encode maybe? I am guessing that you meant g_uri_escape_string, and not g_filename_to_uri because it will not work without absolute paths. We are using g_filename_to_utf8 higher up in the function. Maybe that should be changed too?
Created attachment 267317 [details] [review] screen: Allow getting the cmdline of the current foreground process
Created attachment 267318 [details] [review] app: screen: Add API to map UUIDs to screens
Created attachment 267319 [details] [review] server: Add a search provider
This leaves the issues from comment 35 , comment 36 and comment 37
(In reply to comment #37) > I am guessing that you meant g_uri_escape_string, and not g_filename_to_uri > because it will not work without absolute paths. > > We are using g_filename_to_utf8 higher up in the function. Maybe that should be > changed too? Not sure… We could just ignore the legacy non-UTF-8 case altogether :-) (In reply to comment #35) > > Hmm. So cmdline really is a string array, with \0 as delimiter. Do we need to > > access the args beyond argv[0] ? If not, just return g_strdup(cmdline) ? > > Otherwise, make it a strv ? > > We need to go beyond argv[0] (otherwise process_name would have been enough) > because it helps to search based on, say the hostname, when the foreground > process is ssh (eg., ssh master.gnome.org). Similarly for editors (eg., emacs > -nw /path/to/a/particular-file-i-am-editing). > > We could still use an array of strings, if you want, but the way in which we > are using this API in terminal-search-provider a string was more convenient. Ok.
(In reply to comment #42) > (In reply to comment #37) > > I am guessing that you meant g_uri_escape_string, and not g_filename_to_uri > > because it will not work without absolute paths. > > > > We are using g_filename_to_utf8 higher up in the function. Maybe that should be > > changed too? > > Not sure… We could just ignore the legacy non-UTF-8 case altogether :-) So what is left here to do? :)
From #vte on GIMPNet: <chpe> I think it's finished