GNOME Bugzilla – Bug 633933
MediaServer2 plugin
Last modified: 2011-04-06 14:23:32 UTC
MediaServer2[1] specification allows applications to expose content through D-Bus. Rygel-grilo is an daemon that is able to provide Grilo[2] content using this specification. Thus, totem will be able to get content from Grilo: Jamendo, Youtube, UPnP, Podcasts, and so forth. [1] http://live.gnome.org/Rygel/MediaServer2Spec [2] http://live.gnome.org/Grilo
Created attachment 173790 [details] [review] Patch that implements MediaServer2 plugin Right now, this patch only allows to browse through content. I expect to add searching capabilities as soon as possible.
Review of attachment 173790 [details] [review]: Here are a few things I spotted. ::: src/plugins/mediaserver2/Makefile.am @@ +6,3 @@ +plugin_in_files = mediaserver2.plugin.in + +uidir = $(plugindir) This line is redundant. ::: src/plugins/mediaserver2/mediaserver2.plugin.in @@ +3,3 @@ +IAge=1 +_Name=MediaServer2 Browser +_Description=A plugin to let you browse media content through MediaServer2 DBUS specification. “using the MediaServer2 D-Bus specification.” ::: src/plugins/mediaserver2/totem-mediaserver2.c @@ +149,3 @@ + MS2Client *provider; + + gtk_tree_model_get (model, iter, MODEL_PROVIDER, &provider, -1); The provider is being leaked here, I think. @@ +285,3 @@ + + type = ms2_client_get_item_type (child->data); + icon = load_icon (type); Instead of loading the icon once for each row, it would be an order of magnitude more memory efficient to write a custom data function for the tree column which lazy loads the different icons (at most once each), and just returns the appropriate one. The column's type could be changed from a pixbuf to an MS2ItemType. @@ +365,3 @@ + MODEL_TYPE, &type, + MODEL_URL, &url, + MODEL_TITLE, &title, Looks like you're leaking provider, object_path, url and title. @@ +375,3 @@ + data->canceled = FALSE; + data->offset = 0; + data->parent_iter = iter; This looks like a bad idea. The tree's structure may change between this async call being started and it finishing, in which case the iter will be invalid in the callback. You should probably be using a GtkTreeRowReference instead. @@ +396,3 @@ + g_slice_free (GtkTreeIter, iter); + if (url) { + totem_add_to_playlist_and_play (priv->totem, url, title, TRUE); What happens if url == NULL? @@ +444,3 @@ + + gtk_widget_show_all (box); + totem_add_sidebar_page (priv->totem, "mediaserver2", "MediaServer2", box); Shouldn't this be translatable?
Note that I also don't want to see "MediaServer2" appearing in the UI. If it's going to allow browsing, it should be something like "Browse for Movies" or something.
(In reply to comment #2) > Review of attachment 173790 [details] [review]: > Thanks for the review. I'll take care of them and provide an updated version of the patch. > @@ +396,3 @@ > + g_slice_free (GtkTreeIter, iter); > + if (url) { > + totem_add_to_playlist_and_play (priv->totem, url, title, TRUE); > > What happens if url == NULL? That the element is not played :) Actually, it would be a better idea to not expose content that doesn't have an url. So I'll change it.
Grilo support would give us UPnP support for free. *** This bug has been marked as a duplicate of bug 628648 ***