Closed
Bug 1317142
Opened 8 years ago
Closed 8 years ago
Remove duplicate files from mail
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(thunderbird51 wontfix, thunderbird52 fixed, thunderbird53 fixed)
RESOLVED
FIXED
Thunderbird 53.0
People
(Reporter: Paenglab, Assigned: Paenglab)
Details
Attachments
(2 files, 3 obsolete files)
73.99 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
75.99 KB,
patch
|
Paenglab
:
review+
jorgk-bmo
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
In mail/installer/allowed-dupes.mn there are a lot of files we can dedupe.
Assignee | ||
Comment 1•8 years ago
|
||
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 | ||
Comment 2•8 years ago
|
||
oops, this wasn't the last version.
Attachment #8810176 -
Attachment is obsolete: true
Attachment #8810176 -
Flags: review?(aleth)
Attachment #8810177 -
Flags: review?(aleth)
Comment 3•8 years ago
|
||
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-
Assignee | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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?
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
(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.
Comment 9•8 years ago
|
||
(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 10•8 years ago
|
||
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-
Assignee | ||
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Assignee | ||
Comment 13•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/e56ac840cd40af51556ded30f931ae460cb0c9d7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
Assignee | ||
Comment 14•8 years ago
|
||
Bustage fix: https://hg.mozilla.org/comm-central/rev/2a2eeac814eed44d5be13fe3c7e868b623b35aa8
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8810980 -
Flags: review+
Assignee | ||
Comment 16•8 years ago
|
||
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?
Updated•8 years ago
|
Attachment #8810980 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Comment 17•8 years ago
|
||
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.
Updated•8 years ago
|
status-thunderbird51:
--- → wontfix
status-thunderbird52:
--- → fixed
status-thunderbird53:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•