GNOME Bugzilla – Bug 657282
Add an IndividualAggregator.individuals_changed_detailed signal
Last modified: 2011-09-08 10:02:04 UTC
Alex has pointed out that it's very hard to keep track of which Individuals are added and removed as a result of linking and unlinking, because one has to listen to both IndividualAggregator.individuals_changed and the Individual.removed signals for each Individual. We've got all the relevant information in the IndividualAggregator already, so I propose the following signal to supersede IndividualAggregator.individuals_changed: IndividualAggregator { public signal void individuals_changed_detailed (MultiMap<Individual?, Individual?> changes); } Added individuals are maps: null → individual Removed individuals are maps: individual → replacement_individual (or null) Unlinked individuals are maps: unlinked_individual → set of one or more replacement individuals Linked individuals are mapped as per removed individuals, all pointing to the shiny new linked individual.
Created attachment 194659 [details] [review] Add IndividualAggregator.individuals_changed_detailed https://www.gitorious.org/folks/folks/commits/657282-individuals-changed-detailed This adds the new signal, and deprecates (but doesn't break or remove) the original IndividualAggregator.individuals_changed in favour of it. The rationale for using a MultiMap over (for example) adding another parameter, Map<Individual, Individual> replaced_individuals, is that the client code to handle removed and replaced individuals is likely to be largely similar, so having to look in two places for the relevant individuals isn't good. Using a MultiMap also allows us to express unlinking in a nicer way, giving a mapping from the old individual to a set of one _or more_ replacement individuals, which wouldn't be possible if we just added a map of replacement_individuals. Note, however, that the IndividualAggregator code doesn't currently keep track of which new individuals arose out of an unlinking operation properly, so unlinking will just appear as a series of normal added and removed individuals at the moment. This patch doesn't fix that. If people like this approach and the patch is acked, we can go through and s/individuals_changed/individuals_changed_detailed/ in the tests, utilities and Empathy and GNOME Contacts.
I forgot to mention: I purposely haven't added the message, actor and reason parameters to the new signal, because I've never actually seen them being used. I guess I could be convinced that they need to stay, although if that's the case we need to ensure that they actually make sense (i.e. is it really the case that all the individual adds and removes being notified of by a single signal emission are the result of one actor, and are happening for a single reason?).
(In reply to comment #2) > I forgot to mention: I purposely haven't added the message, actor and reason > parameters to the new signal, because I've never actually seen them being used. > I guess I could be convinced that they need to stay, although if that's the > case we need to ensure that they actually make sense (i.e. is it really the > case that all the individual adds and removes being notified of by a single > signal emission are the result of one actor, and are happening for a single > reason?). As far as I can tell, these are only used in Empathy for reporting who kicked or banned someone else from a channel. I think, hypothetically, it could be used to report who invited someone, but that seems even less important. I'd be OK either with or without these parameters, which makes me lean toward "don't include them". If there ever is a good reason to include them, we could add a new signal individuals-changed-with-reason and deprecate this one.
Review of attachment 194659 [details] [review]: Looks good, but please update the existing tests to use this new signal and add a test (could be a copy of the existing one) to protect backwards compatibility. That test program will need to strip out --fatal-warnings from its VALAFLAGS so it will build from git.
Created attachment 195120 [details] [review] Add IndividualAggregator.individuals_changed_detailed (updated) https://www.gitorious.org/folks/folks/commits/657282-individuals-changed-detailed Done. I can't be bothered to port the Tracker tests. I've modified the core tests to check both signals, and ported the remaining tests to just use the new signal. I haven't done the --fatal-warnings change since Vala master no longer warns about use of deprecated symbols within a package (only for external packages).
Review of attachment 195120 [details] [review]: Looks good except for this part: aggregation.vala:148.7-148.36: warning: Folks.IndividualAggregator.individuals_changed has been deprecated since UNRELEASED. Use IndividualAggregator.individuals_changed_detailed ... (repeated many times) Note that this only shows up after a "make maintainer-clean", since it's forcing a recompile from Vala for that test (you may have missed that in your own testing). And this is using the latest Vala (which includes the change to only care about deprecated symbols in external packages): Vala 0.13.3.41-7d9f I think the problem is that the test is treating Folks as an external package - maybe there's some compiler flag or other setting you could add to work around this?
(In reply to comment #6) > Review of attachment 195120 [details] [review]: > > Looks good except for this part: > > aggregation.vala:148.7-148.36: warning: > Folks.IndividualAggregator.individuals_changed has been deprecated since > UNRELEASED. Use IndividualAggregator.individuals_changed_detailed > ... (repeated many times) Erk. That makes sense. Looks like I'll have to port the Tracker tests then. I'll get on that later tonight or tomorrow.
I've pushed two additional commits to https://www.gitorious.org/folks/folks/commits/657282-individuals-changed-detailed which: • port the Tracker tests; and • disable warnings from valac for the folks tests. The latter is the only solution I found to the problem of valac warning us about using deprecated API in the aggregation test. I don't think it's worth squashing another patch just for these changes, since they're fairly trivial.
Comment on attachment 195120 [details] [review] Add IndividualAggregator.individuals_changed_detailed (updated) Committed with the changes mentioned above. Tests pass for me apart from three eds test failures, but they seem unrelated (and can be fixed later; this branch is blocking a lot of stuff so needs to be pushed now). commit 8a4d6afc237e9ea1a3b845bd32ed41b48795c58b Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Aug 29 21:59:02 2011 +0100 tests: Port the remaining tests to use individuals_changed_detailed Helps: bgo#657282 tests/eds/add-contacts-stress-test.vala | 22 +++++++---- tests/eds/add-persona.vala | 20 ++++++---- tests/eds/avatar-details.vala | 21 ++++++---- tests/eds/email-details.vala | 22 +++++++---- tests/eds/im-details.vala | 22 +++++++---- tests/eds/individual-retrieval.vala | 22 +++++++---- tests/eds/link-personas.vala | 27 ++++++++---- tests/eds/name-details.vala | 22 +++++++---- tests/eds/phone-details.vala | 22 +++++++---- tests/eds/postal-address-details.vala | 22 +++++++---- tests/eds/remove-persona.vala | 23 +++++++--- tests/eds/removing-contacts.vala | 23 +++++++--- tests/eds/set-avatar.vala | 22 +++++++---- tests/eds/set-birthday.vala | 22 +++++++---- tests/eds/set-emails.vala | 22 +++++++---- tests/eds/set-gender.vala | 22 +++++++---- tests/eds/set-im-addresses.vala | 22 +++++++---- tests/eds/set-names.vala | 24 +++++++---- tests/eds/set-notes.vala | 22 +++++++---- tests/eds/set-phones.vala | 22 +++++++---- tests/eds/set-postal-addresses.vala | 22 +++++++---- tests/eds/set-properties-race.vala | 22 +++++++---- tests/eds/set-structured-name.vala | 22 +++++++---- tests/eds/set-urls.vala | 22 +++++++---- tests/eds/updating-contacts.vala | 22 +++++++---- tests/key-file/individual-retrieval.vala | 25 ++++++++++-- tests/libsocialweb/aggregation.vala | 33 +++++++++++++-- tests/libsocialweb/dummy-lsw.vala | 32 ++++++++++++--- tests/telepathy/individual-properties.vala | 42 +++++++++++++++++--- tests/telepathy/individual-retrieval.vala | 31 ++++++++++++-- tests/tracker/add-contact.vala | 22 +++++++---- tests/tracker/add-persona.vala | 22 +++++++---- tests/tracker/additional-names-updates.vala | 22 +++++++---- tests/tracker/avatar-details-interface.vala | 22 +++++++--- tests/tracker/avatar-updates.vala | 22 +++++++---- tests/tracker/birthday-details-interface.vala | 23 +++++++---- tests/tracker/birthday-updates.vala | 23 +++++++---- tests/tracker/default-contact.vala | 22 +++++++---- tests/tracker/duplicated-emails.vala | 22 +++++++---- tests/tracker/duplicated-phones.vala | 22 +++++++---- tests/tracker/email-details-interface.vala | 24 +++++++---- tests/tracker/emails-updates.vala | 22 +++++++---- tests/tracker/family-name-updates.vala | 22 +++++++---- tests/tracker/favourite-details-interface.vala | 24 +++++++---- tests/tracker/favourite-updates.vala | 22 +++++++---- tests/tracker/fullname-updates.vala | 22 +++++++---- tests/tracker/gender-details-interface.vala | 23 +++++++---- tests/tracker/given-name-updates.vala | 22 +++++++---- tests/tracker/im-details-interface.vala | 24 +++++++---- tests/tracker/imaddresses-updates.vala | 22 +++++++---- tests/tracker/individual-retrieval.vala | 22 +++++++---- tests/tracker/link-personas-via-local-ids.vala | 29 +++++++++---- tests/tracker/link-personas.vala | 29 +++++++++---- tests/tracker/match-all.vala | 22 +++++++---- tests/tracker/match-email-addresses.vala | 22 +++++++---- tests/tracker/match-im-addresses.vala | 22 +++++++---- tests/tracker/match-known-emails.vala | 22 +++++++---- tests/tracker/match-name.vala | 22 +++++++---- tests/tracker/match-phone-number.vala | 22 +++++++---- tests/tracker/name-details-interface.vala | 24 +++++++---- tests/tracker/nickname-updates.vala | 22 +++++++---- tests/tracker/note-details-interface.vala | 22 +++++++---- tests/tracker/phone-details-interface.vala | 24 +++++++---- tests/tracker/phones-updates.vala | 23 +++++++---- .../tracker/postal-address-details-interface.vala | 22 +++++++---- tests/tracker/prefix-name-updates.vala | 23 +++++++---- tests/tracker/remove-contact.vala | 25 ++++++++---- tests/tracker/remove-persona.vala | 23 +++++++--- tests/tracker/role-details-interface.vala | 22 +++++++---- tests/tracker/set-avatar.vala | 22 +++++++---- tests/tracker/set-birthday.vala | 22 +++++++---- tests/tracker/set-duplicate-email.vala | 23 +++++++---- tests/tracker/set-emails.vala | 22 +++++++---- tests/tracker/set-favourite.vala | 22 +++++++---- tests/tracker/set-full-name.vala | 22 +++++++---- tests/tracker/set-gender.vala | 22 +++++++---- tests/tracker/set-im-addresses.vala | 22 +++++++---- tests/tracker/set-nickname.vala | 22 +++++++---- tests/tracker/set-notes.vala | 22 +++++++---- tests/tracker/set-null-avatar.vala | 22 +++++++---- tests/tracker/set-phones.vala | 22 +++++++---- tests/tracker/set-postal-addresses.vala | 22 +++++++---- tests/tracker/set-roles.vala | 22 +++++++---- tests/tracker/set-structured-name.vala | 22 +++++++---- tests/tracker/set-urls.vala | 22 +++++++---- tests/tracker/suffix-name-updates.vala | 22 +++++++---- tests/tracker/url-details-interface.vala | 24 +++++++---- tests/tracker/website-updates.vala | 22 +++++++---- 88 files changed, 1336 insertions(+), 695 deletions(-) commit 87b3c40a28fe8312f2f34630ebbb5f3981bb3a34 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Aug 29 16:32:27 2011 +0100 tests: Expand the aggregation test to check both individuals_changed signals Helps: bgo#657282 tests/folks/Makefile.am | 3 + tests/folks/aggregation.vala | 543 ++++++++++++++++++++++++++++-------------- 2 files changed, 370 insertions(+), 176 deletions(-) commit 0dd7dc1aaa2e3b647f9e62d4a4960df60fd97220 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Wed Aug 24 22:38:16 2011 +0100 Bug 657282 — Add an IndividualAggregator.individuals_changed_detailed signal This supersedes IndividualAggregator.individuals_changed, providing more information about the relationships between the added and removed individuals. Closes: bgo#657282 NEWS | 3 + folks/individual-aggregator.vala | 222 ++++++++++++++++++++++++++------------ 2 files changed, 156 insertions(+), 69 deletions(-)
This doesn't quite seem to work. I've added support for this to gnome-contacts, see: http://git.gnome.org/browse/gnome-contacts/commit/?id=6f44ccd372a6d91c4d95e4689e510bab6b153763 However, sometimes I do get a signal where it claims an individual was replaced by another, and I had never seen the original individual. To see this happen, add some spew (or gdb break) in this branch: + } else { + // TODO: This should not really happen, but seems to do anyway + this.add (new Contact (this, replacement));
So, to debug this i added this to my individuals_changed_detailed signal: foreach (var old_individual in changes.get_keys ()) { foreach (var new_individual in changes.get (old_individual)) { print ("new_individual %p replaces old_individual %p\n", new_individual, old_individual); if (new_individual != null) { print ("New individual personas %p\n", new_individual); foreach (var p in new_individual.personas) print (" %s\n", p.uid); } if (old_individual != null) { print ("Old individual personas %p\n", old_individual); foreach (var p in old_individual.personas) print (" %s\n", p.uid); } And then later: var c = Contact.from_individual (old_individual); if (c == null) print ("WTF? unseen old_individual %p\n", old_individual); I'll attach the full output, but it hits the WTF only once, with this: new_individual 0x31983f0 replaces old_individual 0x316fe70 New individual personas 0x31983f0 eds:system:pas-id-4E5E0C4B00000009 eds:system:pas-id-4E5E0BF500000008 eds:system:pas-id-4E5E0C6E0000000A Old individual personas 0x316fe70 eds:system:pas-id-4E5E0C4B00000009 eds:system:pas-id-4E5E0BF500000008 WTF? unseen old_individual 0x316fe70 The old individual pointer has never been printed before in the log, and neither has the two persona uids in the new old_individual.
Created attachment 195762 [details] Full debug output
(In reply to comment #10) > This doesn't quite seem to work. > I've added support for this to gnome-contacts, see: > http://git.gnome.org/browse/gnome-contacts/commit/?id=6f44ccd372a6d91c4d95e4689e510bab6b153763 I managed to reproduce your problem, and it's fixed for me by attachment #195828 [details] in bug #656689; if that also fixes the problem for you then please close this bug as FIXED again once that patch gets reviewed and committed.
This was fixed by #656689: alex | I don't get any WTF? anymore