GNOME Bugzilla – Bug 644867
Add interface for linkable web service contact UIDs
Last modified: 2011-04-08 19:55:36 UTC
In order for the libsocialweb backend's Personas to be linkable to other Personas, we're going to need a trivial interface like ImDetails but for contact IDs on web services. The basic idea is to provide globally-addressable IDs (as used by web services) which we can't directly send IMs to (which is why we can't just use the ImDetails interface). I don't have immediate plans to support anything beyond a hash of {service name: set(IDs)}, as we do in the ImDetails interface. With this interface, we'll be able to link Swf.Personas to other Personas by these "web-service-addresses" values.
Implemented by: http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw4 No unit test implemented yet
(In reply to comment #1) > Implemented by: > http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw4 > > No unit test implemented yet This generally looks good. Please write a test case to make sure it works as expected. Some things to fix: backends/libsocialweb/lib/swf-persona.vala ========================================== + var web_service_address_array = new LinkedHashSet<string> (); + web_service_address_array.add (id); + this._web_service_addresses.insert ("mysocialnetwork", + (owned) web_service_address_array); It looks like you included a test/debugging name here. This should be the actual name of the web service you're adding the ID for. folks/individual.vala ===================== + private void _update_web_service_addresses () + { + /* populate the web service addresses as the union of our Personas' addresses */ + foreach (var persona in this.personas) + { + if (persona is WebServiceDetails) + { + var web_service_details = (WebServiceDetails) persona; + web_service_details.web_service_addresses.foreach ((k, v) => + { + var cur_web_service = (string) k; ... Be careful about corner cases with null values. See bug #646244 and bug #645570 folks/web-service-details.vala ============================== + * There must be no duplicate web service addresses in each ordered set, + * though a given web service address may be present in the sets for + * different web services. The uniqueness of members is guaranteed by the LinkedHashSet, so you may want to say "Web service addresses are guaranteed to be unique per web service, but not necessarily unique amongst all web services."
Since nobody's contradicted me yet in bug #646079, I think you should probably use HashMap instead of HashTable in the definition of the WebServiceDetails interface. It's nicer to use from Vala, and if we're exposing libgee types in other places (such as with LinkedHashSet), we lose nothing by exposing HashMap as well. This would mean that you could then use foreach{} blocks instead of .foreach() lambda functions to iterate over the HashMap, which is more efficient. Note that as per comment #3 in bug #645685, if we end up adding an OrderedSet data structure, WebServiceDetails would have to be converted to use it instead of LinkedHashSet. That can be dealt with if and when necessary, though.
Branch updated with an unit test: http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw4 It does not adress Philip's concern yet. (In reply to comment #2) > backends/libsocialweb/lib/swf-persona.vala > ========================================== > + var web_service_address_array = new LinkedHashSet<string> (); > + web_service_address_array.add (id); > + this._web_service_addresses.insert ("mysocialnetwork", > + (owned) web_service_address_array); > > It looks like you included a test/debugging name here. This should be the > actual name of the web service you're adding the ID for. Fixed. > folks/individual.vala > ===================== > + private void _update_web_service_addresses () > + { > + /* populate the web service addresses as the union of our Personas' > addresses */ > + foreach (var persona in this.personas) > + { > + if (persona is WebServiceDetails) > + { > + var web_service_details = (WebServiceDetails) persona; > + web_service_details.web_service_addresses.foreach ((k, v) => > + { > + var cur_web_service = (string) k; > ... > > Be careful about corner cases with null values. See bug #646244 and bug #645570 I don't think it can be NULL here. When a persona implements WebServiceDetails, it should be initialized correctly. > folks/web-service-details.vala > ============================== > > + * There must be no duplicate web service addresses in each ordered set, > + * though a given web service address may be present in the sets for > + * different web services. > > The uniqueness of members is guaranteed by the LinkedHashSet, so you may want > to say "Web service addresses are guaranteed to be unique per web service, but > not necessarily unique amongst all web services." Updated.
Branch rebased on the last master: http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw7
(In reply to comment #3) > Since nobody's contradicted me yet in bug #646079, I think you should probably > use HashMap instead of HashTable in the definition of the WebServiceDetails > interface. It's nicer to use from Vala, and if we're exposing libgee types in > other places (such as with LinkedHashSet), we lose nothing by exposing HashMap > as well. > > This would mean that you could then use foreach{} blocks instead of .foreach() > lambda functions to iterate over the HashMap, which is more efficient. Agreed. Alban, please convert this data type to Gee.HashMap.
(In reply to comment #5) > Branch rebased on the last master: > http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw7 Beyond comment #6, please make these adjustments: + /* Check the result of link_personas */ + aggregator.individuals_changed.connect ((added, removed, m, a, r) => + { + debug ("individuals_changed after link: added:%u removed:%u", + added.length (), removed.length ()); + assert (added.length () == 1); + assert (removed.length () == 2); + Individual i = added.nth_data (0); + assert (i.personas.length () > 0); Shouldn't this check == 2 instead? + /* The signal individuals_changed is emitted by link_personas when + * the main loop is not yet started. */ + Idle.add (() => { main_loop.quit (); return false; }); + }); This seems a little odd - could we instead move the call to link_personas() into an Idle handler? That seems like it'd be more readable. + /* Wait 1 second for saving the .ini file asynchronously */ + timer_id = Timeout.add_seconds (1, () => + { + main_loop.quit (); + return false; + }); I'd rather you add an extra ini file (which enables exactly the key-file and linking backends) if this timing really is an issue. After making these changes, assuming 'make check' still passes, please merge to master.
(In reply to comment #7) > (In reply to comment #5) > > Branch rebased on the last master: > > http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw7 > > Beyond comment #6, please make these adjustments: Updated to Gee.HashMap. > + /* Check the result of link_personas */ > + aggregator.individuals_changed.connect ((added, removed, m, a, r) => > + { > + debug ("individuals_changed after link: added:%u removed:%u", > + added.length (), removed.length ()); > + assert (added.length () == 1); > + assert (removed.length () == 2); > + Individual i = added.nth_data (0); > + assert (i.personas.length () > 0); > > Shouldn't this check == 2 instead? Actually "== 3" because there is a new persona from the writable backend (keyfile in this case) to store the link. I updated it to "== 3". > > + /* The signal individuals_changed is emitted by link_personas when > + * the main loop is not yet started. */ > + Idle.add (() => { main_loop.quit (); return false; }); > + }); > > This seems a little odd - could we instead move the call to link_personas() > into an Idle handler? That seems like it'd be more readable. Done. > + /* Wait 1 second for saving the .ini file asynchronously */ > + timer_id = Timeout.add_seconds (1, () => > + { > + main_loop.quit (); > + return false; > + }); > > I'd rather you add an extra ini file (which enables exactly the key-file and > linking backends) if this timing really is an issue. It was about the relationships.ini file from the keyfile backend, not the other ini file. The keyfile backend saves it asynchronously so if the program quits before it is saved, the changes are not written on the filesystem. I removed that because the test does not read the relationships.ini file after the contacts are linked anyway. > After making these changes, assuming 'make check' still passes, please merge to > master. It passes. Branch updated on: http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw7 And the same changes squashed appropriately: http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw_linking
(In reply to comment #8) > Branch updated on: > http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw7 > And the same changes squashed appropriately: > http://git.collabora.co.uk/?p=user/alban/folks.git;a=shortlog;h=refs/heads/lsw_linking There are still a few instances of the string literals "im-addresses" and "web-service-addresses". Please replace these with the appropriate PersonaDetail constants and push to master.
Created attachment 185457 [details] [review] lsw_linking.patch Attached branch merged in git master. The string literals will be changed later as agreed on IRC.
(In reply to comment #10) > Created an attachment (id=185457) [details] [review] > lsw_linking.patch > > Attached branch merged in git master. The string literals will be changed later > as agreed on IRC. The tests worked for me before this was merged to master, but now this fails: /DummyLsw/dummy libsocialweb: (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: Using environment variable FOLKS_BACKEND_STORE_KEY_FILE_PATH = './data/backend-lsw-only.ini' to load the backends key file. ** (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): DEBUG: backend.vala:95: Start() called. (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: Using environment variable FOLKS_BACKEND_PATH = '../../backends/key-file/.libs/libfolks-backend-key-file.so:../../backends/telepathy/.libs/libfolks-backend-telepathy.so:../../backends/libsocialweb/.libs/libfolks-backend-libsocialweb.so:../../backends/tracker/.libs/libfolks-backend-tracker.so' to look for backends (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: backend-store.vala:562: Loaded module source: '/home/treitter/checkout/gnome/folks/backends/libsocialweb/.libs/libfolks-backend-libsocialweb.so' (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: backend-store.vala:562: Loaded module source: '/home/treitter/checkout/gnome/folks/backends/key-file/.libs/libfolks-backend-key-file.so' (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: backend-store.vala:562: Loaded module source: '/home/treitter/checkout/gnome/folks/backends/tracker/.libs/libfolks-backend-tracker.so' (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: backend-store.vala:562: Loaded module source: '/home/treitter/checkout/gnome/folks/backends/telepathy/.libs/libfolks-backend-telepathy.so' (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: Found no entry for backend 'telepathy'.enabled in backend keyfile. Disabling according to 'all-others' setting. (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: Found no entry for backend 'tracker'.enabled in backend keyfile. Disabling according to 'all-others' setting. (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: backend-store.vala:272: New backend 'libsocialweb' prepared (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: Found no entry for backend 'key-file'.enabled in backend keyfile. Disabling according to 'all-others' setting. ** (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): DEBUG: dummy-lsw.vala:105: mysocialnetwork.OpenViewCalled.connect ** (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): DEBUG: backend.vala:95: Start() called. ** (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): DEBUG: dummy-lsw.vala:109: OpenViewCalled.connect (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): libsocialweb-DEBUG: swf-persona.vala:190: Creating new Sw.Persona 'libsocialweb:mysocialnetwork:id01' for (null) UID 'id01': 0x20ad180 (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): libsocialweb-DEBUG: swf-persona.vala:190: Creating new Sw.Persona 'libsocialweb:mysocialnetwork:id02' for (null) UID 'id02': 0x20ad2c0 (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:347: Aggregating persona 'libsocialweb:mysocialnetwork:id01' on 'mysocialnetwork:id01'. (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:449: Did not find any candidate individuals. (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:768: Running _update_is_favourite() on 'libsocialweb:mysocialnetwork:id01' (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:795: Updating alias for individual 'libsocialweb:mysocialnetwork:id01' (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:814: got alias '(null)' from writeable personas (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:849: got alias '(null)' from non-writeable personas (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: No aliases available for individual; using display ID instead: id01 (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:867: Changing alias of individual 'libsocialweb:mysocialnetwork:id01' from '(null)' to 'id01'. (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:454: Created new individual 'libsocialweb:mysocialnetwork:id01' with personas: (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:460: libsocialweb:mysocialnetwork:id01 (is user: no, IID: mysocialnetwork:id01) (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:347: Aggregating persona 'libsocialweb:mysocialnetwork:id02' on 'mysocialnetwork:id02'. (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:449: Did not find any candidate individuals. (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:768: Running _update_is_favourite() on 'libsocialweb:mysocialnetwork:id02' (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:795: Updating alias for individual 'libsocialweb:mysocialnetwork:id02' (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:814: got alias '(null)' from writeable personas (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:849: got alias '(null)' from non-writeable personas (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: No aliases available for individual; using display ID instead: id02 (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual.vala:867: Changing alias of individual 'libsocialweb:mysocialnetwork:id02' from '(null)' to 'id02'. (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:454: Created new individual 'libsocialweb:mysocialnetwork:id02' with personas: (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:460: libsocialweb:mysocialnetwork:id02 (is user: no, IID: mysocialnetwork:id02) (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:595: Removing Personas: (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:626: Removing Individuals due to removed links: (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:657: Relinking Personas: ** (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): DEBUG: dummy-lsw.vala:143: Aggregator got some data! (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): folks-DEBUG: individual-aggregator.vala:692: Replacing Individuals due to linking: ** (/home/treitter/checkout/gnome/folks/tests/libsocialweb/.libs/lt-dummy-lsw:30497): DEBUG: dummy-lsw.vala:194: Aggregator changed some data! Aborted FAIL: dummy-lsw
Created attachment 185480 [details] Output from failed lsw test Sorry, disregard comment #11 - this attachment is the same content but not mangled by word-wrap.
(In reply to comment #11) > (In reply to comment #10) > > Created an attachment (id=185457) [details] [review] [details] [review] > > lsw_linking.patch > > > > Attached branch merged in git master. The string literals will be changed later > > as agreed on IRC. > > The tests worked for me before this was merged to master, but now this fails: It passes for me... do you have any stack with the failure? Did you test with libsocialweb 0.25.15 or an earlier version? libsocialweb-client 0.25.15 (which is dynamically linked to folks) has a fix to receive the D-Bus signal "ContactRemoved" correctly.
(In reply to comment #13) > (In reply to comment #11) > > (In reply to comment #10) > > > Created an attachment (id=185457) [details] [review] [details] [review] [details] [review] > > > lsw_linking.patch > > > > > > Attached branch merged in git master. The string literals will be changed later > > > as agreed on IRC. > > > > The tests worked for me before this was merged to master, but now this fails: > > It passes for me... do you have any stack with the failure? > > Did you test with libsocialweb 0.25.15 or an earlier version? > libsocialweb-client 0.25.15 (which is dynamically linked to folks) has a fix to > receive the D-Bus signal "ContactRemoved" correctly. I'm closing this bug, since the new code has already been merged. Please follow up for the test bug in bug #647200.