Created attachment 187965 [details] [review]
Add Open Containing Folder command in EOG

Review of attachment 187965 [details] [review]:

Works pretty nice. :)

Please recheck the indentation in the XML-files. You seem to have used TABs while we use just spaces there.

::: data/eog-ui.xml
@@ +9,3 @@
       <menuitem action="ImageSave"/>
+	  <menuitem action="ImageSaveAs"/> 
+      <menuitem action="ImageOpenContainingFolder"/>

I am not quite sure if putting it into the same group as Save As is really intuitive as these are quite different actions. Maybe put it further down in its own group or together with Set As Background (same for the rightclick menus)?

::: src/eog-window.c
@@ +3286,3 @@
+	priv = window->priv;
+
+	file = eog_image_get_file (priv->image);

You probably might want to prepend a g_return_if_fail (priv->image != NULL) here. Just in case the action is not disabled correctly.

Also, the 
if(file)
  g_object_unref(file)

from the end of the function could be moved after this line and be extended to include the get_parent call. That way even the case the image returning a NULL-file would be handled and the file would be unreffed as soon as we are done using it (after get_parent).

@@ +3315,3 @@
+
+	if (parent)
+		g_object_unref (parent);

This could probably be merged into the first "if(parent)" block directly above. ;)

@@ +4170,3 @@
 
+		action = gtk_action_group_get_action (window_group, "ImageOpenContainingFolder");
+		g_object_set (action, "short_label", _("Open Folder"), NULL);

This is producing runtime warnings. I think the "window_group" in the first line should be "image_group" (at least the used action group implies that).

Thanks for looking at this, Felix. :)

===
This is producing runtime warnings. I think the "window_group" in the first
line should be "image_group" (at least the used action group implies that)."
===
That was an old commit, wasn't supposed to creep in the patch :(

===
Also, the 
if(file)
  g_object_unref(file)

from the end of the function could be moved after this line and be extended to
include the get_parent call. That way even the case the image returning a
NULL-file would be handled and the file would be unreffed as soon as we are
done using it (after get_parent).
===
Merging the get_parent call in the if(file) test would result in a warning about a possible case of it never being initialized with anything. I have moved up the both the deref statements though.

Fixed the rest of the issues.

Created attachment 188142 [details] [review]
Add Open Containing Folder command in EOG (2)

Thanks for the patch, Akshay! :)

commit 9df5fd43cc9cea9ded47953d22d82cfea1ace015
Author: Akshay Gupta <>
Date:   Fri May 20 12:07:19 2011 +0200

    Add an "Open containing folder" command
    
    https://bugzilla.gnome.org/show_bug.cgi?id=650402

I removed it from the default toolbar though as we only want to have the more important functions there by default. Also, "Open Containing Folder" became "Show Containing Folder" as it is clearer IMHO and also less ambigious with eog's own "Open Folder" function.


(In reply to comment #3)
> Also, the 
> if(file)
>   g_object_unref(file)
> 
> from the end of the function could be moved after this line and be extended to
> include the get_parent call. That way even the case the image returning a
> NULL-file would be handled and the file would be unreffed as soon as we are
> done using it (after get_parent).
> ===
> Merging the get_parent call in the if(file) test would result in a warning
> about a possible case of it never being initialized with anything. I have moved
> up the both the deref statements though.

Yes, but that can be fixed by pre-initializing "parent" with NULL. ;)
Did that to your patch before checking it in.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.

That just opens the parent folder, and doesn't select the file in question. Nautilus has API to do that correctly, use it.

Comment on attachment 188142 [details] [review]
Add Open Containing Folder command in EOG (2)

Updating patch status as it showed up again due to the reopening.

(In reply to comment #6)
> That just opens the parent folder, and doesn't select the file in question.
> Nautilus has API to do that correctly, use it.

I think we'll wait until the cross-desktop(-filemanager) API (http://lists.freedesktop.org/archives/xdg/2011-May/011949.html) becomes available. :)

The advantage of using nautilus' API for this is IMHO not that big that it's worth a "strong dependency" on nautilus (filemanager-choice appears to be a controversial topic to some users from time to time).

I think that if it brings a consistent user experience in GNOME 3.2 it is worth the strong dependency. I'd go for pragmatism here. :)

Allright, I got a first running version using the new DBus-API that landed in Nautilus yesterday, that seems to work well (couldn't test launch over DBus activation yet) and falls back to the old code if it isn't available.

But I have a question: What is the "StartupId" parameter in the dbus calls good for? What data should eog submit here?

I'm blind guessing here, but isn't that for what's specified in the StartupWMClass in the desktop files?

Hmm, okay, I'm currently resorting to transmitting a string of the form "_TIME"+gtk_get_current_event_time(). This seems to be expected by GTK (although it calls them "fake startup ids") and causes the new Nautilus window to get the focus while arbitrary strings cause it to be shown in the background.

Created attachment 203598 [details] [review]
Extend eog to use the new DBus API

As it is my first time with GDBus I'm posting this for review before pushing it.

If nothing bad comes up, I'll be pushing this in time for the 3.3.4 release next week.

commit fa744735911a7445bd83f2119385b82e674b020c
Author: Felix Riemann <>
Date:   Thu Dec 15 19:36:06 2011 +0100

    Use new DBus API to show the current image in the file browser
    
    This improves the functionality implemented with commit 9df5fd43.
    The new API that will be included in Nautilus 3.3.4 not only opens a
    view for the containing folder but also marks the image in the view.
    The old behaviour is still available as fallback if the new API is
    not offered on the system.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=650402

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.