Re: [Rhythmbox-devel] playlist source burner

[Colin Walters <walters verbum org>]

On Fri, 2004-08-27 at 18:04 -0400, William Jon McCann wrote:
> Hi,
> 
> I wrote a playlist source burner patch (for CVS head):
> http://acs.pha.jhu.edu/~mccannwj/rhythmbox/rb-burn-0.1.patch

Cool!

> TODO:
>   - verify the playlist size
>   - verify the temporary directory space available

Would be nice, yes.

> Otherwise, it seems to work pretty well.

Some comments:

I'd like to move rb-recorder* into player/ instead of lib/; The ultimate
goal would be for lib/ to not exist, it's mostly code that isn't
Rhythmbox specific and should be in some other library.  

I agree with Bastien about the temporary folder UI (and GConf key).
nautilus-cd-burner just uses g_get_tmp_dir, this should be fine.  I
don't see a reason why an even halfway modern system wouldn't have space
for a CD image on the normal temporary directory paths.  And if they
don't, then we can add some code to autodetect large local volumes.

I see you are including the patch to do DND on the filtered model in
this patch.  I guess that's a reminder for me to look at the issue :)
Still not convinced that's the right thing.

Also, instead of having a preference for the device in Rhythmbox, I
think this makes more sense inside the new "Removable Storage" capplet;
n-c-b could use it too.
Most people will only have one CD recorder, those few who have multiple
and want one other than the default can use the preference.

+        if (recorder->priv->src_uri)
+                g_free (recorder->priv->src_uri);

No need to do this, g_free does the NULL test itself.

+        if (audiocd_mode || iradio_mode) {
+                g_set_error (error,
+                             RB_RECORDER_ERROR,
+                             RB_RECORDER_ERROR_INTERNAL,
+                             _("No AudioCD or iradio support"));
+                return;
+        }

This seems more like a case for g_return_if_fail; the program logic
should prevent it from passing audiocd:// URIs.  Also we probably want
to allow http:// URIs, just hope they're DAV.  The whole http:// URI
thing is a bit of a mess...

+        recorder->priv->src_uri = g_strdup (src_uri);

Looks like you're leaking src_uri.

+        recorder->priv->dest_file = g_strdup (dest_file);
+        g_free (dest_file);

Could just do recorder->priv->dest_file = dest_file; also looks
like a leak of recorder->priv->dest_file.

+static void
+error_dialog (RBPlaylistSourceRecorder *source,
+              GtkWindow                *parent,
+              const char               *primary,
+              const char               *secondary,
+              ...)

Isn't this duplicating rb_error_dialog?

+        if (source->priv->songs)
+                g_slist_free (source->priv->songs);

The test is unneeded here too.


On a larger issue, have you investigated how much code-sharing is
possible with n-c-b?  Maybe Bastien has some comments on this.