Closed Bug 885740 Opened 6 years ago Closed 6 years ago

Account central 2x icons for mac layout

Categories

(Thunderbird :: Theme, defect)

All
macOS
defect
Not set

Tracking

(thunderbird24+ fixed)

RESOLVED FIXED
Thunderbird 25.0
Tracking Status
thunderbird24 + fixed

People

(Reporter: alta88, Assigned: Paenglab)

References

Details

Attachments

(3 files)

a css adjustment is needed for 2x mac icons, after Bug 529131.

richard could you help with this?
To fix this problem you need only change the background-size: 32px 32px; to background-size: 16px 16px; on your newly added icons.

The rule for #CreateAccountChat isn't needed when you are using the same icon as in LoDPI.
(In reply to Richard Marti [:Paenglab] from comment #1)
> To fix this problem you need only change the background-size: 32px 32px; to
> background-size: 16px 16px; on your newly added icons.
> 
> The rule for #CreateAccountChat isn't needed when you are using the same
> icon as in LoDPI.

thanks!  i'll submit the patch here in a bit..
Attached image chat-32.png
32px icon for #CreateAccountChat in HiDPI mode. I can't check if it's looking good because I have no Retina Mac.
hmm, since you have a new chat icon, maybe it's just easier if you could do the patch, with the patch in Bug 529131 applied?

thanks again.

oh, it also occurs to me that due to the feed icon revamp, there's a mismatch with the New Account feed icon and the server icon.  perhaps we could slip in a fix to make that icon match server rather than the new folder icon (consistent with the other account types).
Attached patch patchSplinter Review
Patch setting the correct dimensions for HiDPI icons in Account Central. For Windows and Linux I added a new RSS icon. OS X is using a completely different and I leave it as it is.

Mark, please can you check on OS X if the icons are showing correctly?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #767927 - Flags: ui-review?(mconley)
Attachment #767927 - Flags: review?(mconley)
Attachment #767927 - Flags: feedback?(mbanner)
Comment on attachment 767927 [details] [diff] [review]
patch

Looks good with the patch
Attachment #767927 - Flags: feedback?(mbanner) → feedback+
Comment on attachment 767927 [details] [diff] [review]
patch

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

Yes, this looks much better. Thanks Richard!
Attachment #767927 - Flags: ui-review?(mconley)
Attachment #767927 - Flags: ui-review+
Attachment #767927 - Flags: review?(mconley)
Attachment #767927 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d3fcee9dc98d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
Comment on attachment 767927 [details] [diff] [review]
patch

[Triage Comment]
We definitely want this on 24, thanks.
Attachment #767927 - Flags: approval-comm-aurora+
Comment on attachment 767927 [details] [diff] [review]
patch

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

This patch contains chat/ changes and landed without appropriate review. None of the chat peers were cc'ed on the bug.

It looks to me like this bug was really intended to be a Mac-only Thunderbird-only front-end change, so the new file should have been placed in mail/themes/osx/mail/icons/.

::: chat/themes/jar.mn
@@ +19,5 @@
>  	skin/classic/chat/unknown.png
>  	skin/classic/chat/unknown-16.png
>  	skin/classic/chat/chat-16.png
>  	skin/classic/chat/chat-left-16.png
> +	skin/classic/chat/chat-32.png

You use this file only on Mac, so it shouldn't be packaged on all platforms.

::: mail/themes/osx/mail/accountCentral.css
@@ +229,4 @@
>    }
>  
>    #CreateAccountChat {
> +    background: url("chrome://chat/skin/chat-32.png") no-repeat;

This is really a 16px icon with a higher definition, so IMHO it should be named chat-16@2x.png to be consistent with the other icons here.
Attachment #767927 - Flags: review-
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch chat/ cleanupSplinter Review
I think this is what was intended. If we had discovered earlier I would have just backed out the patch.
Attachment #830993 - Flags: review?(mconley)
Comment on attachment 830993 [details] [diff] [review]
chat/ cleanup

Note: this patch is completely untested.
(In reply to Florian Quèze [:florian] [:flo] from comment #12)

> It looks to me like this bug was really intended to be a Mac-only
> Thunderbird-only front-end change, so the new file should have been placed
> in mail/themes/osx/mail/icons/.

But if I misunderstood and you are really trying to fix chat/ files to look better on retina Macs, we are happy to work with you on it (likely in a different bug though, as this one really seems Thunderbird specific).
Quote from the minutes of last week's Thunderbird status meeting:
"Please request review from Florian (:florian) or Patrick (:clokep) on all changes that touch chat/! If you know you do not need our review (e.g. jcranmer's mass build infrastructure changes are fine), CCing us on the bug would still be appreciated!"

I'm adding a needinfo flag on both Mike and Mark to ensure you have seen this, as you both reviewed/approved the patch without sending us a heads-up.
Flags: needinfo?(mconley)
Flags: needinfo?(mbanner)
Note, if this patch is expected to go forward to branches, please file it on a new bug, to make it easier to track.
Flags: needinfo?(mbanner)
(In reply to Mark Banner (:standard8) from comment #17)
> Note, if this patch is expected to go forward to branches, please file it on
> a new bug, to make it easier to track.

We don't need it on branches, but I would like it to land on central before bug 920801.
(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> Quote from the minutes of last week's Thunderbird status meeting:
> "Please request review from Florian (:florian) or Patrick (:clokep) on all
> changes that touch chat/! If you know you do not need our review (e.g.
> jcranmer's mass build infrastructure changes are fine), CCing us on the bug
> would still be appreciated!"
> 
> I'm adding a needinfo flag on both Mike and Mark to ensure you have seen
> this, as you both reviewed/approved the patch without sending us a heads-up.

Yikes! My apologies - I should have been paying more attention. :(
Flags: needinfo?(mconley)
Comment on attachment 830993 [details] [diff] [review]
chat/ cleanup

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

You'll have to get rid of the chat-32.png reference in chat/themes/jar.mn, but other than that, this works fine. Thanks Florian!
Attachment #830993 - Flags: review?(mconley) → review+
https://hg.mozilla.org/comm-central/rev/39f6af451ff3
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.