Re: [Rhythmbox-devel] [PATCH] check remaining space before copying tracks to a removable device

[Jonathan Matthew <jonathan d14n org>]

On Fri, Nov 6, 2009 at 12:28 PM, Kim Nguyen <Kim Nguyen nicta com au> wrote:
> Hi,
>
> (first of all, I would have prefered to go through the bugzilla to
> report this but it seems that bugzilla.gnome.org is not available at
> the moment).

Seems to be working fine now. To save some effort, here's a suitable
bug to attach this patch to:
https://bugzilla.gnome.org/show_bug.cgi?id=520046

Your patch looks like a reasonable approach for the current track
transfer system in rhythmbox. There are some code style issues and
other minor things that I'll go over once the patch is in bugzilla.

I'm very slowly working on a better track transfer system that would
make it possible to do this better, but it's not going to be ready any
time soon.

> I attach the following proof of concept patch (against git master)
> which corrects this behaviour for all removable devices.
> I took the simple approach of checking that the size of all tracks to
> be copied is  less than the available space on the destination and
> abort
> the copy if this isn't the case (the user is warned with a rb_error_dialog).
>
> This approach seems to cover 90% of the use cases but has some problems.
>
> 1) It checks the size of the original file, which means that this can
> be innacurate in case of transcoding. If transcoding occurs and the
> transcoded
> files are bigger than the original one there is still a problem.
>
> 2) It only works for RHYTHMDB_ENTRY_NORMAL at the moment, I don't
> really know if it's possible to copy a stream to a removable device,
> and don't cover this case.

I don't think this is a problem worth worrying about. It's not really
meaningful to copy a stream, if it's even possible to try.

>
> Obviously a better approach would be to do this per track but warning
> the user for each track is cumbersome. So a better way would be:
>
> - For each track check (after transcoding) that the remaining size is
> more than the track size.
> - if it's not, abort and maybe ask the user if (s)he wants to remove
> the tracks that where added during the operation (as a user I don't
> like to have
> half albums on my player but that's a matter of taste I guess, one can
> always manually remove half finished albums).

The problem here is that as things stand, once a track is added to the
transfer queue, it's no longer associated with any other tracks being
transferred to the same device, and there's no way to cancel it.

> I did not implement this for the moment since it seems that rb
> transcodes directly to the destination device
> (see rb-removable-media-manager.c/do_transfer). It seems more robust
> to transcode to /tmp and then copy the track to the device.

I'm not sure it really buys you anything - transcoding to /tmp, then
checking if the resulting file will fit on the device is much the same
as transcoding onto the device, then deleting the file if you run out
of space, except there's more i/o and you don't find out that it's a
problem until you've transcoded the whole track.