Thanks for a patch. I've few concerns:
a) I would call it e_contact_photo_get_inlined ();
b) return NULL when not an inlined photo
c) write to the comment of the function how to free the returned pointer

Thus instead of the switch and the g_warning might work g_return_val_if_fail (, NULL); pretty well.

Created attachment 189104 [details] [review]
Add getters/setters for EContactPhoto

I've addressed Milan's comments on the bug and also what we've discussed on irc (adding setters/getters for the rest of the union inside EContactPhoto).

I've also added the corresponding sections in the documentation files.

Thanks, it looks mostly fine, only the inlined part is strange to me. What about using similar (but not the same) model as with g_file_get_contents? I would expect something like: const guchar *e_contact_get_inlined (...*photo, gsize *len); and the e_contact_set_inlined may also use "const guchar *" and make a copy of the memory, to avoid expectations from where the caller took the memory chunk. Does it make sense? The rest is perfectly fine.

(In reply to comment #4)
> Thanks, it looks mostly fine, only the inlined part is strange to me. What
> about using similar (but not the same) model as with g_file_get_contents? I
> would expect something like: const guchar *e_contact_get_inlined (...*photo,
> gsize *len);

Hrm. I am not sure how introspection friendly that would be. I recall being suggested to use GArray by the introspection people (and from my little testing it generates nice and simple GIR & bindings). Introspection support it my initial and final motivation but I agree it also has to be consistent with the rest of e-d-s' API as it will probably be used from Evo.

The problem I see with going the guchar * + gsize * way is that we'll end up with more annotation magic, though it shouldn't be that bad. 

> and the e_contact_set_inlined may also use "const guchar *" and
> make a copy of the memory, to avoid expectations from where the caller took the
> memory chunk. Does it make sense? The rest is perfectly fine.

I have no strong opinions here. Whatever makes it more consistent with the rest of e-d-s' API. So lets make a copy.

Created attachment 189149 [details] [review]
Add getters/setters for EContactPhoto

Updated the patch according to received comments.

With this new approach it'll make it a little bit harder for Vala bindings to immediately work (cause out parameter for the length of returned arrays is not supported atm)  but its fixable. Not sure about other (introspected) language bindings.

(In reply to comment #6)
> With this new approach it'll make it a little bit harder for Vala bindings to
> immediately work (cause out parameter for the length of returned arrays is not
> supported atm)  but its fixable. Not sure about other (introspected) language
> bindings.

There actually is an easy workaround, so this patch will be good enough.

Also, I'll follow up with patch (separate bug report) to use these new methods from inside e-d-s to handle EContactPhotos.

Thanks for a quick update. There is a typo in the e_contact_photo_set_inlined(), you may not g_malloc with 'photo->data.inlined.length', but with 'len' instead. I can easily fix it before committing, if you do not have commit rights to Gnome's git. I would also add more paranoid checks, like if photo != NULL, if any pointer passing in isn't NULL (except of the case where the array will be "NULL-ified", to free its content).

Evolution's co de also rarely adds new lines in g_return_val_if_fail() calls, both the expression and the return value use to be on one line, as far as I know.

Anyway, the patch can be committed after the typo change.

Merged:

commit 876e2b5add7f9ed8269870998cf6a1a49a88e17c
Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk>
Date:   Mon May 30 11:24:17 2011 +0100

    Bug #651458 - Add getters/settrs for EContactPhoto
    
    This is needed for introspected bindings since gobject-introspection
    can't cope with the current layout of the EContactPhoto structure.