Re: [Rhythmbox-devel] playlist source burner 0.5

[Bastien Nocera <hadess hadess net>]

On Fri, 2004-09-03 at 14:01 -0400, William Jon McCann wrote:
> Hi Colin,
> > 
> >>+        <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.

The burn dialog is necessary anyway, just to confirm things before write
starts (whether or not to have a 2 sec gap, the space on the CD, the
usage, and the ability to cancel a possibly destructive process, etc.)
so we might as well have the CD burner selection in there.
And it also allows not to break the flow. If I was to start a burn with
the wrong device, i'd have to cancel, and select the right drive, and go
back to the burning dialog.

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

You can use the nautilus-cd-burner one, on the condition that there's a
bug filed so that we can separate n-c-b and libnautilus-burn
preferences.

Please also attach your other patch ("show-recorders-only") to bugzilla.
I can't commit them because of the GNOME freeze.
<snip>
> > +        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.

Actually, if (path && path[0] != '\0') is faster :)

<snip>
> > +        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);

Colin, doesn't your solution skip the first 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.

Add a "make multiple copies" checkbox in the burn dialog, off by
default.

---
Bastien Nocera <hadess hadess net> 
I was just a 15-year-old porn star. Big fucking deal. -- Traci Lords