GNOME Bugzilla – Bug 712345
Complete implementation of shell search provider
Last modified: 2014-10-24 20:49:47 UTC
Here's a basic search provider for hotssh. I haven't added a service file or figured out how to make hotssh launch in the background - it just works if hotssh is already running - this is partly laziness, and partly that it seems a little strange to me that if hotssh is simply installed on the system it will start doing stuff when you search in the shell.
Created attachment 259848 [details] [review] Add infrastructure for a GNOME Shell search provider Add a stubbed-out GNOME Shell search provider that currently just matches for foo.example.com and does nothing when activated. The XML for the search provider interface is imported to avoid creating a build-dependency on GNOME Shell.
Created attachment 259849 [details] [review] Hook GNOME Shell search provider up to tab hostnames Search windows and tabs for a tab with a hostname that matches the search string, and switch to that tab and window when the search result is activated. Currently, when multiple tabs match, only one search result is shown and a random tab is selected; it's probably best to switch to the last-active tab for that hostname rather than a random one.
Review of attachment 259848 [details] [review]: Will review in combination with the second patch.
Review of attachment 259849 [details] [review]: Cool, this looks quite good! Just two minor things. First, you forgot to attach the hotssh-search-provider.ini file, but I just adapted one from gnome-documents. ::: src/hotssh-search-provider.c @@ +174,1 @@ + icon = g_themed_icon_new ("gnome-terminal"); Should be "hotssh" I think. I fixed this locally.
(In reply to comment #4) > Review of attachment 259849 [details] [review]: > > Cool, this looks quite good! Just two minor things. First, you forgot to > attach the hotssh-search-provider.ini file, but I just adapted one from > gnome-documents. Sorry about that - yeah, I just adapted that same file as well === [Shell Search Provider] DesktopId=hotssh.desktop BusName=org.gnome.HotSsh.SearchProvider ObjectPath=/org/gnome/hotssh/SearchProvider Version=2 === > ::: src/hotssh-search-provider.c > @@ +174,1 @@ > + icon = g_themed_icon_new ("gnome-terminal"); > > Should be "hotssh" I think. I fixed this locally. I think it's weird to have the same icon for the application for the search results. Ideally we'd have some sort of recognizable server specific icon, or ?perhaps? a thumbnail of the open session, but leaving that aside, it seems that the icon should represent a server. What do you think about using network-server (/usr/share/icons/gnome/256x256/places/network-server.png)
(In reply to comment #5) > > I think it's weird to have the same icon for the application for the search > results. Ah; I was unsure of the design. > Ideally we'd have some sort of recognizable server specific icon, or > ?perhaps? a thumbnail of the open session, but leaving that aside, it seems > that the icon should represent a server. What do you think about using > network-server > (/usr/share/icons/gnome/256x256/places/network-server.png) That sounds fine by me.
Created attachment 260150 [details] [review] Use network-server icon for GNOME Shell search results With the GNOME Shell UI, it doesn't seem right to use the same icon for both the application and the search results. Use the network-server icon for servers.
Attachment 260150 [details] pushed as 51afa40 - Use network-server icon for GNOME Shell search results
Okay, so I admit to not testing this really beyond "jhbuild make" and reading the code for gnome-documents. Specifically since I was running it in jhbuild (on RHEL7 fwiw), I didn't actually test it performing searching, since I couldn't be bothered to figure out how to convince my system gnome-shell to pick up search providers/dbus from my jhbuild shell. Now that I try hotssh in Continuous, I get exceptions since we're not installing a dbus .service file. We'll need to implement a mode like gnome-documents does where our binary can start headless (just to return no results in this case). Of course if we're only searching *active* tabs the whole machinery is just burning power when the app isn't running =( But maybe we'll change the design later to search history and not just active connections.
(In reply to comment #9) > Okay, so I admit to not testing this really beyond "jhbuild make" and reading > the code for gnome-documents. > > Specifically since I was running it in jhbuild (on RHEL7 fwiw), I didn't > actually test it performing searching, since I couldn't be bothered to figure > out how to convince my system gnome-shell to pick up search providers/dbus from > my jhbuild shell. > Now that I try hotssh in Continuous, I get exceptions since we're not > installing a dbus .service file. Hmm, I didn't have any problems, but I was actually testing with 3.8 not 3.10. There might have been change since 3.8 that cause tracebacks. > We'll need to implement a mode like gnome-documents does where our binary can > start headless (just to return no results in this case). Of course if we're > only searching *active* tabs the whole machinery is just burning power when the > app isn't running =( > > But maybe we'll change the design later to search history and not just active > connections. My main concern is conceptual - that if searching in the overview requires activating every application that you have installed, it's not going to be responsive - but that's been a problem with the DBus application-based search design since day 1. There's nothing about hotssh that makes it worse than with other apps - and since it's small and written in C, it's probably less of an issue than with many applications. If you've seen tracebacks, I can look into adding the activation and headless mode. Should it time out or keep running in the background until the session terminates once search has been triggered once? Timing out doesn't really make sense to me, since once somebody has searched once, they are likely to search again.
(In reply to comment #10) > > Hmm, I didn't have any problems, but I was actually testing with 3.8 not 3.10. > There might have been change since 3.8 that cause tracebacks. Ah, no it's quite simple I think - the shell only throws an error if hotssh is *not* running, since bus activation fails. Probably you were testing searching with it running. > My main concern is conceptual - that if searching in the overview requires > activating every application that you have installed, it's not going to be > responsive - but that's been a problem with the DBus application-based search > design since day 1. Right. Maybe if it becomes a problem down the line we can only search recently used apps or something... > There's nothing about hotssh that makes it worse than with > other apps - and since it's small and written in C, it's probably less of an > issue than with many applications. Yeah, I have no objections; it's a nice feature I think. > If you've seen tracebacks, I can look into adding the activation and headless > mode. Should it time out or keep running in the background until the session > terminates once search has been triggered once? Timing out doesn't really make > sense to me, since once somebody has searched once, they are likely to search > again. It looks like gnome-documents sets: inactivity_timeout: 12000 as a property of GApplication.
(In reply to comment #11) > (In reply to comment #10) > > > > Hmm, I didn't have any problems, but I was actually testing with 3.8 not 3.10. > > There might have been change since 3.8 that cause tracebacks. > > Ah, no it's quite simple I think - the shell only throws an error if hotssh is > *not* running, since bus activation fails. Probably you were testing searching > with it running. Pretty sure I tested specifically for this. Still don't see errors trying again. Will look at home where I have a 3.10 install. [...] > > If you've seen tracebacks, I can look into adding the activation and headless > > mode. Should it time out or keep running in the background until the session > > terminates once search has been triggered once? Timing out doesn't really make > > sense to me, since once somebody has searched once, they are likely to search > > again. > > It looks like gnome-documents sets: inactivity_timeout: 12000 as a property of > GApplication. I was going to just copy this to avoid thinking about it too hard, but 12000 is a weird value. 12 seconds? Why 12 seconds? Is it meant to be 120000 - 2 minutes - 1200000 - 20 minutes? - Owen
Created attachment 262893 [details] [review] Add DBus activation as a search provider Apparently in some cases GNOME Shell will traceback if a search provider is not activatable. Provide a DBus service file for ourselves and a --no-default-window command line option to support that. Activating when we're not running may eventually be useful for searching across history. Consistently use org.gnome.hotssh as a namespace, not org.gnome.HotSsh.
Review of attachment 262893 [details] [review]: One comment, feel free to commit with or without addressing. ::: src/hotssh-search-provider.c @@ +138,3 @@ + */ + g_application_hold (G_APPLICATION (priv->app)); + g_application_release (G_APPLICATION (priv->app)); So I get that this doesn't actually matter in terms of implementation currently, but it'd make me feel better if we did the _release just before we return. You could imagine a future where this does matter; maybe we plumb the GApplication bits down so the OS can garbage-collect idle applications. If that somehow worked, we wouldn't want to be GC'd while we're processing the search results. @@ +143,2 @@ hot_ssh_search_shell_search_provider2_complete_get_initial_result_set (skeleton, invocation, (const char * const *)results); ...here.
Attachment 262893 [details] pushed as 399b185 - Add DBus activation as a search provider