Re: [Bug-gnupod] Add --copy option to gnupod_search?

[H. Langos]

Hi Stuart,

The patch looks fine. I guess it could be a little bit more compact
here and there... but all in all it look ok.

For example you can roll definition and conditional assignment 
into one row.

> my $foo = defined($file{foo}) ? $file{foo} : 'Unknown Foo' ;

actually you could write 

> my $foo = ( $file{foo} or 'Unknown Foo' );

but that is harder to read for people (like me) who read code
in order to understand it, and not to admire the poetry ;-)
Also it behaves differently for variabled that are defined but
who's content evaluates as false, like ''.


The two "tr" operations that you do for every variable could
be collapsed into one.


Usability wise I think that instead of two options "--copy" and 
"--path" how about trying to put it in one "--copy" with an optional
value for the target path? So that without a value the current directory
is used as target.

check out http://perldoc.perl.org/Getopt/Long.html#Options-with-values


Also I'd place the copy block before the delete block. So that in
case somebody uses both "--copy" and "--delete" at the same time
he will not end up losing his data.

On the other hand you can't protect ppl from everything.
Maybe it would even be better not to have the script die when a copy
operation fails, and have it instead generate a waring. Just line the
delete code does:

> unlink(GNUpod::XMLhelper::realpath($opts{mount},$el->{file}->{path})) or warn 
> "[!!] Remove failed: $!\n";


One more word on path separators:

> my $targetdir = $prefix . "/" . $artist . "::" . $album . "/";

Carefull with the "::".  On MAC the ":" is a path separator like "/" on 
unix. How about "_-_" instead?


I hope I don't discourage you with the amount of nitpicking :-)


Cheers
-henrik

On Mon, Mar 21, 2011 at 07:55:01AM -0700, Stuart McLaren wrote:
> Hi Henrik,
> 
> Please find a patch attached. 
> 
> Thanks for your helpful suggestions. I've tried to take
> into account that a song may only have its title set (and
> not artist or other parameters.) I also took account of
> '/' and '\'. I decided to remove spaces too from song
> names and target directories. This is more of a judgement
> call -- sometimes for scripting etc spaces can cause
> some IFS confusion.
> 
> I've cowardly ignored both podcast issues and artwork.
> I'm more used to writing C rather than Perl, but I tried
> to follow your coding conventions where they were
> obvious to me.
> 
> Any feedback welcome.
> 
> -Stuart
> 
> 
> --- On Wed, 3/16/11, H. Langos <address@hidden> wrote:
> 
> > From: H. Langos <address@hidden>
> > Subject: Re: [Bug-gnupod] Add --copy option to gnupod_search?
> > To: address@hidden
> > Date: Wednesday, March 16, 2011, 10:36 AM
> > Hi Richard and Stuart,
> > 
> > On Wed, Mar 16, 2011 at 05:12:50PM +0100, Richard van den
> > Berg wrote:
> > > On 16 mrt. 2011, at 16:44, Stuart McLaren <address@hidden>
> > wrote:
> > > 
> > > > Ok. What I plan to do is have it work as
> > follows:
> > > >> 
> > > > 
> > > > 1) update gnupod_search --help --> Would
> > include info on the --copy option
> > > > 2) gnupod_search --copy --> Would backup whole
> > ipod to current directory
> > > > 3) gnupod_search --copy --directory=/tmp -->
> > Would backup whole ipod to /tmp
> > > > 4) gnupod_search --copy --directory=/tmp -a
> > John_Doe -l Rock_On 
> > > > --> Would backup artist John_Doe's Rock_On
> > album to /tmp
> > > > (ie the behaviour with matching would be exactly
> > the same as delete)
> > > > 
> > > > Note that output would be in the form:
> > > > 
> > > > "John_Doe - Rock_On"/01_Song_One.m4a
> > > > "John_Doe - Rock_On"/02_Song_Two.m4a
> > > > 
> > > > Does that work for you?
> > > 
> > > Sounds good to me, but let's hear Henrik's opinion as
> > well (he is the main gnupod coder). 
> > 
> > Sounds very nice indeed.
> > 
> > I didn't put any work into gnupod_search recently but if
> > the patch fits
> > nicely into the existing code... no problem.
> > I was missing features and flexibility in gnupod_search too
> > but
> > as people are used to it I have instead taken to writing
> > smaller,
> > single purpose tools that are (hopefully) easier to read
> > and maintain.
> > (gnupod_find, gnupod_delete and gnupod_modify)
> > 
> > 
> > Some minor questions:
> > 
> > How do you select the target filename for podcasts?
> > 
> > How do you select a target directory if no artist or album
> > is given? 
> > (The only mandatory attribute for any song is the title.)
> > 
> > How do you handle character conversion? E.g. If the target
> > filesystem
> > doesn't handle unicode filenames, or if any of the
> > attributes that you 
> > use contains a "/" "\" or ":" ?
> > 
> > How about extracting artwork?
> > 
> > > > 
> > > >> Interesting. Is this an iPod or OS
> > limitation? What OS are
> > > >> you using?
> > > > 
> > > > Debian 6.0, 2.6.32-5-amd64
> > > > 
> > > > Without this option there's a mismatch between
> > the
> > > > read ipod path for the file and the real mount
> > point, ie
> > > > gnupod looks for
> > > > 
> > > > /mnt/ipod/f19/file.mp3
> > > > 
> > > > but that doesn't exist, without the mount option
> > its actually
> > > > 
> > > > /mnt/ipod/F1/file.mp3. With the mount option
> > they
> > > > match up.
> > > 
> > > According to Google shortname=lower is the default for
> > linux vfat mounts. Will accessing a directory u6sing F19 not
> > work when f19 exists on vfat? Silly case insensitive file
> > systems..
> > > 
> > 
> > My default mount options also contain shortname=lower. I
> > guess we
> > could call gnupod_INIT.pl guilty. It creates those folders
> > with a
> > capital F. But then again it probably only mimics what
> > iTunes does.
> > 
> > 
> > cheers
> > -henrik
> > 
> > 
> > 
> > _______________________________________________
> > Bug-gnupod mailing list
> > address@hidden
> > http://lists.gnu.org/mailman/listinfo/bug-gnupod
> >
> 
> 
>       

> --- gnupod_search.pl  2011-03-21 14:40:34.390287810 +0000
> +++ gnupod_search.pl.patched  2011-03-21 14:45:43.205914391 +0000
> @@ -27,6 +27,9 @@
>  use GNUpod::FooBar;
>  use GNUpod::ArtworkDB;
>  use Getopt::Long;
> +use File::Copy;
> +use File::Basename;
> +use Cwd;
>  
>  use vars qw(%opts @keeplist %rename_tags);
>  
> @@ -47,7 +50,7 @@
>                     "album|l=s", "title|t=s", "id|i=s", "rename=s@", 
> "artwork=s",
>                     "playcount|c=s", "rating|s=s", "podcastrss|R=s", 
> "podcastguid|U=s",
>                     "bitrate|b=s",
> -                   "view=s","genre|g=s", "match-once|o", "delete");
> +                   "view=s","genre|g=s", "match-once|o", "delete", "copy", 
> "path|p=s");
>  GNUpod::FooBar::GetConfig(\%opts, {view=>'s', mount=>'s', 'match-once'=>'b', 
> 'automktunes'=>'b', model=>'s', bgcolor=>'s'}, "gnupod_search");
>  
>  $opts{view} ||= 'ialt'; #Default view
> @@ -56,6 +59,8 @@
>  version() if $opts{version};
>  #Check if input makes sense:
>  die "You can't use --delete and --rename together\n" if($opts{delete} && 
> $opts{rename});
> +die "Can only specify --path when using --copy\n" if($opts{path} && 
> !$opts{copy});
> +print "Copying all matching files\n" if($opts{copy});
>  
>  # -> Connect the iPod
>  my $connection = GNUpod::FooBar::connect(\%opts);
> @@ -104,9 +109,10 @@
>  sub newfile {
>       my($el) =  @_;
>                            # 2 = mount + view (both are ALWAYS set)
> -     my $ntm      = keys(%opts)-2-(defined $opts{'match-once'})-(defined 
> $opts{'automktunes'})-(defined $opts{'delete'})-(defined 
> $opts{'rename'})-(defined $opts{'artwork'})-(defined $opts{'model'});
> +     my $ntm      = keys(%opts)-2-(defined $opts{'match-once'})-(defined 
> $opts{'automktunes'})-(defined $opts{'delete'})-(defined 
> $opts{'copy'})-(defined $opts{'rename'})-(defined $opts{'artwork'})-(defined 
> $opts{'model'})-(defined $opts{path});
>       my $matched  = 0;
>       my $dounlink = 0;
> +     my $docopy = 0;
>       foreach my $opx (keys(%opts)) {
>               next if $opx =~ 
> /mount|match-once|delete|view|rename|artwork|model/; #Skip this
>               
> @@ -143,6 +149,9 @@
>               if($opts{delete}) {
>                       $dounlink = 1; # Request deletion
>               }
> +             if($opts{copy}) {
> +                     $docopy = 1; # Request copy
> +             }
>               elsif(defined($opts{artwork})) {
>                       # -> Add/Set artwork
>                       $el->{file}->{has_artwork} = 1;
> @@ -157,6 +166,57 @@
>               
> unlink(GNUpod::XMLhelper::realpath($opts{mount},$el->{file}->{path})) or warn 
> "[!!] Remove failed: $!\n";
>               $dirty++;
>       }
> +     if($docopy) {
> +             # Note: May need ipod mounted with option 'shortname=lower' for 
> copy to work
> +             my $source = 
> GNUpod::XMLhelper::realpath($opts{mount},$el->{file}->{path});
> +
> +             my $artist;
> +             if (defined($el->{file}->{artist})) {
> +                     $artist = $el->{file}->{artist};
> +             }
> +             else {
> +                     $artist = "Unknown_Artist";
> +             }
> +             $artist =~ tr/ /_/;
> +             $artist =~ tr/\/\\/--/;
> +             my $album;
> +             if (defined($el->{file}->{album})) {
> +                     $album = $el->{file}->{album};
> +             }
> +             else {
> +                     $album = "Unknown_Album";
> +             }
> +             $album =~ tr/ /_/;
> +             $album =~ tr/\/\\/--/;
> +             my $songnumber_prefix = "";
> +             if (defined($el->{file}->{songnum})) {
> +                     $songnumber_prefix =  
> sprintf("%02d",$el->{file}->{songnum}) . "_";
> +             }
> +             my $prefix;
> +             if ($opts{path}) {
> +                     $prefix = $opts{path};
> +             }
> +             else {
> +                     # Output to current directory by default
> +                     $prefix = cwd();
> +             }
> +             my $title;
> +             if (defined($el->{file}->{title})) {
> +                     $title = $el->{file}->{title};
> +             }
> +             else {
> +                     $title = "Unknown_Album";
> +             }
> +             $title =~ tr/ /_/;
> +             $title =~ tr/\/\\/--/;
> +             my $targetdir = $prefix . "/" . $artist . "::" . $album . "/";
> +             my $ext;
> +             (undef,undef,$ext) = fileparse($el->{file}->{path},qr"\..*");
> +             my $targetfile = $songnumber_prefix . $title . $ext;
> +             my $target = $targetdir . $targetfile; 
> +             mkdir $targetdir;
> +             copy($source, $target) or die "File $source cannot be copied to 
> $target.";
> +     }
>       else {
>               # -> Keep file: add it to XML
>               GNUpod::XMLhelper::mkfile($el);
> @@ -259,6 +319,8 @@
>     -b, --bitrate=BITRATE   search songs by Bitrate
>     -o, --match-once        Search doesn't need to match multiple times (eg. 
> -a & -l)
>         --delete            REMOVE (!) matched songs
> +       --copy              Copy matched songs to local disk 
> +   -p, --path=directory    Output path to use when copying songs to local 
> disk
>         --view=ialt         Modify output, default=ialt
>                              t = title    a = artist   r = rating      p = 
> iPod Path
>                              l = album    g = genre    c = playcount   i = id
> @@ -493,6 +555,12 @@
>       # Delete all songs by the artist called 'Schlumminguch'
>       gnupod_search.pl -m /mnt/ipod --artist="Schlummiguch" --delete
>  
> +     # Copy album 'Gnu' by the artist called 'Schlumminguch' to /tmp
> +     gnupod_search.pl -m /mnt/ipod --artist="Schlummiguch" --album="Gnu" 
> --copy --path=/tmp
> +
> +     # Backup all songs on iPod to current directory 
> +     gnupod_search.pl -m /mnt/ipod --copy
> +
>       # Record the changes to the iTunes database (this is essential)
>       mktunes.pl -m /mnt/ipod
>  

> _______________________________________________
> Bug-gnupod mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/bug-gnupod