The feature needs nautilus-sendto master, for a version with the --xid argument. Unfortunately, I had to remove the configure-time version check just before making the release, as it did not work for older versions of nautilus-sendto which do not have the --version argument.

Patricia wrote the sharing feature, and might have some ideas on how to improve the version check so that it can be added back, and improve the user feedback if there is an error.

Created attachment 205519 [details] [review]
Check nautilus-sendto runtime dependency

This partially solves the bug. I will also work on the UI to inform the user about the error.

again, i don't think you need to check for n-s on configure time, as it is not a dependency. basically i would just check whether n-s exists during runtime and if there is a version flag available. depending on that check you can make the share functionality sensitive or insensitive.

(In reply to comment #3)
> again, i don't think you need to check for n-s on configure time, as it is not
> a dependency.

There is no need to check for nautilus-sendto at configure time, but apparently if there is no warning message, users (and packagers) might expect that they have everything installed that is required to use the sharing feature. A warning is a good, non-fatal way to verify the setup.

> basically i would just check whether n-s exists during runtime
> and if there is a version flag available. depending on that check you can make
> the share functionality sensitive or insensitive.

The problem with that approach is that it implies executing a binary each time the share action is displayed to the user in a widget, as nautilus-sendto could get installed or uninstalled during runtime. The asynchronous nature of executing a binary makes this difficult to get right, and I think that making the runtime check robust enough to work in all situations is too much effort. It would simplify the problem if nautilus-sendto offered a shared library instead, but that is not the current state.

(In reply to comment #4)

> The problem with that approach is that it implies executing a binary each time
> the share action is displayed to the user in a widget, as nautilus-sendto could
> get installed or uninstalled during runtime. The asynchronous nature of
> executing a binary makes this difficult to get right, and I think that making
> the runtime check robust enough to work in all situations is too much effort.
> It would simplify the problem if nautilus-sendto offered a shared library
> instead, but that is not the current state.

right. so what about the packagekit approach? you could simply check whether n-s is available if a user uses the share functionality and install it on the fly. see the totem video-codec installation for an example.

(In reply to comment #5)
 
> right. so what about the packagekit approach? you could simply check whether
> n-s is available if a user uses the share functionality and install it on the
> fly. see the totem video-codec installation for an example.

Yes, I was (and am) already investigating this. Not all distributions use Packagekit though.

Comment on attachment 205519 [details] [review]
Check nautilus-sendto runtime dependency

I pushed a modified patch to master in commit 8a35adc6dbed9a2e672dc6243bee81092948f21a.

Created attachment 206506 [details] [review]
Use PackageKit for nautilus-sendto runtime dependency

In this patch, PackageKit is used in case the user wants to use the sharing functionality and nautilus-sendto is not already installed, allowing the user to install it at runtime.

Review of attachment 206506 [details] [review]:

There needs to be some text added to the README to explain that PackageKit will be used to search for nautilus-sendto. The changes to add the GLib namepace are redundant, as the namespace is implicit. Additionally, they are unrelated to the rest of the patch, so remove them. Check the spelling in the commit message too.

::: src/cheese-window.vala
@@ +213,3 @@
       Gtk.show_uri (screen, uri, Gtk.get_current_event_time ());
     }
+    catch (GLib.Error err)

No, leave this and all further catch statements as just Error, without the GLib.

@@ +396,3 @@
+
+  /**
+   * Show a dialog where the user can choose whereas to

That should be ‘whether’, not ‘whereas’.

@@ +402,3 @@
+  private void install_missing_packages ()
+  {
+     MessageDialog dialog = new MessageDialog (this,

I do not think that it is a good idea to display a dialog, asking the user whether to install nautilus-sendto, which will then pop up a GNOME PackageKit dialog asking almost the same thing. Remove all the user interaction from this method.

@@ +431,3 @@
+
+     try {
+        PkProxy pk_proxy = yield GLib.Bus.get_proxy(GLib.BusType.SESSION,

Leave off the GLib namespace.

@@ +442,3 @@
+                                              interaction); 
+
+     } catch (GLib.IOError error) {

Leave off the GLib namespace.

@@ +444,3 @@
+     } catch (GLib.IOError error) {
+        critical ("D-Bus error: %s\n", error.message);
+        return;

This return is unnecessary.

Created attachment 206516 [details] [review]
Updated patch based on review in comment https://bugzilla.gnome.org/show_bug.cgi?id=668072#c9

Comment on attachment 206516 [details] [review]
Updated patch based on review in comment https://bugzilla.gnome.org/show_bug.cgi?id=668072#c9

Pushed a modified patch to master as commit b2c1cec7dd6f8df9e8e41341dab5d18e47b4f40e. Please be more careful with indentation and trailing whitespace in future.

Why o why do you need all that code to popup PackageKit? You should have done like Evolution, Rhythmbox or Totem and hidden the "Share..." menu item if nautilus-sendto wasn't available.

(In reply to comment #12)
> Why o why do you need all that code to popup PackageKit?

I do not think that 40 lines of code is an unacceptable amount of code to do the task at hand.

> You should have done
> like Evolution, Rhythmbox or Totem and hidden the "Share..." menu item if
> nautilus-sendto wasn't available.

Cheese previously made the share action insensitive if nautilus-sendto was not found, but using PackageKit provides some useful extra feedback. There is still some room for improvement, such as prompting for installation only on the first activation of the share action, and then hiding the action unless nautilus-sendto becomes available in the meantime.

Rhythmbox does not hide the share menu item from what I can see, but merely spawns nautilus-sendto unconditionally:

http://git.gnome.org/browse/rhythmbox/tree/plugins/sendto/sendto.py

I cannot find any nautilus-sendto functionality in Totem, did you mean the publish plugin, using libepc? The Evolution nautilus-sendto plugin acts as you describe:

http://git.gnome.org/browse/evolution/tree/widgets/misc/e-attachment-handler-sendto.c

While Cheese could follow the Evolution behaviour, I do not see a good justification for why it should do so, and I think that the current behaviour is an improvement.

(In reply to comment #13)
> (In reply to comment #12)
> > Why o why do you need all that code to popup PackageKit?
> 
> I do not think that 40 lines of code is an unacceptable amount of code to do
> the task at hand.

It's also a problem that should be solved at the distribution level. Either it's required, or not.

> > You should have done
> > like Evolution, Rhythmbox or Totem and hidden the "Share..." menu item if
> > nautilus-sendto wasn't available.
> 
> Cheese previously made the share action insensitive if nautilus-sendto was not
> found, but using PackageKit provides some useful extra feedback. There is still
> some room for improvement, such as prompting for installation only on the first
> activation of the share action, and then hiding the action unless
> nautilus-sendto becomes available in the meantime.
> 
> Rhythmbox does not hide the share menu item from what I can see, but merely
> spawns nautilus-sendto unconditionally:
> 
> http://git.gnome.org/browse/rhythmbox/tree/plugins/sendto/sendto.py
> 
> I cannot find any nautilus-sendto functionality in Totem, did you mean the
> publish plugin, using libepc? The Evolution nautilus-sendto plugin acts as you
> describe:
> 
> http://git.gnome.org/browse/evolution/tree/widgets/misc/e-attachment-handler-sendto.c
> 
> While Cheese could follow the Evolution behaviour, I do not see a good
> justification for why it should do so, and I think that the current behaviour
> is an improvement.

Popping up dialogue to implement basic functionality isn't an improvement.

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Why o why do you need all that code to popup PackageKit?
> > 
> > I do not think that 40 lines of code is an unacceptable amount of code to do
> > the task at hand.
> 
> It's also a problem that should be solved at the distribution level. Either
> it's required, or not.
> 
> > While Cheese could follow the Evolution behaviour, I do not see a good
> > justification for why it should do so, and I think that the current behaviour
> > is an improvement.
> 
> Popping up dialogue to implement basic functionality isn't an improvement.

While the functionality might be basic, it is not essential and it is not currently a hard requirement. It is possible to request that PackageKit should not prompt for confirmation before installing a package, but this seems a little magic, as in that case (for me) only a PolicyKit authentication dialog is displayed, which does not give as much information as the PackageKit dialog, such as the name of the package that will be installed and which application is requesting the installation.

However, it would be better to use the default interaction mode, rather than hardcoding one, so I changed that in commit 35b3ce67fafdff135005c410b6b9484a77d10521 on master.

I removed the sharing support in commit ca982ad034dc183d072ee1bd19ab235a5a729d07, so this is now obsolete.