After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 647968 - gnome-menus 4.0: GObject based, use GDesktopAppInfo, allow asynchronous loading
gnome-menus 4.0: GObject based, use GDesktopAppInfo, allow asynchronous loading
Status: RESOLVED FIXED
Product: gnome-menus
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-menus dummy account
gnome-menus dummy account
: 647979 (view as bug list)
Depends on: 655044
Blocks: 648149 649327 655110
 
 
Reported: 2011-04-16 19:45 UTC by Colin Walters
Modified: 2011-08-01 17:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
DesktopEntry: Make basename const reference (2.18 KB, patch)
2011-04-16 19:46 UTC, Colin Walters
reviewed Details | Review
DesktopEntry: Clean up structure, make TryExec evaluation lazy (8.61 KB, patch)
2011-04-16 19:46 UTC, Colin Walters
accepted-commit_now Details | Review
desktop-entries.c: Split structure explicitly between .desktop and .directory (20.62 KB, patch)
2011-04-16 19:46 UTC, Colin Walters
needs-work Details | Review
Rebase DesktopEntry on GDesktopAppInfo (30.36 KB, patch)
2011-04-16 19:46 UTC, Colin Walters
reviewed Details | Review
Fold sorting into GMenuTreeFlags (10.68 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
GMenuTreeFlags: Register with GType (2.02 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
Drop GMenuTree caching and support for absolute paths (13.34 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
Convert to GObject, drop static Python bindings (70.12 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
Replace monitor API with a simple "changed" signal (5.24 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
Rename gmenu_tree_item_get_type() to _get_item_type() (2.84 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
GMenuTreeItem: Register boxed types, drop user data (5.27 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
Lower gmenu_tree_item_get_parent to gmenu_tree_directory_get_parent (2.39 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
Add explicit gmenu_tree_load_sync() (5.39 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
Further propagate GError for gmenu_tree_load_sync() (4.58 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
Rename gmenu_tree_get_menu_file() to gmenu_tree_get_menu_path() (2.89 KB, patch)
2011-04-17 14:02 UTC, Colin Walters
reviewed Details | Review
Remove gmenu_tree_directory_get_tree() (3.03 KB, patch)
2011-05-02 18:38 UTC, Colin Walters
reviewed Details | Review
layout: Use thread-default main context for callbacks (3.15 KB, patch)
2011-05-02 18:38 UTC, Colin Walters
reviewed Details | Review
gmenu_tree_entry_get_parent: New function (1.78 KB, patch)
2011-05-02 18:39 UTC, Colin Walters
reviewed Details | Review
Document a few functions (1.54 KB, patch)
2011-05-02 18:39 UTC, Colin Walters
reviewed Details | Review
introspection: scan gmenu-tree.c (979 bytes, patch)
2011-05-02 18:39 UTC, Colin Walters
reviewed Details | Review
Remove GMenuTreeItem from public API (15.75 KB, patch)
2011-05-02 18:39 UTC, Colin Walters
reviewed Details | Review
Replace Python example with JS (5.91 KB, patch)
2011-05-02 18:39 UTC, Colin Walters
reviewed Details | Review
Bump to GMenu-4.0.gir; this matches the package version. (1.44 KB, patch)
2011-05-02 18:39 UTC, Colin Walters
reviewed Details | Review

Description Colin Walters 2011-04-16 19:45:59 UTC
See patches
Comment 1 Colin Walters 2011-04-16 19:46:01 UTC
Created attachment 186102 [details] [review]
DesktopEntry: Make basename const reference

Avoids another malloc block.
Comment 2 Colin Walters 2011-04-16 19:46:04 UTC
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.
Comment 3 Colin Walters 2011-04-16 19:46:06 UTC
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.
Comment 4 Colin Walters 2011-04-16 19:46:09 UTC
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).
Comment 5 Colin Walters 2011-04-17 14:02:00 UTC
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.
Comment 6 Colin Walters 2011-04-17 14:02:04 UTC
Created attachment 186137 [details] [review]
GMenuTreeFlags: Register with GType
Comment 7 Colin Walters 2011-04-17 14:02:07 UTC
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.
Comment 8 Colin Walters 2011-04-17 14:02:09 UTC
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.
Comment 9 Colin Walters 2011-04-17 14:02:11 UTC
Created attachment 186140 [details] [review]
Replace monitor API with a simple "changed" signal

So much simpler...
Comment 10 Colin Walters 2011-04-17 14:02:14 UTC
Created attachment 186141 [details] [review]
Rename gmenu_tree_item_get_type() to _get_item_type()

The _get_type() namespace suffix is reserved for GType.
Comment 11 Colin Walters 2011-04-17 14:02:16 UTC
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.
Comment 12 Colin Walters 2011-04-17 14:02:19 UTC
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.
Comment 13 Colin Walters 2011-04-17 14:02:21 UTC
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).
Comment 14 Colin Walters 2011-04-17 14:02:24 UTC
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().
Comment 15 Colin Walters 2011-04-17 14:02:27 UTC
Created attachment 186146 [details] [review]
Rename gmenu_tree_get_menu_file() to gmenu_tree_get_menu_path()

Document it and clean it up.
Comment 16 Colin Walters 2011-04-17 14:02:49 UTC
*** Bug 647979 has been marked as a duplicate of this bug. ***
Comment 17 Colin Walters 2011-04-18 18:58:48 UTC
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.
Comment 18 Matthias Clasen 2011-04-28 21:59:35 UTC
Review of attachment 186102 [details] [review]:

Looks fine to me, if a bit minor - does this really matter ?
Comment 19 Matthias Clasen 2011-04-28 22:11:22 UTC
Review of attachment 186103 [details] [review]:

Looks fine to me
Comment 20 Matthias Clasen 2011-04-28 22:21:53 UTC
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
Comment 21 Matthias Clasen 2011-04-28 22:30:29 UTC
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
Comment 22 Matthias Clasen 2011-04-28 22:41:23 UTC
Review of attachment 186136 [details] [review]:

Looks fine to me
Comment 23 Matthias Clasen 2011-04-28 22:44:09 UTC
Review of attachment 186137 [details] [review]:

Looks fine
Comment 24 Matthias Clasen 2011-04-28 22:45:59 UTC
Review of attachment 186138 [details] [review]:

Looks good to me
Comment 25 Matthias Clasen 2011-04-28 22:57:36 UTC
Review of attachment 186139 [details] [review]:

The gobject part looks ok (no idea about the python)
Comment 26 Matthias Clasen 2011-04-29 01:24:40 UTC
Review of attachment 186139 [details] [review]:

The gobject part looks ok (no idea about the python)
Comment 27 Matthias Clasen 2011-04-29 01:25:23 UTC
Review of attachment 186140 [details] [review]:

I agree, much nicer.
Comment 28 Matthias Clasen 2011-04-29 01:26:51 UTC
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.
Comment 29 Matthias Clasen 2011-04-29 01:28:58 UTC
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.
Comment 30 Matthias Clasen 2011-04-29 01:30:38 UTC
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...
Comment 31 Matthias Clasen 2011-04-29 01:32:32 UTC
Review of attachment 186144 [details] [review]:

Looks ok
Comment 32 Matthias Clasen 2011-04-29 01:43:17 UTC
Review of attachment 186145 [details] [review]:

Looks fine
Comment 33 Matthias Clasen 2011-04-29 01:50:56 UTC
Review of attachment 186146 [details] [review]:

Looks ok
Comment 34 Colin Walters 2011-04-29 02:19:48 UTC
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.
Comment 35 Colin Walters 2011-04-29 02:23:28 UTC
See also:
https://bugzilla.gnome.org/show_bug.cgi?id=648149
for the gnome-shell porting.
Comment 36 Colin Walters 2011-05-02 18:38:55 UTC
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.
Comment 37 Colin Walters 2011-05-02 18:38:59 UTC
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.
Comment 38 Colin Walters 2011-05-02 18:39:02 UTC
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.
Comment 39 Colin Walters 2011-05-02 18:39:05 UTC
Created attachment 187064 [details] [review]
Document a few functions
Comment 40 Colin Walters 2011-05-02 18:39:07 UTC
Created attachment 187065 [details] [review]
introspection: scan gmenu-tree.c

Now that we're actually adding gtk-doc.
Comment 41 Colin Walters 2011-05-02 18:39:11 UTC
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".
Comment 42 Colin Walters 2011-05-02 18:39:14 UTC
Created attachment 187067 [details] [review]
Replace Python example with JS
Comment 43 Colin Walters 2011-05-02 18:39:17 UTC
Created attachment 187068 [details] [review]
Bump to GMenu-4.0.gir; this matches the package version.
Comment 44 Matthias Clasen 2011-05-03 02:46:50 UTC
Review of attachment 187061 [details] [review]:

Looks fine to me
Comment 45 Matthias Clasen 2011-05-03 02:59:12 UTC
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.
Comment 46 Matthias Clasen 2011-05-03 03:01:15 UTC
Review of attachment 187063 [details] [review]:

Looks fine
Comment 47 Matthias Clasen 2011-05-03 03:01:16 UTC
Review of attachment 187063 [details] [review]:

Looks fine
Comment 48 Matthias Clasen 2011-05-03 03:02:02 UTC
Review of attachment 187064 [details] [review]:

no problems
Comment 49 Matthias Clasen 2011-05-03 03:02:47 UTC
Review of attachment 187065 [details] [review]:

looks fine
Comment 50 Matthias Clasen 2011-05-03 03:02:48 UTC
Review of attachment 187065 [details] [review]:

looks fine
Comment 51 Matthias Clasen 2011-05-03 03:07:48 UTC
Review of attachment 187066 [details] [review]:

I must say I prefer stack-allocated iters, but it probably doesn't make a big difference.
Comment 52 Matthias Clasen 2011-05-03 03:08:43 UTC
Review of attachment 187067 [details] [review]:

why not
Comment 53 Matthias Clasen 2011-05-03 03:09:25 UTC
Review of attachment 187068 [details] [review]:

looks fine
Comment 54 Matthias Clasen 2011-05-03 03:09:25 UTC
Review of attachment 187068 [details] [review]:

looks fine
Comment 55 Matthias Clasen 2011-05-03 03:09:39 UTC
Review of attachment 187065 [details] [review]:

looks fine
Comment 56 Vincent Untz 2011-05-05 14:13:01 UTC
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):
Comment 57 Vincent Untz 2011-05-05 14:13:49 UTC
Review of attachment 186102 [details] [review]:

I don't see the point of this either: how much does it matter?
Comment 58 Vincent Untz 2011-05-05 14:45:38 UTC
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.
Comment 59 Colin Walters 2011-05-16 21:25:26 UTC
Ping on this - would like to make progress here.
Comment 60 Vincent Untz 2011-06-12 07:50:59 UTC
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.
Comment 61 Vincent Untz 2011-06-12 08:01:41 UTC
g) Some gtk-doc comments reference the non-existing gmenu_tree_iter_get_next_type().
Comment 62 Vincent Untz 2011-06-12 10:00:40 UTC
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.
Comment 63 Vincent Untz 2011-06-12 10:25:59 UTC
I filed bug 652385 with a patch to get g_desktop_app_info_get_nodisplay().
Comment 64 Vincent Untz 2011-06-12 10:57:44 UTC
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
Comment 65 Colin Walters 2011-06-13 22:16:16 UTC
(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.
Comment 66 Vincent Untz 2011-06-14 07:07:34 UTC
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)
Comment 67 Colin Walters 2011-06-14 16:22:13 UTC
(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?
Comment 68 Vincent Untz 2011-06-14 16:46:50 UTC
(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.
Comment 69 Colin Walters 2011-06-14 17:56:38 UTC
(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.
Comment 70 Vincent Untz 2011-07-21 13:35:09 UTC
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.
Comment 71 Vincent Untz 2011-07-22 11:09:01 UTC
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?
Comment 72 Vincent Untz 2011-08-01 17:52:26 UTC
We merged the branch. The last bit that, I believe, wasn't fully covered has been split to bug 655737.

Thanks!