GNOME Bugzilla – Bug 640365
[PATCH] Zeitgeist plugin
Last modified: 2011-03-11 15:36:45 UTC
Created attachment 179124 [details] [review] Zeitgeist plugin Hey, I'd like to propose our Zeitgeist plugin for inclusion in Totem. The plugin sends events to Zeitgeist when a media file is played and stopped, which provides Zeitgeist with much finer precision than just watching what is pushed into GtkRecentManager.
Review of attachment 179124 [details] [review]: ::: src/plugins/zeitgeist-dp/bacon-video.vapi @@ +1,1 @@ +[CCode (cprefix = "Bacon", lower_case_cprefix = "bacon_")] Why is that needed? If it's going to be used in plugins, I'd rather it was added properly through introspection. ::: src/plugins/zeitgeist-dp/totem-zeitgeist-dp-plugin.vala @@ +45,3 @@ + templates.add (event); + var ds = new Zeitgeist.DataSource.full ( + "org.gnome.Totem,dataprovider", Is the comma a typo? If not, then you might want to review the API. @@ +154,3 @@ + Zeitgeist.NFO_VIDEO : Zeitgeist.NFO_AUDIO; + + var f = File.new_for_uri (current_media.mrl); That'll fail when playing DVDs or even RTSP streams. @@ +155,3 @@ + + var f = File.new_for_uri (current_media.mrl); + if (f.query_exists (null)) { No sync queries in plugins please.
Created attachment 179252 [details] [review] Zeitgeist plugin > Why is that needed? If it's going to be used in plugins, I'd rather it was > added properly through introspection. There are multiple plugins that already use it (properties, screenshot, screensaver...). Unfortunately the Bacon module would require quite a lot of changes to be introspectable (and should probably have a separate dynamic library once it is)... Anyway the only function we need from there is the HAS_VIDEO flag, perhaps this could be added directly to TotemObject, as it's quite useful. > Is the comma a typo? If not, then you might want to review the API. No, it's not a typo, it's just an identifier of the data source, we chose to use this kind of format for those. > That'll fail when playing DVDs or even RTSP streams. File.new_for_uri() never fails, the operations on the returned File will and we now don't mind. > No sync queries in plugins please. Fixed to use async.
Review of attachment 179252 [details] [review]: In addition to the changes mentioned below, I would rather avoid the bacon-video.vapi changes, but the fact that they're in a separate directory probably makes this easier to ignore while we think of a better way to do this. Please file a separate bug about exporting this API to vala. ::: src/plugins/zeitgeist-dp/totem-zeitgeist-dp-plugin.vala @@ +14,3 @@ +class ZeitgeistDpPlugin: GLib.Object, Peas.Activatable { + private MediaInfo current_media; + // timer waiting while we get some info about current playing media Could you please use C-style comments as opposed to C++-style ones? (We even avoid them in the C++ used for the web browser plugin). @@ +71,3 @@ + + public void update_state () { + // FIXME: should we do something here? Should you? @@ +95,3 @@ + // wait a bit for the media info + if (media_info_timeout == 0) { + media_info_timeout = Timeout.add (250, wait_for_media_info); Don't do that. There's a"got_metadata" event which you should be using instead. @@ +155,3 @@ + + if (current_media.mrl != mrl || !object.is_playing ()) return; + current_media.mimetype = fi.get_content_type (); I'm pretty sure this is a sync function. Could we avoid doing that? @@ +193,3 @@ + if (!object.is_playing () && current_media.sent_access) { + // FIXME: sends leave event even if the user just pauses + // the playback for a little while Is there anything for us to figure out here? A link to a bug where a decision will be made would be appreciated. @@ +203,3 @@ + Zeitgeist.ZG_LEAVE_EVENT : Zeitgeist.ZG_ACCESS_EVENT; + /* + debug ("About to send %s event to ZG for: %s [%s] (%s)", Please remove the debug if it's not used.
> In addition to the changes mentioned below, I would rather avoid the > bacon-video.vapi changes, but the fact that they're in a separate directory > probably makes this easier to ignore while we think of a better way to do this. > Please file a separate bug about exporting this API to vala. Filed as https://bugzilla.gnome.org/show_bug.cgi?id=640870
Created attachment 179565 [details] [review] Zeitgeist plugin > Could you please use C-style comments as opposed to C++-style ones? (We even > avoid them in the C++ used for the web browser plugin). Changed to use C-style comments. > Don't do that. There's a"got_metadata" event which you should be using instead. I don't see any "got-metadata" signal in the gir, there's "metadata-updated" and we're already using that, but by itself it wasn't good enough (sometimes fired just once, sometimes multiple times, ...), so we added also the timer. > I'm pretty sure this is a sync function. Could we avoid doing that? What do you mean? is_playing()? You don't provide async variant of that. get_content_type() is a simple read from memory which was set during query_info_async() call, there's no blocking IO involved. > Is there anything for us to figure out here? A link to a bug where a decision > will be made would be appreciated. It was more of a note for us. > Please remove the debug if it's not used. Removed.
Review of attachment 179565 [details] [review]: Apart from the one C++-style comment, it looks fine to commit. I'll add a note saying that the zeitgeist API is horrible. It seems to leak implementation details into the API, and I'm not sure that anyone outside the Zeitgeist team would have been able to come up with the same code you did (short of copy-pasting the code). You might also want to double-check that the LGPLv3 is compatible with the GPLv2+ with exceptions that Totem uses (I'm not sure it is wrt. the patent stuff). ::: src/plugins/zeitgeist-dp/totem-zeitgeist-dp-plugin.vala @@ +96,3 @@ + if (media_info_timeout == 0) { + media_info_timeout = Timeout.add (250, wait_for_media_info); + // but make sure we dont wait indefinitely C++ comment.
Did somebody actually verify that the licenses were compatible?
I believe this clears up any license issues > https://lists.launchpad.net/libzeitgeist-friends/msg00008.html
commit 499028669c6a695551bafa85a198b191b76b9726 Author: Bastien Nocera <hadess@hadess.net> Date: Fri Mar 11 15:35:47 2011 +0000 build: Require Zeitgeist 0.3.6 to be license compliant https://bugzilla.gnome.org/show_bug.cgi?id=640365#c8