GNOME Bugzilla – Bug 522210
Should be able to share videos and images through different technologies
Last modified: 2012-01-16 21:34:30 UTC
and possibly other websites. See: http://www.youtube.com/blog?entry=yFlR6EEySg8 and: bug 488827
Make that bug 459505.
conduit would allow uploading to youtube. i think it would be better to use a framework, than every app doing its own stuff... what do you think?
Fine by me, making it a blocker.
*** Bug 530609 has been marked as a duplicate of this bug. ***
I just merged video upload support into conduit. I notice there is two branches of cheese currently (trunk and vala). I think I will work on adding this feature to cheese. Which branch should I target?
John, I'd target the trunk branch first for the moment. On the vala branch I'm still working on getting the features on par with trunk, but I would appreciate if you also write the patch for the vala branch. Jaap
do you think this bug report is still relevant?
Yes. Did you read the bug? :) It's still waiting on the Cheese side of things.
i just thought, that people dont want to have that feature anymore ;) hmm.. now whats the best way to do that?
Not sure conduit is the best way anymore. Philip, is there a way to do this in libgdata? I notice that google CL can do that: http://lifehacker.com/5568817/five-really-handy-google-command-line-tricks This could be implemented as part of nautilus-sendto as well, though we'll need mime-types filtering. See bug 622457. Then somebody would need to implement a "youtube-sendto".
Yeah, conduit is basically unmaintained at this point. No other apps use it as a service and I can't really be bothered porting it from PyGtk to Gtk-3.0. I encourage cheese to implement this as needed themselves.
(In reply to comment #10) > Not sure conduit is the best way anymore. > > Philip, is there a way to do this in libgdata? I notice that google CL can do > that: > http://lifehacker.com/5568817/five-really-handy-google-command-line-tricks Yup: gdata_youtube_service_upload_video(). I could quite easily add an async version too. > This could be implemented as part of nautilus-sendto as well, though we'll need > mime-types filtering. See bug 622457. Then somebody would need to implement a > "youtube-sendto".
Is Shotwell's Youtube upload enough to handle this?
Created attachment 204318 [details] [review] Add sharing videos and pictures support This patch adds support for sharing videos and images through the integration of nautilus-sendto in Cheese. Nautilus-sendto is not completely working though, so I will also spend some time on it.
Created attachment 204319 [details] [review] Add sharing videos and pictures support
Sorry, I forgot to add the bug number :))
Review of attachment 204319 [details] [review]: As we discussed on IRC, it makes sense to check for nautilus-sendto during configure, and warn if it is not available. The runtime dependency should also be mentioned in the README. Additionally, it might make sense to use PackageKit to try to install nautilus-sendto if it is not available. ::: Makefile.am @@ +67,3 @@ --pkg cheese-thumbview \ + --pkg cheese-common \ + --pkg gdk-x11-3.0 Should indent with a tab, not spaces. @@ -191,2 +192,3 @@ src/cheese-window.vala \ src/cheese-main.vala \ + src/cheese-shareable-media.vala \ Also use a tab here. ::: src/cheese-shareable-media.vala @@ +1,1 @@ +/* Used for sharing images or videos within different technologies The usual copyright header needs to be included, with your name, email address and year at the top. @@ +2,3 @@ + * through nautilus-sendto + */ +namespace ShareableMedia { Not sure if it makes sense to keep this in a separate namespace. I think that a class Cheese.ShareableMedia would be better. @@ +6,3 @@ + private const string XID = "--xid="; + private Cheese.MainWindow window; + private int num_children = 0; This should be an unsigned int. @@ +8,3 @@ + private int num_children = 0; + + void child_finished (Pid pid, int status) There should be some Valadoc comments here. @@ +11,3 @@ + { + // status will be 0 if the child terminates normally: + // calling exit(3) or _exit(2), or by returning from main() Careful here, as this is not what the waitpid() manpage says! The status is an integer that can be checked with various macros, which are described in waitpid(2). The exit code is merely the least significant 8 bits of the int, so this test is wrong. The test macros do not appear to be part of the POSIX VAPI: http://valadoc.org/posix/index.htm so they will probably need to be added, maybe first to cheese-common.vapi, before you can use them. @@ +13,3 @@ + // calling exit(3) or _exit(2), or by returning from main() + if (status != 0) + warning ("A problem occured while loading the file(s)"); It is better to be more specific here: ‘nautilus-sendto exited abnormally’ or similar. @@ +22,3 @@ + } + + public void files_share (Cheese.MainWindow mainWindow, GLib.List<GLib.File> files) Also need a Valadoc comment here. Additionally, use main_window, not mainWindow. @@ +50,3 @@ + } + + } catch (Error error) { This should be the specific error domain, which is SpawnError: http://valadoc.org/glib-2.0/GLib.Process.spawn_async.html
Created attachment 204350 [details] [review] Updated patch based on review in comment https://bugzilla.gnome.org/show_bug.cgi?id=522210#c17 In this patch I fixed all the suggestions done in https://bugzilla.gnome.org/show_bug.cgi?id=522210#c17. Nautilus-sendto does not appear yet as a run-time dependency since this is something that I will be able to do just once bug 667030 has been resolved. I have written a short text in the TODO and the README files about this issue.
Created attachment 204865 [details] [review] Add sharing videos and pictures support Since bug 667030 has been resolved, I updated the patch with a check in the configure.ac file for the nautilus-sendto run time dependency. Now the patch is complete.
instead of checking for nautilus-sendto on configure you could also hide the menu entry if n-s can't be found on the system during run time
Created attachment 204889 [details] [review] Add sharing videos and pictures support Inspired on Daniel's advice, I included a new check to decide if the "Share" action will show sensitive, in case nautilus-sendto is installed, or insensitive, if it is not.
awesome patricia! that seems to work quite good. now do you think we still have to check on configure time? afaik i understand n-s is no real run time dependency anymore?
I think it is very useful :)), since this way we are doing two things, on one hand, we are explaining the user why the 'Share' action is shown insensitive and on the other hand, we are informing about the package name and version that is needed in order to make use of this functionality.
Review of attachment 204889 [details] [review]: In addition to the specific comments, I would say that the ShareableMedia functionality should be a member of MainWindow. ::: configure.ac @@ -148,0 +148,5 @@ +# Check for nautilus-sendto runtime dependency. +NAUTILUS_SENDTO_REQUIRED=2.91.0 +AC_PATH_PROGS([NAUTILUS_SENDTO], [nautilus-sendto], [notfound]) ... 2 more ... A better warning would be ‘Unable to find nautilus-sendto in the path’. You could also mention that it is a runtime dependency, but only for the sharing functionality (in addition to what wasw laready mentioned in the README). @@ +151,3 @@ +AS_IF([test "x$NAUTILUS_SENDTO" = "xnotfound"], + [AC_MSG_WARN([Unable to check for nautilus-sendto >= $NAUTILUS_SENDTO_REQUIRED])], + [NAUTILUS_SENDTO_SYSTEM=`nautilus-sendto --version | sed 's/nautilus-sendto //g'` Rather than ‘sed’, use ‘$SED’. @@ -148,0 +148,7 @@ +# Check for nautilus-sendto runtime dependency. +NAUTILUS_SENDTO_REQUIRED=2.91.0 +AC_PATH_PROGS([NAUTILUS_SENDTO], [nautilus-sendto], [notfound]) ... 4 more ... Some extra quotes (square brackets, []) around the macro arguments would make things more consistent. @@ +153,3 @@ + [NAUTILUS_SENDTO_SYSTEM=`nautilus-sendto --version | sed 's/nautilus-sendto //g'` + AS_VERSION_COMPARE($NAUTILUS_SENDTO_SYSTEM, $NAUTILUS_SENDTO_REQUIRED, + COMPARE=0, ‘COMPARE’ would be better as ‘NS_VERSION_COMPARE’, or something similar. @@ +157,3 @@ + COMPARE=1) + AS_IF([test $COMPARE = 0], + [AC_MSG_WARN([Unable to check for nautilus-sendto $NAUTILUS_SENDTO_SYSTEM <= $NAUTILUS_SENDTO_REQUIRED])])]) A better error message would be ‘Unable to check the version of nautilus-sendto, it is probably too old. At least $NAUTILUS_SENDTO_REQUIRED is required’. ::: data/cheese-actions.ui @@ -254,2 +262,3 @@ <menuitem action="open"/> <separator/> + <menuitem action="share"/> The share action should also be added to the ‘Edit’ menu. ::: src/cheese-shareable-media.vala @@ +18,3 @@ + */ + +static uint num_children = 0; This is class private data, and should not be static and outside the class. @@ +86,3 @@ + { + string[] argv = {}; + argv += SENDTO_EXEC; I see that SENDTO_EXEC was defined in MainWindow, and not this class. There should be a comment to explain where this is defined. ::: src/cheese-window.vala @@ +354,3 @@ + public void on_files_share (Gtk.Action action) + { + ShareableMedia shareable_media = new Cheese.ShareableMedia (this); This instantiates a new instance of ShareableMedia each time that the share action is called, which suggests that all the methods inside ShareableMedia could just be static. @@ +1273,3 @@ "about", "open", + "share", If nautilus-sendto was not installed, and then the widget sensitivity was updated, for example by taking a photo, the share action would then be set to sensitive. This is not the correct behaviour, so there needs to be either some persistence of whether nautilus-sendto was found in the path, or it should be checked each time that the sharing action is displayed. @@ +1403,3 @@ + share_action = gtk_builder.get_object ("share") as Gtk.Action; + + bool nautilus_sendto_installed = Environment.find_program_in_path(SENDTO_EXEC) != null; Of course, this only checks at startup time, which deserves a comment.
Created attachment 205396 [details] [review] Updated patch based on review in comment https://bugzilla.gnome.org/show_bug.cgi?id=522210#c24 Apart from the fixes done based on Dave comments I checked that the 'Share' action was not shown sensitive if there were no elements selected by the user.
Comment on attachment 205396 [details] [review] Updated patch based on review in comment https://bugzilla.gnome.org/show_bug.cgi?id=522210#c24 Pushed to master as commit b7079b4b55dac0fc5b8a4c0a5b0c420736e22677. I had to remove the configure check for nautilus-sendto, as it did not successfully check for an old version. It can be added later.