GNOME Bugzilla – Bug 638281
Add an EDS backend
Last modified: 2011-07-12 18:52:30 UTC
To access and (in the future) store local contacts we need to implement an EDS backend. Raúl has an initial implementation of it at http://git.collabora.co.uk/?p=user/rgs/folks/.git;a=summary
What changes to e-d-s does this depend on?
Basically a few annotations were needed to generate a complete GIR file that then could be fed to vapigen. No code changes were needed. Changes can be found here: http://git.collabora.co.uk/?p=user/rgs/evolution-data-server/.git;a=summary I haven't tried to upstream them yet since e-d-s is undergoing an re-write now so I wanted to wait until the whole things settles. But in the meanwhile, to avoid blocking on them, I bundled the generated VAPIs with the backend. Is that even close to being acceptable?
I know this is a work in progress, but I have some quick comments on the branch. Don't just concatenate path with +, see g_build_filename(). Testing for the existence of a file/directory and then creating it is not really nice. You could just use g_mkdir_with_parents. The trust level of a local address book should be full. You put data in it and usually people mostly trust themselves. Something like e_book_query_any_field_contains ("") should be the query you want to use to match every contact. In _get_book_view, why do you need those casts of self and of the IDs? I'm not sure if it's better to keep all the values coming from EDS on the Persona or if it's better to keep the EContact around and when you do something like persona.foo the getter of the "foo" property just call e_contact_get. How can contact.get(E.Contact.field_id("family_name")) work? Shouldn't you just get the FN and N fields from the vcard and map them to the properties of the Names interface? + uint8[] v = {}; + for (int i=0; i<p.photo_data_length; i++) + { + uint8 temp = (uint8) p.photo_data[i]; + v += temp; + } This is going to reallocate the array for every byte.
In addition to Marco's comments: From eds-persona-store.vala: 141 // FIXME: There is no better error for this. 142 throw new PersonaStoreError.UNSUPPORTED_ON_USER ( 143 "Personas cannot be added to this store."); Add a new PersonaStoreError.READ_ONLY error code and use it instead. 188 catch (GLib.Error e1) 189 { 190 stdout.printf("Couldn't open addressbook with URI: %s\n", e1.message); 191 } Throw a suitable PersonaStoreError error instead. Why are you using a queue instead of a list in contacts_added() and contacts_removed()? From eds-persona.vala: 33 * TBD interfaces: Names, ExtendedInfo Looks like these have been implemented. :-) 71 private set 72 { 73 this._postal_addresses = new GLib.List<PostalAddress> (); 74 foreach (unowned PostalAddress pa in value) 75 { 76 this._postal_addresses.prepend (pa); 77 } 78 this._postal_addresses.reverse (); 79 } Couldn't you just do “this._postal_addresses = value.copy ()”? Same for phone numbers, e-mail addresses and URLs. 190 string uid = this.build_uid ("folks", store.id, id); The first parameter is supposed to be the backend name (“eds”). Does e-d-s provide some way of determining which Eds.Persona is the user? 241 emails.prepend (fd); You never reverse the emails list before assigning it to this.emails. 245 if (this.emails != emails) 246 { 247 this.emails = emails; 248 } I can't see how this check could ever fail. 282 if (avatar_path != "") 283 { 284 var avatar_file = File.new_for_path (avatar_path); 285 if (this.avatar != avatar_file) 286 { 287 this.avatar = avatar_file; 288 } 289 } this.avatar doesn't get updated if avatar_path == "". I don't see how the check this.avatar != avatar_file could ever fail unless Vala magically converts it into this.avatar.equals(avatar_file). 301 urls.prepend (new FieldDetails (u)); You never reverse the urls list before assigning it to this.urls. 316 this._get_im_eds_map().foreach ((proto, proto_aliases) => 317 { Is it possible to use foreach() here rather than a lambda function? It would be more efficient. 332 GLib.stdout.printf("Problem when trying to normalise address: %s\n", e.message); This should be GLib.warning(…). 356 if (FileUtils.test(photo_path, FileTest.EXISTS)) 357 { 358 FileUtils.remove(photo_path); 359 } Is it necessary to check for existence before removing? Why not just try to remove the file and ignore any errors? 403 GLib.List<string> properties = new GLib.List<string> (); 404 properties.append ("im_" + p + "_home_1"); 405 properties.append ("im_" + p + "_home_2"); 406 properties.append ("im_" + p + "_home_3"); 407 properties.append ("im_" + p + "_work_1"); 408 properties.append ("im_" + p + "_work_2"); 409 properties.append ("im_" + p + "_work_3"); It would be more efficient to prepend these in reverse order. 429 urls.append(u); It would be more efficient to prepend to urls and then reverse the list before returning. 447 phones.prepend (fd); You don't reverse the list before assigning it to this.phone_numbers. 483 addresses.prepend (pa); As above, the list isn't reversed before being assigned to this.postal_addresses. From tests/eds/individual-retrieval.vala: 29 /* Ignore the error caused by not running the logger */ 30 Test.log_set_fatal_handler ((d, l, m) => 31 { 32 return !m.has_suffix ("couldn't get list of favourite individuals: " + 33 "The name org.freedesktop.Telepathy.Logger was not provided by " + 34 "any .service files"); 35 }); Since the telepathy backend is disabled, you shouldn't need this. The backend loading changes look fine to me. A couple of really picky points: the folks coding style specifies a space between function names and opening brackets (e.g. “strcmp (foo, bar)” rather than “strcmp(foo, bar)”), and I noticed a few places where there were two consecutive blank lines, which should be fixed.
(In reply to comment #4) > Add a new PersonaStoreError.READ_ONLY error code and use it instead. That code is copied from the libsocialweb backend. The reason I didn't add it earlier is that I wanted to discuss writable things more. One of the options I saw was to have a different interface for writable backends. If not maybe juts modify the base class implementation to define this method returning an error. > Why are you using a queue instead of a list in contacts_added() and > contacts_removed()? That's something that is done a lot in tp-glib and gabble too. GLists are evil and GLib.List are even more evil :) > 71 private set > 72 { > 73 this._postal_addresses = new GLib.List<PostalAddress> (); > 74 foreach (unowned PostalAddress pa in value) > 75 { > 76 this._postal_addresses.prepend (pa); > 77 } > 78 this._postal_addresses.reverse (); > 79 } > > Couldn't you just do “this._postal_addresses = value.copy ()”? Same for phone > numbers, e-mail addresses and URLs. No, it doesn't copy the values inside the list. > Does e-d-s provide some way of determining which Eds.Persona is the user? I think to remember it does, but you don't get the self user with a normal query. Another problem is that photos can also be not inside the EContact, but just a URL pointing to the image file. Your code doesn't seem to handle it. It's probably fine for now just to add a FIXME and avoid crashing in that case.
(In reply to comment #5) > (In reply to comment #4) > > Add a new PersonaStoreError.READ_ONLY error code and use it instead. > > That code is copied from the libsocialweb backend. The reason I didn't add it > earlier is that I wanted to discuss writable things more. One of the options I > saw was to have a different interface for writable backends. If not maybe juts > modify the base class implementation to define this method returning an error. I see. How about adding a PersonaStoreError.READ_ONLY error code for now, and if we end up adding a different interface for writeable backends, or majorly rewriting the current one, it can be removed again (along with a load of other API, presumably). > > Why are you using a queue instead of a list in contacts_added() and > > contacts_removed()? > > That's something that is done a lot in tp-glib and gabble too. GLists are evil > and GLib.List are even more evil :) It might be better to use libgee's LinkedList implementation, since we're using libgee already. > > 71 private set > > 72 { > > 73 this._postal_addresses = new GLib.List<PostalAddress> (); > > 74 foreach (unowned PostalAddress pa in value) > > 75 { > > 76 this._postal_addresses.prepend (pa); > > 77 } > > 78 this._postal_addresses.reverse (); > > 79 } > > > > Couldn't you just do “this._postal_addresses = value.copy ()”? Same for phone > > numbers, e-mail addresses and URLs. > > No, it doesn't copy the values inside the list. Oh, the prepend() call adds a reference to pa? That makes sense. > > Does e-d-s provide some way of determining which Eds.Persona is the user? > > I think to remember it does, but you don't get the self user with a normal > query. The backend will need to support exposing the user.
(In reply to comment #6) > The backend will need to support exposing the user. Here's the relevant API: http://library.gnome.org/devel/libebook/stable/EBook.html#e-book-get-self e_book_get_self(), e_book_set_self(), e_book_is_self(). I'm not sure if it requires a special query, as Marco suggested, if you're using these functions. So it'd be good to double-check with the evolution developers.
I'll help in the next round of review, to avoid a little duplicate work here.
(In reply to comment #6) > It might be better to use libgee's LinkedList implementation, since we're using > libgee already. But then I have to create a new GList from it. Or not?
(In reply to comment #9) > (In reply to comment #6) > > It might be better to use libgee's LinkedList implementation, since we're using > > libgee already. > > But then I have to create a new GList from it. Or not? Aargh. Good point. Ignore me.
(In reply to comment #3) > I know this is a work in progress, but I have some quick comments on the > branch. > > Don't just concatenate path with +, see g_build_filename(). Testing for the > existence of a file/directory and then creating it is not really nice. You > could just use g_mkdir_with_parents. Fixed. > The trust level of a local address book should be full. You put data in it and > usually people mostly trust themselves. Maybe only for local and Gmails addressbooks? While not for ones that come from LDAP? > Something like e_book_query_any_field_contains ("") should be the query you > want to use to match every contact. That isn't accepted by e-d-s. Trying with "(contains \"x-evolution-any-field\" \"\")". Also, according to the type of addressbooks (local, LDAP, etc) we might want to scope the query in the future. > In _get_book_view, why do you need those casts of self and of the IDs? Fixed, actually there is no need to get the book view async-cally (and the libebook VAPI file stills needs some work to get the signatures for callbacks correctly). > I'm not sure if it's better to keep all the values coming from EDS on the > Persona or if it's better to keep the EContact around and when you do something > like persona.foo the getter of the "foo" property just call e_contact_get. I also wonder whats more convenient. > How can contact.get(E.Contact.field_id("family_name")) work? Shouldn't you just > get the FN and N fields from the vcard and map them to the properties of the > Names interface? Oops. Fixed (mapping EContactName to the fields in StructuredName). > > + uint8[] v = {}; > + for (int i=0; i<p.photo_data_length; i++) > + { > + uint8 temp = (uint8) p.photo_data[i]; > + v += temp; > + } > > This is going to reallocate the array for every byte. Ooops. Fixed. Attaching two patches: 1) that approaches (hopefully) or your comments 2) a patch that updates the tests to make them place nicely with the new backends I've also added a couple of simple tests for the e-d-s backend. Thanks for your patience and reviews guys!
Created attachment 177538 [details] [review] eds backend I'll address the rest of the comments/needed-fixes in the next run.
Created attachment 177539 [details] [review] updated tests to make them play nicely with the new backends tests fail when adding new backends
(In reply to comment #4) > 188 catch (GLib.Error e1) > 189 { > 190 stdout.printf("Couldn't open addressbook with URI: > %s\n", e1.message); > 191 } > > Throw a suitable PersonaStoreError error instead. I am not sure which of the available PersonaStoreErrors (INVALID_ARGUMENT, CREATE_FAILED, UNSUPPORTED_ON_USER, STORE_OFFLINE) is the suitable one for this case. Going with INVALID_ARGUMENT but it might be misleading. Is PersonaStoreError the right error domain? In folks/persona-store.vala: public abstract async void prepare () throws GLib.Error; > From eds-persona.vala: > > 33 * TBD interfaces: Names, ExtendedInfo > > Looks like these have been implemented. :-) Fixed. > 190 string uid = this.build_uid ("folks", store.id, id); > > The first parameter is supposed to be the backend name (“eds”). Fixed. > Does e-d-s provide some way of determining which Eds.Persona is the user? It does as noticed by Travis but i have to check if our default query is filtering this out. > 241 emails.prepend (fd); > You never reverse the emails list before assigning it to this.emails. > > 245 if (this.emails != emails) > 246 { > 247 this.emails = emails; > 248 } Fixed. > I can't see how this check could ever fail. > > 282 if (avatar_path != "") > 283 { > 284 var avatar_file = File.new_for_path (avatar_path); > 285 if (this.avatar != avatar_file) > 286 { > 287 this.avatar = avatar_file; > 288 } > 289 } > > this.avatar doesn't get updated if avatar_path == "". I don't see how the check > this.avatar != avatar_file could ever fail unless Vala magically converts it > into this.avatar.equals(avatar_file). Fixed. > 301 urls.prepend (new FieldDetails (u)); > > You never reverse the urls list before assigning it to this.urls. Fixed. > 316 this._get_im_eds_map().foreach ((proto, proto_aliases) => > 317 { > > Is it possible to use foreach() here rather than a lambda function? It would be > more efficient. Fixed. > 332 GLib.stdout.printf("Problem when trying to normalise > address: %s\n", e.message); > > This should be GLib.warning(…). Fixed. > 356 if (FileUtils.test(photo_path, FileTest.EXISTS)) > 357 { > 358 FileUtils.remove(photo_path); > 359 } > > Is it necessary to check for existence before removing? Why not just try to > remove the file and ignore any errors? Changed. > 403 GLib.List<string> properties = new GLib.List<string> (); > 404 properties.append ("im_" + p + "_home_1"); > 405 properties.append ("im_" + p + "_home_2"); > 406 properties.append ("im_" + p + "_home_3"); > 407 properties.append ("im_" + p + "_work_1"); > 408 properties.append ("im_" + p + "_work_2"); > 409 properties.append ("im_" + p + "_work_3"); > > It would be more efficient to prepend these in reverse order. Changed. > 429 urls.append(u); > > It would be more efficient to prepend to urls and then reverse the list before > returning. Changed. > 447 phones.prepend (fd); > > You don't reverse the list before assigning it to this.phone_numbers. Fixed. > 483 addresses.prepend (pa); > > As above, the list isn't reversed before being assigned to > this.postal_addresses. Fixed. > From tests/eds/individual-retrieval.vala: > > 29 /* Ignore the error caused by not running the logger */ > 30 Test.log_set_fatal_handler ((d, l, m) => > 31 { > 32 return !m.has_suffix ("couldn't get list of favourite > individuals: " + > 33 "The name org.freedesktop.Telepathy.Logger was not provided > by " + > 34 "any .service files"); > 35 }); > > Since the telepathy backend is disabled, you shouldn't need this. Fixed. > A couple of really picky points: the folks coding style specifies a space > between function names and opening brackets (e.g. “strcmp (foo, bar)” rather > than “strcmp(foo, bar)”), and I noticed a few places where there were two > consecutive blank lines, which should be fixed. That should be mostly fixexd, let me know if i fixed one.
Created attachment 177573 [details] [review] eds backend Address Phillip's observations.
(In reply to comment #5) > Another problem is that photos can also be not inside the EContact, but just a > URL pointing to the image file. Your code doesn't seem to handle it. It's > probably fine for now just to add a FIXME and avoid crashing in that case. Fixed: now we cover both cases (inlined or URI). This is still not so robust since i am not sure if e-d-s deals with other mime-types (i am assuming image/jpeg).
Created attachment 177580 [details] [review] eds backend i've extended the patch to catch both the inlined and URI contact photos that come from e-d-s
(In reply to comment #11) > > The trust level of a local address book should be full. You put data in it and > > usually people mostly trust themselves. > Maybe only for local and Gmails addressbooks? While not for ones that come from > LDAP? That sounds reasonable. Add a whitelist of types of address books which are trusted by the backend. > > I'm not sure if it's better to keep all the values coming from EDS on the > > Persona or if it's better to keep the EContact around and when you do something > > like persona.foo the getter of the "foo" property just call e_contact_get. > > I also wonder whats more convenient. I suppose it depends on the lifecycle of the pointers exposed by e-d-s. If we're notified every single time they're changed, we could just call e_contact_get() in the getter of the relevant property. If they're liable to change without us being notified, I guess we have to copy them. Without having looked at the code, not copying them sounds better. We're already quite heavy on memory usage. (In reply to comment #14) > (In reply to comment #4) > > 188 catch (GLib.Error e1) > > 189 { > > 190 stdout.printf("Couldn't open addressbook with URI: > > %s\n", e1.message); > > 191 } > > > > Throw a suitable PersonaStoreError error instead. > I am not sure which of the available PersonaStoreErrors (INVALID_ARGUMENT, > CREATE_FAILED, UNSUPPORTED_ON_USER, STORE_OFFLINE) is the suitable one for this > case. Going with INVALID_ARGUMENT but it might be misleading. Is > PersonaStoreError the right error domain? In folks/persona-store.vala: > > public abstract async void prepare () throws GLib.Error; PersonaStoreError is the correct domain; the signature for prepare() is just being a little vague (we can't tighten it up without breaking API). I guess the error code which is used depends on the error returned by e-d-s. INVALID_ARGUMENT would be fine for errors caused by the book URI being invalid. STORE_OFFLINE might be applicable in the case that the view can't be created due to it being an LDAP address book and the machine being offline (or something). For any other cases, we might have to create new error codes in PersonaStoreError. Thanks!
(In reply to comment #18) > > > I'm not sure if it's better to keep all the values coming from EDS on the > > > Persona or if it's better to keep the EContact around and when you do something > > > like persona.foo the getter of the "foo" property just call e_contact_get. > > > > I also wonder whats more convenient. > > I suppose it depends on the lifecycle of the pointers exposed by e-d-s. If > we're notified every single time they're changed, we could just call > e_contact_get() in the getter of the relevant property. If they're liable to > change without us being notified, I guess we have to copy them. > > Without having looked at the code, not copying them sounds better. We're > already quite heavy on memory usage. If you take a look at the _update* methods in backend/eds/eds-persona.vala you'll see there is a decent amount of computation to transform the data coming from e-d-s to the format in which the persona interfaces/properties are defined. We should consider the overhead cost of doing that on every property request. Regards the life cycle of contacts pointers given to us by e-d-s, we depend on the following signals from BookView: public virtual signal void contacts_added (GLib.List<weak void*> contacts); public virtual signal void contacts_changed (GLib.List<weak void*> contacts); public virtual signal void contacts_removed (GLib.List<weak void*> ids);
Created attachment 177602 [details] [review] eds backend Refreshed the patch with 2 little enhancements: 1) if the address book is writable the trust level is FULL. If it isn't we set the trust level to PARTIAL. 2) we set the is-user property to true if the contact is the user of the address book
Created attachment 177653 [details] [review] add libebook and libedataserver VAPI files I am breaking up the e-d-s backend patch into 2 commits: 1) the needed e-d-s VAPI files which in the future might be shipped with e-d-s' libs 2) the actual e-d-s backend. Sorry for the noise, hopefully this'll make reviews easier.
Created attachment 177654 [details] [review] eds backend e-d-s backend without libebook and libedataserver VAPI files
Created attachment 177655 [details] [review] updated tests to make them play nicely with the new backends last patch of the updated patch set.
(In reply to comment #16) > (In reply to comment #5) > > Another problem is that photos can also be not inside the EContact, but just a > > URL pointing to the image file. Your code doesn't seem to handle it. It's > > probably fine for now just to add a FIXME and avoid crashing in that case. > > Fixed: now we cover both cases (inlined or URI). This is still not so robust > since i am not sure if e-d-s deals with other mime-types (i am assuming > image/jpeg). Last I remember, it hard-coded support for image/jpeg and image/png. Please double-check this (in the code, if necessary).
This review is just for the eds backend itself - I'll review the tests in a following review. These may keep you busy for a while anyhow :) And we'll need to do a little more work after we finalize the pending interfaces that your branch is based upon, but I think we're making good progress. =================== A rather major change I think you should make is to split the backend into two libraries, as we have for the Telepathy backend. One is just a thin GModule that contains the Backend and BackendFactory. This will be installed in the backend directory. The "feature" library contains the core functionality. The GModule library links to the feature library and any client that wants to take advantage of EDS-specific functionality will also link against this feature library. This sort of separation lets us keep libfolks as simple as possible while still exposing backend-specific functionality through their feature libraries. + -DG_LOG_DOMAIN=\"LibEBook\" \ + public override string name { get { return "eds"; } } I recently factored out strings like this and made them the same for each backend/core. See these commits: http://git.gnome.org/browse/folks/commit/?id=7b39b7188be6bf37925f76615c9004c5151e543c http://git.gnome.org/browse/folks/commit/?id=cfdd138a035c6aa13889660852e13b2fb458dbd5 http://git.gnome.org/browse/folks/commit/?id=cf95d8dcce95840188c510d0f1cfe78af7a44ecc eds-backend-factory.vala should rename backend_factory to _backend_factory, since it's private. + string use_address_book = Environment.get_variable ("FOLKS_BACKEND_EDS_ADDRESSBOOK"); + if (use_address_book != null && use_address_book != "") + { + if (s.peek_name () == use_address_book) + { + add_addressbook(s); + } + } I think this variable should be named something like FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS and accept a list of address. There are a number of places where there's a missing space between a function name and its parameter list or between the closing ) of a cast and its operand. You should be able to find most of them with "egrep '[a-zA-Z]\(|\)[a-zA-Z]' *.vala" Also make sure your lines are < 80 characters wide. + public override async Folks.Persona? add_persona_from_details ( + // FIXME: There is no better error for this. + throw new PersonaStoreError.UNSUPPORTED_ON_USER ( + "Personas cannot be added to this store."); + public override async void remove_persona (Folks.Persona persona) + // FIXME: There is no better error for this. + throw new PersonaStoreError.UNSUPPORTED_ON_USER ( + "Personas cannot be removed from this store."); As discussed previously on this bug, add READ_ONLY to PersonaStoreError. It would be best to move this commit before the initial eds backend commit and then modify the backend commit to use this new error here. + private const string[] _linkable_properties = {}; This should have at least "im-addresses" and "emails" (though this should be "email-addresses" for consistency, but I'll that's an issue for bug #638279; it has to match the property name, so your code should match whatever name it ends up having). + string uid = this.build_uid ("eds", store.id, id); Another place to avoid a magic string. + this._update_avatar(contact); + this._update_avatar(contact); We probably don't need to do this twice in a row :) + * FIXME: save contact as an instance var and then call all the others. Agreed. I don't think it needs to be passed in as an argument, and we'll certainly want an accessor for it (for client programs, if they want to take advantage of EContact-specific functionality) + public void update (E.Contact contact) This should be internal instead of public (and thus renamed "_update"). + HashTable<string, GLib.List<string>> im_eds_map = this._get_im_eds_map () This can be simplified (without a loss of type safety) as: var im_eds_map = this._get_im_eds_map (); + private HashTable<string, GLib.List<string>> _get_im_eds_map() This should just use an internal "singleton" (ie, build the table the first time this function is called and return the existing table every time after that). + /* + * We save images in ~/.cache/folks/avatars/$ID_OF_THE_PERSONA + */ I think the code is simple enough that someone reading it could determine this path. But more importantly, if GLib.Environment.get_user_cache_dir() ends up returning a different value at some point, this comment will be wrong until someone notices the problem and fixes it (which tends to be a long time, regardless of project). And, beyond that, it's configurable at run-time in Unix (with XDG_CACHE_HOME) and this path is completely different in Windows (see the documentation for g_get_user_cache_dir()). + private string _get_photo_path(E.Contact contact) + { + string filename = this.uid + ".jpg"; We can't hard-code .jpg here, since the photo stored for a contact can be PNG (or, theoretically or actually, another format). + if (p.type == ContactPhotoType.INLINED) + { + try + { + GLib.FileUtils.set_data (photo_path, (uint8 []) p.photo_data); Use p.mime_type to determine the proper filename extension (I believe there's a GLib utility function to do that). diff --git a/configure.ac b/configure.ac ... + libxml-2.0]) Are you sure this is actually a direct requirement? (ie, do we actually use libxml Vala bindings ourselves?) +BACKEND_SW='$(top_builddir)/backends/libsocialweb/.libs/libfolks-backend-libsoc +AC_SUBST([BACKEND_SW]) +BACKEND_EDS='$(top_builddir)/backends/eds/.libs/libfolks-backend-eds.so' +AC_SUBST([BACKEND_EDS]) # All of the backend libraries in our tree; to be used by the tests -BACKEND_UNINST_PATH='$(BACKEND_KF):$(BACKEND_TP)' +BACKEND_UNINST_PATH='$(BACKEND_KF):$(BACKEND_TP):$(BACKEND_SW):$(BACKEND_EDS)' The addition of the libsocialweb components to these variables should be done in a separate commit since it's unrelated. Post-rebase changes =================== Here are some changes that you should make after rebasing your branch upon the latest version of master. You may want to defer these changes a bit, since you're already based upon a few outstanding branches that are based on the older version of master. I just pushed some changes to the way debugging output works, so you'll need to call Debug._register_domain() to redirect your debugging output. See the recent commits. Please add a bug for each of the outstanding TODOs and FIXMEs that remain when this gets merged.
The final commit in your public repo (updating the existing tests) looked fine. Here are my comments for the new eds backend tests: + c1.set(Folks.Backends.Eds.Persona.email_fields[0], "bernie@somedomain.org"); It's probably best to use an example.{org,net,com} domain, since those are guaranteed to not exist (in case we add tests where we send an email to our contacts, we don't want them to accidentally spam someone). + this.eds_backend.set_up("addressbook" + (Random.next_int() % 1000).to_string()); Tests need to be as consistent as possible, so don't use pseudo-random numbers here. If there were a good reason to use a bunch of pseudo-random data (eg, for fuzzing), we'd want to at least provide a consistent seed to begin with. In this case, it would be good to use the name of the test (factor it out to avoid having magic strings). + // BROKEN: haven't figure how to instantiate EContactAddress + //this.add_test ("postal addresses interface", this.test_postal_addresses); If you don't have a fix for this yet, just cut it out of this commit and add it in a latter commit. I'd like to avoid merging commented-out code. + public void test_postal_addresses () I think this test should validate the results a little more carefully - eg, in the individuals-changed handler, iterate through every key/value in c1 and assert the equivalent detail is in the persona (otherwise remove the key/value pair from the hash). After the loop, assert that c1 is empty. And the same for the other tests. Otherwise, if we introduced some bug that dropped/corrupted a specific address field, we probably wouldn't notice. + c3.set(Folks.Backends.Eds.Persona.email_fields[0], "barack@whitehouse.gov"); I know this is sometimes used for examples, but moreso than the somedomain.org addresses, I'd like to avoid accidentally sending spam here :) + void test_removal () As long as the e-d-s backend is read-only, this test should confirm that removing a contact results in a PersonaStoreError.READ_ONLY error, not that it was successful (since it shouldn't be). + void test_updates () Same here - this should confirm PersonaStoreError.READ_ONLY is thrown when it tries to change the persona's full name. The first pass of this backend should be read-only for simplicity. We can still wait to merge the branch until it's read-write if you'd like, but I think we should save writability for later commits (since this is a pretty huge commit as it is). +public class EdsTest.Backend +{ + private string addressbook_name; + private E.Book addressbook; + private GLib.List<E.Contact> e_contacts; These should be prepended with a _, since they're private. + private void _prepare_addressbook () throws BackendSetupError + { + this.addressbook.open (false); Make this function async and use open_async (). Even in tests, we should prefer async functions (since that's how we should be doing it behind the scenes as well). Same for all the instances of commit, add_contact, remove, etc. (And it's even more important that we do so in the backend itself). + private void _set_contact_fields (E.Contact contact, Gee.HashMap<string, stri Cut the commented code in this function + if (k.length > 12 && k.slice(0, 12) == "contact_name") This seems a bit fragile - is there any way you can determine the number instead of hardcoding it? If you do need to hard-code it for some reason, please define it as a constant with a name that explains its meaning.
As mentioned in the review for the tests, please ensure all the libebook and libedataserver functions we use are the async version (this includes at least Addressbook.open, .add_contact, .remove_contact, .commit, .remove, etc.). Folks is designed to never block on I/O (which can certainly block a long time in the case of e-d-s (particularly with an LDAP backend)), so we need to make sure that, eg, Eds.PersonaStore.prepare() (which is an async function) doesn't block waiting on E.Addressbook.open().
(In reply to comment #25) > =================== > A rather major change I think you should make is to split the backend into two > libraries, as we have for the Telepathy backend. One is just a thin GModule > that contains the Backend and BackendFactory. This will be installed in the > backend directory. The "feature" library contains the core functionality. The > GModule library links to the feature library and any client that wants to take > advantage of EDS-specific functionality will also link against this feature > library. > > This sort of separation lets us keep libfolks as simple as possible while still > exposing backend-specific functionality through their feature libraries. Done. > + -DG_LOG_DOMAIN=\"LibEBook\" \ > > + public override string name { get { return "eds"; } } > > I recently factored out strings like this and made them the same for each > backend/core. See these commits: > > http://git.gnome.org/browse/folks/commit/?id=7b39b7188be6bf37925f76615c9004c5151e543c > http://git.gnome.org/browse/folks/commit/?id=cfdd138a035c6aa13889660852e13b2fb458dbd5 > http://git.gnome.org/browse/folks/commit/?id=cf95d8dcce95840188c510d0f1cfe78af7a44ecc Done. > eds-backend-factory.vala should rename backend_factory to _backend_factory, > since it's private. Done. > + string use_address_book = Environment.get_variable > ("FOLKS_BACKEND_EDS_ADDRESSBOOK"); > > + if (use_address_book != null && use_address_book != "") > + { > + if (s.peek_name () == use_address_book) > + { > + add_addressbook(s); > + } > + } > > I think this variable should be named something like > FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS and accept a list of address. Done. I've also made it a class constant to avoid accumulating magic strings. > There are a number of places where there's a missing space between a function > name and its parameter list or between the closing ) of a cast and its operand. > You should be able to find most of them with "egrep '[a-zA-Z]\(|\)[a-zA-Z]' > *.vala" Done. > Also make sure your lines are < 80 characters wide. Done. > + public override async Folks.Persona? add_persona_from_details ( > > + // FIXME: There is no better error for this. > + throw new PersonaStoreError.UNSUPPORTED_ON_USER ( > + "Personas cannot be added to this store."); > > + public override async void remove_persona (Folks.Persona persona) > > + // FIXME: There is no better error for this. > + throw new PersonaStoreError.UNSUPPORTED_ON_USER ( > + "Personas cannot be removed from this store."); > > As discussed previously on this bug, add READ_ONLY to PersonaStoreError. It > would be best to move this commit before the initial eds backend commit and > then modify the backend commit to use this new error here. Done. > + private const string[] _linkable_properties = {}; > > This should have at least "im-addresses" and "emails" (though this should be > "email-addresses" for consistency, but I'll that's an issue for bug #638279; it > has to match the property name, so your code should match whatever name it ends > up having). Done. > + string uid = this.build_uid ("eds", store.id, id); > > Another place to avoid a magic string. Done. > + this._update_avatar(contact); > + this._update_avatar(contact); > > We probably don't need to do this twice in a row :) Oops. Fixed. > + * FIXME: save contact as an instance var and then call all the others. > > Agreed. I don't think it needs to be passed in as an argument, and we'll > certainly want an accessor for it (for client programs, if they want to take > advantage of EContact-specific functionality) Done. > + public void update (E.Contact contact) > > This should be internal instead of public (and thus renamed "_update"). Done. > + HashTable<string, GLib.List<string>> im_eds_map = this._get_im_eds_map > () > > This can be simplified (without a loss of type safety) as: > > var im_eds_map = this._get_im_eds_map (); Done. > + private HashTable<string, GLib.List<string>> _get_im_eds_map() > > This should just use an internal "singleton" (ie, build the table the first > time this function is called and return the existing table every time after > that). Done. > + /* > + * We save images in ~/.cache/folks/avatars/$ID_OF_THE_PERSONA > + */ > > I think the code is simple enough that someone reading it could determine this > path. But more importantly, if GLib.Environment.get_user_cache_dir() ends up > returning a different value at some point, this comment will be wrong until > someone notices the problem and fixes it (which tends to be a long time, > regardless of project). And, beyond that, it's configurable at run-time in Unix > (with XDG_CACHE_HOME) and this path is completely different in Windows (see the > documentation for g_get_user_cache_dir()). Agreed, comment is gone (it accidentally made it pass my initial tests). > + private string _get_photo_path(E.Contact contact) > + { > + string filename = this.uid + ".jpg"; > > We can't hard-code .jpg here, since the photo stored for a contact can be PNG > (or, theoretically or actually, another format). > + if (p.type == ContactPhotoType.INLINED) > + { > + try > + { > + GLib.FileUtils.set_data (photo_path, (uint8 []) p.photo_data); > > Use p.mime_type to determine the proper filename extension (I believe there's a > GLib utility function to do that). Hmmm, looking but couldn't find it. I'll keep digging. > diff --git a/configure.ac b/configure.ac > ... > + libxml-2.0]) > > Are you sure this is actually a direct requirement? (ie, do we actually use > libxml Vala bindings ourselves?) libebook and libedataserver have public methods that take libxml stuff as parameters... Good enough reason? > +BACKEND_SW='$(top_builddir)/backends/libsocialweb/.libs/libfolks-backend- libsoc > +AC_SUBST([BACKEND_SW]) > +BACKEND_EDS='$(top_builddir)/backends/eds/.libs/libfolks-backend-eds.so' > +AC_SUBST([BACKEND_EDS]) > > # All of the backend libraries in our tree; to be used by the tests > -BACKEND_UNINST_PATH='$(BACKEND_KF):$(BACKEND_TP)' > +BACKEND_UNINST_PATH='$(BACKEND_KF):$(BACKEND_TP):$(BACKEND_SW):$(BACKEND_EDS)' > > The addition of the libsocialweb components to these variables should be done > in a separate commit since it's unrelated. Done.
Created attachment 177962 [details] [review] eds backend
(In reply to comment #26) > + c1.set(Folks.Backends.Eds.Persona.email_fields[0], > "bernie@somedomain.org"); > > It's probably best to use an example.{org,net,com} domain, since those are > guaranteed to not exist (in case we add tests where we send an email to our > contacts, we don't want them to accidentally spam someone). Yup, changed. > + this.eds_backend.set_up("addressbook" + (Random.next_int() % > 1000).to_string()); > > Tests need to be as consistent as possible, so don't use pseudo-random numbers > here. If there were a good reason to use a bunch of pseudo-random data (eg, for > fuzzing), we'd want to at least provide a consistent seed to begin with. > > In this case, it would be good to use the name of the test (factor it out to > avoid having magic strings). True, changed. > + // BROKEN: haven't figure how to instantiate EContactAddress > + //this.add_test ("postal addresses interface", > this.test_postal_addresses); > > If you don't have a fix for this yet, just cut it out of this commit and add it > in a latter commit. I'd like to avoid merging commented-out code. Done. > + public void test_postal_addresses () > > I think this test should validate the results a little more carefully - eg, in > the individuals-changed handler, iterate through every key/value in c1 and > assert the equivalent detail is in the persona (otherwise remove the key/value > pair from the hash). After the loop, assert that c1 is empty. > > And the same for the other tests. > > Otherwise, if we introduced some bug that dropped/corrupted a specific address > field, we probably wouldn't notice. Done. > + c3.set(Folks.Backends.Eds.Persona.email_fields[0], > "barack@whitehouse.gov"); > > I know this is sometimes used for examples, but moreso than the somedomain.org > addresses, I'd like to avoid accidentally sending spam here :) Changed. > + void test_removal () > > As long as the e-d-s backend is read-only, this test should confirm that > removing a contact results in a PersonaStoreError.READ_ONLY error, not that it > was successful (since it shouldn't be). > > + void test_updates () > > Same here - this should confirm PersonaStoreError.READ_ONLY is thrown when it > tries to change the persona's full name. The first pass of this backend should > be read-only for simplicity. We can still wait to merge the branch until it's > read-write if you'd like, but I think we should save writability for later > commits (since this is a pretty huge commit as it is). These tests are actually aimed to test how the e-d-s backend reacts to updates and removal from the e-d-s side of the world not from Folks itself. Unless I am misunderstanding you... > +public class EdsTest.Backend > +{ > + private string addressbook_name; > + private E.Book addressbook; > + private GLib.List<E.Contact> e_contacts; > > These should be prepended with a _, since they're private. Done. > + private void _prepare_addressbook () throws BackendSetupError > + { > > + this.addressbook.open (false); > > Make this function async and use open_async (). Even in tests, we should prefer > async functions (since that's how we should be doing it behind the scenes as > well). > > Same for all the instances of commit, add_contact, remove, etc. (And it's even > more important that we do so in the backend itself). I'll take a go at making sure everything is async in my next iteration. Whatever isn't at the time was probably because I was struggling with getting the libebook VAPI file in good shape. Speaking of the e-d-s Vala bindings, who should we depend on to get those from (Vala?, e-d-s?) ? > + private void _set_contact_fields (E.Contact contact, Gee.HashMap<string, > stri > > Cut the commented code in this function Done. > + if (k.length > 12 && k.slice(0, 12) == "contact_name") > > This seems a bit fragile - is there any way you can determine the number > instead of hardcoding it? If you do need to hard-code it for some reason, > please define it as a constant with a name that explains its meaning. I can by using "contact_name".length (which is what i've done for now). It would be nice if Vala would let you slice up strings regardless of their length. I.e.: "foobar".slice(0, 10000) ---> "foobar" instead of giving you null as it does now).
Created attachment 178080 [details] [review] eds backend Updated tests. I need to make one final run to make sure every e-d-s operation is done in async fashion.
(In reply to comment #28) > > diff --git a/configure.ac b/configure.ac > > ... > > + libxml-2.0]) > > > > Are you sure this is actually a direct requirement? (ie, do we actually use > > libxml Vala bindings ourselves?) > libebook and libedataserver have public methods that take libxml stuff as > parameters... Good enough reason? Sure, that's a good reason. I just couldn't recall any places where that happened.
Sorry, this turned out to be bad for distcheck (and I've adjusted for it in master): + -DBACKEND_NAME=\"$(BACKEND_NAME)\" \ + -DG_LOG_DOMAIN=\"$(BACKEND_NAME)\" \ +-include backend.mk Could you just define BACKEND_NAME in backends/eds/Makefile.am and backends/eds/lib/Makefile.am? + public static const string use_addressbooks = + "FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS"; This should be named starting with a _ Because of that, it's currently shadowed by this definition: + string [] use_addressbooks = this._get_addressbooks_from_env (); These strings in the tests could similarly be factored out (just within their own files or the test library): + Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS", + this._addressbook_name, true); + Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS", "", true);
(In reply to comment #30) ... > > > > Same here - this should confirm PersonaStoreError.READ_ONLY is thrown when it > > tries to change the persona's full name. The first pass of this backend should > > be read-only for simplicity. We can still wait to merge the branch until it's > > read-write if you'd like, but I think we should save writability for later > > commits (since this is a pretty huge commit as it is). > > These tests are actually aimed to test how the e-d-s backend reacts to updates > and removal from the e-d-s side of the world not from Folks itself. Unless I am > misunderstanding you... I think the tests should test both sides of it (ie, both that we react correctly to e-d-s itself and that we pass along the proper errors). Feel free to start new additional tests if you don't want to cram it all into one file. Though, like you said, we don't want to test Folks itself here (that's for the general tests). > > + private void _prepare_addressbook () throws BackendSetupError > > + { > > > > + this.addressbook.open (false); > > > > Make this function async and use open_async (). Even in tests, we should prefer > > async functions (since that's how we should be doing it behind the scenes as > > well). > > > > Same for all the instances of commit, add_contact, remove, etc. (And it's even > > more important that we do so in the backend itself). > > I'll take a go at making sure everything is async in my next iteration. > Whatever isn't at the time was probably because I was struggling with getting > the libebook VAPI file in good shape. Speaking of the e-d-s Vala bindings, who > should we depend on to get those from (Vala?, e-d-s?) ? (Just mentioning this here so we've got a record of it) As discussed on IRC, the e-d-s Vala bindings should go within e-d-s itself (ideally). Certainly the gobject-introspection work should go directly into e-d-s. Though for testing and development purposes (short-term) we can maintain and ship our own vapi for e-d-s.
Raul, please see the latest attachment at bug #638279 and rebase your branch upon it. Note that the e-d-s tests (at least in my branch) don't pass. And we'll need to split the main eds backend library from its GModule library (the way we do for the Telepathy backend).
(In reply to comment #35) > Raul, please see the latest attachment at bug #638279 and rebase your branch > upon it. > > Note that the e-d-s tests (at least in my branch) don't pass. And we'll need to > split the main eds backend library from its GModule library (the way we do for > the Telepathy backend). I've got a slightly newer branch on that bug, in case you already started (re-rebasing would be trivial if you have). It also fixes the references for the e-d-s backend commits about which prior commits to squash them on.
(In reply to comment #33) > Sorry, this turned out to be bad for distcheck (and I've adjusted for it in > master): > > + -DBACKEND_NAME=\"$(BACKEND_NAME)\" \ > + -DG_LOG_DOMAIN=\"$(BACKEND_NAME)\" \ > > +-include backend.mk > > Could you just define BACKEND_NAME in backends/eds/Makefile.am and > backends/eds/lib/Makefile.am? > Done. > + public static const string use_addressbooks = > + "FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS"; > > This should be named starting with a _ > > Because of that, it's currently shadowed by this definition: > > + string [] use_addressbooks = this._get_addressbooks_from_env (); Done (and made it private too). > These strings in the tests could similarly be factored out (just within their > own files or the test library): > > + Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS", > + this._addressbook_name, true); > > + Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESSBOOKS", "", > true); Done.
Created attachment 179411 [details] [review] e-d-s backend implementation Almost everything should be adhering to the reviews on this bug report. There might be a place or two that still need be to switch to their async counterparts though.
Created attachment 179412 [details] [review] updated tests to make them play nicely with the new backends Removed a few old references to libsocialweb stuff.
Review of attachment 179411 [details] [review]: ::: backends/eds/lib/edsf-persona.vala @@ +319,3 @@ + } + + private string _get_photo_path () Since this calls _save_photo(), this should be async (see below). @@ +347,3 @@ + * If so, don't overwrite. + */ + private bool _save_photo (string photo_path, E.ContactPhoto p) This should be async. @@ +355,3 @@ + try + { + GLib.FileUtils.set_data (photo_path, (uint8 []) p.photo_data); Use GIO's async methods here (g_file_replace_contents_async() should work). @@ +370,3 @@ + File src_file = File.new_for_uri (p.photo_uri); + File dst_file = File.new_for_path (photo_path); + src_file.copy (dst_file, FileCopyFlags.OVERWRITE); This should also be async.
Review of attachment 179412 [details] [review]: Just a few whitespace issues. ::: tests/folks/backend-loading.vala @@ +56,3 @@ store.prepare.begin ((o, r) => { + store.disable_backend("eds"); Needs to be a space before "(". @@ +76,2 @@ assert (backends_expected.size == 0); + store.enable_backend("eds"); Needs to be a space before "(". ::: tests/key-file/individual-retrieval.vala @@ +51,3 @@ + store.disable_backend("eds"); + store.disable_backend("telepathy"); + store.disable_backend("libsocialweb"); Needs to be a space before "(". @@ +102,3 @@ + store.disable_backend("eds"); + store.disable_backend("telepathy"); + store.disable_backend("libsocialweb"); Needs to be a space before "(".
Add some more goal bugs for the 0.3.5 release.
(Actually setting the new milestone for these bugs)
Created attachment 180329 [details] [review] add (updated) libebook and libedataserver VAPI files I updated the libebook vapi file to treat ContactPhoto as a struct since we have no constructor for it and we need an instance of it for tests.
Created attachment 180330 [details] [review] e-d-s backend implementation New updates: * cleaned tests a little (everything should pass now!) * made avatar handling async * updated tests to match ESourceGroup's new URI format * fixed missing spaces in a few places
Created attachment 180331 [details] updated tests to make them play nicely with the new backend Fixed missing spaces in a few places
Created attachment 180332 [details] [review] updated tests to make them play nicely with the new backend Fixed missing spaces in a few places
Created attachment 180333 [details] [review] updated tests to make them play nicely with the new backend
Review of attachment 180330 [details] [review]: Lots of very minor comments. Sorry to keep putting you through this. ::: backends/eds/Makefile.am @@ +37,3 @@ + libedataserver-1.2 \ + libxml-2.0 \ + posix \ Is this still necessary? @@ +57,3 @@ + $(EDATASERVER_LIBS) \ + $(LIBXML_LIBS) \ + lib/libfolks-eds.la \ I think the .la libraries should be listed first (L[IB|D]ADD should be ordered from least to most general; see http://cygwin.ru/ml/automake/2003-10/msg00082.html). ::: backends/eds/eds-backend-factory.vala @@ +31,3 @@ + +/** + * The eds backend module entry point. All the Valadoc comments in the backend need @since 0.3.UNRELEASED added. ::: backends/eds/eds-backend.vala @@ +23,3 @@ +using GLib; +using E; +using Posix; Is this still used? @@ +42,3 @@ + + /** + * {@inheritDoc} Same here; all the comments need “@since 0.3.UNRELEASED” added. @@ +75,3 @@ + /** + * {@inheritDoc} + * Stray empty comment line. @@ +83,3 @@ + if (!this._is_prepared) + { + string [] use_addressbooks = this._get_addressbooks_from_env (); Can you put the array brackets (“[]”) next to the type name (i.e. “string[]” instead of “string []”)? @@ +98,3 @@ + if (s.peek_name () in use_addressbooks) + { + add_addressbook (s); Should be “this._add_addressbook (s)”. @@ +140,3 @@ + /** + * Add a new addressbook connected to a Persona Store. + * Stray empty comment line (and the comment needs a “@since” taglet). @@ +142,3 @@ + * + */ + private void add_addressbook (E.Source s) This should be “_add_addressbook” (underscores for private methods). @@ +164,3 @@ + private string [] _get_addressbooks_from_env () + { + string [] addressbooks = {}; s/string []/string[]/ ::: backends/eds/lib/Makefile.am @@ +11,3 @@ +VAPIGENFLAGS += \ + --vapidir=. \ + --vapidir=$(top_srcdir)/folks Please add a $(NULL) to the end of this list. @@ +48,3 @@ + --pkg libedataserver-1.2 \ + --pkg libxml-2.0 \ + --pkg posix \ Could you not use $(addprefix --pkg ,$(folks_backend_eds_deps))? @@ +61,3 @@ + libedataserver-1.2 \ + libxml-2.0 \ + posix \ Is this still needed? @@ +78,3 @@ + $(GEE_LIBS) \ + $(TP_GLIB_LIBS) \ + $(top_builddir)/folks/libfolks.la \ .la files should be listed first in LIBADD. ::: backends/eds/lib/edsf-persona-store.vala @@ +45,3 @@ + * The type of persona store this is. + * + * See {@link Folks.PersonaStore.type_id}. Again, @since should be added to all these Valadoc comments. @@ +54,3 @@ + * See {@link Folks.PersonaStore.can_add_personas}. + * + * @since 0.3.1 This @since should become “@since 0.3.UNRELEASED” — the “UNRELEASED” will be replaced just before the next release. @@ +198,3 @@ + private void _book_view_cb + (E.Book book, E.BookStatus status, E.BookView book_view) + { It looks like something's wrong with the indentation in this method, though it could just be my web browser. @@ +207,3 @@ + this._ebookview.start (); + + this.trust_level = PersonaStoreTrust.PARTIAL; Can we not detect the user's local address book and give it full trust (and all other address books partial trust)? @@ +233,3 @@ + { + string persona_id = this.id + ":" + (string) c.get + (E.Contact.field_id ("id")); It might be an idea to factor out construction of the persona_id into a private method. ::: backends/eds/lib/edsf-persona.vala @@ +25,3 @@ +using E; +using Xml; +using Posix; Is this still used? @@ +36,3 @@ + Phoneable, + Emailable, + URLable, Due to bug #641780, the URLable interface will be renamed to Urlable. @@ +39,3 @@ + GenderOwner +{ + public static const string [] phone_fields = { s/string []/string[]/ @@ +188,3 @@ + * Update attribs of the persona. + * + * FIXME: save contact as an instance var and then call all the others. Does this FIXME still apply? ::: tests/eds/individual-retrieval.vala @@ +5,3 @@ +public class IndividualRetrievalTests : Folks.TestCase +{ + private static const string TEST_SINGLETON_INDIVIDUALS_ADDREBOOK = s/ADDREBOOK/ADDRESSBOOK/ ::: tests/eds/persona-store-tests.vala @@ +8,3 @@ + "test_persona_store"; + private EdsTest.Backend eds_backend; + private HashSet<string> group_flags_received; We should probably follow the prefix-private-members-with-underscores convention in the tests too. ::: tests/lib/eds/Makefile.am @@ +34,3 @@ + $(GEE_LIBS) \ + $(top_builddir)/folks/libfolks.la \ + $(top_builddir)/backends/eds/libfolks-backend-eds.la \ .la files should be listed first.
Review of attachment 180333 [details] [review]: Looks good to commit once the backend's been committed.
Punting to 0.3.7.
e-d-s master now has the introspection and vapi files added.
(In reply to comment #52) > e-d-s master now has the introspection and vapi files added. Yeah - though annotations need some work. I've pushed a couple of new/fixed annotations to libebook this weekend, I need to push some more for libedataserver and then rebase the e-d-s backend on top of master.
I am rebasing this on top of master here: http://cgit.collabora.com/git/user/rgs/folks/log/?h=eds-0.5 I still need to: - update the tests - get rid of deprecated calls in e-d-s - fight a couple rough edges with introspection support / Vala bindings in e-d-s - general cleanups
Ok, the tests now compile too. Hopefully I'll get some time this afternoon to get rid of deprecated e-d-s stuff, work with the Vala devs to figure out one last bug (# 651132) thats preventing e-d-s from generating 100% usable Vala bindings and do general testing. Ah, and implement a couple of missing interfaces.
Pushed more changes: - all tests now pass - updated to new e-d-s API. Mysteriously, the async version of get_book_view () for BookClient is broken. Got debug that, might be the same thing alexl got bitten by Still missing: - add the other interfaces (Notes, etc) - write support
Why does the backend seem to repeat the full name both as the nickname (even if the nickname in the eds contact is empty) and the alias in folks? I don't want to show the name three times in the ui.
In fact, even if nickname is set in evo its not set by the backend. There is also some issues with the email subtypes, for instance i have a user with home and work email specified and they show up as email_1 and email_2 types. Also, the phone subtypes are e.g. mobile_phone, or other_phone. Is this some standard? I can't show them like that in the UI so i need to have some mapping from a well known standard backend set then (need that nevertheless since i need to translate these). And, how are custom phone types shown?
(In reply to comment #57) > Why does the backend seem to repeat the full name both as the nickname (even if > the nickname in the eds contact is empty) and the alias in folks? I don't want > to show the name three times in the ui. Oops. Fixed the nickname part. The AliasDetails interface is not implemented yet since I have to add write support first so we can write the Alias back to e-d-s.
(In reply to comment #58) > In fact, even if nickname is set in evo its not set by the backend. Fixed as noted in previous comment. > There is also some issues with the email subtypes, for instance i have a user > with home and work email specified and they show up as email_1 and email_2 > types. > > Also, the phone subtypes are e.g. mobile_phone, or other_phone. Is this some > standard? I can't show them like that in the UI so i need to have some mapping > from a well known standard backend set then (need that nevertheless since i > need to translate these). And, how are custom phone types shown? Hmm. I grabbed the emails from: http://git.gnome.org/browse/evolution-data-server/tree/addressbook/libebook/e-contact.h#n48 I think we should maybe use: http://git.gnome.org/browse/evolution-data-server/tree/addressbook/libebook/e-contact.h#n163 which might contain pretty-print values. I'll check with the Evo people. Same for phone. I'll try to get this fixed by this afternoon.
Proposed a patch for e-d-s to get inlined avatars (the only type e-d-s is using at the time?) working.
A couple of additions: commit bd7ea84d01ae0fda707a35e7d895106ce843c6f9 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 23:42:02 2011 +0100 e-d-s: support for removing personas commit ebfe6a73348890b40bddc5b67e2ce96b46018ccf Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 23:21:15 2011 +0100 e-d-s: support for structured names commit 49c9864d2944a5cca1c7c7d3a272e37a45c7e9c4 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 21:04:47 2011 +0100 e-d-s: support adding postal addresses commit b650e5f3a27da68662e856e88efa6cf0343a3316 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 20:50:09 2011 +0100 e-d-s: test for postal addresses details interface commit 7e4b53ce327769fbe69328107baf9c31e308da4c Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 18:54:47 2011 +0100 e-d-s: API change for test data backend commit 156d9a6c5712469930af76f5dcdfd8eace46fc05 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 15:29:15 2011 +0100 e-d-s: support adding phone numbers commit 67150e6298c6870a8ef95e6b9779ac2a4b875c17 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 15:12:19 2011 +0100 e-d-s: support adding im-addresss commit 4c4cf53e99b89927055e8ccafe8dcf09c8d1c02b Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 14:57:38 2011 +0100 e-d-s: test for im-details interface commit 8a5bd1adf1153b4463157568009fb96ebf98a45c Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 14:28:34 2011 +0100 e-d-s: support adding avatar and e-mails commit 01c5879ba569c2bfa9d4231f34e6869a3a189ad1 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 13:42:07 2011 +0100 e-d-s: test for adding personas commit 542ab99208ba895804ab4dc05b39c531d3203307 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 13:41:25 2011 +0100 e-d-s: basic support for adding Edsf.Personas commit beca5a3be88e5630ffb0cdcd93d3d599c557554b Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Jun 5 11:42:57 2011 +0100 e-d-s: Add test for AvatarDetails interface commit 4378075dc24ada998ad9c8813657f01d55b6f29d Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk> Date: Mon May 30 12:00:22 2011 +0100 e-d-s: improve avatar handling Next up: - Attribute updates - Pending interfaces - Linking support
Created attachment 189502 [details] [review] Readonly support for notes This patch adds readonly support for notes (i needed it for gnome-contacts testing of the notes feature).
(In reply to comment #63) > Created an attachment (id=189502) [details] [review] > Readonly support for notes > > This patch adds readonly support for notes (i needed it for gnome-contacts > testing of the notes feature). Merged into my branch, thanks. Also pushed support to update a couple of attributes: - full name - emails - im-addresses Still to go: - phones - postal addresses - names And also support to perform linking inside e-d-s.
Created attachment 189622 [details] [review] Update to build with latest API get_inlined now takes an out size_t parameter
Created attachment 189623 [details] [review] Fix the use of FieldDetails parameters for emails We now use the EVCard apis so that we get the correct vcard-style parameters for emails.
(In reply to comment #66) > Created an attachment (id=189623) [details] [review] > Fix the use of FieldDetails parameters for emails > > We now use the EVCard apis so that we get the correct vcard-style > parameters for emails. Pushed, thanks. Also pushed changes to update: - phones - postal addresses - names Would be nice to get some feedback once you've got adding/updates support in Gnome Contacts. Still to go: - support for linking (should we just go for linking via im addresses for the time being?) - a couple of interfaces (RoleDetails, BirthdayDetails, UrlDetails)
Created attachment 189771 [details] [review] Initial implementation of an e-d-s backend This is squashed to ease up review. It also includes support for linking personas.
Review of attachment 189771 [details] [review]: Review of everything except the test programs, which can be looked at later. Several of the points from my review in comment #49 haven't been fixed yet; they should all be fairly easy to fix. (Just me being picky, really.) I might've accidentally repeated a few of them in this review. (Sorry.) Nice work. :-) ::: backends/eds/Makefile.am @@ +18,3 @@ + +backenddir = $(BACKEND_DIR)/eds +backend_LTLIBRARIES = libfolks-backend-eds.la By bug #649296, we might want to just call it eds.la. @@ +34,3 @@ + libedataserver-1.2 \ + libxml-2.0 \ + posix \ Is posix really used? It's referenced as a dependency in several places, but does any code actually use it? ::: backends/eds/eds-backend.vala @@ +1,2 @@ +/* + * Copyright (C) 2010 Collabora Ltd. 2011! @@ +100,3 @@ + if (s.peek_name () in use_addressbooks) + { + add_addressbook (s); Missing a “this.”. @@ +105,3 @@ + else + { + add_addressbook (s); Missing a “this.”. @@ +153,3 @@ + var store = new Edsf.PersonaStore (s); + this._persona_stores.set (store.id, store); + store.removed.connect (this.store_removed_cb); Shouldn't you disconnect from this signal in store_removed_cb()? @@ +164,3 @@ + } + + private string [] _get_addressbooks_from_env () No space in “string[]”. ::: backends/eds/lib/Makefile.am @@ +45,3 @@ + --pkg libedataserver-1.2 \ + --pkg libxml-2.0 \ + --pkg posix \ This list of --pkg arguments could be produced using $(addprefix --pkg ,$(folks_backend_eds_deps)). ::: backends/eds/lib/edsf-persona-store.vala @@ +124,3 @@ + * Create a new PersonaStore. + * + * Create a new persona store to store the {@link Persona}s for the contacts Missing documentation for the “s” parameter. @@ +200,3 @@ + Set<PostalAddress> postal_addresses = + (Set<PostalAddress>) v.get_object (); + yield this._set_contact_postal_addresss (contact, s/sss/ss/. :-D Also, the indentation's wrong on this line. @@ +256,3 @@ + * Remove a {@link Persona} from the PersonaStore. + * + * See {@link Folks.PersonaStore.remove_persona}. Missing documentation for the “persona” parameter. @@ +271,3 @@ + { + /* FIXME: we need a specific error contact removal + * failures. */ File a separate bug report about adding such an error code. @@ +301,3 @@ + if (this._addressbook.is_opened ()) + { + Extraneous new line. @@ +356,3 @@ + private async void _set_contact_avatar (E.Contact contact, + File avatar) + { Indentation in this method is broken. @@ +360,3 @@ + { + uint8 []photo_content; + avatar.load_contents (null, out photo_content); Shouldn't there be a “yield” here? @@ +391,3 @@ + catch (GLib.Error error) + { + GLib.warning ("Can't update emails: %s\n", s/emails/e-mail addresses/. @@ +467,3 @@ + { + GLib.List <E.VCardAttribute> attributes = + new GLib.List <E.VCardAttribute>(); You could use “var” in the declaration here. @@ +475,3 @@ + foreach (var param_name in e.parameters.get_keys ()) + { + var param = new E.VCardAttributeParam (param_name.up()); Missing space after “.up”. @@ +485,3 @@ + } + + attributes.reverse (); Does it matter what order the attributes are in? @@ +554,3 @@ + catch (GLib.Error error) + { + GLib.warning ("Can't update emails: %s\n", error.message); s/emails/IM addresses/. @@ +594,3 @@ + lock (this._personas) + { + foreach (E.Contact c in (GLib.List<E.Contact>) contacts) Why is the cast necessary? @@ +622,3 @@ + { + persona._update (c); + } If (persona == null), should we perhaps add the persona to the store? @@ +641,3 @@ + } + + if (removed_personas.size> 0) Missing space before the “>”. ::: backends/eds/lib/edsf-persona.vala @@ +41,3 @@ + PostalAddressDetails +{ + public static const string [] phone_fields = { Why are these public? @@ +56,3 @@ + "blog_url", "fburl", "homepage_url", "video_url" + }; + private const string[] _linkable_properties = {"im-addresses"}; Need spaces between the braces and quotation marks. @@ +115,3 @@ + private set + { + } Perhaps a FIXME comment here would be better than nothing? @@ +241,3 @@ + internal static string build_iid (string store_id, string contact_id) + { + return "%s:%s".printf (store_id, contact_id); Do either of these IDs need escaping for colons first? Might be helpful to put an example of the input/output strings' formats in the documentation. @@ +244,3 @@ + } + + Extraneous new line. @@ +279,3 @@ + "full_name"); + + debug ("Creating new Edsf.Persona with id '%s'", iid); s/id/IID/, so that the message isn't ambiguous. @@ +349,3 @@ + this._email_addresses = new HashSet<FieldDetails> (); + + var attrs = _contact.get_attributes(E.ContactField.EMAIL); Missing space before the opening bracket. @@ +427,3 @@ + private void _update_avatar () + { + string filename = this.uid.delimit ("/", '-'); Might be more portable and clearer to use G_DIR_SEPARATOR instead of a “"/"”. @@ +439,3 @@ + var content_new = this._get_avatar_content_from_contact (p); + + if (content_old != content_new) It would be cheaper to do a comparison of just the files' sizes first; if they're different, it saves loading the old cached avatar for an expensive string comparison. @@ +446,3 @@ + content_new.length, + null, false, FileCreateFlags.REPLACE_DESTINATION, + null); This should really be async. @@ +458,3 @@ + { + this._avatar = null; + FileUtils.remove (cached_avatar_path); It would be better to consistently use the GFile version of cached_avatar_path, since that means we can make this remove() call async. @@ +497,3 @@ + { + string address = (owned) normalise_im_address + ((owned) field_value, proto); Missing an “ImDetails.” here. @@ +523,3 @@ + { + uint8 []content_temp; + this._avatar.load_contents (null, out content_temp); This should be async. @@ +549,3 @@ + uint8 []temp_content; + var file = File.new_for_uri (p.get_uri ()); + file.load_contents (null, out temp_content); This should be async. @@ +566,3 @@ + */ + internal static HashTable<string, GLib.List<string>> _get_im_eds_map () + { Might it be worth making this initialisation thread-safe? @@ +575,3 @@ + foreach (string p in protos) + { + GLib.List<string> properties = new GLib.List<string> (); You can use “var” here. @@ +624,3 @@ + private void _update_phones () + { + this._phone_numbers = new HashSet<FieldDetails> (); It'd be slightly cleaner to call “this._phone_numbers.clear ()” instead of replacing it with a new instance; and it would also save having to re-initialise this._phone_numbers_ro (which entails another object instantiation). Same with all the other _update_*() methods. @@ +626,3 @@ + this._phone_numbers = new HashSet<FieldDetails> (); + + var attrs = _contact.get_attributes(E.ContactField.TEL); Missing space before the opening bracket. @@ +661,3 @@ + { + /* FIXME: might be my broken setup, but it looks like + * e-d-s is ignoring the address_format param */ Has this been resolved? ::: backends/eds/lib/folks-eds.deps @@ +3,3 @@ +folks +libebook-1.2 +libedataserver-1.2 Missing libgee here? ::: tests/lib/eds/Makefile.am @@ +35,3 @@ + $(top_builddir)/folks/libfolks.la \ + $(top_builddir)/backends/eds/libfolks-backend-eds.la \ + $(NULL) You're missing EBOOK_LIBS and EDATASERVER_LIBS here. ::: tests/tools/eds.sh @@ +1,2 @@ +# +# Helper functions to start your own Tracker instance. This depends Tracker?
(In reply to comment #69) > @@ +439,3 @@ > + var content_new = this._get_avatar_content_from_contact (p); > + > + if (content_old != content_new) > > It would be cheaper to do a comparison of just the files' sizes first; if > they're different, it saves loading the old cached avatar for an expensive > string comparison. Most of the times (always?) the image coming from e-d-s won't be inlined, so file size comparison might be bogus. How about calculating a hash of the saved image and using that instead of full content comparison for next time?
(In reply to comment #70) > (In reply to comment #69) > > @@ +439,3 @@ > > + var content_new = this._get_avatar_content_from_contact (p); > > + > > + if (content_old != content_new) > > > > It would be cheaper to do a comparison of just the files' sizes first; if > > they're different, it saves loading the old cached avatar for an expensive > > string comparison. > > Most of the times (always?) the image coming from e-d-s won't be inlined, so > file size comparison might be bogus. How about calculating a hash of the saved > image and using that instead of full content comparison for next time? Meant to say: "will be inlined".
(In reply to comment #69) > @@ +446,3 @@ > + content_new.length, > + null, false, FileCreateFlags.REPLACE_DESTINATION, > + null); > > This should really be async. If this was async then _update_avatar () should be async too and we wouldn't be able to call _update_avatar () from Edsf.Persona's constructor, right? Unless we figure out what to do about properties that should be set in construction time vs thought that should be lazily (or asynchronously) loaded. Note that at the the time the IndividualAggregator expects that the (recently) created Personas it receives are in a consistent state. So if we still are setting properties while the IndividualAggregator is working on the Persona we'll have problem (as I've just experienced by seeing tests fail when making the above async).
Created attachment 189804 [details] [review] Initial implementation of an e-d-s backend I've address most of the points noted on comment #69 and made some further question about some of them. I'll address what's pending on comment #49 at some point today (hopefully). Thanks for the review!
Created attachment 189806 [details] [review] Pass null to E.ContactPhoto.get_inlined() size out argument It seems this function got a new argument at some point.
Created attachment 189807 [details] [review] Append to the writable url set, not the readonly one
Created attachment 189808 [details] [review] Implenment UrlDetails in Edsf.Persona Without this the merging of urls doesn't happen. Note that url setting is still not supported.
(In reply to comment #75) > Created an attachment (id=189807) [details] [review] > Append to the writable url set, not the readonly one Applied, thanks!
(In reply to comment #76) > Created an attachment (id=189808) [details] [review] > Implenment UrlDetails in Edsf.Persona > > Without this the merging of urls doesn't happen. Note that url setting > is still not supported. Applied, thanks!
(In reply to comment #74) > Created an attachment (id=189806) [details] [review] > Pass null to E.ContactPhoto.get_inlined() size out argument > > It seems this function got a new argument at some point. Fixed with latest Vala version.
Created attachment 189814 [details] [review] Initial implementation of an e-d-s backend Addressed issues of comment #49 and merged Alex's latest patches.
Review of attachment 189814 [details] [review]: Here's the first round of reviews. It's almost entirely building, running the tests, and edsf-persona-store.vala. I still need to review the other files. ======================= edsf-persona.vala:466.30-466.45: error: 1 missing arguments for `uint8[] E.ContactPhoto.get_inlined (out size_t len)' content = (string) p.get_inlined (); ^^^^^^^^^^^^^^^^ =================================== VALAC libfolks_eds_la_vala.stamp edsf-persona-store.vala:486.44-486.47: warning: Argument 1: Cannot pass null to non-null parameter type var attr = new E.VCardAttribute (null, attrib_name); The first argument to the constructor (group name) needs to be marked "allow-none=1" in the gobject annotation in EDS ====================== edsf-persona-store.vala ----------------------- /** * Create a new PersonaStore. * * Create a new persona store to store the {@link Persona}s for the contacts * There's redundancy here. this._source = s; this._addressbook_uri = uri; Source should be a readable property. Possibly the addressbook URI as well. public override async Folks.Persona? add_persona_from_details ( ... lock (this._personas) { string added_uid; var result = yield this._addressbook.add_contact (contact, out added_uid); This lock needs to happen after the async call to avoid its lifespan being multiple function calls. We should add a test case that adds a ton of contacts in parallel and make sure that they all successfully add without duplicates. public override async void remove_persona (Folks.Persona persona) throws Folks.PersonaStoreError { try { var contact_id = ((Edsf.Persona) persona).contact_id; E.Contact contact; yield this._addressbook.get_contact (contact_id, out contact); yield this._addressbook.remove_contact (contact); Why isn't the contact just a property on the Persona? Then we wouldn't need to fetch it here and the many other places we call get_contact() (which must end up burning a lot of time yielding instead of the constant-time lookup). /* FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=652425 */ throw new PersonaStoreError.INVALID_ARGUMENT ( "Can't remove contact: %s\n", e.message); Let's fix this bug so we can fix this code. /* FIXME: The async version of open () is mysteriously * broken. */ this._addressbook.open_sync (true, null); The async version needs to get fixed upstream (whether it's a problem in libebook or its introspection annotations) and we need to use it here. Edsf.PersonaStore.prepare() needs to throw GLib.Error catch (GLib.Error e1) { throw new PersonaStoreError.INVALID_ARGUMENT ( "Couldn't open addressbook with URI: %s\n", e1.message); } This is the result of bug #652425 again. private async void _set_contact_avatar (E.Contact contact, File avatar) { try { uint8[] photo_content; yield avatar.load_contents_async (null, out photo_content); We should optimize to use URIs as much as possible, though we can defer that until we fix bug #650414. /* FIXME: do we care about the order? */ attributes.reverse (); Nope. /* Lets reset everything first */ Let's In general, I think we should move the backend from using EContact to using EVCard (which EContact derives from). Then we can eliminate or dramatically simplify the equivalent of Edsf.Persona._get_im_eds_map(). We'll be able to use, eg, the VCard (attribute: X-AIM, parameter: TYPE=HOME) pair instead of im_aim_home_1, won't need to have the hard-coded _1, _2_, etc. ========================== backend.vala:51.11-51.46: warning: local variable `pa' declared but never used var pa = (PostalAddress) v.get_object (); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ==================== *** Warning: Linking the shared library libeds-test.la against the loadable module *** libfolks-backend-eds.so is not portable! make[4]: Leaving directory `/home/treitter/checkout/gnome/folks/tests/lib/eds' make[4]: Entering directory `/home/treitter/checkout/gnome/folks/tests/lib' ==================== All the EDS backend tests fail like: FAIL: set-postal-addresses /LinkPersonasTests/test linking personas: libebook-WARNING **: e_book_client_new: Cannot get book from factory: The name org.gnome.evolution.dataserver.AddressBook1 was not provided by any .service files I think this should be fixed by the port to GDBus, though.
(In reply to comment #81) > Review of attachment 189814 [details] [review]: > ======================= > > edsf-persona.vala:466.30-466.45: error: 1 missing arguments for `uint8[] > E.ContactPhoto.get_inlined (out size_t len)' > content = (string) p.get_inlined (); > ^^^^^^^^^^^^^^^^ > You've seem to have an old Vala version (get_inlined takes no argument, thats a transparent out param that holds the length of the returned array). > =================================== > > VALAC libfolks_eds_la_vala.stamp > edsf-persona-store.vala:486.44-486.47: warning: Argument 1: Cannot pass null to > non-null parameter type > var attr = new E.VCardAttribute (null, attrib_name); > > The first argument to the constructor (group name) needs to be marked > "allow-none=1" in the gobject annotation in EDS > > ====================== I don't get this, again - old Vala? > public override async Folks.Persona? add_persona_from_details ( > ... > lock (this._personas) > { > string added_uid; > var result = yield this._addressbook.add_contact (contact, > out added_uid); > > This lock needs to happen after the async call to avoid its lifespan being > multiple function calls. > This has been addressed. > public override async void remove_persona (Folks.Persona persona) > throws Folks.PersonaStoreError > { > try > { > var contact_id = ((Edsf.Persona) persona).contact_id; > E.Contact contact; > yield this._addressbook.get_contact (contact_id, out contact); > yield this._addressbook.remove_contact (contact); > > Why isn't the contact just a property on the Persona? Then we wouldn't need to > fetch it here and the many other places we call get_contact() (which must end > up burning a lot of time yielding instead of the constant-time lookup). I wasn't sure about the lifecycle of the E.Contact object, will check. > /* FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=652425 */ > throw new PersonaStoreError.INVALID_ARGUMENT ( > "Can't remove contact: %s\n", e.message); > > Let's fix this bug so we can fix this code. Lets please not block on this :( We can fix it afterwards. > /* FIXME: The async version of open () is mysteriously > * broken. */ > this._addressbook.open_sync (true, null); > > The async version needs to get fixed upstream (whether it's a problem in > libebook or its introspection annotations) and we need to use it here. I am hunting this. > In general, I think we should move the backend from using EContact to using > EVCard (which EContact derives from). Then we can eliminate or dramatically > simplify the equivalent of Edsf.Persona._get_im_eds_map(). We'll be able to > use, eg, the VCard (attribute: X-AIM, parameter: TYPE=HOME) pair instead of > im_aim_home_1, won't need to have the hard-coded _1, _2_, etc. > Agreed - I am moving towards thanks to alexl's patches. > All the EDS backend tests fail like: > This is odd - they work for me ... are you sure there ain't something old on your system?
(In reply to comment #82) > (In reply to comment #81) > > Review of attachment 189814 [details] [review] [details]: > > ======================= > > > > edsf-persona.vala:466.30-466.45: error: 1 missing arguments for `uint8[] > > E.ContactPhoto.get_inlined (out size_t len)' > > content = (string) p.get_inlined (); > > You've seem to have an old Vala version (get_inlined takes no argument, thats a > transparent out param that holds the length of the returned array). > Sorry - I think that was leftover from my original review (which I started last week, when I was running Vala 0.12). You can just ignore this part for now. > > =================================== > > > > VALAC libfolks_eds_la_vala.stamp > > edsf-persona-store.vala:486.44-486.47: warning: Argument 1: Cannot pass null to > > non-null parameter type > > var attr = new E.VCardAttribute (null, attrib_name); > > > > The first argument to the constructor (group name) needs to be marked > > "allow-none=1" in the gobject annotation in EDS > > > > ====================== > > > I don't get this, again - old Vala? I'm sure I saw this yesterday (after maintainer-clean) and I'm using Vala 0.12.0.296-1e90d. But I just tried again now and I didn't hit this error. > > /* FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=652425 */ > > throw new PersonaStoreError.INVALID_ARGUMENT ( > > "Can't remove contact: %s\n", e.message); > > > > Let's fix this bug so we can fix this code. > > Lets please not block on this :( We can fix it afterwards. But it should only take a minute. Just make an errordomain with the few values you need here and we'll expand as we need - we don't need to come up with all possible values now. > > All the EDS backend tests fail like: > > > > This is odd - they work for me ... are you sure there ain't something old on > your system? I'm checking this out - will follow-up. I forgot to mention this build error as well (which happens after a 'make maintainer-clean'): make[4]: Entering directory `/home/treitter/checkout/gnome/folks/backends/eds/lib' make[4]: *** No rule to make target `folks-eds.vapi', needed by `all-am'. Stop. make[4]: *** Waiting for unfinished jobs.... VALAC libfolks_eds_la_vala.stamp edsf-persona-store.vala:346.19-347.78: warning: unhandled error `Folks.PersonaStoreError' Compilation succeeded - 1 warning(s) You probably just need to adjust the stamp target - should it really just be empty as it is now?
Please add this trivial patch as well (so we can run all the eds tests through gdb easily): diff --git a/backends/eds/lib/Makefile.am b/backends/eds/lib/Makefile.am index 90f7c1e..1c68ef1 100644 --- a/backends/eds/lib/Makefile.am +++ b/backends/eds/lib/Makefile.am @@ -115,3 +115,4 @@ EXTRA_DIST = \ -include ../backend.mk -include $(top_srcdir)/git.mk +-include $(top_srcdir)/check.mk
(In reply to comment #84) > Please add this trivial patch as well (so we can run all the eds tests through > gdb easily): > > diff --git a/backends/eds/lib/Makefile.am b/backends/eds/lib/Makefile.am > index 90f7c1e..1c68ef1 100644 > --- a/backends/eds/lib/Makefile.am > +++ b/backends/eds/lib/Makefile.am > @@ -115,3 +115,4 @@ EXTRA_DIST = \ > > -include ../backend.mk > -include $(top_srcdir)/git.mk > +-include $(top_srcdir)/check.mk Actually, only once we've merged bug #652434. So, disregard unless that's merged first.
Created attachment 189888 [details] [review] Initial implementation of an e-d-s backend A couple of minor updates (most notable: now we can use the async version of e_client_open, things should be way faster).
(In reply to comment #84) > Please add this trivial patch as well (so we can run all the eds tests through > gdb easily): > > diff --git a/backends/eds/lib/Makefile.am b/backends/eds/lib/Makefile.am > index 90f7c1e..1c68ef1 100644 > --- a/backends/eds/lib/Makefile.am > +++ b/backends/eds/lib/Makefile.am > @@ -115,3 +115,4 @@ EXTRA_DIST = \ > > -include ../backend.mk > -include $(top_srcdir)/git.mk > +-include $(top_srcdir)/check.mk Furthermore, I patched the wrong Makefile.am. It really should be: diff --git a/tests/eds/Makefile.am b/tests/eds/Makefile.am index 9b41868..ff3ce3c 100644 --- a/tests/eds/Makefile.am +++ b/tests/eds/Makefile.am @@ -196,3 +196,4 @@ EXTRA_DIST = \ $(NULL) -include $(top_srcdir)/git.mk +-include $(top_srcdir)/check.mk Trying to react to this bug while being in a meeting is a bad idea. :)
(In reply to comment #86) > Created an attachment (id=189888) [details] [review] > Initial implementation of an e-d-s backend > > A couple of minor updates (most notable: now we can use the async version of > e_client_open, things should be way faster). + Signal.connect (this._addressbook, "opened", + (GLib.Callback) this._addressbook_opened_cb, this); We have to do this because this._addressbook.opened.connect (this._addressbook_opened_cb); fails due to EClient.opened being the property (and not the signal with the same name)? Couldn't we just connect to notify["opened"] in that case? + private void _addressbook_opened_cb (void *error, Edsf.PersonaStore self) + { + self.get_book_view (); + } + + public async void get_book_view () + { I don't think get_book_view() should be public - anything to do with starting the view should be a consequence of prepare(). You might as well just move its body into _addressbook_opened_cb(). And if it were public, it should be named something like start_book_view().
(In reply to comment #88) > (In reply to comment #86) > > Created an attachment (id=189888) [details] [review] [details] [review] > > Initial implementation of an e-d-s backend > > > > A couple of minor updates (most notable: now we can use the async version of > > e_client_open, things should be way faster). > > + Signal.connect (this._addressbook, "opened", > + (GLib.Callback) this._addressbook_opened_cb, this); > > We have to do this because > > this._addressbook.opened.connect (this._addressbook_opened_cb); > > fails due to EClient.opened being the property (and not the signal with the > same name)? Couldn't we just connect to notify["opened"] in that case? > > + private void _addressbook_opened_cb (void *error, Edsf.PersonaStore self) > + { > + self.get_book_view (); > + } > + > + public async void get_book_view () > + { > > I don't think get_book_view() should be public - anything to do with starting > the view should be a consequence of prepare(). You might as well just move its > body into _addressbook_opened_cb(). > > And if it were public, it should be named something like start_book_view(). I'll revert all of this now that: https://bugzilla.gnome.org/show_bug.cgi?id=652530 has been fixed.
I've reverted the previous commit and also reworked prepare(): http://cgit.collabora.com/git/user/rgs/folks/commit/?h=eds-0.5&id=403631eb7935032d3a44bf849780ee3a60896906
(In reply to comment #81) > All the EDS backend tests fail like: > > FAIL: set-postal-addresses > /LinkPersonasTests/test linking personas: > libebook-WARNING **: e_book_client_new: Cannot get book from factory: The name > org.gnome.evolution.dataserver.AddressBook1 was not provided by any .service > files > > I think this should be fixed by the port to GDBus, though. Good news, everyone! I rebuilt everything in my development PREFIX from Folks down and that seems to have eliminated this. All the tests pass as expected.
CC individual-retrieval.o link-personas.vala:270.17-270.63: warning: unhandled error `Folks.IndividualAggregatorError' yield this._aggregator.link_personas (this._personas); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Build warning.
My last set of comments - once these and my prior comments are addressed, we can merge this backend. edsf-persona.vala ----------------------- public class Edsf.Persona : Folks.Persona, This should also implement LocalIdAddressDetails and WebServiceAddressDetails. private const string[] _linkable_properties = { "im-addresses" }; Please support linking by web-service-addresses and local-ids. /* The following 4 definitions are used by the tests */ public static const string[] phone_fields = { "assistant_phone", "business_phone", "business_phone_2", "callback_phone", "car_phone", "company_phone", "home_phone", "home_phone_2", "isdn_phone", "mobile_phone", "other_phone", "primary_phone" }; public static const string[] address_fields = { "address_home", "address_other", "address_work" }; public static const string[] email_fields = { "email_1", "email_2", "email_3", "email_4" }; public static const string[] url_properties = { "blog_url", "fburl", "homepage_url", "video_url" }; These don't need to be changed immediately, but we will change these when we switch to EVCard (which should hopefully be shortly after mering). private weak E.Contact _contact; This should be a public read-only property (to replace all the this._addressbook.get_contact() calls in Edsf.PersonaStore). I think you said something about the EBookView replacing instances of EContacts, but in that case, we'll just need the PersonaStore to compare the contents of contacts-changed and update the contact property of each Persona where the contact UIDs match (and we'll need to make the Edsf.Persona.contact internally-writable). public Set<Note> notes { get { return this._notes_ro; } private set { /* FIXME: to be implemented */ Please implement. /* FIXME: should we escape/replace colons in store_id and * contact_id? We still have copies of both these * parameters saved so I am not sure if its worth it. */ return "%s:%s".printf (store_id, contact_id); No need to escape them, since iid is officially opaque. Cut this FIXME. /* FIXME: * * For some unknown reason when we create an e-d-s contact from * add_persona_from_details (), the contact's id might not be * immediately retrievable. So we workaround by using contact_uid * which was returned by BookClient.add_contact () instead of * querying the contact for its id property. * * This should be tracked with the e-d-s devs. */ Is this still true? internal static HashTable<string, GLib.List<string>> _get_im_eds_map () Let's simplify this when we port completely to EVCard /* * TODO: we should check if addresses corresponding to different types * are the same and if so instantiate only one PostalAddress * (with the given types). */ private void _update_addresses () We could do a simple comparison (ie, all fields case-insensitively-identical). Doing it properly (normalizing addresses) would be infeasible though. edsf-persona-store.vala ----------------------- This doesn't seem to stop the book view and close the ebookclient. Both should happen when disposing of the Edsf.PersonaStore and when unprepare()ing a store. And we should be unprepare()ing all the known stores in Edsf.Backend.unprepare() add-persona.vala ----------------------- We should add a variation on this (later in our ordered list of TESTS) that adds a ton of contacts (on the order of a few thousand) and makes sure they're successfully added and we don't get duplicates. The point of this is to make sure our locking issue has truly been resolved.
(In reply to comment #93) > /* FIXME: should we escape/replace colons in store_id and > * contact_id? We still have copies of both these > * parameters saved so I am not sure if its worth it. */ > return "%s:%s".printf (store_id, contact_id); > > No need to escape them, since iid is officially opaque. Cut this FIXME. I think there is, otherwise (e.g.) store_id and contact_id could be "file://foo:bar:baz" and "file://bish:bosh:bash" resulting in an ambiguous string being constructed. It's not likely that the IDs could be in such pathogenic formats, but iirc they're both URIs, so could contain colons. Raul, is this correct?
(In reply to comment #94) > (In reply to comment #93) > > /* FIXME: should we escape/replace colons in store_id and > > * contact_id? We still have copies of both these > > * parameters saved so I am not sure if its worth it. */ > > return "%s:%s".printf (store_id, contact_id); > > > > No need to escape them, since iid is officially opaque. Cut this FIXME. > > I think there is, otherwise (e.g.) store_id and contact_id could be > "file://foo:bar:baz" and "file://bish:bosh:bash" resulting in an ambiguous > string being constructed. > > It's not likely that the IDs could be in such pathogenic formats, but iirc > they're both URIs, so could contain colons. Raul, is this correct? Right - there will be colons. But as Travis mentioned, do we really care this stays parse-able? I.e.: I don't know of any need to de-construct the iid to recover the store_id and contact_id. The only real need is that it has to be unique (and that is still accomplished even with the extra colons).
(In reply to comment #95) > (In reply to comment #94) > > (In reply to comment #93) > > > /* FIXME: should we escape/replace colons in store_id and > > > * contact_id? We still have copies of both these > > > * parameters saved so I am not sure if its worth it. */ > > > return "%s:%s".printf (store_id, contact_id); > > > > > > No need to escape them, since iid is officially opaque. Cut this FIXME. > > > > I think there is, otherwise (e.g.) store_id and contact_id could be > > "file://foo:bar:baz" and "file://bish:bosh:bash" resulting in an ambiguous > > string being constructed. > > > > It's not likely that the IDs could be in such pathogenic formats, but iirc > > they're both URIs, so could contain colons. Raul, is this correct? > > Right - there will be colons. But as Travis mentioned, do we really care this > stays parse-able? I.e.: I don't know of any need to de-construct the iid to > recover the store_id and contact_id. The only real need is that it has to be > unique (and that is still accomplished even with the extra colons). Well, by opaque, I mean, externally-opaque. We may want to keep it internally-parseable (like we do for parsing the Persona.uid).
This also needs to depend upon Vala >= 0.13.0 (the first release to fulfill its requirements).
Now that bug #652434 has been fixed in master, please update all the eds test Makefile.am file(s) accordingly.
One major issue that is still left for gnome-contacts is that the type field for postal addresses is still not using the vcard style TYPE field. This means we don't sanely display postal address types in contacts, nor can you edit it.
(In reply to comment #99) > One major issue that is still left for gnome-contacts is that the type field > for postal addresses is still not using the vcard style TYPE field. This means > we don't sanely display postal address types in contacts, nor can you edit it. I'll push an update for that today.
(In reply to comment #99) > One major issue that is still left for gnome-contacts is that the type field > for postal addresses is still not using the vcard style TYPE field. This means > we don't sanely display postal address types in contacts, nor can you edit it. Note that it's stored slightly differently for PostalAddress fields. Instead of being wrapped by a FieldDetails object (which provides the type), PostalAddress contains the member Set<string> types. The reason we did it this way is that FieldDetails is based upon simple string values. If we changed it to be completely generic, it would have been a pain to use from C.
Travis: That is unfortunate, because that means its impossible to do custom labels or other vcard extension stuff for postal addresses. Is it to late to at least make it a full MultiSet member?
(In reply to comment #102) > Travis: That is unfortunate, because that means its impossible to do custom > labels or other vcard extension stuff for postal addresses. Is it to late to at > least make it a full MultiSet member? I've filed the following to deal with the remaining details which can't currently support arbitrary parameters and parameter values: * bug #653679 * bug #653680 * bug #653682 (these are all blocking our API-breaking meta bug #653684). I want to make Folks 0.5.4 our last API-breaking release for the indefinite future, so we might as well get these in in our final push.
(In reply to comment #103) > (In reply to comment #102) > > Travis: That is unfortunate, because that means its impossible to do custom > > labels or other vcard extension stuff for postal addresses. Is it to late to at > > least make it a full MultiSet member? > > I've filed the following to deal with the remaining details which can't > currently support arbitrary parameters and parameter values: > > * bug #653679 > * bug #653680 > * bug #653682 > > (these are all blocking our API-breaking meta bug #653684). I want to make > Folks 0.5.4 our last API-breaking release for the indefinite future, so we > might as well get these in in our final push. By the way, I intentionally skipped over supporting arbitrary parameters for Role (ie, ORG in vCards), since they don't seem particularly useful. If anyone can think of good use cases, I'll reconsider.
Hmm, i just found a weird bug. If i have an eds contact with two phone numbers, say one WORK and one OTHER, but both happent to have the same value (i.e. the phone number). Now folks only shows this as one item, even though the fielddetails parameters should be different...
Just a couple trivial comments on the latest version in your eds-0.5 branch. Please do the following: * fix these trivial points * ensure NEWS is updated * ensure it builds with and without --enable-docs * ensure it builds with and without --enable-eds-backend * squash commits as appropriate * merge to master Thanks for all the work on this! Now things get a bit more interesting :) @@ -233,6 +234,12 @@ public class Edsf.PersonaStore : Folks.PersonaStore yield this._set_contact_web_service_addresses (contact, web_service_addresses); } + else if (k == Folks.PersonaStore.detail_key (PersonaDetail.NOTES)) + { + var notes = (Gee.HashSet<Note>) v.get_object (); + yield this._set_contact_notes (contact, notes); + } + } Excessive empty line at the end. + /* First lets remove everything */ lets -> let's
commit 31f40df6da4766401b576635a8a632db84ee0a01 Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk> Date: Tue Jul 12 19:00:00 2011 +0100 Add an EDS backend Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=638281