GNOME Bugzilla – Bug 645413
Write support for Tracker
Last modified: 2011-03-31 18:04:53 UTC
The Tracker back-end should support: - adding Personas - modifying existing Personas - remove Personas
I've started implementing this here: http://git.collabora.co.uk/?p=user/rgs/folks/.git;a=shortlog;h=refs/heads/tracker-write-support Still a bit to go but early reviews are welcomed.
(In reply to comment #1) > I've started implementing this here: > > http://git.collabora.co.uk/?p=user/rgs/folks/.git;a=shortlog;h=refs/heads/tracker-write-support > > Still a bit to go but early reviews are welcomed. For posterity, here's the first part of the review (sent by email while bgo was down): ===================================================================== Something I should have caught in the read-only review: backends/tracker/lib/trf-persona-store.vala =========================================== this._connection = Tracker.Sparql.Connection.get (); This needs to yield on Tracker.Sparql.Connection.get_async (); backends/tracker/lib/trf-persona-store.vala =========================================== + debug ("_insert_persona: %s", query); + variant = this._connection.update_blank (query); yield on update_blank_async() and make _insert_persona() async. + VariantIter iter1, iter2, iter3; + string anon_var = null; + iter1 = variant.iterator (); + + while (iter1.next ("aa{ss}", out iter2)) + { Needs to safely handle iter2 = null + while (iter2.next ("a{ss}", out iter3)) + { Needs to safely handle iter3 = null + while (iter3.next ("{ss}", out anon_var, out contact_urn)) + { + debug ("Inserted persona with URN %s", contact_urn); + return contact_urn; + } + } + } This (and it seems, the following commits) add private functions without using them. You should just squash these together, since they don't make sense in isolation. backends/tracker/lib/trf-persona-store.vala =========================================== + private string _single_value_query (string query) + { + var ret = ""; + try + { + Sparql.Cursor cursor = this._connection.query (query); yield on this function and make the parent function async backends/tracker/lib/trf-persona-store.vala =========================================== + private string _urn2tracker_id (string urn) I think _tracker_id_from_urn fits the naming conventions of Folks better. + return this._single_value_query (query); yield on this function and make the parent function async backends/tracker/lib/trf-persona-store.vala =========================================== + string contact_urn = this._insert_persona (builder.result); ... + string tracker_id = this._urn2tracker_id (urn); yield on both these functions tests/tracker/add-persona.vala ============================== + private async void _add_persona (PersonaStore pstore) + { + HashTable<string, Value?> details = new HashTable<string, Value?> + (str_hash, str_equal); + Value? v = Value (typeof (string)); + v.set_string (this._persona_fullname); + string attrib = "nco:fullname"; + details.insert ((owned) attrib, (owned)v); We should define an enum of common attribute types so clients can do a Persona add without having to care about the implementation of the backend. The reason we have an open-ended {string: GLib.Value} map for add_persona_from_details() is for PersonaStore-specific functions (eg, adding Tracker-specific tags to a persona), but this shouldn't be necessary for things as common as "full name". backends/tracker/lib/trf-persona-store.vala =========================================== + var urn = this._trackerid2urn (id); yield here (see below) + private string _trackerid2urn (string tracker_id) + { + string query = "SELECT fn:concat('<', tracker:uri(%s), '>') WHERE {}"; + return this._single_value_query (query.printf (tracker_id)); yield on this function and make the parent function async tests/tracker/remove-persona.vala ================================= + // Slightly higher timer because we need to sleep + // before removing an Individual because of Tracker's + // unpredictable way of firing GraphUpdate events. ... + // HACK: + // + // Tracker's GraphUpdated signals are delayed so + // we don't want to race with the Persona being + // added. Surely there's some signal or callback we can wait on instead of sleeping. Have you asked the Tracker developers about this? How would this Individual exist before we create its Persona (in reaction to the list of contacts in Tracker)? Or am I misunderstanding the situation? + string attrib = "nco:fullname"; Replace with enum value (see above) + foreach (unowned Individual i in added) + { ... + this._pstore.personas_changed.connect (this._personas_cb); Connect to this exactly once, not once per matching Individual (this should only be one time, but it's a bit fragile to do it this way). backends/tracker/lib/trf-persona-store.vala =========================================== + internal async void change_alias (Trf.Persona persona, string alias) internal functions should begin with a _ like private functions + { + const string query_t = "DELETE { ?p nco:nickname ?n } " + + "WHERE { ?p a nco:PersonContact ; nco:nickname ?n " + + ". FILTER(tracker:id(?p) = %s) } " + + "INSERT { ?p nco:nickname '%s' } " + + "WHERE { ?p a nco:PersonContact . " + + "FILTER (tracker:id(?p) = %s) } "; Use the existing static strings definitions for "nco:nickname", etc. backends/tracker/lib/trf-persona.vala ===================================== + this._alias = value; + this.notify_property ("alias"); + ((Trf.PersonaStore) this.store).change_alias (this, value); I wish we could do this all atomically, but this is probably the best option, since we don't want to block on a .begin()/.end() pair. Clients get immediate feedback, and if we fail to set this in Tracker due to its fault (eg, crashing), it's not a big loss. public GLib.List<FieldDetails> phone_numbers @@ -291,6 +312,7 @@ public class Trf.Persona : Folks.Persona, { if (nickname != null && this._nickname != nickname) { + this._alias = nickname; Don't do this - a person's nickname is something they've set themselves and an alias is a name they've given for that person. The user's alias for should override that person's nickname for themselves. We have logic for falling back to nickname if this._alias is empty, so it shouldn't be set unless the user truly filled it in. + // NOTE: + // setting the alias causes nco:nickname in Tracker + // to be modified. So if we see a nickname change, + // setting alias was correctly propagated to Tracker This makes it sound like we conflated nickname and alias in the Tracker backend. As long as nco:nickname doesn't get set by another Tracker client with the content of a person's nickname (eg, from Jabber), then we can just map nco:nickname <-> Folks.AliasDetails.alias. If not, let's pick an arbitrary field name (as close as we can get to "alias") to write for Trf.Personas as their alias. + // FIXME: + // it would be nice if we could just do: + // i.alias = "foobar" + // but we depend on the IndividualAggregator not + // having key-file hard-coded as the only writeable backend. + Trf.Persona p = (Trf.Persona)i.personas.nth_data (0); Will this be cleaned up with the primary backend work you're doing in Folks proper? backends/tracker/lib/trf-persona-store.vala =========================================== + internal async void change_is_favourite (Folks.Persona persona, Same concerns as change_alias() Furthermore, these "change_*" functions should be renamed "_set_*" for consistency /RemovePersonaTests/test adding personas to Tracker : Aborted FAIL: remove-persona This test fails for me
Part 2: backends/tracker/lib/trf-persona-store.vala =========================================== + internal async void _set_phones (Folks.Persona persona, + owned GLib.List<FieldDetails> phone_numbers) + { + const string del_q_t = "DELETE { ?p nco:hasAffiliation ?affl } WHERE " + + "{ ?p a nco:PersonContact ; nco:hasAffiliation ?affl . ?affl " + + "nco:hasPhoneNumber ?n . FILTER(tracker:id(?p) = %s) } "; + var p_id = ((Trf.Persona) persona).tracker_id (); Format del_q_t in a more readable way; use the OntologyDefs + builder.append (filter); + builder.where_close (); + + try + { + debug ("set_phones: %s", del_q + builder.result); The alignment here is off commit 5611cd65df26ada64fa6ea17e5050dc50bfe44b0 Add support to set a Trf.Persona's phone numbers This commit actually applies to emails, though I'm guessing you'll end up squashing it with other commits. + internal async void _set_emails (Folks.Persona persona, + owned GLib.List<FieldDetails> phone_numbers) + { + yield this._set_attrib (persona, (owned) phone_numbers, + Trf.Attrib.EMAILS); + } Change the parameter to email_addresses backends/tracker/lib/trf-persona-store.vala =========================================== + const string query_t = "DELETE { ?c nco:photo ?p } WHERE " + + "{ ?c a nco:PersonContact ; nco:photo ?p . " + + " FILTER(tracker:id(?c) = %s) } " + + "INSERT { _:i a nfo:Image, " + + "nie:DataObject ; nie:url '%s' . " + + " ?c nco:photo _:i } WHERE { ?c a nco:PersonContact . " + + " FILTER(tracker:id(?c) = %s) }"; Format this to be more readable tests/tracker/family-name-updates.vala ====================================== commit 33475a9885ff9c864b741cd2a1cbbc4a1aab1629 Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk> Date: Wed Mar 23 17:08:35 2011 +0000 Updated family name test to deal with null StructuredName diff --git a/tests/tracker/family-name-updates.vala b/tests/tracker/family-name- index dbcb7ee..137f858 100644 --- a/tests/tracker/family-name-updates.vala +++ b/tests/tracker/family-name-updates.vala @@ -77,7 +77,7 @@ public class FamilyNameUpdatesTests : Folks.TestCase this._test_family_name_updates_async (); - Timeout.add_seconds (5, () => + Timeout.add_seconds (6, () => Did this change really bump the maximum runtime to 6 seconds? I'd be very suspicious of it taking so long. backends/tracker/lib/trf-persona-store.vala =========================================== + persona._update_full_name + (details.lookup ("nco:fullname").get_string ()); Use the defined string + if (what != Trf.Attrib.URLS) + { ... + } + else + { It's generally nice to minimize the number of negatives, so please change the conditional to == and swap these two blocks tests/tracker/set-urls.vala =========================== + this._url_1 = "http://one.example.org"; + this._url_2 = "http://two.example.org"; + this._url_3 = "http://three.example.org"; + this._url_type_1 = "blog"; + this._url_type_2 = "website"; + this._url_type_3 = "url"; Fill these values into a HashMap; it will cut down on repetitive variables. When a new URL is "found", assrt that it's in this HashMap, then remove it (will will act as a "is found" mark). Assert that the HashMap has size = 0 at the end (instead of checking the *_found variables). backends/tracker/lib/trf-persona-store.vala =========================================== + /* FIXME: + * - this conversion should go away once we've switched to use the + * same data structure for each property that is a list of something. + */ Please file a bug for this (otherwise, it'll never get fixed) + * FIXME: + * - we are ignoring parameters attached to each + * FieldDetails for some cases. + * - we are leaking related objects Address these issues + addrs_1.add ("one@example.org".dup ()); + addrs_1.add ("two@example.org".dup ()); + im_addresses.insert ("aim".dup (), + (owned) addrs_1); + + var addrs_2 = new LinkedHashSet<string> (); + addrs_2.add ("three@example.org".dup ()); + addrs_2.add ("four@example.org".dup ()); + im_addresses.insert ("yahoo".dup (), + (owned) addrs_2); Don't use .dup() -- the data structures will copy keys and values as necessary. backends/tracker/lib/trf-persona-store.vala =========================================== + const string del_t = "DELETE { ?p nco:hasAffiliation ?a } " + + "WHERE { ?p a nco:PersonContact ; nco:hasAffiliation ?a . " + + "OPTIONAL { ?a nco:org ?o } . " + + "OPTIONAL { ?a nco:role ?r } . " + + "FILTER(tracker:id(?p) = %s) } "; Format and use pre-defined strings for nco:* backends/tracker/lib/trf-persona-store.vala =========================================== + const string del_t = "DELETE { ?p nco:note ?n } WHERE " + + "{ ?p a nco:PersonContact ; nco:note ?n . FILTER(tracker:id(?p) = %s)}" + Format and use pre-defined strings for nco:* backends/tracker/lib/trf-persona-store.vala =========================================== + const string q_t = "DELETE { ?p nco:birthDate ?b } " + + "WHERE { ?p a nco:PersonContact ; nco:birthDate ?b . " + + "FILTER (tracker:id(?p) = %s ) } " + + "INSERT { ?p nco:birthDate '%s' } " + + "WHERE { ?p a nco:PersonContact . " + + "FILTER (tracker:id(?p) = %s) } "; Format and use pre-defined strings for nco:* backends/tracker/lib/trf-persona-store.vala =========================================== + const string del_t = "DELETE { ?p nco:gender ?g } " + + "WHERE { ?p a nco:PersonContact ; nco:gender ?g . " + + "FILTER (tracker:id(?p) = %s) } "; + const string ins_t = "INSERT { ?p nco:gender %s } " + + "WHERE { ?p a nco:PersonContact . " + + "FILTER (tracker:id(?p) = %s) } "; Format and use pre-defined strings for nco:* backends/tracker/lib/trf-persona-store.vala =========================================== + * The following keys/values are valid: + * - full_name -> string + * - is_favourite -> bool ... + * - im-addresses -> HashTable<string, LinkedHashSet<string>> We need to be consistent with _ vs - (my vote is for -). But, perhaps more importantly, we need these defined in a single place, so the compiler can catch typos for everyone (otherwise it will be at runtime, which is a lot less useful). + * TODO: maybe we should define these keys? Yes - see earlier comment + details.insert ("full_name".dup (), (owned)v1); + + Value? v2 = Value (typeof (string)); + v2.set_string (this._persona_alias); + details.insert ("alias".dup (), (owned)v2); tests/tracker/add-persona.vala ============================== + details.insert ("full_name".dup (), (owned)v1); + + Value? v2 = Value (typeof (string)); + v2.set_string (this._persona_alias); + details.insert ("alias".dup (), (owned)v2); Don't call .dup(); the parent structure will copy tests/tracker/add-persona.vala ============================== + StructuredName sname = new StructuredName (null, null, null, null, null); + sname.family_name = this._family_name; + sname.given_name = this._given_name; Why set these seprately? You can just plug them into the constructor, can't you? + v4.set_object (sname); + details.insert ("structured_name", (owned)v4); tests/tracker/add-persona.vala ============================== - Timeout.add_seconds (5, () => + Timeout.add_seconds (8, () => Again, I'm a bit suspicious if this is really necessary. + private void _try_to_exit () Rename to _exit_if_all_properties_found() + private void _check_sname (StructuredName sname) Rename to something like _ack_structured_name_if_valid tests/tracker/add-persona.vala ============================== + if (verbose) + GLib.stdout.printf ("%s = %s\n", k, + v ? "true" : "false"); Make this an unconditional debug() - this._main_loop.quit (); + if (!dontquit) + this._main_loop.quit (); The dontquit part shouldn't exist in checked-in code. It looks like it's purely initial-development-only debugging. tests/tracker/add-persona.vala ============================== + this._address = new PostalAddress (null, null, null, null, null, + null, null, null, types, null); + this._address.po_box = this._po_box; + this._address.locality = this._locality; + this._address.postal_code = this._postal_code; + this._address.street = this._street; + this._address.extension = this._extension; + this._address.country = this._country; + this._address.region = this._region; Why not just plug these into the constructor? + PostalAddress postal_a = new PostalAddress (null, null, null, null, null, + null, null, null, types, null); + postal_a.po_box = this._po_box; + postal_a.locality = this._locality; ... Same here
All comments have been addressed here: http://git.collabora.co.uk/?p=user/rgs/folks/.git;a=shortlog;h=refs/heads/tracker-write-support
(In reply to comment #4) > All comments have been addressed here: > > http://git.collabora.co.uk/?p=user/rgs/folks/.git;a=shortlog;h=refs/heads/tracker-write-support In the future, please make changes as additional commits on the existing branch. Please don't rebase this branch (or if you do, do so as a separate branch). It's harder to review when all the older commit hashes change, Please see this article on git-rebase best practices: http://lwn.net/Articles/328436/
(In reply to comment #4) > All comments have been addressed here: > > http://git.collabora.co.uk/?p=user/rgs/folks/.git;a=shortlog;h=refs/heads/tracker-write-support This looks pretty good - please deal with the issues below, squash commits as necessary, merge your patches for bug #645989, rebase, then merge this to master. =========================================== This test fails: /AdditionalNamesUpdates/additional names updates: folks-CRITICAL **: folks_structured_name_get_additional_names: assertion `self != NULL' failed aborting...
+ Trace 226513
This test spits out a lot of unwanted debug output: /AddPersonaTests/test adding personas to Tracker : ** (/home/treitter/checkout/gnome/folks/tests/tracker/.libs/lt-add-persona:12020): DEBUG: add-persona.vala:490: full_name = true ** (/home/treitter/checkout/gnome/folks/tests/tracker/.libs/lt-add-persona:12020): DEBUG: add-persona.vala:490: url-2 = true ** (/home/treitter/checkout/gnome/folks/tests/tracker/.libs/lt-add-persona:12020): DEBUG: add-persona.vala:490: alias = true ** (/home/treitter/checkout/gnome/folks/tests/tracker/.libs/lt-add-persona:12020): DEBUG: add-persona.vala:490: url-1 = true ** (/home/treitter/checkout/gnome/folks/tests/tracker/.libs/lt-add-persona:12020): DEBUG: add-persona.vala:490: note-1 = true ... tests/tracker/remove-persona.vala ================================= + foreach (unowned Individual i in removed ) Cut this excess space backends/tracker/lib/trf-persona-store.vala =========================================== + var urn = yield this._urn_from_persona (persona); + yield this._remove_attributes (urn, _REMOVE_ALL_ATTRIBS); ... + private async void _remove_attributes_from_persona (Folks.Persona persona, + char remove_flag) + { + var urn = yield this._urn_from_persona (persona); + yield this._remove_attributes (urn, remove_flag); + } I think you meant to call this function from the first location instead of duplicating it.
Merged: commit 8b35f9a6498d48225e31b2dd4a86fd1dc64c664e Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk> Date: Fri Mar 18 14:45:23 2011 +0000 Add write support for Tracker Closes: bgo#645413 NEWS | 1 + backends/tracker/lib/trf-persona-store.vala | 1415 ++++++++++++++++++++++++--- backends/tracker/lib/trf-persona.vala | 243 ++++-- backends/tracker/lib/trf-util.vala | 18 + tests/tracker/Makefile.am | 96 ++ tests/tracker/add-contact.vala | 19 +- tests/tracker/add-persona.vala | 514 ++++++++++ tests/tracker/additional-names-updates.vala | 44 +- tests/tracker/family-name-updates.vala | 31 +- tests/tracker/given-name-updates.vala | 19 +- tests/tracker/name-details-interface.vala | 14 +- tests/tracker/nickname-updates.vala | 26 +- tests/tracker/prefix-name-updates.vala | 19 +- tests/tracker/remove-persona.vala | 208 ++++ tests/tracker/set-alias.vala | 168 ++++ tests/tracker/set-avatar.vala | 145 +++ tests/tracker/set-birthday.vala | 154 +++ tests/tracker/set-emails.vala | 160 +++ tests/tracker/set-favourite.vala | 180 ++++ tests/tracker/set-full-name.vala | 144 +++ tests/tracker/set-gender.vala | 142 +++ tests/tracker/set-im-addresses.vala | 177 ++++ tests/tracker/set-notes.vala | 153 +++ tests/tracker/set-phones.vala | 160 +++ tests/tracker/set-postal-addresses.vala | 174 ++++ tests/tracker/set-roles.vala | 153 +++ tests/tracker/set-structured-name.vala | 156 +++ tests/tracker/set-urls.vala | 166 ++++ tests/tracker/suffix-name-updates.vala | 3 + 29 files changed, 4643 insertions(+), 259 deletions(-)