GNOME Bugzilla – Bug 565383
Support for brasero (Video DVDs / (S)VCDs burning)
Last modified: 2009-01-16 17:51:10 UTC
As part of the ongoing effort to integrate brasero in a better way with GNOME desktop, here is a patch against latest totem SVN trunk to add a plugin that allows to burn Video DVDs and (S)VCDs in totem using brasero. This is done by writing a brasero project and then spawning a brasero process that reads the project and behaves accordingly. A few remarks: - I didn't add anything to the sidebar but I modified totem.ui to add two placeholders so as to add two menu entries when the plugin is enabled. ATM brasero hasn't been split between library and app and that shouldn't happen before 0.9 is out. When brasero is fully split it could be a good idea to create a full sidebar widget instead. - something you may not like: since brasero is spawned, it's not possible to set the brasero option dialog as modal for totem which means that if ontop plugin is enabled, the brasero option dialog goes under totem window. As we can consider that if the user clicked on the menu entry it's to see the said brasero option dialog, I chose to call gtk_window_set_keep_above (totem, FALSE) when we successfully spawned brasero. I don't see any other way to do it... Any idea? Review, suggestions, criticism ... are welcome. Thanks for your time.
Created attachment 125151 [details] [review] The said patch
I forgot: this patch allows to burn the current opened file. I had also in mind to allow playlist file burning but since I can't access the GTKUIManager for the playlist, I gave up.
+ plugin_error_or_ignore "you need glib >= 2.15.0 to use the thumbnail plugin" This is obviously wrong. The "evil" part of the code should commented out. Pass the XID of the main totem window to your process, and make sure it's parented properly. I won't accept such a thing in Totem itself. I'd rather you used libnautilus-burn in totem_disc_recorder_has_burner(), as it would remove the hal dependency in totem itself (it has no hal dep in 2.25.x). Can it copy video DVDs? Is not displaying the menu item when there's no burner, considering it could be written to a file image?
(In reply to comment #3) > + plugin_error_or_ignore "you need glib >= 2.15.0 > to use the thumbnail plugin" > This is obviously wrong. > Stupid me, I cut and pasted too fast. Sorry about that. > The "evil" part of the code should commented out. Pass the XID of the main > totem window to your process, and make sure it's parented properly. I won't > accept such a thing in Totem itself. > OK, then that means this plugin will depend of 0.8.5. Since this is not currently supported by brasero. > I'd rather you used libnautilus-burn in totem_disc_recorder_has_burner(), as it > would remove the hal dependency in totem itself (it has no hal dep in 2.25.x). > See below. > Can it copy video DVDs? Yes, it can copy DVD and I realized this morning that I had forgotten that aspect of things. I was about to propose a new patch including this anyway. > Is not displaying the menu item when there's no burner, > considering it could be written to a file image? > Yes, I had not thought about that. That would solve the problem with HAL dependency since we only use it to check the existence of a burner. No need for this check, no need for HAL. Nice.
Created attachment 125308 [details] [review] Updated patch This patch adds video disc duplication, removes the code that unset ontop property for totem, removes HAL dependency and fixes all raised problems in comment #3. It also sets brasero (needs latest trunk) transient to totem. Comments welcome.
Comment on attachment 125308 [details] [review] Updated patch >Index: configure.in >=================================================================== >--- configure.in (révision 5871) >+++ configure.in (copie de travail) >@@ -50,7 +50,7 @@ > AC_SUBST(TOTEM_VERSION_MICRO) > > # The full list of plugins >-allowed_plugins="thumbnail screensaver ontop galago gromit lirc media-player-keys mythtv properties sidebar-test skipto sample-python sample-vala bemused youtube publish tracker pythonconsole jamendo opensubtitles" >+allowed_plugins="thumbnail screensaver ontop galago gromit lirc media-player-keys mythtv properties sidebar-test skipto sample-python sample-vala bemused youtube publish tracker pythonconsole jamendo opensubtitles disc-recorder" Could you rename the plugin to "brasero-disc-recorder", or something to that effect? <snip> >+static GtkActionEntry totem_disc_recorder_plugin_actions [] = { >+ { "VideoBurnToDisc", "media-optical-video-new", N_("_Create Video Disc..."), NULL, >+ N_("Create a video DVD or a (S)VCD from the currently opened media stream"), We call it a movie everywhere else in the interface, so please do as well. >+ G_CALLBACK (totem_disc_recorder_plugin_burn) }, >+ { "VideoDiscCopy", "media-optical-copy", N_("Copy Vide_o Disc..."), NULL, >+ N_("Copy the currently playing video DVD or (S)VCD"), We know whether we have a DVD or (S)VCD, so could you make sure the correct type of disc is mentioned in the UI. <sni> >+GQuark >+totem_disc_recorder_error_quark (void) >+{ >+ static GQuark quark = 0; >+ if (! quark) { >+ quark = g_quark_from_static_string ("totem_disc_recorder_error"); I'm pretty sure that would crash (or make valgrind yell) when activating and deactivating the plugin, as the string is static to the plugin, and might get unloaded. <snip> >+ xid = gdk_x11_drawable_get_xid (GDK_DRAWABLE (GTK_WIDGET (totem_get_main_window (pi->totem))->window)); Split this up and add more error checking. Also note that you should change the requirements in the configure.in to use "gtk+-x11-2.0" as this just won't work on Windows or native MacOS X. >+ cmd = g_strdup_printf ("brasero -r %s -x %d", path, xid); Spaces in path? You'll need to escape this properly. <snip> >+ project = xmlNewTextWriterFilename (path, 0); I believe libxml2 can write directly to an fd, so I'd rather that be used instead of using the path. <snip> >+ if (!totem_is_paused (totem) && !totem_is_playing (totem)) { Why !is_paused? >+ action = gtk_action_group_get_action (pi->action_group, "VideoBurnToDisc"); >+ gtk_action_set_visible (action, FALSE); >+ action = gtk_action_group_get_action (pi->action_group, "VideoDiscCopy"); >+ gtk_action_set_visible (action, FALSE); >+ } >+ else { >+ gchar *mrl; >+ >+ /* NOTE: since we can write to an image then, it's always TRUE >+ * as long as there is a local regular file or a video disc. */ Can't brasero burn using gvfs provided fuse local filenames? In which case comparing file: is a bad idea... <snip> >+ /* Translators: this is the toolbar button label for */ >+ /* Create Audio CD action */ >+ g_object_set (action, "short-label", _("Burn"), NULL); What toolbar?
(In reply to comment #6) > Could you rename the plugin to "brasero-disc-recorder", or something to that > effect? Done. It's now brasero-disc-recorder. But do you intend to add another disc-recorder plugin? ;) > > We call it a movie everywhere else in the interface, so please do as well. Done, I removed the offending "video" word wherever I found it except in "video disc" in an error message. > We know whether we have a DVD or (S)VCD, so could you make sure the correct > type of disc is mentioned in the UI. Done. > I'm pretty sure that would crash (or make valgrind yell) when activating and > deactivating the plugin, as the string is static to the plugin, and might get > unloaded. Done. It didn't crash on my computer but I removed all the GError stuff anyway since that wasn't useful as we didn't pass the error to another part of totem. > Split this up and add more error checking. Also note that you should change the > requirements in the configure.in to use "gtk+-x11-2.0" as this just won't work > on Windows or native MacOS X. I tried to and hope it'll be OK for you the way I did it. At least it shouldn't crash any more > Spaces in path? You'll need to escape this properly. Done > I believe libxml2 can write directly to an fd, so I'd rather that be used > instead of using the path. I wasn't aware of that API and given the one I was using was so simple I had not looked further. I found indeed a way to use fds which should be used in brasero too =). > Why !is_paused? We are checking if a file mrl is available which implies that totem is either in playing or paused state. Am I wrong here? > Can't brasero burn using gvfs provided fuse local filenames? In which case > comparing file: is a bad idea... It does. The comment you're referring to, was an old comment and I don't honestly know what it means any more =(. > What toolbar? > rhythmbox's, that was a stupid cut and paste ....
Created attachment 125341 [details] [review] New patch (version 2) ... and here is the new version. Again comments welcome. Thanks.
Created attachment 125657 [details] [review] Updated patch This is an updated patch to be applied to trunk. It also improves slightly things since it doesn't hardcode path for brasero icon.
We are pretty excited with this, can we get this done before the freeze ?
+ /* As long as it's not a library I don't see how to do it differently */ Could you please move that gross hack to a separate function? + GtkAction *action; No spacing like that needed. +totem_disc_recorder_plugin_write_video_project (TotemDiscRecorderPlugin *pi, + gchar **error) or like that. + if (xid > 0) + cmd = g_strdup_printf ("brasero -c \"%s\" -x %i", device_path, xid); + else /* start brasero anyway but it'll probably be underneath totem window */ + cmd = g_strdup_printf ("brasero -c \"%s\"", device_path); That's still not good. Use g_shell_quote() and use g_spawn_async() instead of the command-line version. Finally, the error messages should use totem_interface_error() instead of home-grown version.
Created attachment 126425 [details] [review] New update This should fix all the issues you saw and tries to improve the situation for the icon theme.
Created attachment 126475 [details] [review] Last patch but with the correct spaces This is the same patch from Philippe but without the spaces Bastien mentioned.
(In reply to comment #13) > Created an attachment (id=126475) [edit] > Last patch but with the correct spaces > > This is the same patch from Philippe but without the spaces Bastien mentioned. It's missing all the new files.
I reworked the command launching code from the horrible code that it was, and removed the dependency on the media library. I still don't see it being available outside of SVN, and Brasero should really be installing its icons in the hicolor icon theme directory, instead of working around the problem. 2009-01-16 Bastien Nocera <hadess@hadess.net> Patch from Philippe Rouquier <bonfire-app@wanadoo.fr> and Luis Medinas <lmedinas@gnome.org> to add a Brasero plugin * configure.in: * data/totem.ui: Add placeholders for Burn items * src/plugins/brasero-disc-recorder/*: Add Brasero plugin to allow creating VCDs from playing video files, and duplicate VCDs and DVDs (Closes: #565383)
We will continue to submit patches as long as the plugin needs updates. Also we plan to release 0.9.1 with the library soon (around 2.25.5 time). Thanks Bastien for the review.
Thanks. As Luis said 0.9.1 will be out soon. Now, what didn't you like about that launching code (not bickering here, just trying to learn not to make the same mistake twice).
Loads of duplicated code in 2 functions, and hard-coded magic numbers (for the free loops).
Point taken. It does look nicer indeed. I hope to be able to replace that part in the short term with something linking with brasero burn backend/library. Thanks for your patience and suggestions.