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?
* there is no visual indicator that podcasts are downloading, or any way
to (that I've figured out) stop downloading one that is.
* am I right in thinking that unsubscribe stop future automatic
downloads?
* having "subscribe to new podcast" in the entry-view context menu,
as well as in the source context menu, would be nice.
* when using the site browser, quite often the info widget display the
information on the wrong podcast.
* the Preferences->General->Date checkbox doesn't seem to do anything.
* the podcast properties dialog probably should have the date in it.
general source things:
* rb-podcast-source.c:1225 - g_object_get should have NULL as an
additional parameter.
* 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.
* rb_podcast_source_add_post doesn't exist, so you can remove it from
rb-podcast-source.h
* RhythmDBEntry->date_str is also not used anywhere, and can be removed.
* 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.
* I don't think you need to distribute rb-podcast-marshal.{c,h} they
should be generated automatically.
* 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.
rb-podcast-download:
* it only needs to monitor CONF_PREFIX "/state/podcast", not CONF_PREFIX
* you don't need the constructor, if all it is doing is calling the
parent 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.
* the get/set property for the db should use the g_value_*_object
functions not the g_value_*_pointer ones.
* should rb_podcast_download_add_url have a g_free (data) at the end?
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)
* the ngettext in impl_get_status should probably be "%d post", "%d
posts".
Cheers,
James "Doc" Livingston
--
"Application encountered an error while failing. Error recovered
successfully, proceeding to fail"
Attachment:
signature.asc
Description: This is a digitally signed message part