GNOME Bugzilla – Bug 662770
ContactPhotos are ignored when mime type is null
Last modified: 2011-10-27 00:06:55 UTC
In backends/eds/lib/edsf-persona.vala#_contact_photo_to_loadable_icon we are returning null when the mime type is not available (i.e.: null) but it turns out this is perfectly valid for an EContact. Also, Edsf.MemoryIcon has an inconsistent approach towards the image_type. In some parts of the class definition documentation says that it (the image type) can be null whereas in other parts (the constructor and the instance member declaration) it is not stated (i.e.: we don't use string?).
Created attachment 200024 [details] [review] patch The most controversial point in this patch is what to do in hash() when image_type is null. I went with starting (uint) hash to 0. I wonder how happy will Philip be about this :)
Note that fixing https://bugzilla.gnome.org/show_bug.cgi?id=662616 will greatly reduce the possibility of hitting this bug.
Review of attachment 200024 [details] [review]: Honestly, I don't see any problem with using 0 as the hash value for null image_types. The content should be the part that matters, right? (Philip, please inform me if I'm missing something) This patch looks fine to me - maybe double-check with Philip.
Review of attachment 200024 [details] [review]: I can't think of a problem with using 0. I was going to suggest using 5381, which is the base value used in g_str_hash() — but then I realised that would make the hashes of a MemoryIcon with a null image type and a MemoryIcon with an empty string image type equal, which probably isn't desirable. 0 is good.
Review of attachment 200024 [details] [review]: Don't forget to modify NEWS, though.
Thanks for the reviews. Merged: commit aa50fcfa896f9102411f1f5b6772e17129f1decc Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Wed Oct 26 15:26:07 2011 +0100 e-d-s: load icon regardless of the mime type being available Also, allow Edsf.MemoryIcon to be instantiated with a null image type. The notion of the image type not being available was half there (some comments mentioned the possibility), but it wasn't contemplated in the constructor and in the instance variable. Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=662770