Re: [Bug-gnupod] mktunes.pl creates corrupt iTunesDB ?

[H. Langos]

Hi Richard,

On Tue, Jun 16, 2009 at 10:05:16PM +0200, Richard van den Berg wrote:
> On 6/16/09 12:44 AM, H. Langos wrote:
>> My first idea now is to
>> strip the iTunesDB of the stuff that is optional.
>>   
>
> Thanks again for the suggestion, that worked really well. I can now see  
> all my 30415 songs! Whoohoo. :-)

Congratulations! Now please don't go overboard with the patch. :-)

The default behavior for our users should not change. Meaning every possible
mhod should be written. The code and the naming of variables should express 
that this is a workaround, not the default. I.e the "keepattr" would better 
be named "low_ram_attr" or "shrink_itunes_db" or "minimal_attr".
Your mhod skipping code should only be active if that option is set in your
.gnupodrc or given as command line switch.


Did you find out if shrinking the existing attributes (in contrast to
skipping) had any effect? If it did we could keep some counters for mhods 
and their cumulative size in mktunes and either print out those stats if we
run in verbose mode (which would have to be added :) ) or print out warnings
if certain limits are reached.

> A second patch is needed for tunes2pod  
> which will read the missing attributes from GNUtunesDB.xml, otherwise  
> they are gone forever after tunes2pod is run. I'll work on that later.

I wouldn't worry about that, AFAIK tunes2pod is only run once when you start
working with gnupod. Afterwards the flow of information is mostly this:

"OTG Playlist"
    ||
    vv
GNUtunesDB.xml    ==>   iTunesDB
    ^^
    ||
"Play Counts"

You only need to rerun tunes2pod if your GNUtunesDB.xml is lost or if you 
played around with iTunes in the meantime ... and we could warn people about
this. 


BTW: Does iTunes manage to get your full collection over to your ipod, or
does it also produce an unusable iTunesDB?


Code remarks: I like the passing of optional arguments in a hash. However I
strongly suggest using lower case keys (keep instead of Keep and item instead
of Item.)


> -     $mktunes->WriteItunesDB;
> +     $mktunes->WriteItunesDB(Keep=>$opts{'keepattr'});

Shouldn't that be :

$mktunes->WriteItunesDB( {Keep => $opts{'keepattr'}} );

With the additional curly brackets to express that you are defining an
anonymous hash? 

Maybe perl quesses right but I am not trusting perl to always do the 
right thing. And how would that line look if you add another hashkey?
Would perl put both into one hash or would it pass two separate hashes?


> +             my($self, %args) = @_;
> +             my $object      = $args{Item};
> +             my @keep        = split(/[ ,]+/, $args{Keep});
>               my $mhit        = ''; # Buffer for the new mhit
>               my $mhod_chunks = ''; # Buffer for the childs (mhods)
>               my $mhod_count  = 0;  # Child counter
> @@ -275,6 +277,7 @@
>               foreach my $key (sort keys(%$object)) {
>                       my $value = $object->{$key};
>                       next unless $value; # Do not write empty values
> +                     next unless ($#keep<0 || grep(/^$key$/,@keep)); # Only 
> keep specific mhods
>                       my $new_mhod = GNUpod::iTunesDB::mk_mhod({stype=>$key, 
> string=>$value});
>                       next unless $new_mhod; # Something went wrong
>                       $mhod_chunks .= $new_mhod;

Instead of @keep, I'd use %keep. and instead of creating it here for 
every mhit, I'd create it only once, and pass it along as a hashref.


cheers
-henrik

PS: I've see the following line in the patch. So it seems my empty desc
attributes wouldn't have made it into the iTunesDB anyway:
>                       next unless $value; # Do not write empty values