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

[Stuart McLaren]

Hi Henrik,

> I hope I don't discourage you with the amount of nitpicking

No, no problem. All your comments are valid -- especially
the MAC and delete vs copy order. Feel free to make these
changes and any other ones you think would improve it.

-Stuart

--- On Tue, 3/22/11, H. Langos <address@hidden> wrote:

> From: H. Langos <address@hidden>
> Subject: Re: [Bug-gnupod] Add --copy option to gnupod_search?
> To: "Stuart McLaren" <address@hidden>
> Cc: address@hidden
> Date: Tuesday, March 22, 2011, 5:38 AM
> 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
> 
>