Ok, so now it's my turn to nitpick. I feel a bit guilty about bugging
you for details given the amount of work you probably put in that
patch ;)
+AM_CONDITIONAL(MKDTEMP_MISSING, test x$mkdtemp_missing = xtrue)
Won't compilation fail on systems without mkdtemp ?
+ /*g_object_set (widget, "show-recorders-only", TRUE, NULL);*/
Why is that commented out?
+burn_playlist_iter_func (GtkTreeModel *model, GtkTreeIter *iter, char
**uri, char **artist, char **title, int *duration)
entry->duration is a gulong, probably better to use a gulong here too
+void
+rb_playlist_source_burn_playlist (RBPlaylistSource *source)
+ g_object_get (source, "name", &name, NULL);
name is never freed
+ if (error)
+ g_error_free (error);
Looks a bit weird to pass a GError to the previous function if it's not
used, NULL would probably be enough. I guess this code means "error
handling is planned, but not implemented yet" ?
+static GObject*
+rb_playlist_source_recorder_constructor (GType type,
Any reason why you are using the object constructor instead of the
object init function?
+ if (error) {
+ g_warning ("Invalid writer device: %s",
device);
+ /* ignore and let rb_recorder try to find a
default */
+ }
Should probably be marked for translation
+GtkWidget *
+rb_playlist_source_recorder_new (GtkWidget *parent,
+ char *name)
name is const char *
+void
+rb_playlist_source_recorder_add_from_model (RBPlaylistSourceRecorder
Any reason why you aren't using gtk_tree_model_get_iter_first and
gtk_tree_model_iter_next in that function instead of
gtk_tree_model_get_iter_from_string?
+static long
+rb_playlist_source_recorder_estimate_total_size
(RBPlaylistSourceRecorder *source)
If I'm not mistaken, this will overflow for about 200 minutes of music:
2^31 / AUDIO_RATE ~= 12000 secs = 200 minutes (don't remember if a long
is signed or not). A guint64 is probably safer. It seems this function
is only called after checking that the total length won't exceed the
length of a CD, so it's probably safe, but maybe there can be "audio
dvds" where this can be an issue?
+static gboolean
+check_dir_has_space (const char *path,
+ long bytes_needed)
I'd recommend implementing this function using
GnomeVFSResult
gnome_vfs_get_volume_free_space (const GnomeVFSURI *vfs_uri,
GnomeVFSFileSize *size);
See the #ifdef mess for portability in gnome-vfs/libgnomevfs/gnome-vfs-
utils.c if you want an explanation ;) The various statfs/statvfs checks
can probably go away too if you agree with using that function.
+ if ((media_duration < 0) && (duration > 4800)) {
Fwiw, the CD could be a 74 min CD, and I think there are 90 and 99
minutes CDs iirc. Maybe we should ask the user what to do when we
couldn't read the media size and the length is greater than 74*60?
+ /* The main playback pipeline, at the end this looks like:
+ * { src ! spider ! audioconvert ! wavenc ! sink }
I think the pipeline needs to be src ! spider ! audioconvert !
audioscale ! audio/x-raw-int,rate=44100,width=16,depth=16 ! wavenc !
sink
Maybe spider will be able to do the right thing with only
src ! spider ! audio/x-raw-int,rate=44100,width=16,depth=16 ! wavenc !
sink
+ dummy = gst_element_factory_make ("fakesink", "fakesink");
+ if (!dummy
+ || !gst_scheduler_factory_make (NULL, GST_ELEMENT (dummy)))
{
+ g_set_error (error, RB_RECORDER_ERROR,
+ RB_RECORDER_ERROR_GENERAL,
+ _("Couldn't initialize scheduler. Did you
run gst-register?"));
+ return NULL;
+ }
This check is probably already done elsewhere in rhythmbox, there's no
need to duplicate it here imo.
+ if (may_pause == -1) {
+ guint major, minor, micro;
+
+ rb_debug ("decoding gst version");
+ gst_version (&major, &minor, µ);
+ /* this makes sure someone removes this later
on */
+ g_assert (major == 0);
+ g_assert (minor == 8);
+ if (micro >= 1)
+ may_pause = 1;
+ else
+ may_pause = 0;
+ }
may_pause doesn't seem to be used apart from that piece of code, which
is there to properly free the sound device when playback is paused, so I
guess it can be removed.
+static char *
+get_dest_from_uri (const char *tmp_dir,
I don't really understand what this function does, to my eyes, it
doesn't seem to guarantee much about the filename: it ends in .wav, it's
in the temporary directory, but maybe a filename with the same name
already exists, and nothing prevents this file from being removed or
replaced with a link or something before we use it. Maybe I missed
something though, I'm tired atm ;) If I'm right, it may be better to
return the open fd from g_mkstemp (we don't care about the .wav
extension, do we?) and use fdsink. This probably doesn't matter much
anyway.
+static gint64
+get_tracks_length (RBRecorder *recorder,
+ GError **error)
We should already know the track length because we converted the track
to wav, maybe we don't need to reread it from the wav file? Actually,
the checks being done in rb_recorder_burn have already been done
previous in rb_playlist_source_recorder_start, didn't they?
Ok, I'm done with all my annoying comments, I hope what I'm saying is
making sense, feel free to disagree with some of them ;)
As a side note, do you think your audio=>wav converter would be easy
enough to reuse for an audio=>another audio format converter? That would
be useful for transferring files to an iPod for example ;)
Thanks for this patch which looks great ;)
Christophe
Le mercredi 01 septembre 2004 à 11:42 -0400, William Jon McCann a
écrit :
> Hi,
>
> New patch that addresses Bastien's comments.
>
> http://acs.pha.jhu.edu/~mccannwj/rhythmbox/rb-burn-0.3.patch
>
> Jon
> _______________________________________________
> rhythmbox-devel mailing list
> rhythmbox-devel gnome org
> http://mail.gnome.org/mailman/listinfo/rhythmbox-devel
>
>
Attachment:
signature.asc
Description: Ceci est une partie de message =?ISO-8859-1?Q?num=E9riquement?= =?ISO-8859-1?Q?_sign=E9e?=