Closed Bug 1317142 Opened 8 years ago Closed 8 years ago

Remove duplicate files from mail

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(thunderbird51 wontfix, thunderbird52 fixed, thunderbird53 fixed)

RESOLVED FIXED
Thunderbird 53.0
Tracking Status
thunderbird51 --- wontfix
thunderbird52 --- fixed
thunderbird53 --- fixed

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(2 files, 3 obsolete files)

In mail/installer/allowed-dupes.mn there are a lot of files we can dedupe.
Attached patch unDupeMail.patch (obsolete) — Splinter Review
This patch removes almost every duplicate image. Only the new-mail-alert.png, and their duplicates are left because it would need code changes that aren't trivial.

To deduplicate them I either removed one and point to the other in CSS ot I made one different through optimize or changed some pixels to make them different when the first solution doesn't work.

Aleth, I changed  also chat files to not pack files into TB (loading.png, loading@2x.png, chat/themes/jar.mn and chat/themes/status.css). Is this okay for you?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8810176 - Flags: review?(aleth)
Attached patch unDupeMail.patch (obsolete) — Splinter Review
oops, this wasn't the last version.
Attachment #8810176 - Attachment is obsolete: true
Attachment #8810176 - Flags: review?(aleth)
Attachment #8810177 - Flags: review?(aleth)
Comment on attachment 8810177 [details] [diff] [review]
unDupeMail.patch

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

::: chat/themes/jar.mn
@@ +29,5 @@
>  	skin/classic/chat/prpl-generic/icon.png		(icons/prpl-generic.png)
>  	skin/classic/chat/prpl-unknown/icon32.png	(icons/prpl-unknown-32.png)
>  	skin/classic/chat/prpl-unknown/icon48.png	(icons/prpl-unknown-48.png)
>  	skin/classic/chat/prpl-unknown/icon.png		(icons/prpl-unknown.png)
>  	skin/classic/chat/icons/secure.png		(icons/secure.png)

What did you change to undupe secure/insecure.png?

@@ +31,5 @@
>  	skin/classic/chat/prpl-unknown/icon48.png	(icons/prpl-unknown-48.png)
>  	skin/classic/chat/prpl-unknown/icon.png		(icons/prpl-unknown.png)
>  	skin/classic/chat/icons/secure.png		(icons/secure.png)
>  	skin/classic/chat/icons/insecure.png		(icons/insecure.png)
> +#ifndef MOZ_THUNDERBIRD

These loading icons are duplicates of files in toolkit. So I would prefer if instead of adding lots of ifdefs everywhere you simply point to the toolkit original file here and delete the copy in chat/. That should solve the problem cleanly for TB and IB.
Attachment #8810177 - Flags: review?(aleth) → review-
Attached patch unDupeMail.patch take 2 (obsolete) — Splinter Review
Removed all loading images and pointing to global now. Secure/insecure.png are now packed for TB from messenger. IB uses still the chat icons.
Attachment #8810177 - Attachment is obsolete: true
Attachment #8810206 - Flags: review?(aleth)
Comment on attachment 8810206 [details] [diff] [review]
unDupeMail.patch take 2

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

Modified Binary File: mail/themes/linux/mail/userIcon.png
Modified Binary File: mail/themes/osx/mail/accountcentral/manage-rss.png
Modified Binary File: mail/themes/osx/mail/accountcentral/manage-rss@2x.png
Modified Binary File: mail/themes/osx/mail/activity/defaultWarningIcon.png
Modified Binary File: mail/themes/osx/mail/icons/insecure.png
...

What are all these modifications? Did you change the pngs?
Like I wrote in comment 1, either by optimizing (shrinking in byte size) the images or changing some pixels transparency or color. The manage-rss.png images are new because they were the same as the newsgroup icons.
Comment on attachment 8810206 [details] [diff] [review]
unDupeMail.patch take 2

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

(In reply to Richard Marti (:Paenglab) from comment #6)
> Like I wrote in comment 1, either by optimizing (shrinking in byte size) the
> images or changing some pixels transparency or color. The manage-rss.png
> images are new because they were the same as the newsgroup icons.

OK, so you basically chose to keep the duplication - just checking I understood correctly.

Did you consider using the jar.mn to point all the chrome:// urls at the same file (i.e. keep one copy)? (Note you can use ../ in the path to get to any directory you like)

Not saying you have to do this, just asking if you knew about that possibility.
(In reply to aleth [:aleth] from comment #7)
> Comment on attachment 8810206 [details] [diff] [review]
> unDupeMail.patch take 2
> 
> Review of attachment 8810206 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, so you basically chose to keep the duplication - just checking I
> understood correctly.

Basically yes, but now they are bitwise different. And this could make it easier for the future to change them without need to look at the other, before duplicated, files.

> Did you consider using the jar.mn to point all the chrome:// urls at the
> same file (i.e. keep one copy)? (Note you can use ../ in the path to get to
> any directory you like)
> 
> Not saying you have to do this, just asking if you knew about that
> possibility.

I knew about this. But as I understand the dupe check checks the target files and when they are from the same source file they have to be treated as dupes in allowed-dupes.mn.
(In reply to Richard Marti (:Paenglab) from comment #8)
> (In reply to aleth [:aleth] from comment #7)
> I knew about this. But as I understand the dupe check checks the target
> files and when they are from the same source file they have to be treated as
> dupes in allowed-dupes.mn.

Ah, you're right. The packager creates duplicates during packaging if the target path differs. :-(

I think I would prefer if you kept the duplicate files in allowed-dupes rather than introducing invisible differences which will be confusing in the future, eg. when updating the icons. 

Alternatively I would suggest changing the chrome:// url path and filename to something which works for all use cases (maybe even moving the icon file to a new shared location if you like) and then using that to get rid of the duplication, and/or moving the OS-dependent but identical files like skin/classic/messenger/userIcon.png from an OS-dependent jar.mn to a shared jar.mn
Comment on attachment 8810206 [details] [diff] [review]
unDupeMail.patch take 2

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

(In reply to Richard Marti (:Paenglab) from comment #6)
> Like I wrote in comment 1, either by optimizing (shrinking in byte size) the
> images or changing some pixels transparency or color. The manage-rss.png
> images are new because they were the same as the newsgroup icons.

OK, so you basically chose to keep the duplication - just checking I understood correctly.

Did you consider using the jar.mn to point all the chrome:// urls at the same file (i.e. keep one copy)? (Note you can use ../ in the path to get to any directory you like)

Not saying you have to do this, just asking if you knew about that possibility.
Attachment #8810206 - Flags: review?(aleth) → review-
Okay, now no optimized PNGs to dedupe but only to shrink. I removed now the buddy_icon.png and use an override from userIcon.png. With this only one image is needed. For other image I point to global. The loading.png images where already removed in the previous patch.
Attachment #8810206 - Attachment is obsolete: true
Attachment #8810524 - Flags: review?(aleth)
Comment on attachment 8810524 [details] [diff] [review]
unDupeMail.patch take 3

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

Thanks for the cleanup!
Attachment #8810524 - Flags: review?(aleth) → review+
https://hg.mozilla.org/comm-central/rev/e56ac840cd40af51556ded30f931ae460cb0c9d7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
Attachment #8810980 - Flags: review+
Comment on attachment 8810980 [details] [diff] [review]
unDupeMail-Aurora.patch

[Approval Request Comment]
User impact if declined: no impact, but I'd like to have the files in 52 as long as possible in sync with c-c to have the easier possibility to uplift future changes without special treatment.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): hard to say, it builds on c-c and until now are no regressions known. ;-)
Attachment #8810980 - Flags: approval-comm-aurora?
Attachment #8810980 - Flags: approval-comm-aurora? → approval-comm-aurora+
Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/92908ca9a5c0f7bf5442c17d1f1ba4bcf804a385

Thanks for the separate Aurora patch. By the looks of it it already contained the bustage-fix from comment #14 and prepending "distribution/" to the calendar files.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: