Last Comment Bug 885740 - Account central 2x icons for mac layout
: Account central 2x icons for mac layout
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Theme (show other bugs)
: unspecified
: All Mac OS X
: -- normal (vote)
: Thunderbird 25.0
Assigned To: Richard Marti (:Paenglab)
:
:
Mentors:
Depends on:
Blocks: 529131
  Show dependency treegraph
 
Reported: 2013-06-21 08:02 PDT by alta88
Modified: 2013-11-27 02:41 PST (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed


Attachments
chat-32.png (503 bytes, image/png)
2013-06-21 13:12 PDT, Richard Marti (:Paenglab)
no flags Details
patch (4.92 KB, patch)
2013-06-26 12:31 PDT, Richard Marti (:Paenglab)
mconley: review+
florian: review-
mconley: ui‑review+
standard8: feedback+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review
chat/ cleanup (2.93 KB, patch)
2013-11-12 11:56 PST, Florian Quèze [:florian] [:flo]
mconley: review+
Details | Diff | Splinter Review

Description alta88 2013-06-21 08:02:19 PDT
a css adjustment is needed for 2x mac icons, after Bug 529131.

richard could you help with this?
Comment 1 Richard Marti (:Paenglab) 2013-06-21 12:24:25 PDT
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.
Comment 2 alta88 2013-06-21 12:33:29 PDT
(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..
Comment 3 Richard Marti (:Paenglab) 2013-06-21 13:12:53 PDT
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.
Comment 4 alta88 2013-06-21 18:03:46 PDT
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.
Comment 5 alta88 2013-06-21 18:07:18 PDT

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).
Comment 6 Richard Marti (:Paenglab) 2013-06-26 12:31:17 PDT
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?
Comment 7 Mark Banner (:standard8, afk until Dec) 2013-07-01 02:25:28 PDT
Comment on attachment 767927 [details] [diff] [review]
patch

Looks good with the patch
Comment 8 Mike Conley (:mconley) 2013-07-19 20:41:57 PDT
Comment on attachment 767927 [details] [diff] [review]
patch

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

Yes, this looks much better. Thanks Richard!
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-07-22 05:49:46 PDT
https://hg.mozilla.org/comm-central/rev/d3fcee9dc98d
Comment 10 Mark Banner (:standard8, afk until Dec) 2013-07-23 04:40:35 PDT
Comment on attachment 767927 [details] [diff] [review]
patch

[Triage Comment]
We definitely want this on 24, thanks.
Comment 11 Mark Banner (:standard8, afk until Dec) 2013-07-23 06:42:01 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/72f71f835916
Comment 12 Florian Quèze [:florian] [:flo] 2013-11-12 11:51:08 PST
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.
Comment 13 Florian Quèze [:florian] [:flo] 2013-11-12 11:56:22 PST
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.
Comment 14 Florian Quèze [:florian] [:flo] 2013-11-12 11:56:56 PST
Comment on attachment 830993 [details] [diff] [review]
chat/ cleanup

Note: this patch is completely untested.
Comment 15 Florian Quèze [:florian] [:flo] 2013-11-12 12:02:45 PST
(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).
Comment 16 Florian Quèze [:florian] [:flo] 2013-11-20 06:05:47 PST
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.
Comment 17 Mark Banner (:standard8, afk until Dec) 2013-11-22 11:08:41 PST
Note, if this patch is expected to go forward to branches, please file it on a new bug, to make it easier to track.
Comment 18 Florian Quèze [:florian] [:flo] 2013-11-22 13:47:31 PST
(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.
Comment 19 Mike Conley (:mconley) 2013-11-25 18:40:36 PST
(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. :(
Comment 20 Mike Conley (:mconley) 2013-11-25 18:47:24 PST
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!
Comment 21 Florian Quèze [:florian] [:flo] 2013-11-27 02:41:11 PST
https://hg.mozilla.org/comm-central/rev/39f6af451ff3

Note You need to log in before you can comment on or make changes to this bug.