GNOME Bugzilla – Bug 657783
Prefer data from primary store when picking Individual values
Last modified: 2011-09-16 16:51:27 UTC
When picking for instance full_name for an individual in Individual._update_full_name() we just iterate over all personas and pick the first one that has a non-empty full_name set. This order is completely random and is different for each run. Could we please have some level of stability here? For instance the primary store persona could always be checked first. That way we at least have a way to edit the display name (I mean, what if some persona had a non-editable full name and that was picked up instead of the primary persona). Ideally we should sort the personas before iterating them such that: * Primary persona is first * More trusted persona stores comes before less trusted * If the above does not distinguish order, pick some stable identifier to order by (like persona store id) so that we at least get stable Individual values over time.
This also means i can't e.g. override a IM avatar if the IM persona ends up ahead of the primary store persona.
The primary store was always meant to be the highest priority for all values, so this is just a flat-out implementation bug in Folks in this case.
Created attachment 195603 [details] [review] Prefer property values from personas in the primary store https://www.gitorious.org/folks/folks/commits/657783-individual-persona-ordering This should fix it for all the single-valued properties in Individual. There's one TODO comment which depends on the outcome of bug #657141 and the e-mail I've just sent out. However, the patch works fine and is ready to commit. (The code covered by the TODO is correct for folks in its current state: it only has to change once bug #657141 is fixed.) All tests pass for me.
Review of attachment 195603 [details] [review]: Looks good to me.
(Punting bugs to 0.6.4)
This patch should still be good to commit. However, _update_single_valued_property() should be modified to also prefer personas which have the property in Persona.writeable-properties over those which don't. From my e-mail of 2011-09-03: • Individual property updates should be handled by preferring values from personas from the primary store (i.e. stores with PersonaStore.is-primary-store == true); and then falling back to preferring personas with that property listed in Persona.writeable-properties.
Comment on attachment 195603 [details] [review] Prefer property values from personas in the primary store Committed. The issues in comment #6 still need dealing with. commit 00a31f63bcc266a53b2e5c6fa1df79cab4f8a664 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Sat Sep 3 18:36:24 2011 +0100 Bug 657783 — Prefer data from primary store when picking Individual values Use a consistent high-level order over personas in an individual to choose which one to use for the value of single-valued properties such as alias, avatar and gender. Closes: bgo#657783 NEWS | 1 + folks/individual.vala | 573 +++++++++++++++++++++++++++++-------------------- 2 files changed, 336 insertions(+), 238 deletions(-) commit 23027313a91d8ee86d43d445402628378a020739 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Sat Sep 3 18:35:24 2011 +0100 core: Clarify nullability of a few properties AliasDetails.alias, NameDetails.nickname and NameDetails.full_name must never be set to null. The empty string represents an unset value for these properties. backends/eds/lib/edsf-persona-store.vala | 19 +++++++++++++++++-- backends/eds/lib/edsf-persona.vala | 14 +++++++++++++- backends/key-file/kf-persona.vala | 14 +++++++++++++- backends/libsocialweb/lib/swf-persona.vala | 14 +++++++++++++- backends/telepathy/lib/tpf-persona-store.vala | 6 ++++++ backends/telepathy/lib/tpf-persona.vala | 11 ++++++++++- backends/tracker/lib/trf-persona.vala | 21 ++++++++++++++++++--- folks/alias-details.vala | 2 ++ folks/name-details.vala | 6 ++++++ 9 files changed, 98 insertions(+), 9 deletions(-)
Created attachment 196549 [details] [review] Prefer property values from personas with writeable properties https://www.gitorious.org/folks/folks/commits/657783-prefer-writeable-properties Here's a patch to address comment #6.
Comment on attachment 196549 [details] [review] Prefer property values from personas with writeable properties >+ else if (a_is_primary == false && b_is_primary == true) >+ { >+ return 1; >+ } >+ Isn't this the other way around? According to our own docs (and GComparableDataFunc's docs): * This function uses the given comparison function to order the personas in * this individual, with the highest-positioned persona finally being passed * to the setter function to use in updating the individual's value for the * given property. i.e. If `compare_func(a, b)` is called and returns > 0, * persona `a` will be passed to the setter. so when B is primary we would want to return -1.. >+ /* If both personas have the same is-primary value, prefer personas >+ * which have the given property as writeable over those which >+ * don't. */ >+ var a_is_writeable = (prop_name in a.writeable_properties); >+ var b_is_writeable = (prop_name in b.writeable_properties); >+ >+ if (a_is_writeable == true && b_is_writeable == false) > { > return -1; > } Same comment as above. >- else if (b_is_primary == true) >+ else if (a_is_writeable == false && b_is_writeable == true) > { > return 1; > } > Same comment as above.
Created attachment 196657 [details] [review] Prefer property values from personas with writeable properties (updated) Argh, good catches. I checked the ordering and thought I had it correct. Turns out I'm just stupid. Fixup patch: https://www.gitorious.org/folks/folks/commit/631e87ad549de91e786a1ebb97727b1b316fe0a7 This also expands the documentation a little.
Comment on attachment 196657 [details] [review] Prefer property values from personas with writeable properties (updated) Looks good.
Comment on attachment 196657 [details] [review] Prefer property values from personas with writeable properties (updated) Thanks for the quick review. commit d0ab83aaccf91b32016649e4c512f37cc5285a08 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Wed Sep 14 22:26:04 2011 +0100 individual: Prefer data from personas with writeable properties As an addition to the fix for bgo#657783, if two personas come from stores which have equal is-primary-store values, we now prefer the persona which lists the given property as writeable. If both personas list (or don't list) the property as writeable, we fall back to the given ordering function as before. Closes: bgo#657783 folks/individual.vala | 74 ++++++++++++++++++++++++++---------------------- 1 files changed, 40 insertions(+), 34 deletions(-)