Created attachment 85511 [details] [review]
Implement podcast playlist support

Add podcasts to the podcasts playlist so that they appear on the proper menu on the ipod.

Created attachment 85521 [details] [review]
Stash the playlist instead of iterating each time

Don't iterate to find the playlist each time add_to_podcasts is called, as suggested by Christophe and James

The patch looks fine to me now, though I have 2 questions for people more knowledgeable in rb internals ;)

* is rhythmdb_entry_get_pointer (entry, RHYTHMDB_PROP_TYPE) the best way to get the type of a RhythmDBEntry, or is there some nicer way?

* the podcast is added to the rb playlist using rb_static_playlist_source_add_location (rbpl, filename, -1); in add_to_podcasts. Given that in the function calling add_to_podcasts, we have a RhythmDBEntry available corresponding to that filename, would it be better to use rb_static_playlist_source_add_entry instead?

Grumble, just found another issues :-/ The ITDB_MEDIATYPE_AUDIO and ITDB_MEDIATYPE_PODCAST constants I recommended using aren't available in the latest libgpod release, so it might be better to define our own constants as you did in your initial patch for now.

Created attachment 85540 [details] [review]
Reintroduce mediatype constants

I noticed that the constants were only in cvs (is why I didn't use them in the first place), but figured that it was okay.

In any case, constants are back, and I fixed a bug in the previous code (I use "itdb_playlist" in one place and "itdb-playlist" in another)

Comment on attachment 85540 [details] [review]
Reintroduce mediatype constants

<snip>
>+/* FIXME:  these should go away once we compile against new-enough libgpod */
>+#define MEDIATYPE_AUDIO         0x0001
>+#define MEDIATYPE_PODCAST       0x0004

This should obviously be:
#ifndef MEDIATYPE_AUDIO
#define MEDIATYPE_AUDIO         0x0001
#endif
etc.

Created attachment 85549 [details] [review]
Fix defines

Obviously.

The constants in libgpod are defined as an enum:
typedef enum
{
    ITDB_MEDIATYPE_AUDIO      = 0x0001,
    ITDB_MEDIATYPE_MOVIE      = 0x0002,
....
};
so I'm not sure at all they will be caught by the #ifndef, does anybody know if they will? Otherwise, the patch from comment #5 is probably better. Sorry Steven for having you modify again and again your patch :-/

Created attachment 85558 [details] [review]
Reposting patch from comment #5

This works fine for me, and I've committed it to svn trunk (with a few code-style fixes, like using tabs to indent instead of spaces). Thanks.

Sorry to reopen that bug, but this seems more appropriate here than in a new bug, here is a patch to create the podcasts playlist on the ipod if it doesn't exist.

Created attachment 85644 [details] [review]
Create the podcasts playlist if it doesn't exist

Looks fine to me.

The patch seems to be incorrect to me.  For one thing, priv->podcast_pl is of type RBStaticPlaylistSource, so assigning the Itdb_Playlist to it is wrong.  Second, podcast_pl gets set by add_rb_playlist, which is called immediately before it.  Removing that assignment should render the patch correct, however.

Just to be particular, it's not necessary to declare a second Itdb_Playlist variable in the if statement; ipod_playlist could be used.  In fact, if it were done this way, the call to g_object_get_data could be put in an else clause and avoided in some cases.

(In reply to comment #14)
> The patch seems to be incorrect to me.  For one thing, priv->podcast_pl is of
> type RBStaticPlaylistSource, so assigning the Itdb_Playlist to it is wrong. 
> Second, podcast_pl gets set by add_rb_playlist, which is called immediately
> before it.  Removing that assignment should render the patch correct, however.

Yep, dunno why this assignment is still there, I compile-tested this patch, so I'm pretty sure I got rid of it.
Fwiw, this assignment is a left-over of the patch from http://bugzilla.gnome.org/show_bug.cgi?id=425117#c2 (initially, I had written the patch attached to that bug on top of the patch in this other bug, then I changed it so that it applies to svn trunk, and I screwed up :p )

> 
> Just to be particular, it's not necessary to declare a second Itdb_Playlist
> variable in the if statement; ipod_playlist could be used.  In fact, if it were
> done this way, the call to g_object_get_data could be put in an else clause and
> avoided in some cases.

Yep, good point for reusing ipod_playlist. I don't feel it's necessary to make the code slightly harder to read to get rid of the g_object_get_data. 

Created attachment 85739 [details] [review]
Create podcasts playlist if it does not exist already.

 plugins/ipod/rb-ipod-source.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

Comment on attachment 85739 [details] [review]
Create podcasts playlist if it does not exist already.

Address the two deficiencies of the previous patch

Ah thanks for the update :)

Any reason why you put the podcast playlist as the second playlist on the ipod instead of letting libgpod deal with it? (I'm referring to the itdb_playlist_add_track (ipod_playlist, song, 1) call). I don't care either way, I'm just curious if it's something that must be done or if you just feel it's better that way.

There is indeed a reason.  I was having some trouble getting the Podcasts playlist created by rhythmbox recognized on my iPod.  I noticed that before I removed it, it was the second playlist, so I tried forcing it to be second.  I think my primary problem was something else, but it is working now, and is the second playlist.

Ok, thanks for the explanation :) gtkpod uses -1 when it adds a new playlist, so it probably doesn't matter, but yeah, it makes sense to put it at 1 and might be safer.

Committed to CVS, thanks for the updated patch.

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

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