I accidentally pressed the submit button too early. The patch is right, but here are the more meaningful log messages:

      JS LOG: Tried adding a display item for nautilus.desktop, but couldn't get the item info.
      JS LOG: Tried adding a display item for evince.desktop, but couldn't get the item info.
      JS LOG: Tried adding a display item for eog.desktop, but couldn't get the item info.
      JS LOG: Tried adding a display item for metacity.desktop, but couldn't get the item info.

Yeah, hit this too and hacked a similar fix into my tree. 

I think this happens only when you have old data in /home/otaylor/.gnome2/shell/applications_usage, so you could work around it by removing that file.

I think probably wait for Colin to take a look at this rather than rushing in a quick fix. (I think that ignoring it is probably right - that is, nautilus and evince shouldn't get favorited just by being used frequently.)

Created attachment 138002 [details] [review]
Major rework of application data structures and caching

Before, we looked up application data in several ways; the ShellAppSystem
exported just application ids (though it parsed the .desktop files internally),
and we'd create a Gio.DesktopAppInfo object (reparsing the desktop file again),
wrapping that inside a JavaScript AppInfo class, and finally the AppDisplay
would again parse the .desktop file to get the categories.

Also, to look up applications by id previously, we traversed the entire
menu structure each time.

Some qualities such as the NoDisplay flag were not easily exposed in the old
system.  And if we wanted to expose them we'd have to change several different
application information wrapper classes.

All in all, it was quite suboptimal.

The theme of this new code is basically "just use libgnome-menus".  We do
not call into Gio for app lookups anymore.  The new Shell.AppInfo class
is a disguised pointer for the GMenuTreeEntry item.

To fix the caching, we keep a simple hash table of desktop id -> ShellAppInfo.

Created attachment 138004 [details] [review]
Fix double-free in previous commit

We don't need to ref the items returned from gather_entries_recurse,
we do need to ref the ones we've already cached.

*** Bug 587548 has been marked as a duplicate of this bug. ***

(In reply to comment #3)
> Created an attachment (id=138002) [edit]
> Major rework of application data structures and caching

Not related to the changes, but in the search comparisons,

     description = description.toLowerCase();

should be:

     description = GLib.utf8_casefold(GLib.utf8_normalize(description, -1, GLib.NormalizeMode.ALL), -1);

(and the same thing with the other strings, including @search) to be properly i18n-y.


the old AppInfo.createIcon is now duplicated in two places (appDisplay.js:createAppIcon() and widget.js:AppsWidgetInfo.createIcon()). Probably should be moved into ShellAppInfo? Likewise with the launch call that calls Clutter.get_current_event_time(). You could just have launch_default() do that itself. And then you don't even need AppsWidgetInfo any more because it's just ShellAppInfo.

+    _getMostUsed: function() {
+        return this._appMonitor.get_most_used_apps(0, 30).map(Lang.bind(this, function (id) {
+            return this._appSystem.lookup_app(id + '.desktop');

It seems odd that ShellAppMonitor returns strings that you need to add '.desktop' to in order to pass to ShellAppSystem... Can we consistently use '.desktop' either everywhere or nowhere, or is that awkward because of the existing gmenu/GAppInfo APIs? Or is this a mistake leftover from the old code? If we need to sometimes use both, the docs need to be clearer about what kind to use where.

  * get_appid_for_window:
  *
- * Returns a desktop file ID for an application, or %NULL if
+ * Returns the a application associated with a window, or %NULL if

s/get_appid/get_app/, s/the a/the/

shell_app_monitor_get_window_app() should check if the MetaWindow isn't found in the hash table? (I guess it doesn't actually crash if it's not, it just eventually spews a warning in gmenu_tree_item_ref.)

 static gpointer
+shell_app_info_copy (gpointer infop)
+{
+  /* Currently punned to a GMenuTreeEntry */
+  GMenuTreeEntry *entry = infop;
+  return gmenu_tree_item_ref ((GMenuTreeItem*)entry);
+}

Why not just pass the gpointer to gmenu_tree_item_ref() directly? Or at least not cast it first to GMenuTreeEntry and then to GMenuTreeItem.

There is a lot of inconsistency in the type casting; shell_app_info_ref/unref use helper functions that cast via gpointer, shell_app_system_lookup_app does its own casts (again casting a gpointer first to a GMenuTreeEntry and then to GMenuTreeItem). Other methods further on in the file use entry_from_app_info().

The gtk-doc on shell_app_system_lookup_heuristic_basename() still claims to be shell_app_system_lookup_basename(). It is also confusing; it seems to say that the input value of @name should have been heuristically determined, but really it means it will use heuristics ON @name. Also, it's broken:

+  tmpid = g_strjoin ("", "gnome-", name, NULL);
+  result = shell_app_system_lookup_app (system, name);

"name" in the second line should be "tmpid", right? Likewise with the "fedora-" check. Also, tmpid gets leaked between the two. Also, that's a wacky use of g_strjoin. You can also probably get rid of the gotos without much fuss.


Everything else looks fine.

Works perfectly for. Only small "glitch": the "Others" category is shown, but it is empty. IMHO empty categories should be hidden, just like in the normal main menu.

(In reply to comment #6)
> 
>      description = description.toLowerCase();
> 
> should be:
> 
>      description = GLib.utf8_casefold(GLib.utf8_normalize(description, -1,
> GLib.NormalizeMode.ALL), -1);

Good point.  Ideally I'd keep the normalized and folded description around with the app, but then I couldn't just have the ShellAppInfo be a cast for a GMenuTreeEntry =/.

Eventually I suspect I'm going to have to hack libgnome-menus.

> the old AppInfo.createIcon is now duplicated in two places
> (appDisplay.js:createAppIcon() and widget.js:AppsWidgetInfo.createIcon()).
> Probably should be moved into ShellAppInfo? Likewise with the launch call that
> calls Clutter.get_current_event_time(). You could just have launch_default() do
> that itself. 

I had some thought that I wanted to avoid a Clutter dependency there, but that's probably stupid.  Will fix.

> And then you don't even need AppsWidgetInfo any more because it's
> just ShellAppInfo.

The reason for that class is to conform to the widget API.  Unless I changed say the docsInfo.js to have the method be named "create_icon" instead of createIcon.

> It seems odd that ShellAppMonitor returns strings that you need to add
> '.desktop' to in order to pass to ShellAppSystem... Can we consistently use
> '.desktop' either everywhere or nowhere, or is that awkward because of the
> existing gmenu/GAppInfo APIs? Or is this a mistake leftover from the old code?

It's a leftover, but I have another huge patch to fix all this up (that predates this new patch, and will need a bit of conflict massaging).

Thanks for the review, will fix up for the above and the other comments.

(In reply to comment #8)
> > Probably should be moved into ShellAppInfo? Likewise with the launch call that
> > calls Clutter.get_current_event_time(). You could just have launch_default() do
> > that itself. 
> 
> I had some thought that I wanted to avoid a Clutter dependency there, but
> that's probably stupid.  Will fix.

You could use meta_display_get_current_time() instead... (I think that will even do the right thing if you're calling from a gtk event instead of a clutter event...)

This is applied now.