GNOME Bugzilla – Bug 629078
Folks needs a full API review to take advantage of our compatibility break in 0.2.x
Last modified: 2011-02-14 21:55:14 UTC
We're going to break API and ABI in the 0.2.x branch to clean it up as much as possible, so we should do a thorough review of our API. This means: * public C API review of libfolks * public VAPI review of libfolks * public API review of libfolks-telepathy * public VAPI review of libfolks-telepathy
Created attachment 169953 [details] [review] Squashed diff of the 629078-properties branch http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629078-properties Here's a branch with the results of a review of the properties in folks.vapi. There's still an open question about PersonaStore.is_writeable and PersonaStore.trust_level, since I'm not sure whether they should actually be set by the PersonaStore, or set by the IndividualAggregator. They can be discussed more next week.
(Mass changing milestones; please search for this phrase to delete the bug spam.)
Created attachment 170473 [details] [review] Remove the exception from Tpf.Persona's constructor http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629078-tpf-persona-new This is a parallel branch which removes an unnecessary exception from Tpf.Persona's constructor.
From bug #629642: Ultimately, we probably want to reconsider the interplay between the IndividualAggregator.individuals_changed and Individual.removed signals, taking into account the possibility of needing to know a replacement Individual when listening to IndividualAggregator.individuals_changed, the requirement that the API still be usable without IndividualAggregator, and the need to not have two signals to listen to in order to get the same information both times.
Folks.Groupable.ChangeReason shouldn't be used everywhere (e.g. in IndividualAggregator.individuals_changed or PersonaStore.personas_changed) as the reason for changes to personas. Either a new enum should be used for such things, or ChangeReason should be moved out of Folks.Groupable.
As pointed out in bug #625969 comment #5, Empathy depends on accessing the local-pending and remote-pending lists. Most clients don't care about these, but since we're exposing the PersonaStores more publicly in 0.3.x, we might be able to conveniently expose them in Tpf.PersonaStore, without adding cruft to the general API.
At the moment you can only unlink all the personas using IndividualsAggregator.unlink_individuals(). I think this API could be replaced or improved by allowing the removal of a single persona.
We should add and clarify the policy that a Persona belongs to exactly 1 Individual. This way we can add a Persona.individual member, to make it convenient to access a given Persona's parent.
(In reply to comment #8) > This way we can add a Persona.individual member, to make it convenient to > access a given Persona's parent. We'd have to be very careful not to introduce reference cycles with that. It would definitely have to be a weak reference.
(In reply to comment #8) > We should add and clarify the policy that a Persona belongs to exactly 1 > Individual. Note that this is not actually true. During the addition/removal of Individuals when you prepare your aggregator the Persona can be attached to multiple Individuals at the same time.
BackendStore.add_backend() should probably add validation (and throw an error).
Given a Tpf.Persona, it should be easy to get both its TpContact and TpAccount, in order to start a chat, call, etc. The cleanest route is to just wait for https://bugs.freedesktop.org/show_bug.cgi?id=29417 to be resolved. Then fetching the TpAccount will just be a matter of getting it from the TpContact. This, coincidentally, would require no changes on our part. Or we could just track the TpAccount ourself. This would be fairly easy, but it does add a little unnecessary API.
The way PersonaStore.is_writeable is meant to be set by IndividualAggregator (and especially that it's a fully-public setter), not the PersonaStores themselves, is awkward.
Each of these comments needs to be turned into a bug that blocks this one.
Marco's working on a vCard-like interface for extended attributes. We should integrate this well into the API (including duplicating IM addresses into it and possibly making this the recommended/only way to access them publicly).
Simon pointed out that there's an important distinction to make for pieces of data, whether they're: * true by definition * eg, this message was received from foo@example.org, so it's part of our contact history with foo@example.org * true because we (the local user) say so * eg, local overrides for aliases (nicknames) or avatars * possibly true, as suggested by a remote data source * eg, an IM contact with extended attributes that list email addresses. These can trivially be falsified and should be treated accordingly. As we stand, we only trust (or even deal with) the first two cases. And for the second case, it's only nicknames. To properly handle the third case, we'll probably need to make trust levels more fine-grained (possibly to the point of adding a trust level per data field instance).
(In reply to comment #16) > * true because we (the local user) say so > * eg, local overrides for aliases (nicknames) or avatars When Telepathy gets a distinction between local-user-set aliases (e.g. the XMPP roster) and remote-user-set nicknames (e.g. the XMPP vCard or PEP nickname), the alias that the user has stored on the server should probably go in this category too. See https://bugs.freedesktop.org/show_bug.cgi?id=14540 for discussion of this.
Committed 629078-properties to master: commit 62d321e39dac00e13184200fada89ca982bbc065 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Fri Sep 10 15:45:30 2010 +0100 Hide setter for Persona.linkable_properties NEWS | 4 ++++ backends/key-file/kf-persona.vala | 11 +++++++++-- backends/telepathy/lib/tpf-persona.vala | 14 +++++++++++--- folks/persona.vala | 2 +- 4 files changed, 25 insertions(+), 6 deletions(-) commit def47dffa31a95ec6bd333f43c69822416098283 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Fri Sep 10 15:25:31 2010 +0100 Remove casting convenience functions in Individual They're not needed for Vala, and weren't particularly well thought out (or complete) in C. NEWS | 1 + folks/individual.vala | 79 ------------------------------------------------- 2 files changed, 1 insertions(+), 79 deletions(-) commit b7cb90a159008fd2cfa8dfcec7cf993c86a0816c Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Fri Sep 10 15:20:51 2010 +0100 Hide setter for Backend.persona_stores backends/key-file/kf-backend.vala | 23 ++++++++++++++++++----- backends/telepathy/tp-backend.vala | 25 +++++++++++++++++++------ folks/backend.vala | 11 +---------- 3 files changed, 38 insertions(+), 21 deletions(-) commit 24234a3c9797f066ea01005f00f67faf73359330 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Fri Sep 10 15:13:29 2010 +0100 Hide setter for Backend.name backends/key-file/kf-backend.vala | 10 +--------- backends/telepathy/tp-backend.vala | 10 +--------- folks/backend.vala | 2 +- 3 files changed, 3 insertions(+), 19 deletions(-) commit 7b3ce82706d4263ee225038e0c914a1c4e94631e Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Fri Sep 10 14:57:01 2010 +0100 Hide setters for PersonaStore.type_id, .display_name and .id backends/key-file/kf-persona-store.vala | 20 +++++------------- backends/telepathy/lib/tpf-persona-store.vala | 26 +++--------------------- folks/persona-store.vala | 6 ++-- 3 files changed, 13 insertions(+), 39 deletions(-)
Committed 629078-tpf-persona-new to master: commit 9a47fc5816ecf71d7d3ac45030a7e776384e2caf Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Fri Sep 17 12:00:19 2010 +0100 Remove the exception from Tpf.Persona's constructor It was not possible for it to ever be thrown (since TpContact.get_identifier() is guaranteed to always return a valid ID, and having *_new() methods throw exceptions is never a good idea. (GInitable should be used instead.) This breaks both the Vala and C APIs. Helps: bgo#629078 NEWS | 2 + backends/telepathy/lib/tpf-persona-store.vala | 59 +++++-------------------- backends/telepathy/lib/tpf-persona.vala | 16 +------ 3 files changed, 14 insertions(+), 63 deletions(-)
(In reply to comment #18) > commit def47dffa31a95ec6bd333f43c69822416098283 > Author: Philip Withnall <philip.withnall@collabora.co.uk> > Date: Fri Sep 10 15:25:31 2010 +0100 > > Remove casting convenience functions in Individual > > They're not needed for Vala, and weren't particularly well thought > out (or complete) in C. > > NEWS | 1 + > folks/individual.vala | 79 Please run make check before committing changes, especially those with API breaks. This made a (trivial) break for one of the tests. Fixed by: commit 9c57682d32c318094b700c65ef708eb97b884945 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Sun Nov 14 13:21:56 2010 -0800 Cast Individuals to Presence as necessary for the tests. tests/telepathy/individual-properties.vala | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
I think bug #627483 is an important one to fix before we stabilise our API. A large proportion of our aggregator bugs have been caused by using lists instead of sets for storing things like Personas. If we could migrate as much of the IndividualAggregator API as possible to use proper set types instead of lists, everything would be a lot simpler. It might be worth using them in the external API too, to prevent having to marshal everything to and from lists.
(In reply to comment #21) > I think bug #627483 is an important one to fix before we stabilise our API. A > large proportion of our aggregator bugs have been caused by using lists instead > of sets for storing things like Personas. If we could migrate as much of the > IndividualAggregator API as possible to use proper set types instead of lists, > everything would be a lot simpler. It might be worth using them in the external > API too, to prevent having to marshal everything to and from lists. Sounds good. I've made an additional comment on that bug.
IndividualAggregator.link_personas(void *_personas) needs a proper argument type. It's implicitly GLib.List<Persona> right now. We can't use it normally since async functions always copy their arguments and GLib.List isn't properly reference-counted. So our options are: * add the 'owned' modifier and require callers transfer ownership/copy the list manually or * use a different type Neither option is great. I don't want to use a Gee type and make libgee a requirement. Perhaps we're best with a NULL-terminated Persona[].
(In reply to comment #23) > IndividualAggregator.link_personas(void *_personas) needs a proper argument > type. > > It's implicitly GLib.List<Persona> right now. We can't use it normally since > async functions always copy their arguments and GLib.List isn't properly > reference-counted. So our options are: > > * add the 'owned' modifier and require callers transfer ownership/copy the list > manually or > * use a different type > > Neither option is great. I don't want to use a Gee type and make libgee a > requirement. > > Perhaps we're best with a NULL-terminated Persona[]. Once my anti-linking work has been committed, we *should* be copying the list of personas inputted to IndividualAggregator.link_personas(), since link_personas() will yield to IndividualAggregator.save_anti_links() and then use the list of personas again afterwards. My vote would go with using a different type. A lot of bugs have been caused by our use of GLib lists (and the ensuing refcounting problems), and I think we should move away from them before we stabilise the API. I agree that exposing libgee as an API dependency wouldn't be brilliant, but I think it would be acceptable. An option would be to wrap the appropriate libgee data structures in some new class exposed by Folks (e.g. a Folks.OrderedSet). See bug #627483.
Bumping these to the next release.
The only thing that stands out to me now (beyond what's covered in bug #639933 and bug #640092) is this function: IndividualAggregator.add_persona_from_details (Folks.Individual? parent, string persona_store_type, string persona_store_id, GLib.HashTable<string,GLib.Value?> details); I think we should replace the persona_store_type and persona_store_id arguments with a PersonaStore argument. Clients can find the PersonaStore on their own via BackendStore.get_backend_by_name(); Backend.persona_stores; and have tighter control over the selection process (which is otherwise too opaque, I think).
(In reply to comment #26) > I think we should replace the persona_store_type and persona_store_id arguments > with a PersonaStore argument. Clients can find the PersonaStore on their own > via BackendStore.get_backend_by_name(); Backend.persona_stores; and have > tighter control over the selection process (which is otherwise too opaque, I > think). Agreed.
Created attachment 180227 [details] [review] Take a PersonaStore in IndividualAggregator.add_persona_from_details http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629078-add-persona-from-details
Created attachment 180345 [details] [review] Use “dup” instead of “get” in method names which return a referenced object Note that this depends on bug #641781 being fixed so that the getter can be renamed. http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629078-dup-instead-of-get
(In reply to comment #29) > Created an attachment (id=180345) [details] [review] > Use “dup” instead of “get” in method names which return a referenced object > > Note that this depends on bug #641781 being fixed so that the getter can be > renamed. > > http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629078-dup-instead-of-get Drat, forgot to add an entry to NEWS.
Review of attachment 180345 [details] [review]: Looks good
Review of attachment 180227 [details] [review]: Looks good
Review of attachment 180345 [details] [review]: (Sorry, commented on this patch by accident earlier) Please update the configure.ac Vala requirement and commit once the Vala bug is fixed and a new release is out.
Jürg expects to make a new Vala release at the weekend, so I'll commit both of these patches and make a libfolks release once the new Vala release is out, then commit the fix to bug #641662.
(In reply to comment #34) > Jürg expects to make a new Vala release at the weekend, so I'll commit both of > these patches and make a libfolks release once the new Vala release is out, > then commit the fix to bug #641662. Sounds great - thanks!
commit c489e94cbf7783946aca62611717aeeee98a535d Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Mon Feb 7 22:25:38 2011 +0000 Use “dup” instead of “get” in method names which return a referenced object This bumps our Vala dependency to 0.11.6, which includes a necessary fix for the CCode annotation we use. Helps: bgo#629078 NEWS | 4 ++++ configure.ac | 2 +- folks/backend-store.vala | 10 +++++++--- tools/import.vala | 2 +- tools/inspect/command-backends.vala | 2 +- 5 files changed, 14 insertions(+), 6 deletions(-) commit 271ee14ed7a1c6373a8f1f5b4d91dc2410c5909d Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Sun Feb 6 12:05:23 2011 +0000 Take a PersonaStore in IndividualAggregator.add_persona_from_details This gives the client more flexibility in choosing which persona store to add the new persona to. Closes: bgo#629078 NEWS | 4 +++ folks/individual-aggregator.vala | 44 ++++++++++++------------------------- 2 files changed, 18 insertions(+), 30 deletions(-)