GNOME Bugzilla – Bug 633781
Allow to set avatar on individuals
Last modified: 2011-09-13 19:26:09 UTC
If at some point we want to allow users to set their own avatar on contacts (bug #579231), folks should gain API to set avatar on individuals. Pretty low priority IMHO.
This will certainly be supported for proper writable backends.
This should be fixed as soon as we merge the EDS backend and make it writable. Technically, this is already satisfied by our Tracker backend, but nobody really uses that.
Created attachment 190637 [details] [review] Make Individual.avatar writeable http://cgit.collabora.com/git/user/pwith/folks/log/?h=633781-writeable-avatars This compiles fine, but hasn't been tested yet as the e-d-s backend hasn't been merged yet.
I forgot to mention that the branch is based on the one for bug #650414, rebased against today's master.
Review of attachment 190637 [details] [review]: What's the idea behind the fall-back "write to all the personas" case? So that the Individual will retain the new avatar until the process terminates? If that's the case, I think it'd be better to return an error and not change any of the avatars. Because I think the user would be confused by the avatar not sticking (especially since it could appear to revert days or weeks later, in cases of long-running programs). Also, I think this deserves a test case in the EDS backend.
(In reply to comment #5) > Review of attachment 190637 [details] [review]: > > What's the idea behind the fall-back "write to all the personas" case? So that > the Individual will retain the new avatar until the process terminates? I copied it from the setter for Individual.alias. > If that's the case, I think it'd be better to return an error and not change > any of the avatars. Because I think the user would be confused by the avatar > not sticking (especially since it could appear to revert days or weeks later, > in cases of long-running programs). I agree. However, I haven't checked, but I'm fairly sure it's not possible to throw errors from properties (in the C implementation, where would the GError come from?). I was going to suggest that we call ensure_property_writeable() (from bug #653777) from within the Individual.avatar (and Individual.alias) setters, but then I realised that's not particularly elegant because it's an async function call from within a property setter (and the caller might not expect that setting an avatar on an Individual may result in a new Persona being magically created). Instead, how about just documenting that setting Individual.avatar is only guaranteed to succeed (in terms of writeable Personas being available) if ensure_property_writeable() is called first. Otherwise, it may fail; in which case, the property will not change value and no property notification will be emitted. I think this is the best we can do without the ability to throw an error. > Also, I think this deserves a test case in the EDS backend. What do you think this test case should do? Construct an individual which contains a persona from eds, set an avatar on the individual and assert that the avatar's appeared on the eds contact?
(In reply to comment #6) > (In reply to comment #5) > > Review of attachment 190637 [details] [review] [details]: > > > > What's the idea behind the fall-back "write to all the personas" case? So that > > the Individual will retain the new avatar until the process terminates? > > I copied it from the setter for Individual.alias. > > > If that's the case, I think it'd be better to return an error and not change > > any of the avatars. Because I think the user would be confused by the avatar > > not sticking (especially since it could appear to revert days or weeks later, > > in cases of long-running programs). > > I agree. However, I haven't checked, but I'm fairly sure it's not possible to > throw errors from properties (in the C implementation, where would the GError > come from?). You're right. I wish we didn't have this limitation. > I was going to suggest that we call ensure_property_writeable() (from bug > #653777) from within the Individual.avatar (and Individual.alias) setters, but > then I realised that's not particularly elegant because it's an async function > call from within a property setter (and the caller might not expect that > setting an avatar on an Individual may result in a new Persona being magically > created). > > Instead, how about just documenting that setting Individual.avatar is only > guaranteed to succeed (in terms of writeable Personas being available) if > ensure_property_writeable() is called first. Otherwise, it may fail; in which > case, the property will not change value and no property notification will be > emitted. I think this is the best we can do without the ability to throw an > error. This sounds like the best option, though not a particularly pretty one. I guess it parallels Telepathy-GLib's "ensure" functions and their use patterns. > > Also, I think this deserves a test case in the EDS backend. > > What do you think this test case should do? Construct an individual which > contains a persona from eds, set an avatar on the individual and assert that > the avatar's appeared on the eds contact? Sounds good to me.
Punting bugs that won't be fixed by Folks 0.6.0.
Created attachment 193684 [details] [review] Make Individual.avatar writeable (attempt 2) https://www.gitorious.org/folks/folks/commits/633781-individual-avatar-attempt2 Attempt 2. This removes the code which sets the avatar on all personas as a fallback, adds a documentation comment saying to call ensure_property_writeable() first, and adds a test case. NOTE: I haven't been able to run the test case and verify that it passes yet, because the eds tests are completely broken on my laptop (I suspect a problem with my jhbuild setup rather than folks itself). I'll make sure the test passes before committing.
(Setting target bugs for Folks 0.6.1. Search for this phrase to deal with bug mail spam.)
Didn't make this release; punting to 0.6.2.
Review of attachment 193684 [details] [review]: This generally looks good to me, but the test eds/set-avatar fails for me: Starting program: /home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar [Thread debugging using libthread_db enabled] /SetAvatar/setting avatar on e-d-s persona: [New Thread 0x2aaab07e2700 (LWP 24920)] (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: Using environment variable FOLKS_BACKEND_STORE_KEY_FILE_PATH = './data/backend-eds-only.ini' to load the backends key file. [New Thread 0x2aaab1678700 (LWP 25069)] (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: Using environment variable FOLKS_BACKEND_STORE_KEY_FILE_PATH = './data/backend-eds-only.ini' to load the backends key file. (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: Using environment variable FOLKS_BACKEND_PATH = '../../backends/key-file/.libs/key-file.so:../../backends/telepathy/.libs/telepathy.so:../../backends/libsocialweb/.libs/libsocialweb.so:../../backends/tracker/.libs/tracker.so:../../backends/eds/.libs/eds.so' to look for backends (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: backend-store.vala:629: Loaded module source: '/home/treitter/checkout/gnome/folks/backends/libsocialweb/.libs/libsocialweb.so' (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: backend-store.vala:629: Loaded module source: '/home/treitter/checkout/gnome/folks/backends/eds/.libs/eds.so' (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: backend-store.vala:629: Loaded module source: '/home/treitter/checkout/gnome/folks/backends/tracker/.libs/tracker.so' (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: backend-store.vala:629: Loaded module source: '/home/treitter/checkout/gnome/folks/backends/telepathy/.libs/telepathy.so' (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: backend-store.vala:629: Loaded module source: '/home/treitter/checkout/gnome/folks/backends/key-file/.libs/key-file.so' (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: Found no entry for backend 'telepathy'.enabled in backend keyfile. Disabling according to 'all-others' setting. (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: Found no entry for backend 'tracker'.enabled in backend keyfile. Disabling according to 'all-others' setting. (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): eds-DEBUG: eds-backend.vala:140: Address book source list changed. (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): eds-DEBUG: eds-backend.vala:208: Adding address book 'local://test'. (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: backend-store.vala:339: New backend 'eds' prepared (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: Found no entry for backend 'libsocialweb'.enabled in backend keyfile. Disabling according to 'all-others' setting. (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): 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/eds/.libs/lt-set-avatar:24905): eds-DEBUG: edsf-persona.vala:492: Creating new Edsf.Persona with IID 'local://test:pas-id-4E5BE2E300000000' (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:853: Removing Personas: (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:882: Removing Individuals due to removed links: (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:912: Adding Personas: (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:915: eds:local\://test:pas-id-4E5BE2E300000000 (is user: no, IID: local://test:pas-id-4E5BE2E300000000) (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:629: Aggregating persona 'eds:local\://test:pas-id-4E5BE2E300000000' on 'local://test:pas-id-4E5BE2E300000000'. (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:716: Did not find any candidate individuals. (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual.vala:721: Creating new Individual with 1 Personas: 0x6e4030 (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual.vala:963: Running _update_is_favourite() on '585453d8d916e7bc5c97229573396cb74bdcc810' (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual.vala:990: Updating alias for individual '585453d8d916e7bc5c97229573396cb74bdcc810' (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual.vala:1009: got alias '(null)' from writeable personas (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual.vala:1044: got alias '(null)' from non-writeable personas (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: No aliases available for individual; using display ID instead: bernie h. innocenti (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual.vala:1066: Changing alias of individual '585453d8d916e7bc5c97229573396cb74bdcc810' from '(null)' to 'bernie h. innocenti'. (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:721: Created new individual '585453d8d916e7bc5c97229573396cb74bdcc810' with personas: (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:727: eds:local\://test:pas-id-4E5BE2E300000000 (is user: no, IID: local://test:pas-id-4E5BE2E300000000) (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:740: Inserting links: (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:764: pas-id-4E5BE2E300000000 (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:925: Relinking Personas: (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual-aggregator.vala:960: Replacing Individuals due to linking: (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual.vala:139: Setting avatar of individual '585453d8d916e7bc5c97229573396cb74bdcc810' to '0x61dd80'? (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): folks-DEBUG: individual.vala:149: written to writeable persona 'eds:local\://test:pas-id-4E5BE2E300000000' OK /SetAvatar/setting avatar on e-d-s individual: (/home/treitter/checkout/gnome/folks/tests/eds/.libs/lt-set-avatar:24905): eds-DEBUG: eds-backend.vala:140: Address book source list changed. Program received signal SIGABRT, Aborted. 0x00002aaaaf812405 in raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 64 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory. in ../nptl/sysdeps/unix/sysv/linux/raise.c (gdb) bt
+ Trace 228262
Created attachment 195523 [details] [review] Make Individual.avatar writeable (attempt 3) https://www.gitorious.org/folks/folks/commits/633781-individual-avatar-attempt3 Since bug #657510 has been fixed, fixing this bug got a whole lot easier. Updated patch which works fine for me, including the test. (The test was failing before because I failed to reset the test suite's state properly after the preceding test.)
(Punting bugs to 0.6.3)
Still waiting for review…
Review of attachment 195523 [details] [review]: Sorry for the delay. Looks good to me.
Comment on attachment 195523 [details] [review] Make Individual.avatar writeable (attempt 3) Committed, thanks. commit 0737c2d78c4041bb211bd005d28a6960babddb70 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Fri Aug 12 12:18:16 2011 +0200 Bug 633781 — Allow to set avatar on individuals Make Individual.avatar writeable and write new avatars back to the personas from writeable persona stores in an individual. Closes: bgo#633781 NEWS | 5 ++ folks/individual.vala | 64 +++++++++++++++++++ tests/eds/set-avatar.vala | 154 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 222 insertions(+), 1 deletions(-)