GNOME Bugzilla – Bug 773028
Add libtracker-notify to wrap GraphUpdated
Last modified: 2016-11-20 19:20:26 UTC
Whenever a client wants to listen to notifications, we point them to the GraphUpdated signal which is 1) badly documented and 2) really finicky. This has resulted in several implementations over the place, which varying degrees of brokenness. I propose to turn this into API, and providing a libtracker-notify library that transparently handles or at least documents clearly the gotchas around GraphUpdated handling. This will hopefully replace all implementations around with a more reliable one. Patch coming.
Created attachment 337784 [details] [review] libtracker-notify: New library to receive notifications of changes This library abstracts GraphUpdated, and is now the recommended method to receive notifications upon changes in Tracker data. The library consists of a single TrackerNotifier object, which emits ::events signals. This signal packs all events received in one go, so as to avoid signal emission overhead on mass changes. The TrackerNotifier behavior can be slightly modified through the flags passed during instantiation. Eg. it can be requested to query some data (urn, url) upfront, although the API is tracker:id centric otherwise.
Created attachment 337797 [details] [review] libtracker-notify: New library to receive notifications of changes This library abstracts GraphUpdated, and is now the recommended method to receive notifications upon changes in Tracker data. The library consists of a single TrackerNotifier object, which emits ::events signals. This signal packs all events received in one go, so as to avoid signal emission overhead on mass changes. The TrackerNotifier behavior can be slightly modified through the flags passed during instantiation. Eg. it can be requested to query some data (urn, url) upfront, although the API is tracker:id centric otherwise.
Review of attachment 337797 [details] [review]: In principle, seems like a very good idea. I'm concerned about adding another small library to Tracker, there are overheads to each public library such as making https://developer.gnome.org/references into a super long list. What is the reason to not put this into libtracker-sparql ? I don't think anyone will want to use libtracker-notify but never query the Tracker database, so everyone will link to both anyway. I scanned the code & didn't see any obvious issues. If I get time, a good test would be to update the code in the functional tests to use this library, and hopefully replace some of the wacky Python code here: https://git.gnome.org/browse/tracker/tree/tests/functional-tests/common/utils/helpers.py#n276 ::: src/libtracker-notify/tracker-notifier.c @@ +817,3 @@ + * an element has multiple RDF types that will be notified + * upon (i.e. have tracker:notify in ontology), one event + * for each of these types will be received. This should go into a bit more detail. If I have a resource with 2 types, will I always receive 2 notifications of every change? Or does the notifications I receive correspond with the domains of the properties that have changed? It might be clearer to split into 2 paragraphs & reword a little, too? .... Returns the RDF type that this notification relates to, for example nmm:MusicPiece or nco:Contact. A resource can have multiple types. In the case of changes to a resource with multiple types, you will receive one notification for each RDF type. For performance reasons, Tracker only sends notifications for types that are explicitly marked with the tracker:notify property in their ontology. @@ +836,3 @@ + * Returns the Uniform Resource Name of the element if the + * notifier has the flag %TRACKER_NOTIFIER_FLAG_QUERY_URN, and + * it can be obtained at the time of emission. We should explain what a URN is. I don't think many people will be sure whether they want get_urn() or get_url() in most cases. @@ +854,3 @@ + * the flag %TRACKER_NOTIFIER_FLAG_QUERY_URL, and it can + * be obtained at the time of emission. + * Again, let's explain the difference from get_urn()... It might even be worth renaming this to get_location(); then it's more obvious that one function gives you the location of the file on disk, the other gives you an internal identifier.
Thanks for the review Sam :) (In reply to Sam Thursfield from comment #3) > Review of attachment 337797 [details] [review] [review]: > > In principle, seems like a very good idea. Cheers, hopefully this will become *the* implementation, and not just another one :). > > I'm concerned about adding another small library to Tracker, there are > overheads to each public library such as making > https://developer.gnome.org/references into a super long list. > > What is the reason to not put this into libtracker-sparql ? I don't think > anyone will want to use libtracker-notify but never query the Tracker > database, > so everyone will link to both anyway. I think you're right, it makes sense to put it together. Perhaps the opposite is true (querying but not caring about updates) for short lived tools, but this API doesn't add enough weight that it makes sense to separate. > > I scanned the code & didn't see any obvious issues. If I get time, a good > test would be to update the code in the functional tests to use this > library, and hopefully replace some of the wacky Python code here: > https://git.gnome.org/browse/tracker/tree/tests/functional-tests/common/ > utils/helpers.py#n276 Nice :), another good candidate for simplification is TrackerDecorator. > > ::: src/libtracker-notify/tracker-notifier.c > @@ +817,3 @@ > + * an element has multiple RDF types that will be notified > + * upon (i.e. have tracker:notify in ontology), one event > + * for each of these types will be received. > > This should go into a bit more detail. If I have a resource with 2 types, > will > I always receive 2 notifications of every change? Or does the notifications > I receive > correspond with the domains of the properties that have changed? > > It might be clearer to split into 2 paragraphs & reword a little, too? > .... Right, if the TrackerNotifier is subscribed to the 2 types, you would indeed receive 2 notifications, one per type. I 'll document that further. Apropos, with files with ambiguous mimetypes (eg. ogvs) we have tracker-miner-fs add several rdf:types (nfo:Video and nfo:Audio in this case), but tracker-extract doesn't drop the known wrong rdf:types after extraction. I think tracker-extract should drop the type in these cases, and TrackerDecorator should detect this "delete" and just avoid notificating if the NOTIFY_UNEXTRACTED flag is unset. > > Returns the RDF type that this notification relates to, for example > nmm:MusicPiece or nco:Contact. > > A resource can have multiple types. In the case of changes to a resource > with multiple types, you will receive one notification for each RDF type. > For performance reasons, Tracker only sends notifications for types that > are explicitly marked with the tracker:notify property in their ontology. > > @@ +836,3 @@ > + * Returns the Uniform Resource Name of the element if the > + * notifier has the flag %TRACKER_NOTIFIER_FLAG_QUERY_URN, and > + * it can be obtained at the time of emission. > > We should explain what a URN is. I don't think many people will be sure > whether they want get_urn() or get_url() in most cases. > > @@ +854,3 @@ > + * the flag %TRACKER_NOTIFIER_FLAG_QUERY_URL, and it can > + * be obtained at the time of emission. > + * > > Again, let's explain the difference from get_urn()... > > It might even be worth renaming this to get_location(); then it's more > obvious that one function gives you the location of the file on disk, the > other gives you an internal identifier. Location sounds good :). I'll try to explain better what an URN is, although the other API entries using this term aren't really verbose about it...
Created attachment 338799 [details] [review] libtracker-sparql: Add TrackerNotifier to subscribe to change notifications This object abstracts GraphUpdated, and is now the recommended method to receive notifications upon changes in Tracker data. After creation, the caller may connect to the ::events signal. This signal packs all events received in one go, so as to avoid signal emission overhead on mass changes. The TrackerNotifier behavior can be slightly modified through the flags passed during instantiation. Eg. it can be requested to query some data (urn, location) upfront, although the API is tracker:id centric otherwise.
Created attachment 340369 [details] [review] libtracker-miner: Use TrackerNotifier on TrackerDecorator Remove the GraphUpdated handler in favor of TrackerNotifier, it's been made to be a good fit here, and the porting results in a nice code removal. Given that the RDF types list is readwrite in TrackerDecorator, but construct-only in TrackerNotifier, this means we need to cater for changes in the RDF types list, just by creating another TrackerNotifier and dropping the older one.
With some more confidence after updating TrackerDecorator to use this and nicely working grilo patches, I've finally pushed this. Attachment 338799 [details] pushed as 8152d6d - libtracker-sparql: Add TrackerNotifier to subscribe to change notifications Attachment 340369 [details] pushed as f3f0033 - libtracker-miner: Use TrackerNotifier on TrackerDecorator