Re: [Rhythmbox-devel] [PATCH] Put podcasts on the Podcasts playlist

[Christophe Fergeau <teuf gnome org>]

Hi,

First, thanks for the patch ;)

Le mercredi 28 mars 2007 �9:56 -0400, Steven Walter a �it :
> This patch does two things.  First, in creating an iPod track from a
> RhythmDBEntry, the mediatype is set to PODCAST appropriately.  Second,
> after adding the track to the master playlist, the track is additionally
> added to the Podcasts playlist, both in the RB UI and in the itdb.

By "the Podcasts playlist in RB UI", do you mean the "main" podcast
source, or the Podcast playlist below the iPod source? (looking at the
code, I think it's the latter)

> 
> Comments on this code?  I'm not terribly familiar with the rhythmbox
> code, so please let me know if there's anything in it that would keep it
> from being applied.

The code globally looks good to me.

In add_to_podcasts, the loop with double indices is scary ;) I think
it's possible to at least store the RBStaticPlaylistSource*
corresponding to the podcast playlist in the RBiPodSourcePrivate struct
when the iPod database is loaded which would make the code easier to
understand and avoid relying on the ipod playlists and rb ipod playlists
being ordered the same way. I'd replace the return; with a break; at the
end of that function (though it's purely cosmetic ;)
And you should use 
    ITDB_MEDIATYPE_AUDIO      = 0x0001,
    ITDB_MEDIATYPE_PODCAST    = 0x0004,
from the Itdb_Mediatype libgpod enum instead of those 2 #defines:
+#define MEDIATYPE_AUDIO 0x1
+#define MEDIATYPE_PODCAST 0x4

Last but not least, could you file a bug in bugzilla where you attach
your patch, on the mailing list it may get lost/forgotten :)

Thanks again!

Christophe