Account central 2x icons for mac layout

RESOLVED FIXED in Thunderbird 25.0

Status

Thunderbird
Theme
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: alta88, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 25.0
All
Mac OS X
Bug Flags:
in-testsuite -

Thunderbird Tracking Flags

(thunderbird24+ fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

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

richard could you help with this?
(Assignee)

Comment 1

4 years ago
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.
(Reporter)

Comment 2

4 years ago
(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..
(Assignee)

Comment 3

4 years ago
Created attachment 766044 [details]
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.
(Reporter)

Comment 4

4 years ago
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.
(Reporter)

Comment 5

4 years ago

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).
(Assignee)

Comment 6

4 years ago
Created attachment 767927 [details] [diff] [review]
patch

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+
tracking-thunderbird24: --- → +
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+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d3fcee9dc98d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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+
https://hg.mozilla.org/releases/comm-aurora/rev/72f71f835916
status-thunderbird24: --- → fixed
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 → ---
Created attachment 830993 [details] [diff] [review]
chat/ cleanup

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
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.