Created attachment 259212 [details] [review]
app-system: Put back support for the installed-changed signal

Use the new GAppInfoMonitor that Ryan added to glib to know when the
set of apps has changed.

Created attachment 259213 [details] [review]
app-system: Add back StartupWMClass matching

While unfortunate that we still have to scan all apps with get_all(),
support for this feature will be short-lived, so hopefully we can drop
it in the future as new apps adapt to the desktop file / app ID
recommendations.

For now, simply scan all desktop IDs.

Created attachment 259214 [details] [review]
appDisplay: Use the desktop file index for app searching

Rather than scanning all apps for searching, use Ryan's new desktop
file index and the glib support APIs for app searching instead of our
own system.

Review of attachment 259214 [details] [review]:

::: src/shell-app.c
@@ -1388,3 @@
 
-static ShellAppSearchMatch
-_shell_app_match_search_terms (ShellApp  *app,

You remove _shell_app_match_search_terms ...

@@ -1467,2 @@
 void
 _shell_app_do_match (ShellApp         *app,

... but not _shell_app_do_match, which uses it

Review of attachment 259214 [details] [review]:

::: js/ui/appDisplay.js
@@ +889,3 @@
     },
 
     getInitialResultSet: function(terms, callback, cancellable) {

Ugh, and getIntialResultSet is defined once here ...

@@ +904,3 @@
+    },
+
+    getInitialResultSet: function(terms) {

... and a second time here, using a non-existent this.searchSystem property :-(

Review of attachment 259214 [details] [review]:

::: js/ui/appDisplay.js
@@ +892,3 @@
+        let query = terms.join(' ');
+        let results = Gio.DesktopAppInfo.search(query, Lang.bind(this, this._compareResults));
+        callback(results);

This is wrong as well - callback() expects an array of IDs, not an array of an array of IDs as returned by g_desktop_app_info_search(). Also it takes a single parameter (query). So this should be something like:

let results = Gio.DesktopAppInfo.search(query).reduce(Lang.bind(this,
    function(prev, current) {
        prev = prev.sort(Lang.bind(this, this._compareResults));
        cur = cur.sort(Lang.bind(this, this._compareResults));
        return prev.concat(cur);
    }), []);
callback(results);

Yeah, I'm working on it.

Should I attach an updated patch?

Created attachment 259225 [details] [review]
appDisplay: Use the desktop file index for app searching

Updated patch to compile and work.

(In reply to comment #8)
> Should I attach an updated patch?

No. Yours also doesn't handle OnlyShowIn/NoDisplay (though I'm talking with Ryan about making him handle that potentially). Review on the other two patches would be more appreciated at this point.

Created attachment 259227 [details] [review]
appDisplay: Use the desktop file index for app searching

Rather than scanning all apps for searching, use Ryan's new desktop
file index and the glib support APIs for app searching instead of our
own system.

Review of attachment 259212 [details] [review]:

LGTM

Review of attachment 259213 [details] [review]:

OK.

Attachment 259212 [details] pushed as e10d2a6 - app-system: Put back support for the installed-changed signal
Attachment 259213 [details] pushed as ad03fb0 - app-system: Add back StartupWMClass matching


Cool. Now about the search patch again?

Review of attachment 259227 [details] [review]:

Trying it out, I think there are some clear regressions in behavior:
 - search for 'irc' rates XChat higher than Polari, however frequent I use the latter
   (because GenericName matches higher than Keywords - IMHO we should consider GenericName
    as Keywords fallback, e.g. rank it lower)
 - searching for 'chat' gives me (in that order):
    'Chat', 'Polari', 'Empathy', 'Online Accounts', 'XChat'
   I'd expect 'XChat' to rank higher there, and 'Online Accounts' should probably not be there

Any chance of convincing Ryan to address those? (except for settings panels, if we want to filter those, we would need to do that ourselves I guess).

::: js/ui/appDisplay.js
@@ +897,3 @@
+            group = group.filter(function(appID) {
+                let app = Gio.DesktopAppInfo.new(appID);
+                return app && app.should_show();

Are you sure about this? A quick look at the code[0] suggests that glib is already doing this.

[0] https://git.gnome.org/browse/glib/tree/gio/gdesktopappinfo.c#n218

Oh, also: not matching on executable names will require some getting used to :-)

(In reply to comment #15)
> IMHO we should consider GenericName as Keywords fallback, e.g. rank it lower

Filed as bug 711640.

(In reply to comment #15)
> Are you sure about this? A quick look at the code[0] suggests that glib is
> already doing this.
> 
> [0] https://git.gnome.org/browse/glib/tree/gio/gdesktopappinfo.c#n218

No. NoDisplay and Hidden are two separate things. NoDisplay is saying "please do not display me in some menus", and Hidden is saying "treat me as if this menu file doesn't exist". We need to check NoDisplay and OnlyShowIn ourselves, and that's really all that makes sense.

I may end up adding checks for NoDisplay and OnlyShowIn to the search code.  It would be annoying, but in the end it would be fairly efficient because we'd only have to do the check once per file and could cache the result.  Ditto TryExec.

fwiw: XChat should put "chat" as a keyword in its desktop file.

(In reply to comment #19)
> I may end up adding checks for NoDisplay and OnlyShowIn to the search code.

That would be nice, thanks! Though we'd probably still want to filter out Settings panels anyway ...


> fwiw: XChat should put "chat" as a keyword in its desktop file.

I don't disagree, but from a user perspective it's still odd that an almost exact match on the search term ranks lower than something like "Online Accounts". Not a biggie certainly, just a quick observation yesterday.

(In reply to comment #20)
> I don't disagree, but from a user perspective it's still odd that an almost
> exact match on the search term ranks lower than something like "Online
> Accounts". Not a biggie certainly, just a quick observation yesterday.

So, it should probably go "prefix name", "anything name", "prefix keywords", "prefix generic name", "prefix x-gnome-fullname", "anything keywords/generic name/x-gnome-fullname", "prefix comment", "anything comment" ?

"anything" will be computationally expensive -- the index is sorted in alphabetical order so that it can be binary searched.  this makes prefix matching very efficient.

Ah, I didn't know we didn't do not-prefix matching.

(In reply to comment #21)
> So, it should probably go "prefix name", "anything name", "prefix keywords",
> "prefix generic name", "prefix x-gnome-fullname", "anything keywords/generic
> name/x-gnome-fullname", "prefix comment", "anything comment" ?

Probably only for name. I can't think of a good use case for substring matches in keywords (and probably the same for GenericName/X-GNOME-FullName). Not sure matching on comment makes much sense at all tbh (pretty sure we dropped that at one point from our own search), certainly not differentiating between prefix and substring matches.

I maintain that free substring matching absolutely doesn't make sense from a user experience point of view.  The only reason it makes sense for something like "XChat" is because there is an implicit break between the "X" and "Chat" that's not actually indicated with whitespace or punctuation... we could step up the tokenisation to take these cases into account, (while still allowing "xchat" to match) or we could just leave it alone and wait for people to notice that "xchat" isn't appearing in search results for "chat" and decide to add a keyword...

btw: one more thing to consider.  If I search for "chat" then it's because I want one of two things:

 1) an application called (exactly) "Chat" (or some extension of) in which
    case matching xchat would be very bad

 2) I don't know what chat applications I have installed and I want to browse
    through them a bit.  In this case, xchat appearing lower in results is
    sane (and again, they could improve their standing with the use of
    keywords).

Once I know about XChat, if I want to explicitly launch it then I will be searching for "XChat" or (more likely) just "xc<enter>".

(In reply to comment #25)
> I maintain that free substring matching absolutely doesn't make sense from a
> user experience point of view.  The only reason it makes sense for something
> like "XChat" is because there is an implicit break between the "X" and "Chat"

It's not an uncommon pattern though. To be clear: I don't think it's a great loss, I do care much more about Settings panels showing up as applications and GenericName (which is meant to be displayed, but we don't show it anywhere) ranking higher than Keywords (which are explicitly targeting search).

Attachment 259227 [details] pushed as ba602c1 - appDisplay: Use the desktop file index for app searching

"Oh, also: not matching on executable names will require some getting used to
:-)"

I think that's somewhat of an understatement.

With this change, 'abrt' no longer finds...abrt. 'gimp' no longer finds...gimp. Who hits 'start' and types 'image' or 'gnu' or 'manipulation' to find the gimp?

We can go around changing the world's .desktop files, but it seems like a lot of work.

Why doesn't abrt find abrt. The app is called "abrt", right? And same for GIMP. The app is calld "The GIMP". If a desktop file doesn't contain the app's name, it's not our fault, is it?

"Why doesn't abrt find abrt. The app is called "abrt", right?"

Name=Automatic Bug Reporting Tool
Comment=View and report application crashes
Icon=gnome-abrt
Exec=gnome-abrt

"And same for GIMP. The app is calld "The GIMP"."

Name=GNU Image Manipulation Program
GenericName=Image Editor
Comment=Create images and edit photographs
Exec=gimp-2.8 %U
TryExec=gimp-2.8
Icon=gimp
Categories=Graphics;2DGraphics;RasterGraphics;GTK;

Sure, we can change their 'names' to "Automatic Bug Reporting Tool (abrt)" and "GNU Image Manipulation Program (gimp)". But that's not what they've been for years, and these are just the two examples I happen to have remembering noticing. It's GNOME that's changing behaviour, and "Automatic Bug Reporting Tool (abrt)" isn't really inherently a better name than "Automatic Bug Reporting Tool". It's not obvious that changing the .desktop files to work around GNOME's behaviour in this case is making them better.

To put this in more abstract terms, less tied to the examples I've found:

* "Open the overview and type what you want to find" is, AIUI, basically the primary intended interaction method for Shell - it's important that it works for anyone who wants to use the Shell
* It is not necessarily the case that the executable name of an application will be, or should be, a part of its name, description, or genericname, or keywords
* The executable name of an application is an attribute that some Shell users are likely to know and use as a search term to try and find that application

OK, if there's a point I'd challenge my own argument, maybe it's keywords: we _could_ go around finding all the cases where an app's executable name is not the same as its name or description, and make sure the executable name is in the keywords. But why is that a better approach than fixing Shell's search? Is this very difficult/complex to fix?

<desrt> it's on my radar
 i plan to add executable name back in to the keys
 or at least desktop file name
<mclasen> seems a clear regression