After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 633781 - Allow to set avatar on individuals
Allow to set avatar on individuals
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: libfolks
git master
Other Linux
: Normal enhancement
: folks-0.6.3
Assigned To: folks-maint
folks-maint
Depends on: 638281 653777
Blocks: 579231
 
 
Reported: 2010-11-02 08:46 UTC by Guillaume Desmottes
Modified: 2011-09-13 19:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Make Individual.avatar writeable (3.49 KB, patch)
2011-06-25 08:49 UTC, Philip Withnall
reviewed Details | Review
Make Individual.avatar writeable (attempt 2) (6.63 KB, patch)
2011-08-12 10:20 UTC, Philip Withnall
needs-work Details | Review
Make Individual.avatar writeable (attempt 3) (9.54 KB, patch)
2011-09-02 19:51 UTC, Philip Withnall
committed Details | Review

Description Guillaume Desmottes 2010-11-02 08:46:36 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.
Comment 1 Travis Reitter 2010-12-23 18:47:46 UTC
This will certainly be supported for proper writable backends.
Comment 2 Travis Reitter 2011-06-20 22:45:31 UTC
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.
Comment 3 Philip Withnall 2011-06-25 08:49:51 UTC
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.
Comment 4 Philip Withnall 2011-06-25 08:51:06 UTC
I forgot to mention that the branch is based on the one for bug #650414, rebased against today's master.
Comment 5 Travis Reitter 2011-07-19 20:50:00 UTC
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.
Comment 6 Philip Withnall 2011-07-24 19:25:03 UTC
(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?
Comment 7 Travis Reitter 2011-07-25 18:16:52 UTC
(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.
Comment 8 Travis Reitter 2011-08-01 18:39:32 UTC
Punting bugs that won't be fixed by Folks 0.6.0.
Comment 9 Philip Withnall 2011-08-12 10:20:47 UTC
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.
Comment 10 Travis Reitter 2011-08-23 16:21:36 UTC
(Setting target bugs for Folks 0.6.1. Search for this phrase to deal with bug mail spam.)
Comment 11 Raul Gutierrez Segales 2011-08-29 12:46:53 UTC
Didn't make this release; punting to 0.6.2.
Comment 12 Travis Reitter 2011-08-29 19:07:39 UTC
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
  • #0 raise
    at ../nptl/sysdeps/unix/sysv/linux/raise.c line 64
  • #1 abort
    at abort.c line 92
  • #2 g_assertion_message
    at gtestutils.c line 1425
  • #3 __lambda1_
    at /home/treitter/checkout/gnome/folks/tests/eds/set-avatar.vala line 77
  • #4 ___lambda1__gsource_func
    at set-avatar.c line 269
  • #5 g_timeout_dispatch
    at gmain.c line 3907
  • #6 g_main_dispatch
    at gmain.c line 2442
  • #7 g_main_context_dispatch
    at gmain.c line 3011
  • #8 g_main_context_iterate
    at gmain.c line 3089
  • #9 g_main_loop_run
    at gmain.c line 3297
  • #10 set_avatar_tests_test_set_individual_avatar
    at /home/treitter/checkout/gnome/folks/tests/eds/set-avatar.vala line 176
  • #11 _set_avatar_tests_test_set_individual_avatar_folks_test_case_test_method
    at /home/treitter/checkout/gnome/folks/tests/eds/set-avatar.vala line 41
  • #12 test_case_run
    at gtestutils.c line 1227
  • #13 g_test_run_suite_internal
    at gtestutils.c line 1280
  • #14 g_test_run_suite_internal
    at gtestutils.c line 1291
  • #15 g_test_run_suite
    at gtestutils.c line 1336
  • #16 _vala_main
    at /home/treitter/checkout/gnome/folks/tests/eds/set-avatar.vala line 251
  • #17 __libc_start_main
    at libc-start.c line 228
  • #18 _start

Comment 13 Philip Withnall 2011-09-02 19:51:24 UTC
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.)
Comment 14 Travis Reitter 2011-09-05 16:51:05 UTC
(Punting bugs to 0.6.3)
Comment 15 Philip Withnall 2011-09-12 21:46:59 UTC
Still waiting for review…
Comment 16 Raul Gutierrez Segales 2011-09-13 09:46:04 UTC
Review of attachment 195523 [details] [review]:

Sorry for the delay. Looks good to me.
Comment 17 Philip Withnall 2011-09-13 19:15:43 UTC
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(-)