GNOME Bugzilla – Bug 774757
[Tracker] Implement source notification through TrackerNotifier
Last modified: 2018-09-24 09:51:05 UTC
I'm attaching a patch implementing source notification in the tracker plugin through the TrackerNotifier API which is already in master. Pros: - Less code - Events are grouped before emitting, less signal emission overhead - GrlMedias notified upon *always* get the right type, this means grl_media_is_* works on every case, including GRL_CONTENT_REMOVED conditions. Cons: - per datasource notification is not implemented. In honesty I don't know what that is for, nor who configures that, but sounds slightly misguided to me. This might be a good bug report to figure out what to do with it. PS: I'm sure there's opened bugs around notification in the tracker plugin, but couldn't find any now... Hence this new one.
Created attachment 340374 [details] [review] tracker: Rewrite tracker GrlSource notification to use TrackerNotifier This is a new facility in Tracker abstracting usage of the GraphUpdated dbus signal, so is a perfect fit to use here. The event resources are rather plainly translated to GrlMedias so they are emitted. Events are also grouped so the tracker source makes use of the grl_source_notify_change_list(), which will avoid high signal emission overhead if there's many changes to notify about. FIXME: no per datasource notification is implemented. I don't know who is supposed to set this, and I question its existence :).
Review of attachment 340374 [details] [review]: We need to bump/improve configure.ac dependency to 1.11.0. ::: src/tracker/grl-tracker-source-notif.c @@ +57,3 @@ { + gchar *id_str = NULL; + GrlMedia *media = NULL; No need to start them initialized. @@ +85,3 @@ source = grl_tracker_source_find (""); + /* FIXME: no per datasource notification */ In that case, do you mind removing it in separated patch? @@ +112,2 @@ { + if (events->len == 0) When this can happen? If interesting enough, might worth to put comment and/or debug message? @@ +147,3 @@ { GrlTrackerSourceNotify *self = GRL_TRACKER_SOURCE_NOTIFY (object); Not 100% sure if someone else might ref self->notifier but removing the signal_handlers does not hurt here: if (self->notifier) { g_signal_handlers_disconnect_by_func (...) g_clear_object (...) }
Thanks for the review :) (In reply to Victor Toso from comment #3) > Review of attachment 340374 [details] [review] [review]: > > We need to bump/improve configure.ac dependency to 1.11.0. Yup, indeed. Now that TrackerNotifier is in, this should change. > > ::: src/tracker/grl-tracker-source-notif.c > @@ +57,3 @@ > { > + gchar *id_str = NULL; > + GrlMedia *media = NULL; > > No need to start them initialized. > > @@ +85,3 @@ > source = grl_tracker_source_find (""); > > + /* FIXME: no per datasource notification */ > > In that case, do you mind removing it in separated patch? Sure, I'm going to attach it to this bug# too. > > @@ +112,2 @@ > { > + if (events->len == 0) > > When this can happen? If interesting enough, might worth to put comment > and/or debug message? Nah, just removed it :). It's a paranoia check, but TrackerNotifier also does it, so no need for it. > > @@ +147,3 @@ > { > GrlTrackerSourceNotify *self = GRL_TRACKER_SOURCE_NOTIFY (object); > > > Not 100% sure if someone else might ref self->notifier but removing the > signal_handlers does not hurt here: > > if (self->notifier) { > g_signal_handlers_disconnect_by_func (...) > g_clear_object (...) > } The TrackerNotifier instance is private to GrlTrackerSourceNotify, so this should be the one and only copy of this object (and it can't be referenced on the outside). I've anyway changed it so we keep the signal ID and cancel it here.
Created attachment 340766 [details] [review] tracker: Remove per-device-source configuration This is not entirely right, and could yield errors similar to commit 73c1aa1e7, where events go undetected because the deletion time of properties is not guaranteed to be atomic, so there might be missed events where files no longer qualify for certain notifications. Given this is a configuration toggle meant for plugins/integrators, it seems we can do without it.
Created attachment 340767 [details] [review] tracker: Rewrite tracker GrlSource notification to use TrackerNotifier This is a new facility in Tracker abstracting usage of the GraphUpdated dbus signal, so is a perfect fit to use here. The event resources are rather plainly translated to GrlMedias so they are emitted. Events are also grouped so the tracker source makes use of the grl_source_notify_change_list(), which will avoid high signal emission overhead if there's many changes to notify about.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/106.