Comment on attachment 169800 [details] [review]
Correctly set track media types.

We used to always set Filetype = "MP3-file". What happens now if it's not set? Can we at least add logging for the filename so we know which files couldn't be detected?

Review of attachment 169800 [details] [review]:

We need to call "ToLower" aswell to prevent casing mismatches.

::: src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceTrackInfo.cs
@@ +264,3 @@
+        {
+            string extension = System.IO.Path.GetExtension (path);
+            if (!String.IsNullOrEmpty (extension)) {

We s hould call ".ToLower ()" on this before hitting the switch statement so that we can catch .MPG and all variations in casing.

I was working on a revised patch which would always set the file type to the media type like RB does, but I've found another issue. When the PodcastPlaylist does not yet exist on the device the libgpod bindings throw a NRE.

I tried modifying the libgpod bindings but without success so far. I tried it from within the libgpod binding and from within Banshee but I still get NRE's when trying to access the podcast playlist.

public Playlist PodcastsPlaylist { 
  get { 
    if (!valid_podcast_playlist) {
      IntPtr playlist = Itdb_iTunesDB.itdb_playlist_by_name (Handle, "Podcasts");
      if (playlist == IntPtr.Zero) {
        playlist = Itdb_iTunesDB.itdb_playlist_new("Podcasts", false);
        Itdb_iTunesDB.itdb_playlist_set_podcasts (playlist);
      }
					
      valid_podcast_playlist = true;
    }
				
    return new Playlist(Itdb_iTunesDB.itdb_playlist_podcasts(Handle));
  }
}

Can we push all playlisting related bugs to: https://bugzilla.gnome.org/show_bug.cgi?id=628675

I'm working on this now, just fixed (hopefully!) the memory corruption issue which has been preventing me from getting playlisting working :)

*** Bug 629122 has been marked as a duplicate of this bug. ***

Created attachment 170280 [details] [review]
Set track media attributes.

Attached is a new version of the patch. It now copies the file mime-type to the track file type just like RB does and calls ".ToLower ()" on the video file extension.

I must say I'm not sure whether the video file extension should be checked. I copied the behavior from gtkpod which only sets the track file type in the same way for video files, but doesn't set anything for any other file.

Review of attachment 170280 [details] [review]:

Thanks for the patch.  It needs a little work.  For the mimetype conversions, see also bug #629103 which I just filed to add some more to libgpod.

::: src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceSource.cs
@@ +530,3 @@
                             playlist.Tracks.Remove (track.IpodTrack);
+
+                        if (track.IpodTrack.MediaType == GPod.MediaType.Podcast)

please put {} around all if statements

::: src/Dap/Banshee.Dap.AppleDevice/Banshee.Dap.AppleDevice/AppleDeviceTrackInfo.cs
@@ +163,3 @@
+                MediaAttributes |= TrackMediaAttributes.Podcast;
+                break;
+            case GPod.MediaType.Audiobook:

You're missing a check for MediaType.MusicTvShow

@@ -165,3 @@
-//                case IPod.MediaType.Podcast:
-//                    MediaAttributes |= TrackMediaAttributes.Podcast;
-//                    // FIXME: persist URL on the track (track.PodcastUrl)

why did you get rid of this FIXME?

@@ -219,3 @@
 
-//            if (HasAttribute (TrackMediaAttributes.Podcast)) {
-//                track.DateReleased = ReleaseDate;

the DateReleased (libgpod-sharp calls it TimeReleased, I believe) is very important for podcasts

@@ +212,3 @@
+            if (HasAttribute (TrackMediaAttributes.VideoStream)) {
+                if (HasAttribute (TrackMediaAttributes.Podcast)) {
+                    track.MediaType = GPod.MediaType.Podcast | GPod.MediaType.Movie;

GPod.MediaType isn't a [Flag] enum -- does this even compile?  For non-flag enums, you can't combine multiple enum values.  Did you just think this was right, or is it somehow necessary?

@@ +213,3 @@
+                if (HasAttribute (TrackMediaAttributes.Podcast)) {
+                    track.MediaType = GPod.MediaType.Podcast | GPod.MediaType.Movie;
+                } else if (HasAttribute (TrackMediaAttributes.Music)) {

No idea if we'll ever actually support MusicTvShow in Banshee, but might as well do it right here -- check if has TvShow and Music attrsk, and if so set MediaType.MusicTvShow.

@@ +220,3 @@
+                    track.MediaType = GPod.MediaType.TVShow;
+                } else {
+                    track.MediaType = GPod.MediaType.Movie;

This fallback for video should be MediaType.AudioVideo, no?

@@ +265,3 @@
         }
+
+        private static string GetVideoFileType (string path)

I must have missed something -- does this value have to get set?  Are these special strings somehow?  For one, they aren't translated.

Created attachment 170775 [details] [review]
Set track media attributes

Attached is another attempt.

I added {}, but since this is not always done in other code I assumed for single line statements there was an exception.

I added the FIXME line again.

DateReleased was already set before. In one of the previous updates it received a try-catch statement to work around a 32-bit issue. See commit     b12bb8bb0e15f523714c0d4d017f02e27b4caeed.

Enums always compile as far as I know. The only difference I know is that a .ToString() on a Flags enum always behaves like a ToString("G").

I think I added correct support for MusicTVShow types now.

The video part came from the gtkpod implementation (where they weren't translated either) and the mimetype part came from Rhythmbox. RB doesn't support video files and gtkpod never sets it for audio types, so I combined both. It doesn't seem to matter what the value is though, even setting it to "Foo Bar" still allows you to play any file on the iPod or iPhone. I now always set it to the mimetype as this seems to be the most logical option.

Review of attachment 170775 [details] [review]:

Fine with me, thanks Jensen!  Alan, would you sign off on this and push it?

There's still an open issue, but it goes beyond the scope of this one.

If there's no podcast playlist on the iDevice (for example, after you restored it to factory settings using iTunes), the podcast playlist is not created by Banshee or the libgpod-sharp bindings. When you try to transfer a podcast file to the iPod it will throw a NRE, but complete the transfer. The track will be on the device as a regular audio file, playable but not removable.

Comment on attachment 170775 [details] [review]
Set track media attributes

I push this