Created attachment 186102 [details] [review]
DesktopEntry: Make basename const reference

Avoids another malloc block.

Created attachment 186103 [details] [review]
DesktopEntry: Clean up structure, make TryExec evaluation lazy

This is preparatory work for rebasing DesktopEntry on top of
GDesktopAppInfo.

First, it doesn't make sense to represet a random subset of booleans
as flags; just flatten these into a bitfield.  Move the refcount to be
a plain guint at top.

Second, previously we were calling g_find_program_in_path when
creating a .desktop file, even if the caller wasn't interested in
whether the TryExec succeeded.  Make this lazy.

Created attachment 186104 [details] [review]
desktop-entries.c: Split structure explicitly between .desktop and .directory

This is code cleaup preparatory work for rebasing DesktopEntry on
GDesktopAppInfo.

These two cases are different; make this explicit via structure subclassing
of a common base structure.

Created attachment 186105 [details] [review]
Rebase DesktopEntry on GDesktopAppInfo

The main motivation for this work is to avoid gnome-shell having
to read all .desktop files *twice* - once from gnome-menus, and
once from gio (when doing MIME assocation etc.)

This patch replaces almost all of the accessors for GMenuTreeEntry
with the simple gmenu_tree_entry_get_app_info().

Note this patch depends on patches from (see bug 647967).

Created attachment 186136 [details] [review]
Fold sorting into GMenuTreeFlags

There's only two sorts right now, and so we can make one the default
and select the other with the flags.

Drop the ability to set the sort at runtime; this never was compatible
with the current GMenuTree caching, and also I'm trying to move
GMenuTree towards being immutable.

Created attachment 186137 [details] [review]
GMenuTreeFlags: Register with GType

Created attachment 186138 [details] [review]
Drop GMenuTree caching and support for absolute paths

This is work towards converting to GObject; the API to create a tree
is now just gmenu_tree_new().

First, the internal caching makes things very complex for little gain
- gnome-shell only creates one GMenuTree, and gnome-panel could pretty
easily be converted to do so.

In a Google Code Search, I couldn't find anyone using absolute paths
for menus, and looking through the revision history, I don't see
a rationale for it.  So, just drop it too.

Created attachment 186139 [details] [review]
Convert to GObject, drop static Python bindings

GMenuTree is now a GObject.  Drop the static Python bindings, since
introspection gives us coverage of most of the API now.

Created attachment 186140 [details] [review]
Replace monitor API with a simple "changed" signal

So much simpler...

Created attachment 186141 [details] [review]
Rename gmenu_tree_item_get_type() to _get_item_type()

The _get_type() namespace suffix is reserved for GType.

Created attachment 186142 [details] [review]
GMenuTreeItem: Register boxed types, drop user data

This was a hack for the static Python bindings, no longer necessary
after we register proper boxed types for the structures.

Convert the refcount to an atomic integer too; the _unref may be
called from a garbage collector thread in bindings.

Created attachment 186143 [details] [review]
Lower gmenu_tree_item_get_parent to gmenu_tree_directory_get_parent

Introspection doesn't know about the GMenuTreeItem "subclassing",
so we would have to duplicate the _get_parent method on all
subclasses - but in practice it only seems to be used on directories,
so just lower it there.

Created attachment 186144 [details] [review]
Add explicit gmenu_tree_load_sync()

Rather than having _get_root_directory() be lazy, require users
to explicitly load via this function (or in the future, an
async variant).

Created attachment 186145 [details] [review]
Further propagate GError for gmenu_tree_load_sync()

We had some GError use internally; clean things up so we propagate
it more consistently to the top of gmenu_tree_load_sync().

Created attachment 186146 [details] [review]
Rename gmenu_tree_get_menu_file() to gmenu_tree_get_menu_path()

Document it and clean it up.

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

Current work is now in:

http://git.gnome.org/browse/gnome-menus/log/?h=wip/gobject-rebase2

Changes:

* Make the test utility call _load_sync() again after "changed" is emitted
* Some smaller cleanup patches

I think this new API is stable, and it's worth merging now.  

The big outstanding item is asynchronous reading.  This has two parts:

1) Convert all the reading code to take a GMenuTreeContents* as distinct from a GMenuTree*, and ensure that each GMenuTreeContents* is only owned by one thread at a time.  Basically when doing an async read, we don't touch the current contents; we create a new thread which reads into a new structure, and then finally swap tree->contents in an idle handler.

2) Untangle all the file monitoring code from static global variables and the default GMainContext.  Right now as a side effect of reading all these files, we set up file monitors (via baroque and unnecessary GFileMonitor wrapping, which is a separate bug).  What we really want to do is only set up these monitors after the tree has been read - so from part 1), the reading code passes back a list of things to monitor, and we set that up again post-read.

Review of attachment 186102 [details] [review]:

Looks fine to me, if a bit minor - does this really matter ?

Review of attachment 186103 [details] [review]:

Looks fine to me

Review of attachment 186104 [details] [review]:

::: libmenu/desktop-entries.c
@@ +458,3 @@
+	{
+	  for (; desktop_entry->categories[i]; i++)
+	    retval_desktop_entry->categories[i] = desktop_entry->categories[i];

The handling of the NULL case looks a bit contorted here, with the i = 0 outside the if and the headless loops.

Could just pull the g_new0 inside the if and add an else case...

@@ +677,2 @@
     {
+      for (; desktop_entry->categories[i]; i++);

Same comment here

Review of attachment 186105 [details] [review]:

::: libmenu/desktop-entries.c
@@ +243,3 @@
+
+      if (!desktop_entry_load_directory (entry, key_file, desktop_entry_group, &error))
+	goto out;

Looks like you are leaking the key_file here in various goto out; cases.

::: libmenu/desktop-entries.h
@@ +51,2 @@
+/* Only valid for DESKTOP_ENTRY_DIRECTORY */
+const char * desktop_entry_desktop_get_icon (DesktopEntry *entry);

typo ? DESKTOP_ENTRY_DIRECTORY vs desktop_entry_desktop

Review of attachment 186136 [details] [review]:

Looks fine to me

Review of attachment 186137 [details] [review]:

Looks fine

Review of attachment 186138 [details] [review]:

Looks good to me

Review of attachment 186139 [details] [review]:

The gobject part looks ok (no idea about the python)

Review of attachment 186139 [details] [review]:

The gobject part looks ok (no idea about the python)

Review of attachment 186140 [details] [review]:

I agree, much nicer.

Review of attachment 186141 [details] [review]:

makes sense. does any of this have docs ? if so, keeping score of renamings like this somewhere might be nice, but then, just patching all existing users might be easier.

Review of attachment 186142 [details] [review]:

::: libmenu/gmenu-tree.c
@@ +72,3 @@
   GMenuTreeDirectory *parent;
   
+  gint refcount;

strictly speaking, this has to be volatile, I think.

Review of attachment 186143 [details] [review]:

Looks ok, assuming your 'in practice' is backed by a codesearch. 
I guess we can always bring back another get_parent method if a user shows up...

Review of attachment 186144 [details] [review]:

Looks ok

Review of attachment 186145 [details] [review]:

Looks fine

Review of attachment 186146 [details] [review]:

Looks ok

Thanks a lot for the reviews; there are more patches in http://git.gnome.org/browse/gnome-menus/log/?h=wip/gobject-rebase2 ; i can attach them here too.

See also:
https://bugzilla.gnome.org/show_bug.cgi?id=648149
for the gnome-shell porting.

Created attachment 187061 [details] [review]
Remove gmenu_tree_directory_get_tree()

This causes a circular reference internally, and API consumers
can just keep track of it easily enough externally.

Created attachment 187062 [details] [review]
layout: Use thread-default main context for callbacks

Rather than hardcoding g_idle_add(); this gives us future
flexibility for thread support.

Created attachment 187063 [details] [review]
gmenu_tree_entry_get_parent: New function

Earlier we moved this down to GMenuTreeDirectory, but it turns
out gnome-shell does expect to be able to get the parent of a
GMenuTreeEntry.  In the future I want to nuke that code, but
for now just readd this functionality.

Created attachment 187064 [details] [review]
Document a few functions

Created attachment 187065 [details] [review]
introspection: scan gmenu-tree.c

Now that we're actually adding gtk-doc.

Created attachment 187066 [details] [review]
Remove GMenuTreeItem from public API

gmenu_tree_directory_get_contents() wasn't actually bindable because
there's no way to express to introspection the inheritance hierarchy
(and to do the runtime type checking necessary via
gmenu_tree_item_get_item_type()).

Therefore, drop it, and instead add explicit functions where each type
is needed.  So for gmenu_tree_directory_get_contents(), we instead
add an explicit typesafe iterator object.

The only other instance was gmenu_tree_alias.

Note that gmenu_tree_item_ref() and gmenu_tree_item_unref() remain;
they're C only, and already just took "gpointer".

Created attachment 187067 [details] [review]
Replace Python example with JS

Created attachment 187068 [details] [review]
Bump to GMenu-4.0.gir; this matches the package version.

Review of attachment 187061 [details] [review]:

Looks fine to me

Review of attachment 187062 [details] [review]:

::: libmenu/menu-layout.c
@@ +176,3 @@
+			     (GSourceFunc) menu_layout_invoke_monitors, nr, NULL);
+      g_source_attach (nr->monitors_idle_handler, nr->main_context);
+      g_source_unref (nr->monitors_idle_handler);

Is the refcounting right here ? 
You create the source with a refcount of 1, then you attach it to the main context (so the main context owns a ref), then you drop your own ref.

@@ +254,3 @@
+          if (nr->monitors_idle_handler != NULL)
+	    g_source_destroy (nr->monitors_idle_handler);
+	  nr->monitors_idle_handler = NULL;

Here the main_context_unref drops the contexts reference on the source, so it may get nuked before you call g_source_destroy on it. In any case, the g_source_destroy here seems to drop a reference that you already gave up above.

Review of attachment 187063 [details] [review]:

Looks fine

Review of attachment 187063 [details] [review]:

Looks fine

Review of attachment 187064 [details] [review]:

no problems

Review of attachment 187065 [details] [review]:

looks fine

Review of attachment 187065 [details] [review]:

looks fine

Review of attachment 187066 [details] [review]:

I must say I prefer stack-allocated iters, but it probably doesn't make a big difference.

Review of attachment 187067 [details] [review]:

why not

Review of attachment 187068 [details] [review]:

looks fine

Review of attachment 187068 [details] [review]:

looks fine

Review of attachment 187065 [details] [review]:

looks fine

Review of attachment 186103 [details] [review]:

Thanks, please commit after doing the changes  below.

::: libmenu/desktop-entries.c
@@ +270,3 @@
+      /* why are we stripping tryexec but not exec? */
+      if (retval->try_exec != NULL)
+	retval->try_exec = g_strstrip (retval->try_exec);

I'd prefer to just do "g_strstrip (retval->try_exec);" as the string is modified in-place. This avoids confusion when reading the code like "wait, we're leaking memory here".

@@ +359,1 @@
   entry->terminal = 0;

If we're resetting fields here, then we should reset all fields -- that includes the new ones you're adding.

@@ +543,3 @@
 {
+  if (entry->try_exec == NULL)
+    return FALSE;

Add an empty line after this.

@@ +554,3 @@
+      entry->tryexec_failed = (path == NULL);
+
+      g_free (path);

Please remove the last two empty lines. Having one empty line every other line is not that nice :-)

To be clear, that'd look like this:

path = g_find_program_in_path (entry->try_exec);
entry->tryexec_failed = (path == NULL);
g_free (path):

Review of attachment 186102 [details] [review]:

I don't see the point of this either: how much does it matter?

Review of attachment 186104 [details] [review]:

::: libmenu/desktop-entries.c
@@ -40,2 @@
   char     *name;
-  char     *generic_name;

According to the spec, generic_name is also valid for .directory.

@@ +185,3 @@
+		   G_KEY_FILE_ERROR,
+		   G_KEY_FILE_ERROR_INVALID_VALUE,
+		   "\"%s\" contains no \"Type\" key\n", entry->path);

Please mark all errors used in g_set_error() for translation. Even if we know that they won't appear anywhere in the UI for now, there's always a chance this could change later on.

@@ +245,3 @@
+  /* why are we stripping tryexec but not exec? */
+  if (desktop_entry->try_exec != NULL)
+    desktop_entry->try_exec = g_strstrip (desktop_entry->try_exec);

(Comment from the other patch: just "g_strstrip (desktop_entry->try_exec);")

@@ +301,3 @@
+		    entry->path, error->message);
+      g_clear_error (&error);
+    }

Add empty line here.

@@ +302,3 @@
+      g_clear_error (&error);
+    }
+  g_key_file_free (key_file);

Add empty line here.

@@ +352,2 @@
   menu_verbose ("Re-loading desktop entry \"%s\"\n", entry->path);
 

Comment from previous patch: more fields need to be reset. Example where this matters: tryexec_evaluated.

@@ +390,3 @@
+      desktop_entry_unref (entry);
+      return NULL;
+    }

Add empty line here.

@@ +458,3 @@
+	{
+	  for (; desktop_entry->categories[i]; i++)
+	    retval_desktop_entry->categories[i] = desktop_entry->categories[i];

+1 to what Matthias said.

@@ +495,3 @@
+      g_free (desktop_entry->full_name);
+      g_free (desktop_entry->exec);
+      g_free (desktop_entry->try_exec);

Either remove the "= NULL;" from earlier, or add them here too (I generally prefer to set the memory back to NULL, as I'm paranoid). Let's be consistent.

@@ +534,3 @@
 {
+  if (entry->type != DESKTOP_ENTRY_DESKTOP)
+    return NULL;

See earlier comment: GenericName is valid for .directory.

@@ +595,3 @@
 desktop_entry_get_tryexec_failed (DesktopEntry *entry)
 {
+  DesktopEntryDesktop *desktop_entry;

The names are getting confusing. Not sure if this is fixed in a later patch, but this should get fixed somehow: desktop_entry is not a DesktopEntry but a DesktopEntryDesktop. Maybe rename the variable to entry_desktop?

(same comment for all the functions below)

@@ +600,1 @@
     return FALSE;

Add empty line here.

Ping on this - would like to make progress here.

Okay, after reviewing the diff between the branches instead of the individual patches, I see several issues that testing seems to confirm:

a) desktop_entry_get_no_display() is wrong for .desktop files.
It should care about NoDisplay, and the code shows that it cares about Hidden. Those are two different things, and adding a NoDisplay=true line to a .desktop file indeed doesn't work. We need new API in glib to access NoDisplay.

b) OnlyShowIn/NotShowIn is totally broken now. The code never look at that for .desktop files, and apparently, it doesn't seem to work for .directory files either (or my quick test was wrong). We likely need API in glib for that. Note that we can't rely on g_desktop_app_info_should_show() as this will look at NoDisplay.

c) g_idle_add() is still used in two places, so the move away from it for thread support is not complete, I guess.

d) by moving to GDesktopAppInfo, we lose support for "KDE Desktop Entry" for .desktop files. I think it's fine, but in that case, we should also drop support for it in the .directory files (in desktop-entries.c)

e) I'm worried about the change in process_layout(): we remove entries before processing the layout. I'd prefer to avoid that, as we could well add flags to keep displaying those entries later on. Obviously, one issue is TryExec that GDesktopAppInfo completely hides from us by failing to load the .desktop file if TryExec fails.

f) As mentioned earlier, GenericName is valid for .directory files, so I believe we should keep support from that in desktop-entries.c.

g) Some gtk-doc comments reference the non-existing gmenu_tree_iter_get_next_type().

I've pushed a wip/gobject-review branch that fixes various small other issues I saw, and item d) from above. Also, I renamed the pkg-config file and the library, to allow parallel-installation with earlier versions.

I filed bug 652385 with a patch to get g_desktop_app_info_get_nodisplay().

Colin: can you answer comment 29 and comment 45?

I fixed items f) and g) from my list above in my branch.

For the remaining items:

a) see bug 652385
b) we probably need API in gio like g_desktop_app_info_get_only_show_in/not_show_in
c) I guess it's just a matter of doing the same GSource change
e) once b) is done, we can add that back, except for TryExec

(In reply to comment #64)
> Colin: can you answer comment 29 

Yes, needs to be volatile.  What tree are you working from?  The wip/gobject-review?  I can make a patch on top of that and we can rebase it later to go in?

> and comment 45?

Yes, we should destroy the source before the context.  I can do a patch as soon as I know what tree to do it from.

Yes, I'm working on wip/gobject-review right now.

FWIW, I fixed item a), since the API just got added to glib.

So right now, the big blocker is b) (OnlyShowIn/NotShowIn being ignored)

(In reply to comment #60)
> 
> b) OnlyShowIn/NotShowIn is totally broken now. The code never look at that for
> .desktop files, and apparently, it doesn't seem to work for .directory files
> either (or my quick test was wrong). We likely need API in glib for that. Note
> that we can't rely on g_desktop_app_info_should_show() as this will look at
> NoDisplay.

Hmm.  So how about just g_desktop_app_info_should_show_in(App* app, const char *env) and it's documented to ignore nodisplay?

(In reply to comment #67)
> (In reply to comment #60)
> > 
> > b) OnlyShowIn/NotShowIn is totally broken now. The code never look at that for
> > .desktop files, and apparently, it doesn't seem to work for .directory files
> > either (or my quick test was wrong). We likely need API in glib for that. Note
> > that we can't rely on g_desktop_app_info_should_show() as this will look at
> > NoDisplay.
> 
> Hmm.  So how about just g_desktop_app_info_should_show_in(App* app, const char
> *env) and it's documented to ignore nodisplay?

That would work for me, but for consistency, we might simply rely on g_desktop_app_info_set_desktop_env() instead of passing an env argument.

(In reply to comment #65)
> (In reply to comment #64)
> > Colin: can you answer comment 29 
> 
> Yes, needs to be volatile.  What tree are you working from?  The
> wip/gobject-review?  I can make a patch on top of that and we can rebase it
> later to go in?
> 
> > and comment 45?
> 
> Yes, we should destroy the source before the context.  I can do a patch as soon
> as I know what tree to do it from.

Both of these are now fixed in the wip/gobject-review branch.

Last bit needed in glib is filed as bug 655044, and I pushed the change to use it in the branch. So we're waiting for this to merge the branch.

There's also item c) in my list that still needs to be completed, but it's not a blocker.

I've updated the gnome-panel patch locally and written a patch for g-c-c, so those components should be ready for the switch to the new API.

Colin: what's the plan for gnome-shell? Would you be ready to use the new API for 3.1.4?

We merged the branch. The last bit that, I believe, wasn't fully covered has been split to bug 655737.

Thanks!