Closed Bug 904870 Opened 8 years ago Closed 8 years ago

Allow to save card if local contact photo has been removed

Categories

(Thunderbird :: Address Book, defect)

defect
Not set
normal

Tracking

(thunderbird25 fixed, thunderbird26 fixed, thunderbird_esr2425+ fixed)

RESOLVED FIXED
Thunderbird 27.0
Tracking Status
thunderbird25 --- fixed
thunderbird26 --- fixed
thunderbird_esr24 25+ fixed

People

(Reporter: samuel.mueller, Assigned: samuel.mueller)

References

Details

Attachments

(1 file)

This is a spin off of #691141 to fix the unsavable cards for the short term.
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.
Assignee: nobody → samuel.mueller
Status: NEW → ASSIGNED
Attachment #789858 - Flags: review?(mconley)
See Also: → 691141
Looks nicely short.
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.
Attachment #789858 - Flags: approval-comm-beta?
Attachment #789858 - Flags: approval-comm-aurora?
Ok, but first you need to get any review+, e.g. from 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?
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.
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.
needinfo re: Gecko 24's imminent ship and c#7 plus approval/review request
Flags: needinfo?(mconley)
Flags: needinfo?(mbanner)
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.
Blocks: 691141
Flags: needinfo?(mconley)
Flags: needinfo?(mbanner)
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.
Attachment #789858 - Flags: review?(mconley) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f2403ad38ac9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 27.0
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.
Attachment #789858 - Flags: approval-comm-release?
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.
Attachment #789858 - Flags: approval-comm-release?
Attachment #789858 - Flags: approval-comm-esr24?
Attachment #789858 - Flags: approval-comm-beta?
Attachment #789858 - Flags: approval-comm-beta+
Attachment #789858 - Flags: approval-comm-aurora?
Attachment #789858 - Flags: approval-comm-aurora+
Attachment #789858 - Flags: approval-comm-esr24? → approval-comm-esr24+
Duplicate of this bug: 933721
You need to log in before you can comment on or make changes to this bug.