Created attachment 191470 [details] [review]
Patch that adds this enhancement

I've added the entry under the Actions menu and the context menu.

Review of attachment 191470 [details] [review]:

Thanks for your patch. It seems mostly correct, but I'd like to comment on some things before pushing it to the repo.

First, from a more global perspective, I have to say that I find confusing the behavior when you select several pictures at the time and click on "Open in the image viewer". I know there's an issue on the underlying mechanism currently in use (spawn the 'gnome-open' command) but still I think that we either fix that somehow or we shouldn't allow the user to click on that option if more than one picture is selected. Perhaps dimming this action when n_selected_pictures != 1 would be good enough for the time being, although if we do this we would probably need to think about dimming other options that are not actually available when n_selected_pictures < 0, and that are currently showing an error message whenever clicked without fulfilling that requirement.

Also, related to this thing of open multiple pictures in the external viewer, if we manage to fix that issue somehow I think we should consider triggering the same action (showing a specific list of pictures in the external viewer) when clicking in the 'multiple pictures' thumbnail used in the details dialog.

So those are the global comments, see below some others specific to the patch...

::: data/gtkbuilder/frogr-main-view.xml
@@ -156,1 +160,15 @@
-      <object class="GtkMenuItem" id="remove_pictures_ctxt_menu_item">
+      <object class="GtkMenuItem" id="open_in_external_viewer_ctxt_menu_item">
+        <property name="visible">True</property>
+        <property name="can_focus">False</property>
... 12 more ...

The change in this last line sounds like a mistake, and it's actually causing frogr to segfault because there's already a "remove_pictures_menu_item" element in this same glade file. Please keep the "remove_pictures_ctxt_menu_item" old name instead.

::: src/frogr-main-view.c
@@ +976,3 @@
+  gchar *uris = NULL;
+  GSList *pictures, *current_pic;
+  FrogrMainViewPrivate *priv = FROGR_MAIN_VIEW_GET_PRIVATE (self);

You should remove this line as you're declaring a variable (priv) that is never used in its scope. 

You would probably have detected this if compiling with -Wall -Werror. For the future I encourage to use --enable-debug with autogen.sh/configure when developing in frogr, since that parameter will set -Wall and -Werror, among other useful things, for you :)

@@ +986,3 @@
+      FrogrPicture *picture = FROGR_PICTURE (current_pic->data);
+      gchar *current_uris = uris;
+  if (!_pictures_selected_required_check (self))

You're missing a NULL centinel here that you would have probably cached with -Wall -Werror too :).

Same suggestion here about using --enable-debug

Argh! Either this 'review' thing screwed things up or I just misused it...

Let me clarify the second part (comments over the patch) now:

(In reply to comment #2)
> [...]
> The change in this last line sounds like a mistake, and it's actually causing
> frogr to segfault because there's already a "remove_pictures_menu_item" element
> in this same glade file. Please keep the "remove_pictures_ctxt_menu_item" old
> name instead.

I meant that the following change is wrong in the glade file

 ::: data/gtkbuilder/frogr-main-view.xml
 -      <object class="GtkMenuItem" id="remove_pictures_ctxt_menu_item">
 [...]  ==> Some additions here
 +      <object class="GtkMenuItem" id="remove_pictures_menu_item">

> ::: src/frogr-main-view.c
> @@ +976,3 @@
> +  gchar *uris = NULL;
> +  GSList *pictures, *current_pic;
> +  FrogrMainViewPrivate *priv = FROGR_MAIN_VIEW_GET_PRIVATE (self);
> 
> You should remove this line as you're declaring a variable (priv) that is never
> used in its scope. 
> [...]

That's right, I meant removing that 'priv' line.


> @@ +986,3 @@
> +      FrogrPicture *picture = FROGR_PICTURE (current_pic->data);
> +      gchar *current_uris = uris;
> +  if (!_pictures_selected_required_check (self))
> 
> You're missing a NULL centinel here that you would have probably cached with
> -Wall -Werror too :).
> [...]

Don't know what happenned here... I meant that there's a missing centinel here:

 +      uris = g_strconcat (frogr_picture_get_fileuri (picture), " ", current_uris);

...as it should be something like this:

 +      uris = g_strconcat (frogr_picture_get_fileuri (picture), " ", current_uris, NULL);


Sorry for the confusion. Hope now it's clearer,
Mario

Comment on attachment 191470 [details] [review]
Patch that adds this enhancement

Committed, together with some follow up patches to fix the issues in it.
Thanks!

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.