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 674381 - Show contact photo from address book doesn't work
Show contact photo from address book doesn't work
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: Mailer
3.6.x (obsolete)
Other Linux
: Normal normal
: ---
Assigned To: evolution-mail-maintainers
Evolution QA team
evolution[webkit]
Depends on:
Blocks:
 
 
Reported: 2012-04-19 10:39 UTC by Milan Crha
Modified: 2013-09-13 01:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test vcard (4.36 KB, text/plain)
2012-04-19 10:40 UTC, Milan Crha
  Details
test message (593 bytes, text/plain)
2012-04-19 10:40 UTC, Milan Crha
  Details
Patch (3.27 KB, patch)
2012-04-19 15:50 UTC, Dan Vrátil
reviewed Details | Review
Fixed patch (3.27 KB, patch)
2012-04-23 06:57 UTC, Dan Vrátil
committed Details | Review

Description Milan Crha 2012-04-19 10:39:23 UTC
Summary says it all, show contact photo from address book doesn't work.
Steps:
a) import attached contact to your addressbook, which is marked
   for autocompletion
b) import attached email
c) make sure Edit->preferences->Mail Preferences->Headers has "Show contact
   photo" checked, I have also "only in local books", as I imported it there
[c1) feel free to restart all evolution processes, even it is not required]
d) in mailer selected imported message

It should show a "photo" from the contact, as 3.4.0 does, but it doesn't.
Comment 1 Milan Crha 2012-04-19 10:40:40 UTC
Created attachment 212345 [details]
test vcard
Comment 2 Milan Crha 2012-04-19 10:40:56 UTC
Created attachment 212346 [details]
test message
Comment 3 Dan Vrátil 2012-04-19 15:50:23 UTC
Created attachment 212367 [details] [review]
Patch

This should make it work. Please try with more various contacts, your addressbook is probably much bigger then mine ;)
Comment 4 Milan Crha 2012-04-20 13:45:02 UTC
Please keep there the check for photo != NULL at:
 		/* keep only up to 10 photos in memory */
-		if (photo && count_not_null >= 10 && first_not_null) {
+		if (count_not_null >= 10 && first_not_null) {
 			pi = first_not_null->data;

The search_address_in_addressbooks() can return TRUE, as it found the contact, but the contact can be without photo, while the em_utils_contact_photo() takes care of contacts with photo only. Or, maybe, what was the purpose for this change?

Fix added new lines in em-format-html.c, please. There are added too many below efh_constructed.

Otherwise looks good. I'm only afraid the change from e-mail-utils.c and movement of e_extensible_load_extensions() should come to gnome-3-4 as well. Could you test it there and possible commit as well, please?
Comment 5 Dan Vrátil 2012-04-23 06:57:29 UTC
Created attachment 212585 [details] [review]
Fixed patch

Removed the newlines below constructor.

(In reply to comment #4)
> Please keep there the check for photo != NULL at:
>          /* keep only up to 10 photos in memory */
> -        if (photo && count_not_null >= 10 && first_not_null) {
> +        if (count_not_null >= 10 && first_not_null) {
>              pi = first_not_null->data;
> 
> The search_address_in_addressbooks() can return TRUE, as it found the contact,
> but the contact can be without photo, while the em_utils_contact_photo() takes
> care of contacts with photo only. Or, maybe, what was the purpose for this
> change?

The point of this change is to store in the cache (which is a sort of a hashtable email=>photo) even contacts without a picture.

The purpose of the cache (at least as I see it) is to speed up lookup of contact photo. If the contact has no photo, we still want to learn about it quickly, without having to search the addressbooks again.

> Otherwise looks good. I'm only afraid the change from e-mail-utils.c and
> movement of e_extensible_load_extensions() should come to gnome-3-4 as well.
> Could you test it there and possible commit as well, please?

Looks good, there were no other potentially conflicting changes in e-mail-utils.c since gnome-3-4 branching, so it should apply and work correctly.
Comment 6 Milan Crha 2012-04-23 08:23:16 UTC
(In reply to comment #5)
> The point of this change is to store in the cache (which is a sort of a
> hashtable email=>photo) even contacts without a picture.
> 
> The purpose of the cache (at least as I see it) is to speed up lookup of
> contact photo. If the contact has no photo, we still want to learn about it
> quickly, without having to search the addressbooks again.

Yup, they are always added to the cache, but only those with picture are removed. That might be the bug you accidentally pointed to. :) So, there is called g_slist_append when adding to photos_cache, thus I would simplify the code to not count_not_null, neither check for first_not_null, but always remove the first item from photos_cache when its size exceeds 10 items.

Feel free to just change it and commit. Thanks.
Comment 7 Dan Vrátil 2012-04-27 18:36:01 UTC
I've fixed the caching to prepend new items and remove the last item when cache is full.

Committed all changes to master as 7981771ea6a6ab2729010fc814d427e25f792b31

http://git.gnome.org/browse/evolution/commit/?id=7981771ea6a6ab2729010fc814d427e25f792b31

Committed changes in libemail-engine and EMailFormatter constructor to gnome-3-4 as c219cb1dec286cea39c0a7d8455319c3677d3600

http://git.gnome.org/browse/evolution/commit/?h=gnome-3-4&id=c219cb1dec286cea39c0a7d8455319c3677d3600