Fwd: [Rhythmbox-devel] Second Podcast Patch

[Renato Araujo <renatox gmail com>]

On 9/7/05, James Livingston <jrl ids org au> wrote:
> G'day Renato,
>
> On Wed, 2005-09-07 at 01:34 -0300, Renato Araujo wrote:
> > In this patch i made various changes:
> > - changes for minimun memory usage;
> > - remove g_thread calls;
> > - created widget for podcast head information;
> > - usage gnome_vfs async calls;
> > - changes in the interface of podcast source;
> >
> > Please let me know what you think about it.
> >
> > Attached there are the diff against the CVS HEAD and a
> > tarball with the new files. To test it just apply the patch and
> > decompress the tarball inside your rhythmbox devel src dir.
>
> Nice work, it looks like it is coming along well. Below is a list of
> quite a few things I've though of. Don't worry about how long it is, a
> lot of them are minor nitpicks and a couple are trivial improvements.
>
>
> UI questions and improvements:
> * podcasts seem to download automatically, what does the download
>   item in the entry-view context menu do?
only the last post is downloaded automatically, the others posts need
to be downloaded manually like iTunes;

> * there is no visual indicator that podcasts are downloading, or any way
>   to (that I've figured out) stop downloading one that is.
The column status show the progress of download, I put this column
always visible;

> * am I right in thinking that unsubscribe stop future automatic
>   downloads?
Yes

> * having "subscribe to new podcast" in the entry-view context menu,
>   as well as in the source context menu, would be nice.
maked;

> * when using the site browser, quite often the info widget display the
>   information on the wrong podcast.
fixed;

> * the Preferences->General->Date checkbox doesn't seem to do anything.
fixed;

> * the podcast properties dialog probably should have the date in it.
fixed;


> general source things:
> * rb-podcast-source.c:1225 - g_object_get should have NULL as an
>   additional parameter.
fixed;

> * I don't think mountpoint should be used to store the uri of the local
>   copy of the podcast. I'm not sure what should, but this should remain
>   as the mountpoint the local copy is on - so it doesn't have strange
>   interactions with things that expect it to be the actual mountpoint.
I am not use the location field because this used as key for searchs,
and i used location field cause i didn't  want to create a new field;
We can think in an alternative later;

> * rb_podcast_source_add_post doesn't exist, so you can remove it from
>   rb-podcast-source.h
fixed;

> * RhythmDBEntry->date_str is also not used anywhere, and can be removed.
fixed;

> * rb_new_podcast_dialog_response_cb: should use g_strstrip to remove
>   whitespace surrounding the url. I recently did this for the iradio
>   source, and should be trivial to copy.
fixed;

> * I don't think you need to distribute rb-podcast-marshal.{c,h} they
>   should be generated automatically.
ok, I had forgotten;

> * it's probably better not to add podcast-specific column into
>   rb-entry-view.c, but add custom columns instead. See what the playlist
>   does for it's "track number" column.
ok, fixed;

>
>
> rb-podcast-download:
> * it only needs to monitor CONF_PREFIX "/state/podcast", not CONF_PREFIX
fixed;

> * you don't need the constructor, if all it is doing is calling the
>   parent constructor.
i use the constructor;

> * you don't need to call g_static_mutex_free in the finaliser, as it has
> an unbounded lifetime (i.e. is declared static in the file).

> * in _set_property you should remove the handlers for the current db
>   object (if there is one), or add G_PARAM_CONSTRUCT_ONLY.
fixed;

> * the get/set property for the db should use the g_value_*_object
>   functions not the g_value_*_pointer ones.
fixed;

> * should rb_podcast_download_add_url have a g_free (data) at the end?
fixed;

>
>
> rb-podcast-source:
> * the priv->title and priv->artist strings are never used, so you can
>   probably get rid of them (then you don't need the finaliser)
fixed;

> * the ngettext in impl_get_status should probably be "%d post", "%d
> posts".
fixed;


Hi

This is a new patch for podcast support.
I fixed all bugs reported and did some UI changes.

Ps.: Tnx James for the fast answer, all bugs reported were fixed

Links:
Patch: http://www.evolutum.gotdns.com/~indt/podcast.diff
Files: http://www.evolutum.gotdns.com/~indt/files.tar.bz2

BR
Renato Araujo Filho
INdT - Instituto Nokia de Tecnologia