GNOME Bugzilla – Bug 745508
Two fixes to the search provider
Last modified: 2015-10-09 21:34:38 UTC
Neat app! I love it. But I found two small bugs in casual testing.
Created attachment 298396 [details] [review] Fix the code point of characters outside the BMP Strings in JS are in UTF-16, so we need to resolve surrogate pairs into full UTF-32 code points. We can't use glib functions to do so because gunichar is translated back into a String by gjs.
Created attachment 298397 [details] [review] Default the search provider to enabled I would say this was copy-pasted from gnome-weather, where default disabled is motivated by privacy concerns, but that doesn't apply here.
Created attachment 298400 [details] [review] SearchProvider: use proper startup notification for launching search This code was copied from gnome-weather, which explicitly tries to avoid Gdk/Gtk+ in the background service (so that it could be one day launched as a user service from systemd). But gnome-characters initializes Gtk+ anyway to handle the clipboard, there is no reason for degraded operation. In particular, this gives a nice spinner while gnome-characters starts up (which is almost invisible, but it can become visible on a slow disk and cold cache)
Created attachment 298401 [details] [review] SearchProvider: fix unbalanced hold() Each query call holds the app alive until the query done, so make sure that is matched with a release(), otherwise the app never dies.
Created attachment 298402 [details] [review] BackgroundService: don't unnecessarily override shutdown GApplication complains if shutdown does not chain up. But since we're doing nothing, we can just as well avoid the chain up.
Created attachment 298403 [details] [review] BackgroundService: reduce the inactivity timeout to 30s gnome-characters does pretty much nothing during initialization: its startup code is paging in the Unicode tables, so the tradeoff is purely between disk access and memory usage. By reducing the inactivity timeout we release memory more often and let the kernel manage disk cache more effectively. There should be no measurable CPU impact by this patch, because the service should be sleeping most of the time anyway.
Review of attachment 298396 [details] [review]: Oops - good point, thanks.
Review of attachment 298397 [details] [review]: I left it disabled by default because the character search might not be very useful for normal users, but if there is no harm, it's fine to enable it.
Review of attachment 298400 [details] [review]: Sure, nice.
Review of attachment 298401 [details] [review]: Oops, good catch.
Review of attachment 298402 [details] [review]: Sure.
Review of attachment 298403 [details] [review]: Sure, thanks for the detailed explanation that sounds reasonable.
Attachment 298396 [details] pushed as edd7c95 - Fix the code point of characters outside the BMP Attachment 298397 [details] pushed as 9b1f2bc - Default the search provider to enabled Attachment 298400 [details] pushed as 2d2752b - SearchProvider: use proper startup notification for launching search Attachment 298401 [details] pushed as 032721b - SearchProvider: fix unbalanced hold() Attachment 298402 [details] pushed as b034bd0 - BackgroundService: don't unnecessarily override shutdown Attachment 298403 [details] pushed as ed45449 - BackgroundService: reduce the inactivity timeout to 30s
(In reply to Daiki Ueno from comment #8) > Review of attachment 298397 [details] [review] [review]: > > I left it disabled by default because the character search might not be very > useful for normal users, but if there is no harm, it's fine to enable it. Note -- I've reverted this due to complaints from Fedora beta testers. I agree with Daiki, this is not really as generally useful as other search providers.
Too bad, I find that Characters is the search provider I use the second most (after nautilus), to insert emojis, symbols or other characters. Well, I can enable it, not a big deal :)
I would also say: if it sorted at the bottom of the list, instead of the top (requires a bit of work in gnome-shell and gsettings-desktop-schemas), and matched only prefixes of words in the character name rather than any substring (currently it gives marginally-relevant results), then I think it would be fine to enable it again.