904870 - Allow to save card if local contact photo has been removed

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Samuel Müller]

This is a spin off of #691141 to fix the unsavable cards for the short term.

Comment 1

[Samuel Müller]

Attachment: fixing unsavable cards due to missing local photo file

It turned out that this quick fix is just two lines of code.
It allows to save the card if the contact has a local file assigned as a photo, but the file has been renamed/removed in the meantime. However the preview image in the photo tab is still empty, but that issue is targeted in the complete rewrite of bug 691141. This patch just solves the issue until the other patch is ready.

Comment 2

[:aceman]

Looks nicely short.

Comment 3

[Samuel Müller]

Comment on attachment 789858 [details] [diff] [review]
fixing unsavable cards due to missing local photo file

Ok let's try to get this fix into TB 24.

[Approval Request Comment]
Regression caused by (bug #): 119459
User impact if declined: Contact cards with certain condition (local photo renamed/removed) cannot be edited/saved anymore.
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): Probably none as it is just two lines.

Comment 4

[:aceman]

Ok, but first you need to get any review+, e.g. from mconley.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 789858 [details] [diff] [review]
fixing unsavable cards due to missing local photo file

Review of attachment 789858 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abCardOverlay.js
@@ +1126,5 @@
>        return false;
>  
> +    // If the local file has been removed/renamed, keep the current photo as is
> +    if (!file.exists() || !file.isFile())
> +      return true;

return true? Are you sure? If this local image file has been removed, we should probably take this opportunity to switch it to the generic photo by returning false. Is there a good reason not to?

Comment 6

[Samuel Müller]

Returning true keeps the photo that is currently assigned to the contact, as a copy of the file exists in the Photos directory and still can be displayed. Resetting the photo to the default image would just confuse users.

The edit card dialog so far (implicitly) creates a new copy of the local file every time that the card is edited. Imho the photo should just be replaced on request of the user.

Comment 7

[Samuel Müller]

How are we going to continue here? It's either restoring the default (return false) or keep displaying the image in the Photos directory (return true). 
In the latter case we could add an additional check that the stored filename really exists before returning true.

Comment 8

[Justin Wood (:Callek)]

needinfo re: Gecko 24's imminent ship and c#7 plus approval/review request

Comment 9

[Mark Banner (:standard8)]

This isn't critical for the release (especially as bug 691141 has been around for ages), and can go into a dot release once we've got review.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 789858 [details] [diff] [review]
fixing unsavable cards due to missing local photo file

Review of attachment 789858 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, yes, I'd forgotten how this all works - we keep a local copy in the profile folder.

Yes, I think it makes sense to continue showing the local copy, even if the file that the copy was created from was removed.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/f2403ad38ac9

Comment 12

[:aceman]

Comment on attachment 789858 [details] [diff] [review]
fixing unsavable cards due to missing local photo file

Review of attachment 789858 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this works in Nightly. I propose getting it into 24.0.1.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 789858 [details] [diff] [review]
fixing unsavable cards due to missing local photo file

We'll get this landed on aurora/beta first, and then do 24 a bit later.

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/5e80f8fc01aa
https://hg.mozilla.org/releases/comm-beta/rev/3ab78494bbbe

Comment 15

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr24/rev/27769e415903