Re: [Rhythmbox-devel] playlist source burner 0.5

[William Jon McCann <mccannwj pha jhu edu>]

Hi Colin,

Colin Walters wrote:
> 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 :)

Discs for the car were part of my motivation too.  However, Mr. James 
can be a bit too much for me while in traffic.  I go with the new 
Interpol or Ulrich Schnauss.  ;)

> --- 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?

For me, dropping in a playlist works with this patch but dropping into 
the source pane itself does not.

> > +        <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?

I wasn't sure so I didn't change it yet.  At the moment, I'm thinking 
that since the majority of people will never need to select a device 
different from the default, they don't really need to see the selector. 
 Showing it to them every time may be a bit much.  Maybe I'm 
misunderstanding your suggestion.

> > +        <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.

I'm not sure either.  So, I suppose adding it later if necessary is 
reasonable.  Unless anyone else has strong objections...

> +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.

Actually, is db doing anything useful here?

> Now some pretty minor bits:
>
> +                             _("Drive %s is cannot burn CDs"),
>
> Little typo.

Nice.

> +        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 "".

OK.

> +#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?

Yup.

> +        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)) {
>  ...
> }

Or this, right?

GtkTreeIter iter;
if (!gtk_tree_model_get_iter_first (model, &iter))
     return;
do {
...
} 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?

It is for efficiency since it reuses the converted audio tracks.  If you 
want to make more than one copy it is fairly painful to have to convert 
the audio each time.

Jon