Created attachment 1455 [details] [patch] Fix contact picture filename retrieval When a person is deleted from the address book, if they have a picture it remains in the addrbook directory. This is because addritem_person_get_picture doesn't return the full filename of a person's picture, but addressbook_del_clicked expects it does. This patch makes addritem_person_get_picture return the full filename so that the picture is found in its actual location and deleted successfully when the contact is deleted.
Correct, but.. do we really want the picture (potentially a - imported - external resource) to be erased as well? Maybe we should ask first.
(In reply to comment #1) > Correct, but.. do we really want the picture (potentially a - imported - > external resource) to be erased as well? Maybe we should ask first. When the picture is imported, it is resized and copied into .claws-mail/addrbook, so it is Claws's copy of the image, not the original. So I think it's safe to delete the picture if the user wants to delete the contact.
Yes, but we should also check that no other AB item uses the picture. (XMl edited intentionnally may have several contacts point to the same image resource)
(In reply to comment #3) > Yes, but we should also check that no other AB item uses the picture. (XMl > edited intentionnally may have several contacts point to the same image > resource) So multiple <person>s with the same uid? Wouldn't that cause problems elsewhere? Checking all the other contacts would be somewhat expensive.
No but the file itself could be hardlinked. Well maybe that's a very specific setup :-).
(In reply to comment #5) > No but the file itself could be hardlinked. Well maybe that's a very > specific setup :-). AFAIK if the file is hard linked the deletion only removes one link, hence the file isn't actually deleted until the number of hard links is 1, so even in that case it's safe to remove it (even when using shred, see claws_unlink in utils.c). The problematic case would be shared pictures but _not_ hard linked :) Anyway I disagree with the idea of returning a constructed path. I'm more inclined to build the path on the caller side and left the addrbook API untouched. If later more callers appear it can be considered if the API should be modified or not.
(In reply to comment #6) > (In reply to comment #5) > > No but the file itself could be hardlinked. Well maybe that's a very > > specific setup :-). > > AFAIK if the file is hard linked the deletion only removes one link, hence > the file isn't actually deleted until the number of hard links is 1, so even > in that case it's safe to remove it (even when using shred, see claws_unlink > in utils.c). > > The problematic case would be shared pictures but _not_ hard linked :) > > Anyway I disagree with the idea of returning a constructed path. I'm more > inclined to build the path on the caller side and left the addrbook API > untouched. s/left/leave/ :-) > If later more callers appear it can be considered if the API should be > modified or not.
(In reply to comment #6) > Anyway I disagree with the idea of returning a constructed path. I'm more > inclined to build the path on the caller side and left the addrbook API > untouched. > > If later more callers appear it can be considered if the API should be > modified or not. What is the use of the addrbook API returning the picture other than as a constructed path? The callers of addritem_person_get_picture are using the result as a filename, and so their operations are failing silently (except when the picture is from an LDAP query, it looks like). See addressbook_del_clicked, addrduplicates_delete_item_person, and addrindex_get_picture_file (which is used in headerview.c and textview.c). Caller-side operations which succeed are not using addritem_person_get_picture(person), but are using person->picture and then constructing the filename, so the proposed patch does not affect them.
Created attachment 1462 [details] Fix filename construction for picture deletion Since not changing addritem is a concern, here is an updated patch which fixes the uses of addritem_person_get_picture for picture deletion on the caller side. This fixes the issue of pictures not getting deleted when their contact is deleted.
(In reply to comment #9) > Created attachment 1462 [details] > Fix filename construction for picture deletion > > Since not changing addritem is a concern, here is an updated patch which > fixes the uses of addritem_person_get_picture for picture deletion on the > caller side. This fixes the issue of pictures not getting deleted when their > contact is deleted. Just tried this patch, but pictures are not deleted yet. Took some address added one picture to it, the partial --debug log reads: editaddress.c:1586:saved picture to /home/mones/.claws-mail/addrbook/313273410.png Then deleted it: addr_compl.c:741:Invalidation request for address completion addr_compl.c:397:read 505 items in (null) addr_compl.c:741:Invalidation request for address completion addr_compl.c:397:read 505 items in (null) alertpanel.c:254:Creating alert panel dialog... alertpanel.c:213:called inc_lock (lock count 1) alertpanel.c:223:called inc_unlock (lock count 0) alertpanel.c:107:return value = 1 addressbook.c:4357:Saving address books... addressbook.c:4360:Exporting addressbook to file... addr_compl.c:741:Invalidation request for address completion addr_compl.c:397:read 503 items in (null) addr_compl.c:768:end_address_completion ref count 0 addressbook.c:4357:Saving address books... addressbook.c:4360:Exporting addressbook to file... Exited Claws Mail, but picture is still there: $ ll /home/mones/.claws-mail/addrbook/313273410.png -rw-r--r-- 1 mones mones 8807 Dec 24 02:51 /home/mones/.claws-mail/addrbook/313273410.png Any idea why?
I've tested this again and seems it works now, so I'm pushing an improved version of your patch :-) The only explanation I can find is either I did a bad testing (most likely) or that something affecting this was fixed at some point.
Changes related to this bug have been committed. Please check latest Git and update the bug accordingly. You can also get the patch from: http://git.claws-mail.org/ ++ ChangeLog 2017-07-03 21:56:02.503542110 +0200 http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=a2162e0bb2f9369356e65514bf78c02b83e3ff2f Merge: 6aa51a1 066e76a Author: Colin Leroy <colin@colino.net> Date: Mon Jul 3 21:56:02 2017 +0200 Merge branch 'master' of file:///home/git/claws http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=066e76a2863a8468b8fd491e50396661b62174e6 Author: Ricardo Mones <ricardo@mones.org> Date: Mon Jul 3 21:55:23 2017 +0200 Fix bug #3348 ‘Contact pictures not deleted when contact is deleted’ Based on original patch submitted by Charles Lehner (thanks!)