Re: [Rhythmbox-devel] playlist source burner 0.5

[Colin Walters <walters verbum org>]

On Thu, 2004-09-02 at 17:38 -0400, William Jon McCann wrote:
> Hi,
> 
> http://acs.pha.jhu.edu/~mccannwj/rhythmbox/rb-burn-0.5.patch

I just tried this out, and will be happy to be listening to my new Aphex
Twin CD during the drive to work tomorrow.  This is great functionality
to have.  The code is looking pretty high quality now too.  Thanks for
being patient :)

--- lib/rb-tree-dnd.c	5 Apr 2004 14:26:48 -0000	1.7
+++ lib/rb-tree-dnd.c	2 Sep 2004 21:36:46 -0000

Is it just me, or does drag and drop to the source list still not work
even with this patch?

> +        <key>/schemas/apps/rhythmbox/state/burn_device</key>

I thought we'd concluded to put this in the burn dialog instead of
having a preference?

> +        <key>/schemas/apps/rhythmbox/state/burn_tmp</key>

I remain very dubious on this one.  It just doesn't feel specific to
Rhythmbox at all, it hits any application that needs large temporary
files.  

This is also something that's very easy to add later, but once we add it
we have to support it forever.

+static void
+burn_playlist_iter_func (GtkTreeModel *model, GtkTreeIter *iter, char **uri, char **artist, char **title, gulong *duration)
+{
+	RhythmDB *db;
+	RhythmDBEntry *entry;
+
+	g_object_get (G_OBJECT (model), "db", &db, NULL);

I just realized from another glance at the GLib docs that g_object_get
actually refs objects retrieved, so you need to g_object_unref db here.
The rest of Rhythmbox is buggy in this regard too, I need to go through
and fix these.  This might explain why valgrind is a bit noisy.

Now some pretty minor bits:

+                             _("Drive %s is cannot burn CDs"),

Little typo.

+        path = eel_gconf_get_string (CONF_STATE_BURN_TMP);
+        if (path

You probably want: if (path && strcmp (path, "")
The default GConf value seems to be "".

+#ifndef __RB_PLAYLIST_SOURCE_RECORDER_H
+#define __RB_PLAYLIST_SOURCE_RECORDER_H
+
+#include <libxml/tree.h>
+
+#include "rb-source.h"
+#include "rhythmdb.h"
+#include "rb-library-source.h"
+#include "rb-query-creator.h"
+#include "rhythmdb-query-model.h"

Looks like a bunch of spurious includes?

+        num_entries_total = gtk_tree_model_iter_n_children (model, NULL);
+
+        for (i = 0; i < num_entries_total; i++) {
+                GtkTreeIter     iter;
+                RBRecorderSong *song = recorder_song_new ();
+                
+                if (i == 0)
+                        gtk_tree_model_get_iter_first (model, &iter);
+                else
+                        gtk_tree_model_iter_next (model, &iter);

This can be written in a cleaner way like this:

GtkTreeIter iter;
if (!gtk_tree_model_get_iter_first (model, &iter))
    return;
while (gtk_tree_model_iter_next (model, &iter)) {
 ...
}


Now, longer term (i.e. after this patch goes in), I think it probably
makes sense to have CD burning be a nonmodal activity.  We already have
a progress bar in the bottom right, we can just display "Burning
CD" [xxx  ] there.  Perhaps clicking on it would pop up a cancel dialog.

Also, why does it ask me to make another copy?  Do you think that will
be a common activity?