Re: [Rhythmbox-devel] playlist source burner 0.2

[Bastien Nocera <hadess hadess net>]

Hello William,

On Tue, 2004-08-31 at 15:23 -0400, William Jon McCann wrote:
> Hi,
> 
> Improved version.
> 
> http://acs.pha.jhu.edu/~mccannwj/rhythmbox/rb-burn-0.2.patch
> 
>   - moved the rb-recorder object into the player directory.
>   - added many size and sanity checks.
>   - removed UI for the temporary directory
>   - automatically determine temporary directory (with GConf override)
>   - add confirmation for quit during disc burn

We'll nitpicking on the reviews:
- no periods at the end of the short description in schemas
- burn_tmp_folder_label still declared but not used
- the bacon_cd_widget lists all the CD devices, not only those you can
burn to. You'll need to filter on the widget side, to make sure that
only recorders appear (add a "show-recorders-only" property)

+enum {
+        PROP_0,
+};

If you don't have any properties, those are useless. So are the empty
get/set property calls.

+GType
+rb_playlist_source_recorder_get_type (void)

Use G_DEFINE_TYPE instead (Colin, are we using gtk+ 2.4 in Rhythmbox
yet? using G_DEFINE_TYPE, and glin/gi18n.h could save quite a lot of
code (and it's an easy task for gnome-love, or a first time
contributor).

You'll need to handle the absence of CD recorders better (ie. the option
should be grayed out in the menus).

"This exceeds the 80 minute length of a standard audio CD."

You don't know that the CD is 80 minutes long. Just say that it doesn't
fit on the CD, it's better than lying on purpose.

MiB is 1000 kiB, not 1024.

+        track->contents.audio.cdtext = g_path_get_basename (filename);

We don't use the cdtext bit yet, but it would be better to get that info
from the metadata in Rhythmbox. It would also mean that you could avoid
non-ASCII filenames for the temporary files (shouldn't be a problem, but
better safe than sorry).

+rb_recorder_get_media_length

In there, why aren't you using the supplied macros, instead of the magic
numbers?

The check for the RIFFWAVEfmt header should probably return
ACB_ERROR_NOT_WAVE_FORMAT if it fails (it's a "bug" in my original
code).

What's the GINT_TO_LE for? It's probably not what you want to do (ie. I
don't think a big endian machine would like being fed a little endian
integer)

> I don't use rb_error_dialog because it is modal and it uses a response 
> handler with gtk_dialog_run which I believe causes undefined behavior. 
> It was causing lots of problems for me.
> 
> This patch includes that filtered model patch because I can't populate 
> playlists without it.
> 
> Have not yet removed the UI for the burner device.  Most people will not 
> need to use it now that I use good default but some might.

Bear in mind, that I haven't tried, or seen this patch in action, so
there might be more work needed than that. But thanks for doing that,
many have failed where you might succeed :)

Cheers

---
Bastien Nocera <hadess hadess net> 
I'm surprised you gave How To Make An American Quilt a four-star rating.
I thought it was just sew-sew. -- reader Glenn Cossey