A few obvious questions: What advantages does brasero provide over nautilus-cd-burner?  How do applications use it?  How is the user experience different?

I don't think there's much point in maintaining two CD burning code bases, so if using brasero seems like a better approach, I think the CD burning plugin should be rewritten to use it.

Hi Jonathan

It provides the advantage of being a full burning application, this means it can do checksum for cd's. It contains an important feature for audio cds that is the audio normalization and files preview.
Exaile (plugins) and Banshee behavior is selecting the files and import to brasero calling "brasero -a" and it imports all the songs to brasero.
I don't say the user experience will improve because n-c-b is very very easy and simple to work but i say the user will have a simple gui with the advantage of having a full burning app that provides interesting features. 

Somebody is more than welcome trying to write a plugin for that then.

OK, here is the promised plugin.
It was developped by reusing some code from the old nautilus-cd-burner plugin. One difference is that it spawns brasero which means dialogs are not modal (which is a good thing IMO).

It was developped in a separate directory so as to be side by side installable (I changed the name to DiscRecorder to do so).

Please try the attached patch (works with rhythmbox trunk). Be careful though, you need brasero from SVN trunk at least r1558 for it to work.

At the moment the "workflow" is very similar to the one provided by the other plugin. After clicking on burn/copy in rhythmbox you're taken straight to the burn option dialog of brasero (you might see a dialog computing project size in between depending you have mp3 or not in the playlist).

- CD-TEXT info is provided by the plugin from rhythmbox and written to disc by brasero
- likewise the plugin tries to find a cover so that after a successful burning brasero can automatically set it in the cover editor and print it. Otherwise, the user is still left with the possibility to create one.
- normalizing (used only in playlist burning)

Created attachment 123212 [details] [review]
Proposed patch against trunk (-p0)

Is it possible to include this on rb source and we can maintain it or should we add this to an "unofficial" rb plugin ?

I can't see why rb shouldn't support brasero as well. This should be the same behaviour as banshee.

I haven't looked at this in much detail, but the retrieve_cover and retrieve_disc_cover methods can't go looking in the cover art directory like that.  The file naming scheme is an implementation detail of the cover art plugin, and I don't want to have multiple copies of the code for dealing with it.
Instead, it should work like the ipod plugin's request_artwork function.

Thanks for the review. I have to agree with you on that and I wasn't aware that such an API existed. Thanks for the heads up. I'll work on that and you can expect a new version for the weekend hopefully.

Created attachment 125186 [details] [review]
new patch

Here it is, a bit later than I expected. This new patch is mainly an updated version of the previous one (updated icon used, updated UI paths for GtkUIManager, some cleaning) and I removed the cover retrieval code. I did try to do something similar to the ipod plugin for covers; the problem is, it is not possible to retrieve a cover URI from cache for a song that is not being played by rhythmbox which makes this useless when you want to get the covers for each song in a playlist. I checked ArtCover code (though I'm not fluent in python) and what I understood of it confirmed the above. I'll try to work around this in brasero itself for 0.9.
Of course, what it changed is just the fact that the back cover won't be set automatically for say a disc duplication. The user can still create a cover in brasero.

Now on this topic of covers, it could be good if there was a separate app (included in rhythmbox package) that could be used to retrieve URI for covers in rhythmbox cache based either on the information passed on the command line (author, album, ...) or straight from the URI (a lot more code and probably not the best).

Last, during my testing I noticed that in audiocd-ui.xml that you used:
<menuitem name="AudioCdSourcePopupCopyCd" action="MusicAudioCDDuplicate" />

and I was wondering if a placeholder wouldn't be more appropriate as it would avoid a warning if no plugin adds an action called "MusicAudioCDDuplicate".

By the way I'm about to add a switch to brasero to make it transient based on a xid. Would you rather have brasero transient to rhythmbox?

(In reply to comment #10)
> I did try
> to do something similar to the ipod plugin for covers; the problem is, it is
> not possible to retrieve a cover URI from cache for a song that is not being
> played by rhythmbox which makes this useless when you want to get the covers
> for each song in a playlist.

It's like this because initially the only use for cover art was displaying the image for the currently playing song.  There's no reason it couldn't provide cover art for any song, but no one has done the work yet.

I don't think there's much point in having a ./configure option to enable or disable the plugin.  If a user doesn't want it, they can leave the plugin disabled; if a distributor doesn't want to ship it, they can exclude the files from their packages.

+	/* As long as it's not a library I don't see how to do it differently */
+	gtk_icon_theme_append_search_path (gtk_icon_theme_get_default (),
+					   "/usr/share/brasero/icons");
+	gtk_icon_theme_append_search_path (gtk_icon_theme_get_default (),
+					   "/usr/local/share/brasero/icons");

Maybe this could work from the path to the brasero executable?  If the executable is in /opt/something/bin/, add /opt/something/share/brasero/icons to the icon search path.

> Now on this topic of covers, it could be good if there was a separate app
> (included in rhythmbox package) that could be used to retrieve URI for covers
> in rhythmbox cache based either on the information passed on the command line
> (author, album, ...) or straight from the URI (a lot more code and probably not
> the best).

I don't really want to get into this sort of business myself, but perhaps something like this will come out of the 'media art storage spec': http://live.gnome.org/MediaArtStorageSpec

Created attachment 125656 [details] [review]
Updated patch

Here is the new patch which addresses the 2 problems you raised.

And thanks for the link, the spec is promising. I could use it in brasero.

+	/* Translators: this is the toolbar button label for */
+	/* Create Audio CD action                            */
+	g_object_set (action, "short-label", _("Burn..."), NULL);
+
+        action = gtk_action_group_get_action (pi->action_group,
+					      "MusicAudioCDDuplicate");
+	/* Translators: this is the toolbar button label for */
+	/* Duplicate Audio CD action                         */
+	g_object_set (action, "short-label", _("Copy CD..."), NULL);

Philippe, as per GNOME HIG, toolbar buttons don't have to use trailing ellipsis.

Should be s/"Copy CD"/"Copy" too.

See http://library.gnome.org/devel/hig-book/stable/toolbars-labels-tooltips.html.en

Created attachment 126228 [details] [review]
Updated patch

Thanks for the heads up. I fixed it in the new version of the patch.

+plugins/disc-recorder/Makefile

Could you please call the plugin something else? There's already cd-recorder, call it brasero-disc-recorder or something.

Also, this plugin isn't quite good enough to replace ncb's, as long as it has to spawn a separate program to do the burning.

A lot of the code seems to be cut'n'paste from the original plugin, so the code should really be shared instead of duplicated.

Created attachment 126651 [details] [review]
version 4

This version moves plugin to brasero-disc-recorder directory and rename it.
It uses libbrasero-media (requires brasero trunk, upcoming 0.9.1) and drops HAL flags. It also fixes the problem with icon theme.
Finally, I reworked spawning to stick to what Bastien did for totem module. But I can't avoid spawning at the moment.

A few additional notes:
- icon theme: I read somewhere Bastien was not keen on brasero's handling of icon theme. See #374700 and #485484 for more details on why we did it.
- spawning: true, brasero is spawned but that's also the case when copying with ncb. Moreover, the current brasero plugin reduces the code size since all the audio transcoding is handled by brasero himself
- code sharing: in previous patch there is one thing Bastien required which was code sharing between the two plugins. Actually, I read the code and it doesn't seem to me there is much to share or that it's actually worth the work. Now if you really insist on it I can do it.
- licence: relicencing of libbrasero-media has started. I'll include the same restrictions rhythmbox uses. All of its files (but one, as unfortunately the copyright holder hasn't answered me yet) should be relicenced for 0.9.1. The rest of brasero should follow but as I said I need more time there. If GNOME (and I wish it did) provided a more general and common licence with restrictions I'd update it of course.

What is the 'res' variable intended to do in the loop in rb_playlist_source_recorder_add_from_model?  It doesn't appear to be set anywhere, but it's used as a condition to break the loop.

It looks like this line:

+	if (pi->enabled && playlist_active /* && rb_recorder_enabled () */) {

in update_source() should call rb_disc_recorder_has_burner().

My concern about the icon theme was that rhythmbox had no way to determine the proper path to the brasero icons.  Since the brasero library always knows the right path, I'm fine with it now.  I'm not sure what Bastien's concerns were.

Created attachment 127214 [details] [review]
Updated patch

The res thing is probably a leftover from the time when I tried to get the cover.
Thanks for your review.

btw, libbrasero-media is now fully GPL v2 + restrictions (actually copied from rhythmbox).

Just a couple of potshots:
-NCB_MIN_REQS=2.21.6
+BRASERO_MIN_REQS=0.9.1

This makes the ncb detection always fail.

-if test "x$have_libnautilus_burn" = "xyes" -a "x$with_cd_burning" != "xno"; then
+if test "x$have_libnautilus_burn" = "xyes" -o "x$have_libbrasero_media" = "xyes" -a "x$with_cd_burning" != "xno"; then

This is probably not something you want to do unless you have a great understanding of operator precedence algorithms in various implementations of test (eg. it always fails)

+elseif HAVE_LIBBRASERO_MEDIA 

That doesn't exist and creates a broken Makefile.

I've fixed those locally.

Given that your current work seems to completely replace the ncb support that was in rhythmbox already, we have a few problems.

1.
Getting a few warnings on startup:
(lt-rhythmbox:18152): Gtk-WARNING **: AudioCdSourcePopupCopyCd: missing action MusicAudioCDDuplicate

Your code probably only works when ncb already adds the action (see the uses of rb_cd_recorder_plugin_actions).

2.
When trying to burn a long CD, I get:
Unable to create audio CD.
This playlist is too long to write to an audio CD.

Whereas I used to get:
This playlist is %s minutes long. If the destination media is larger than a standard audio CD please insert it in the drive and try again.

3.
Missing changes to the schemas to make the brasero plugin hidden and builtin.

Created attachment 128021 [details] [review]
rb-brasero-build-fixes.patch

With the fixes mentioned in comment 24

Thanks for updating the patch and fixing the issues you mentionned.

(In reply to comment #25)
> Given that your current work seems to completely replace the ncb support that
> was in rhythmbox already, we have a few problems.
> 
> 1.
> Getting a few warnings on startup:
> (lt-rhythmbox:18152): Gtk-WARNING **: AudioCdSourcePopupCopyCd: missing action
> MusicAudioCDDuplicate
> 
> Your code probably only works when ncb already adds the action (see the uses of
> rb_cd_recorder_plugin_actions).

I think (but I may be wrong again) it was probably triggered by the fact that when you started rhythmbox the first time, brasero plugin was not enabled yet. And since that action is hardcoded, it is expected to be supplied by a plugin hence the warning.
I do indeed have the warning when there are no burning plugin (ncb/brasero) enabled. But if brasero is enabled then I don't have it.

What could be done here to quiet that warning is to use a placeholder instead of a hardcoded action. In this case we wouldn't have a warning even if no burning plugins (ncb or brasero) have been enabled (like I did in the totem patch).

> 2.
> When trying to burn a long CD, I get:
> Unable to create audio CD.
> This playlist is too long to write to an audio CD.
> 
> Whereas I used to get:
> This playlist is %s minutes long. If the destination media is larger than a
> standard audio CD please insert it in the drive and try again.
> 

That's one more mistakes. This check should be removed from the plugin. We shouldn't try to handle that case here as brasero handles that size problem much more gracefully (allowing for overburning and such in that case).

There would be an interesting change to make to brasero to handle that case. Instead of blocking and erroring out because the disc is too small we could:
- show a warning box (something like "your selection is too large for the inserted disc. Would you like to burn it to multiple CDs?")
- if the user clicks yes
- burn what we can (keeping the right order) then ask for another CD, burn what we can, and so on until we don't have any more song left to burn.

> 3.
> Missing changes to the schemas to make the brasero plugin hidden and builtin.
> 

I missed this part indeed.

Well it looks like I've got work for the weekend.

Created attachment 128220 [details] [review]
Updated patch #6

Here is the patch updated to fix the three issues mentioned above.

NOTES:
- I fixed the warning with using a placeholder and I updated ncb plugin to make use of it.
- The idea I mentioned above can't be implemented for brasero 2.25.x/2.26.x as UI is frozen. So that's something I'll keep in mind for next developement cycle.

(In reply to comment #29)
<snip>
> - The idea I mentioned above can't be implemented for brasero 2.25.x/2.26.x as
> UI is frozen. So that's something I'll keep in mind for next developement
> cycle.

Rhythmbox isn't in the Desktop release, so you can implement this right now :)

This seems to work pretty well now.  I'd like to get this committed before I release 0.12.0 (soon, hopefully).  Since brasero is apparently replacing ncb altogether, we should favour building the brasero plugin over the ncb plugin, not the other way around as it currently does.

Since this completely replaces the ncb plugin, we should set the plugin name, file name, and install location to be the same so you can only have one installed at a time.  So, the .rb-plugin file should be named cd-recorder.rb-plugin, the shared library should be libcd-recorder.so, and the install directory should be $(PLUGINDIR)/cd-recorder.

We've never had a situation before where one implementation of a plugin replaces another, particularly where the old implementation stays around for older systems, so I'm not entirely sure it'd work..

Some review comments I've already taken care of in my own copy of the patch:
- there's no need for the USE_CD_BURNING bit in configure.ac any more
- rather than converting the playlist query model to a list of RBRecorderSong structures, then using that to write the project file, we could write the project file directly from the query model

(In reply to comment #31)
> This seems to work pretty well now.  I'd like to get this committed before I
> release 0.12.0 (soon, hopefully).  Since brasero is apparently replacing ncb
> altogether, we should favour building the brasero plugin over the ncb plugin,
> not the other way around as it currently does.
> 
> Since this completely replaces the ncb plugin, we should set the plugin name,
> file name, and install location to be the same so you can only have one
> installed at a time.  So, the .rb-plugin file should be named
> cd-recorder.rb-plugin, the shared library should be libcd-recorder.so, and the
> install directory should be $(PLUGINDIR)/cd-recorder.
> 
> We've never had a situation before where one implementation of a plugin
> replaces another, particularly where the old implementation stays around for
> older systems, so I'm not entirely sure it'd work..
> 
> Some review comments I've already taken care of in my own copy of the patch:
> - there's no need for the USE_CD_BURNING bit in configure.ac any more
> - rather than converting the playlist query model to a list of RBRecorderSong
> structures, then using that to write the project file, we could write the
> project file directly from the query model
> 
agreed.

I won't be available next week and hardly for the week after. I can't update the patch to match all the requirements listed above but if you can wait I will do it gladly when I'm free.


As a side note:
Bastien I tried to implement the multi disc burning feature but it led nowhere if this is implemented in rhythmbox. This should really be implemented in brasero as otherwise the user get the option dialog for every disc he wants to burn.

Created attachment 129711 [details] [review]
updated

I've cleaned up a few things and modified the plugin/file names to match the ncb plugin.  Seems to work OK.

I fixed up a few more things (error checking in a few cases - g_mkstemp returns -1 on error, and return values from xmlSaveDoc and close() needed to be checked; some message strings too) and committed it.

2009-03-08  Jonathan Matthew  <jonathan@d14n.org>

        patch by:  Philippe Rouquier <brasero-app@wanadoo.fr>, Bastien Nocera
        <hadess@hadess.net> and me

        * plugins/cd-recorder/rb-cd-recorder-plugin.c:
        * plugins/audiocd/audiocd-ui.xml:
        Slight adjustment to the way the 'duplicate CD' menu item is created;
        should stop things complaining when no CD burning plugin is enabled.

        * configure.ac:
        * plugins/Makefile.am:
        * plugins/brasero-disc-recorder/Makefile.am:
        * plugins/brasero-disc-recorder/cd-recorder.rb-plugin.in:
        * plugins/brasero-disc-recorder/rb-disc-recorder-plugin.c:
        New CD burning plugin using Brasero.  If libbrasero-media is 
        available, this plugin is built in preference to the 
        nautilus-cd-burner plugin.  The two plugins use the same plugin name,
        and install to the same location, so only one can be available at a 
        time.  Fixes #536732.

Perhaps an unwelcome change;

As a user, i'd like to make a comment on this;

It is all very fine to 'upgrade' a feature. But for me, someone who has nothing but trouble from brasero (which has come across on my system as buggy and unstable), the previous use of nautilus cd burner on rhythmbox was a saving grace. It meant that I was actually able to burn a CD, because Brasero wouldn't work. 

It works now in the current version of linux I am using, but It makes me worried that, as far as I can see, there is no way to revert to the older plugin! I see the new brasero plugin not listed in the 'plugins' of Rhythmbox, and I see no way in switching from one to the other in the preferences. 

Should a future version of Ubuntu (which i use) come shipped with yet another buggy version of brasero (lucid's version doesn't seem to work so well) then I am up the proverbial creek without a paddle and all that. 

Could the situation be improved please? An option to switch please?!

If you have problems with brasero, you should provide bug reports and help get them fixed.  The nautilus CD burner code (not just the rhythmbox plugin that uses it) is unmaintained and probably won't be available in most distributions in the future.