GNOME Bugzilla – Bug 657602
Telepathy backend fails to set Personas' phone numbers from ContactInfo
Last modified: 2011-10-11 16:59:53 UTC
This user has a phone number in his XMPP vCard but is not exposed in his Individual. That's too bad because Empathy can't call his phone number then. :( RequestContactInfo(): [(u'tel', [u'type=home'], [u'+321111111111']), (u'bday', [], [u'ven. 29 d\xe9c. 2006']), (u'url', [], [u'test']), (u'nickname', [], [u'dev11']), (u'email', [u'type=work'], [u'badger']), (u'fn', [], [u'Test'])] Individual 'fe766b3f70792cd1ad83c86963a3ce5b4aedbef0' with 1 personas: alias dev11 avatar (null) birthday calendar-event-id (null) email-addresses { } full-name (null) gender FOLKS_GENDER_UNSPECIFIED groups { } im-addresses { 'jabber' : { 'cassidy-test1@jabber.belnet.be' } } is-favourite FALSE local-ids { } nickname notes { } phone-numbers { } postal-addresses { } presence-message presence-status available presence-type FOLKS_PRESENCE_TYPE_AVAILABLE roles { } structured-name (null) urls { } web-service-addresses { } trust-level FOLKS_TRUST_LEVEL_PERSONAS is-user FALSE id fe766b3f70792cd1ad83c86963a3ce5b4aedbef0 personas List of 1 personas
(Punting bugs to 0.6.4)
I'd really like to have this fixed for 3.2 as it makes the new "call contact" feature much more useful.
(In reply to comment #2) > I'd really like to have this fixed for 3.2 as it makes the new "call contact" > feature much more useful. We'll see what we can do. Note that we're really time-crunched now and that the 0.6.3 release (the one before this is scheduled) is tentatively planned for this Sunday, so it's possible Folks 0.6.4 will be before Gnome 3.2.0 anyhow.
Created attachment 196976 [details] [review] patch Implements PhoneDetails for Tpf.Persona; I had to move the helper code from Edsf.Util into folks/util so it can be reused here.
Review of attachment 196976 [details] [review]: I haven't tested this, but it looks fine to me.
Review of attachment 196976 [details] [review]: I haven't tested it either, but the utils.vala stuff looks icky to me. :-( ::: backends/telepathy/lib/tpf-persona.vala @@ +438,3 @@ + foreach (var info in contact_info) + { + if (info.field_name != "tel") Is there any possibility of this comparison needing to be case-insensitive? ::: folks/utils.vala @@ +168,3 @@ } + +public class Folks.SetComparator<G> : GLib.Object Can we avoid making this public? Internal utility API like this really doesn't need to be public. It's just another thing to maintain and another API to avoid breaking. Also, why is this an object? Vala supports generic static functions without problems.
(In reply to comment #6) > Review of attachment 196976 [details] [review]: > > I haven't tested it either, but the utils.vala stuff looks icky to me. :-( > > ::: backends/telepathy/lib/tpf-persona.vala > @@ +438,3 @@ > + foreach (var info in contact_info) > + { > + if (info.field_name != "tel") > > Is there any possibility of this comparison needing to be case-insensitive? Don't know, but I guess we better just make case-insensitive (though I'll check with the TP crowd). > > ::: folks/utils.vala > @@ +168,3 @@ > } > + > +public class Folks.SetComparator<G> : GLib.Object > > Can we avoid making this public? Internal utility API like this really doesn't > need to be public. It's just another thing to maintain and another API to avoid > breaking. This is meant to be used by PersonaStores which are compiled on a separate valac invocation, so marking them internal won't work (as far as I understand Vala's access control). I guess we could make this (privately) part of Folks.PersonaStore? > > Also, why is this an object? Vala supports generic static functions without > problems. Ah great!
Created attachment 197051 [details] [review] patch Here is a new take; I am still not sure how to mark code as internal that is suppose to be use by multiple PersonaStores.
Review of attachment 197051 [details] [review]: ::: backends/telepathy/lib/tpf-persona.vala @@ +300,3 @@ + { + get { return this._phone_numbers_ro; } + set { this.change_phone_numbers.begin (value); } This should have a comment after it saying it's not writeable. @@ +438,3 @@ + foreach (var info in contact_info) + { + if (info.field_name != "tel") According to the API documentation, Tp guarantees it to be lower-case if the comparison should be case-insensitive — so this is fine. @@ +443,3 @@ + foreach (var phone_num in info.field_value) + { + var phone_fd = new PhoneFieldDetails (phone_num); It would be nice if we handled TYPE parameters here and copied them across to the PhoneFieldDetails. @@ +448,3 @@ + } + + if (!Folks.PersonaStore.equal_sets<PhoneFieldDetails> (new_phone_numbers, Double space. ::: folks/persona-store.vala @@ +656,3 @@ public bool is_user_set_default { get; internal set; default = false; } + + public static bool equal_sets<G> (Set<G> a, Set<G> b) I think the only way to do this properly is to create a new noinst utility library, perhaps called libfolks-util.so, which gets statically linked into libfolks.so and all the backends which use it. It's a bit more work, but if we're going to end up with a number of utility functions which shouldn't be public, it's probably the only way to keep them private and allow them to be used in backends. Unfortunately, it will mean that they can't be used in out-of-tree backends, but I think that's acceptable (you may disagree though).
I tried this patch and it didn't work: Persona 'telepathy:/org/freedesktop/Telepathy/Account/gabble/jabber/cassidy_40jabber_2ebelnet_2ebe0:cassidy-test1@jabber.belnet.be': alias dev11 avatar (null) groups { } im-addresses { 'jabber' : { 'cassidy-test1@jabber.belnet.be' } } is-favourite FALSE phone-numbers { } presence-message presence-status available presence-type FOLKS_PRESENCE_TYPE_AVAILABLE iid jabber:cassidy-test1@jabber.belnet.be uid telepathy:/org/freedesktop/Telepathy/Account/gabble/jabber/cassidy_40jabber_2ebelnet_2ebe0:cassidy-test1@jabber.belnet.be display-id cassidy-test1@jabber.belnet.be is-user FALSE store 0xdb0e40: telepathy, /org/freedesktop/Telepathy/Account/gabble/jabber/cassidy_40jabber_2ebelnet_2ebe0 (cassidy@jabber.belnet.be) individual 0x18ce2a0 linkable-properties { 'im-addresses' } writeable-properties { 'alias', 'is-favourite', 'groups', 'is-favourite' } is-in-contact-list TRUE contact 0x1209c30 Furthermore, I got this crash when starting empathy: Program received signal SIGSEGV, Segmentation fault. 0x00007ffff3d675a6 in gee_iterable_iterator (self=0x0) at iterable.c:78 78 return GEE_ITERABLE_GET_INTERFACE (self)->iterator (self); (gdb) bt
+ Trace 228539
Created attachment 198190 [details] [review] Expose ContactInfo phone number fields; add ContactInfo support to our tests Patches from: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo657602-tp-phone-numbers ==== This doesn't address all of Philip's concerns (yet), but it adds a few things already: 1. it makes the patch work (see the line about preparing the CONTACT_INFO_FEATURE) 2. it adds ContactInfo support (both read and write) to our Telepathy test library 3. it adds some test code for the functionality in 2 So, this still needs Philip's remaining fixes and to support fields beyond just TEL, but the remaining changes should be straightforward (and this is ready for an initial review).
Review of attachment 198190 [details] [review]: Some notes from a (very) brief review. It's looking good to me apart from these. ::: backends/telepathy/lib/tpf-persona-store.vala @@ +2158,3 @@ + + var field = new ContactInfoField ("tel", parameters, values); + info_list.append (field); Prepend + reverse would be faster. @@ +2184,3 @@ + { + throw new PersonaStoreError.STORE_OFFLINE ( + _("Extended information cannot be written because its store is disconnected.")); s/its/the/, perhaps? ::: backends/telepathy/lib/tpf-persona.vala @@ +37,3 @@ PresenceDetails { private HashSet<string> _groups; Shouldn't “phone-numbers” be added to writeable-properties when appropriate? @@ +328,3 @@ + { + throw new PropertyError.UNKNOWN_ERROR (e3.message); + } Isn't it possible to consolidate the second two catch blocks into a single “catch (GLib.Error e2)” block? @@ +475,3 @@ + { + var phone_fd = new PhoneFieldDetails (phone_num); + new_phone_numbers.add (phone_fd); Handle parameters? ::: tests/lib/telepathy/contactlist/contact-list-manager.c @@ +1133,3 @@ g_signal_emit (self, signals[ALIAS_UPDATED], 0, handle); g_signal_emit (self, signals[PRESENCE_UPDATED], 0, handle); + g_signal_emit (self, signals[CONTACT_INFO_UPDATED], 0, handle); Indentation.
Created attachment 198502 [details] [review] Expose ContactInfo phone number fields; add ContactInfo support to our tests (try 2) Patches from branch: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo657602-tp-phone-numbers ======== This should resolve all the remaining issues except for splitting the utils functions in their own internal library (which I'll do next, then do a merge). I'd also like to support the remaining fields that show up in the Empathy Personal Information dialog (EMAIL, BDAY, URL) before closing this bug. Those should be a lot faster than the first two. @@ +328,3 @@ + { + throw new PropertyError.UNKNOWN_ERROR (e3.message); + } Isn't it possible to consolidate the second two catch blocks into a single “catch (GLib.Error e2)” block? The reason I kept them separate is that they ultimately have different causes (they just happen to resolve to the same error). If you'd still like me to simplify it, I'll do so (I'm not too attached to my logic).
Review of attachment 198502 [details] [review]: ::: backends/telepathy/lib/tpf-persona-store.vala @@ +603,3 @@ + /* Ensure the connection is prepared as necessary. */ + yield this.account.connection.prepare_async ( + this._connection_features); If the condition just below this is still legitimate, this.account.connection could be null at this point. @@ +961,3 @@ + if ((ci_flags & ContactInfoFlags.CAN_SET) != 0) + { + this._supported_fields.clear (); Would it make more sense for this._supported_fields to be cleared regardless of whether (ci_flags & ContactInfoFlags.CAN_SET) != 0? @@ +2220,3 @@ + } + + Double blank line. @@ +2230,3 @@ + { + throw new PersonaStoreError.INVALID_ARGUMENT ( + _("Extended information may only be set on the user's Telepathy contact.")); Might be worthwhile to add a PersonaStoreError.UNSUPPORTED_ON_NON_USER error code and use that. @@ +2242,3 @@ + success = + yield this.account.connection.set_contact_info_async ( + info_list); I haven't read the tp-glib code, but the API documentation seems to imply that info_list will be used to overwrite all existing contact information for the connection. In that case, setting the FN will delete any existing phone numbers, and vice-versa. Is this the case? ::: backends/telepathy/lib/tpf-persona.vala @@ +140,3 @@ + { + throw new PropertyError.UNKNOWN_ERROR (e3.message); + } I see your point. Leave the catch blocks separate then. :-)
Created attachment 198732 [details] [review] Expose ContactInfo phone number fields; add ContactInfo support to our tests (try 3) Patches from branch: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo657602-tp-phone-numbers > @@ +2242,3 @@ > + success = > + yield this.account.connection.set_contact_info_async ( > + info_list); > > I haven't read the tp-glib code, but the API documentation seems to imply that > info_list will be used to overwrite all existing contact information for the > connection. In that case, setting the FN will delete any existing phone > numbers, and vice-versa. Is this the case? I've asked in #telepathy but a bit too late in the day, so I may have to follow-up on this. However, Gabble has elaborate machinery to treat each entry in this info_list as an "edit" (rather than simply wiping it all and using the new content). (Our test library CM follows this behavior, which is why I didn't run into this as a bug in our tests; otherwise, they'd fail). So, I think we're OK here. Even if the Specification turns out to be right (and Gabble has to change its behavior, which seems unlikely), we could just bundle up the existing values and send them along (to make our transmissions absolute).
Review of attachment 198732 [details] [review]: A few minuscule comments and then I have no qualms with this being committed. Nice work! :-D > So, I think we're OK here. Even if the Specification turns out to be right (and > Gabble has to change its behavior, which seems unlikely), we could just bundle > up the existing values and send them along (to make our transmissions > absolute). That makes sense. ::: backends/telepathy/lib/tpf-persona.vala @@ +167,3 @@ + * ContactInfo has no equivalent field, so this is unsupported. + * + * @since 0.6.2 s/0.6.2/UNRELEASED/? @@ +180,3 @@ + * {@inheritDoc} + * + * @since 0.6.2 And here. @@ +192,3 @@ + * {@inheritDoc} + * + * @since 0.6.2 And here. ::: folks/internal.vala @@ +1,1 @@ +using Gee; File needs a licence header. ::: folks/persona-store.vala @@ +668,3 @@ public bool is_user_set_default { get; internal set; default = false; } + + public static bool equal_sets<G> (Set<G> a, Set<G> b) Shouldn't this be removed now in favour of the version in internal.vala?
commit 0436f69e0cf70e23ee447f3a30df256cddb1cf96 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Mon Oct 10 10:54:58 2011 -0700 Don't prepare the Telepathy Connection until it's guaranteed to exist. backends/telepathy/lib/tpf-persona-store.vala | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-) commit a91dd88b08997bef53593fed7c40441c05580478 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Mon Oct 10 11:07:01 2011 -0700 Distinguish Telepathy errors with new UNSUPPORTED_ON_NON_USER NEWS | 1 + backends/telepathy/lib/tpf-persona-store.vala | 2 +- folks/persona-store.vala | 8 ++++++++ 3 files changed, 10 insertions(+), 1 deletions(-) commit 4e44a1b104e6f7b929d16808d18d9667fc4c5368 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Sun Oct 9 19:11:07 2011 -0700 Support Birthdays for the Telepathy backend. NEWS | 1 + backends/telepathy/lib/tpf-persona-store.vala | 20 ++++ backends/telepathy/lib/tpf-persona.vala | 97 ++++++++++++++++++++ tests/lib/telepathy/contactlist/conn.c | 7 ++ .../telepathy/contactlist/contact-list-manager.c | 5 + tests/telepathy/individual-properties.vala | 33 +++++++ 6 files changed, 163 insertions(+), 0 deletions(-) commit 3a3ddc16698519ee396eaf9b51c319f820e11009 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Sun Oct 9 16:39:52 2011 -0700 Support UrlDetails for the Telepathy backend. NEWS | 1 + backends/telepathy/lib/tpf-persona.vala | 54 +++++++++++++++++++- tests/lib/telepathy/contactlist/conn.c | 7 +++ .../telepathy/contactlist/contact-list-manager.c | 5 ++ tests/telepathy/individual-properties.vala | 37 +++++++++++++ 5 files changed, 103 insertions(+), 1 deletions(-) commit c3909745b8e3308cabe44bc0a57749ca30e0ccb8 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Fri Oct 7 13:39:03 2011 -0700 Support EmailDetails for the Telepathy backend. NEWS | 1 + backends/telepathy/lib/tpf-persona.vala | 54 ++++++++++++++++++++++++++++ tests/lib/telepathy/contactlist/conn.c | 7 ++++ tests/telepathy/individual-properties.vala | 41 +++++++++++++++++++++ 4 files changed, 103 insertions(+), 0 deletions(-) commit c8f9b3ed9efb2256c5d9e87a27c807528e29139f Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Fri Oct 7 11:26:30 2011 -0700 Split utility functions into an internal library. This lets us avoid stuffing them into unrelated public libraries. backends/eds/lib/Makefile.am | 2 + backends/eds/lib/edsf-persona-store.vala | 2 +- backends/eds/lib/edsf-persona.vala | 12 ++++---- backends/telepathy/lib/Makefile.am | 2 + backends/telepathy/lib/tpf-persona.vala | 5 +-- folks/Makefile.am | 41 +++++++++++++++++++++++++++++- folks/internal.vala | 38 +++++++++++++++++++++++++++ folks/persona-store.vala | 14 ---------- 8 files changed, 91 insertions(+), 25 deletions(-) commit 9ca70640c72073c82223b1f55486c0b7c1f0c92c Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Oct 6 15:46:22 2011 -0700 Memoize the ContactInfo SupportedFields values. backends/telepathy/lib/tpf-persona-store.vala | 42 ++++++++++++++++++ backends/telepathy/lib/tpf-persona.vala | 57 ++++++++++--------------- 2 files changed, 65 insertions(+), 34 deletions(-) commit ec64e78109a90f32703401aa14388a58b25620ca Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Oct 6 11:37:59 2011 -0700 Factor in server-side writeability of ContactInfo fields. backends/telepathy/lib/tpf-persona.vala | 44 ++++++++++++++++++++++++++- tests/telepathy/individual-properties.vala | 40 ++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 3 deletions(-) commit 978c4d71d2136ff08e0aac4aae422e0c85a023db Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Oct 6 15:00:46 2011 -0700 Include required Telepathy Connection ContactInfo features. These are required to look up the connection's ContactInfoFlags.Can_Set and SupportedFields. We need both to determine which ContactInfo fields are writeable (for Tpf.Persona.writeable_properties). backends/telepathy/lib/tpf-persona-store.vala | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-) commit 6576e1df6211d7d04dc8a93f5539cb725d79ba6a Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Oct 6 09:57:57 2011 -0700 Reorganize tp/individual-properties test. tests/telepathy/individual-properties.vala | 73 ++++++++++++++-------------- 1 files changed, 36 insertions(+), 37 deletions(-) commit 8520eba7a8a7f95fba72f097988ced0b6ab1a0c9 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Oct 6 09:54:54 2011 -0700 Add assertions about Telepathy writeable properties to our tests. tests/telepathy/individual-properties.vala | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-) commit 1d5106a563efafb751a44ba2727195033edd05b5 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Wed Oct 5 15:08:15 2011 -0700 Ensure that our test self contact has an identifier. .../telepathy/contactlist/contact-list-manager.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) commit 3bc990722e040bd0c0ed24ae7d2f986e1c4791ae Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Wed Oct 5 14:40:58 2011 -0700 Print contact info for Telepathy contacts in tests. .../telepathy/contactlist/contact-list-manager.c | 40 ++++++++++++++++++++ 1 files changed, 40 insertions(+), 0 deletions(-) commit 0d22764d076f2eb0fd14629cf0191ed8d62c1b2f Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Fri Sep 30 10:54:15 2011 -0700 Support writing extended info for Telepathy user contacts Helps: bgo#657602 - Telepathy backend fails to set Personas' phone numbers from ContactInfo backends/telepathy/lib/tpf-persona-store.vala | 104 +++++++++++++ backends/telepathy/lib/tpf-persona.vala | 158 ++++++++++++++++++-- tests/lib/telepathy/contactlist/conn.c | 30 ++++ .../telepathy/contactlist/contact-list-manager.c | 50 ++++++ .../telepathy/contactlist/contact-list-manager.h | 2 + tests/telepathy/individual-properties.vala | 138 +++++++++++++++++ 6 files changed, 472 insertions(+), 10 deletions(-) commit 9000d07cfcc00dbbbc6e118fca34a19675756877 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Sep 29 14:44:35 2011 -0700 Implement ContactInfo in the Telepathy test backend. Helps: bgo#657602 - Telepathy backend fails to set Personas' phone numbers from ContactInfo tests/lib/telepathy/contactlist/conn.c | 234 +++++++++++++++++++- tests/lib/telepathy/contactlist/conn.h | 1 + .../telepathy/contactlist/contact-list-manager.c | 80 +++++++ .../telepathy/contactlist/contact-list-manager.h | 2 + tests/telepathy/individual-properties.vala | 4 + 5 files changed, 318 insertions(+), 3 deletions(-) commit 87e04d0b796c48fe3fdd2c0e46704ca2f02a97fb Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Mon Sep 19 18:24:34 2011 +0100 Telepathy: implement PhoneDetails for Tpf.Persona Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=657602 NEWS | 6 ++ backends/telepathy/lib/tpf-persona-store.vala | 3 +- backends/telepathy/lib/tpf-persona.vala | 77 +++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletions(-) commit 47568e3443cfbe5bdf14cfdb24ac7c4d009c0bae Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Mon Sep 19 18:11:56 2011 +0100 Move SetComparator into folks/utils so it can be used by other backends Helps: https://bugzilla.gnome.org/show_bug.cgi?id=657602 backends/eds/lib/Makefile.am | 1 - backends/eds/lib/edsf-persona-store.vala | 3 +- backends/eds/lib/edsf-persona.vala | 22 ++++++++--------- backends/eds/lib/edsf-util.vala | 38 ------------------------------ folks/persona-store.vala | 14 +++++++++++ 5 files changed, 25 insertions(+), 53 deletions(-)
It works now; thanks guys!
(In reply to comment #18) > It works now; thanks guys! By the way, it's not just { read } X { phone } (this bug) -- it's { read, write } X { phone, email, url, birthday, full name }, so you should be able to implement the Personal Information dialogs in Empathy just using the native Folks PhoneDetails, EmailDetails, etc. (that's why my additions weren't just the original 1-line fix :)