GNOME Bugzilla – Bug 344300
Make CD recorder a plugin
Last modified: 2006-07-31 11:07:01 UTC
I'd been wondering for a while whether I could disable the CD-burning support in Rhythmbox entirely, as I never use it (my system doesn't even have a burner). As it turns out I couldn't, so I've written a patch to support this. Default behaviour is as current, but with the --disable-nautilus-burn flag given to configure, CD burning is switched off entirely. As well as the patch enclosed here, you'll also need the rhythmbox-ui.xml.in and sifter.pl (used to generate rhythmbox-ui.xml from the .in with or without burner menu/popup options). Both of those need to be placed in data/ui/
Created attachment 66983 [details] [review] Patch to implement this feature
Created attachment 66984 [details] Belongs in data/ui/, derived from rhythmbox-ui.xml
Created attachment 66985 [details] Script to generate rhythmbox-ui.xml from the .in attached earlier
I don't think this is the right way to go about it. I think it'd be better to move the audio cd source and the cd burning stuff into a plugin. I haven't looked at how feasible this is.
Would it be possible to ask HAL if there is a CD writer available and disable the 'write' the functionality if there is none ?
(In reply to comment #5) > Would it be possible to ask HAL if there is a CD writer available and disable > the 'write' the functionality if there is none ? Yes, but that isn't the actual aim of the patch. What the reporter wants (and would be good if it is done cleanly) is to have libnautilusburn be an optional dependency, rather than a hard dependency.
Created attachment 68637 [details] [review] patch to make cd-recorder plugin Look OK?
Looks fairly good, with a couple of nitpicks. plugins/cd-recorder/Makefile.am: "uixmldir = $(pkgdatadir)" should probably be "uixmldir = $(pkgdatadir)/plugins/cd-recorder/", which is where plugin-specific data files should go. But rb_plugin_find_file does look in the former too. It's probably better to build rb-recorder-gst.c and rb-playlist-source-recorder.c et al out-of-directory until we can move them properly (cvs surgery or after the change to svn), so the history doesn't get lost like it does with "cvs remove; cvs add". It would be good to make rb-recorder-gst use rb-encoder, so it doesn't have to depend on gst. This is orthogonal to making it a plugin, and may require some rb-encoder improvements, so I've filed bug 347012 for that.
Created attachment 68670 [details] [review] patch How's this?
Looks good. Another two things I just noticed: The "Burn CD" menu item gets put at the bottom of the menu, which is next to the "Delete" item in the context menu. It might be worth adding some placeholder items to them in the main UI .xml file, so we can put it in the middle (or at last add a separator). RBPlaylistSource still has the burning action in the source-specific toolbar thing, which causes a GTK warning to be emitted. Currently there isn't a way for plugins to add items to the source-specific toolbar - perhaps we should change it to emit a "collect toolbar actions" signal, and have signal handlers return a list of them, which the accumulator inserts into the toolbar?
Created attachment 68683 [details] [review] patch Uses placeholders for ui merging and removes the source specific action for now. I suppose the signal approach would work. Or perhaps the plugin should just do ui merging itself when the selected source changes.
Looks good to me. Yeah, doing the UI merging itself is probably better than screwing around with signals - unless we want it to be able to override the source and not have the normal ones.
2006-07-09 William Jon McCann <mccann@jhu.edu> * Makefile.am: * configure.ac: * data/ui/rhythmbox-ui.xml: * plugins/Makefile.am: * plugins/cd-recorder/.cvsignore: * plugins/cd-recorder/Makefile.am: * plugins/cd-recorder/cd-recorder.rb-plugin.desktop.in: * plugins/cd-recorder/rb-cd-recorder-plugin.c: (rb_cd_recorder_plugin_class_init), (rb_cd_recorder_plugin_init), (rb_cd_recorder_plugin_finalize), (burn_source_iter_func), (source_burn), (cmd_burn_source), (playlist_entries_changed), (playlist_row_inserted_cb), (update_source), (shell_selected_source_notify_cb), (impl_activate), (impl_deactivate): * shell/Makefile.am: * shell/rb-playlist-manager.c: (rb_playlist_manager_set_source), (rb_playlist_manager_cmd_save_playlist): * shell/rb-shell.c: (rb_shell_select_source): * sources/Makefile.am: * sources/rb-playlist-source-recorder.c: (rb_playlist_source_recorder_new): * sources/rb-playlist-source.c: (rb_playlist_source_class_init), (rb_playlist_source_add_to_map): * sources/rb-playlist-source.h: Move cd burning into a plugin. Fixes #344300. There are a couple of other places that nautilus-burn is used too.
Problems compiling when using a separate build directory (probably just needs a $(top_builddir)/lib in the Makefile.am to fix): /home/alex/src/remote-cvs/gnome.org/rhythmbox/player/rb-recorder-gst.c:55:24: error: rb-marshal.h: No such file or directory /home/alex/src/remote-cvs/gnome.org/rhythmbox/player/rb-recorder-gst.c: In function 'rb_recorder_class_init': /home/alex/src/remote-cvs/gnome.org/rhythmbox/player/rb-recorder-gst.c:169: error: 'rb_marshal_VOID__DOUBLE_LONG' undeclared (first use in this function) /home/alex/src/remote-cvs/gnome.org/rhythmbox/player/rb-recorder-gst.c:169: error: (Each undeclared identifier is reported only once /home/alex/src/remote-cvs/gnome.org/rhythmbox/player/rb-recorder-gst.c:169: error: for each function it appears in.) /home/alex/src/remote-cvs/gnome.org/rhythmbox/player/rb-recorder-gst.c:201: error: 'rb_marshal_BOOLEAN__BOOLEAN_BOOLEAN_BOOLEAN' undeclared (first use in this function) /home/alex/src/remote-cvs/gnome.org/rhythmbox/player/rb-recorder-gst.c:213: error: 'rb_marshal_INT__VOID' undeclared (first use in this function) make[3]: *** [rb-recorder-gst.lo] Error 1
Created attachment 68695 [details] [review] Allow for separate build directory
I'd just committed that change to cvs :)
Marking FIXED since it is.