Last Comment Bug 576402 - Add small icons for Address Book toolbar to Classic theme
: Add small icons for Address Book toolbar to Classic theme
Status: RESOLVED FIXED
: classic
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.7
Assigned To: Stanimir Stamenkov
:
Mentors:
Depends on: 575956
Blocks: 523274
  Show dependency treegraph
 
Reported: 2010-07-01 16:02 PDT by Ian Neal
Modified: 2011-10-23 12:07 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
new image (16.54 KB, image/png)
2010-09-15 08:06 PDT, tymerkaev
no flags Details
Preliminary patch (43.53 KB, patch)
2011-10-13 13:57 PDT, Stanimir Stamenkov
no flags Details | Diff | Review
Suggested patch (47.27 KB, patch)
2011-10-13 14:42 PDT, Stanimir Stamenkov
stefanh: review+
philip.chee: feedback+
Details | Diff | Review
Patch v3 (48.33 KB, patch)
2011-10-19 14:33 PDT, Stanimir Stamenkov
stanio: review+
Details | Diff | Review
Final patch r=stefanh f=Ratty [checked-in Comment 27) (48.34 KB, patch)
2011-10-19 15:38 PDT, Stanimir Stamenkov
stanio: review+
philip.chee: feedback+
Details | Diff | Review

Description Ian Neal 2010-07-01 16:02:38 PDT
Once the fix to bug 575956 lands it would be good to have small icons for the Address Book toolbar in the Classic theme. A suitable image resizing tool on the existing large icons should do.
Comment 1 tymerkaev 2010-09-15 08:06:55 PDT
Created attachment 475516 [details]
new image
Comment 2 Stefan [:stefanh] 2010-09-25 09:24:34 PDT
(In reply to comment #1)
> Created attachment 475516 [details]
> new image

There's a mac image as well at http://mxr.mozilla.org/comm-central/source/suite/themes/classic/mac/messenger/addressbook/icons/
Comment 3 Stanimir Stamenkov 2011-10-13 13:57:01 PDT
Created attachment 566931 [details] [diff] [review]
Preliminary patch

I don't have a Mac to try the relevant changes, but while editing the Mac addressbook.css I've noticed an odd selector which may be an error:

#button-newim:hover[disabled] {

I've let myself changing it to:

#button-newim[disabled] {

... still I may be in error.
Comment 4 Ian Neal 2011-10-13 14:28:06 PDT
(In reply to Stanimir Stamenkov from comment #3)
> Created attachment 566931 [details] [diff] [review] [diff] [details] [review]
> Preliminary patch
> 
> I don't have a Mac to try the relevant changes, but while editing the Mac
> addressbook.css I've noticed an odd selector which may be an error:
> 
> #button-newim:hover[disabled] {
> 
> I've let myself changing it to:
> 
> #button-newim[disabled] {
> 
> ... still I may be in error.

Best to ask stefanh or mnyromyr for feedback / ui-review
Comment 5 Stefan [:stefanh] 2011-10-13 14:29:24 PDT
Comment on attachment 566931 [details] [diff] [review]
Preliminary patch

Thanks for doing this :-). I'll be happy to review the mac part. I haven't tested the patch, but it looks like you're missing the jar.mn changes - see http://mxr.mozilla.org/comm-central/source/suite/themes/classic/jar.mn#224 for how the large icon set is packaged for mac (windows and linux part is further down the file).
Comment 6 Stanimir Stamenkov 2011-10-13 14:42:01 PDT
Created attachment 566944 [details] [diff] [review]
Suggested patch

Adds the necessary (correct I hope) changes to "suite/themes/classic/jar.mn".

Requesting feedback on the Mac changes.
Comment 7 Stefan [:stefanh] 2011-10-14 13:13:27 PDT
Comment on attachment 566944 [details] [diff] [review]
Suggested patch

I can do the whole review here, but I'll need someone f+:ing for the win/nix part.
Comment 8 Stefan [:stefanh] 2011-10-14 13:55:14 PDT
Comment on attachment 566944 [details] [diff] [review]
Suggested patch

> I don't have a Mac to try the relevant changes, but while editing the Mac
> addressbook.css I've noticed an odd selector which may be an error:
> 
> #button-newim:hover[disabled] {
> 
> I've let myself changing it to:
> 
> #button-newim[disabled] {
> 
> ... still I may be in error.

Yeah, the #button-newim:hover[disabled] is wrong. In fact, none of the  #id[disabled] rules works, since they lack !important (#id:hover:active is more specific than #id[foo]). This is the only mac file that lacks !important overrides for disabled buttons. We're lucky that these buttons are not disabled normally... In general, !important should be avoided and  it's possible we might want to change all files in the future to something in line with "#id:hover:active:not([disabled])", which would make it possible to avoid !important on the #id[disabled] rules. That's a separate discussion, though.

So for now, please add an !important (like in the windows file) for all #id[disabled] rules. r=me with that fixed (you can just put up a new patch with that fixed once Philip have given you f+)
Comment 9 Stefan [:stefanh] 2011-10-14 15:14:50 PDT
btw, if you have pngcrush ('pngcrush -rem gAMA -rem cHRM -rem iCCP -rem sRGB -brute') or optipng ('optipng -nx -o7'), you should be able to reduce the size of the .png files a bit. I think you only win a few percent, though.
Comment 10 Stanimir Stamenkov 2011-10-15 08:38:31 PDT
I've actually used pngcrush on the images but haven't specified the -brute option.  With it the Mac image got about 600 bytes less.  There was no change for the other image.  I'll include the updated image in the next patch version (along with the !important additions), once we get feedback from Philip.
Comment 11 Stanimir Stamenkov 2011-10-15 09:05:31 PDT
I've now tried optipng - it says both images in my working copy are already optimized.
Comment 12 Stefan [:stefanh] 2011-10-15 16:37:46 PDT
(In reply to Stanimir Stamenkov from comment #10)
> I've actually used pngcrush on the images but haven't specified the -brute
> option.  With it the Mac image got about 600 bytes less.  There was no
> change for the other image.  I'll include the updated image in the next
> patch version (along with the !important additions), once we get feedback
> from Philip.

Ah, that's excellent :-) The resaon I mentioned it is mainly because I like to keep our standard to what was done in http://hg.mozilla.org/comm-central/rev/2be65fe3798c
Comment 13 Stanimir Stamenkov 2011-10-16 01:14:52 PDT
Just for info, I had been previously removing chunks like gAMA and sRGB manually using a Windows utility - TweakPNG.  I'll now know I could do this in one step using pngcrush.
Comment 14 Philip Chee 2011-10-19 09:51:42 PDT
Comment on attachment 566944 [details] [diff] [review]
Suggested patch

Tested on Windows only.

> --- a/suite/themes/classic/messenger/addressbook/addressbook.css
> +++ b/suite/themes/classic/messenger/addressbook/addressbook.css

>  #button-abdelete[disabled] {
>    -moz-image-region: rect(0 119px 29px 90px) !important;
>  }

For consistency with the other .CSS files please add a comment like this here:

/* ::::: small primary toolbar buttons ::::: */

> +toolbar[iconsize="small"] > #button-newcard,
> +toolbar[iconsize="small"] > toolbarpaletteitem > #button-newcard {
> +  list-style-image: url("chrome://messenger/skin/addressbook/icons/addressbookicons-small.png");
> +  -moz-image-region: rect(40px 19px 59px 0);
> +}

r=me with that change.
Comment 15 Philip Chee 2011-10-19 09:53:12 PDT
Comment on attachment 566944 [details] [diff] [review]
Suggested patch

Oops forgot to set the flag.
Comment 16 Stefan [:stefanh] 2011-10-19 12:08:50 PDT
(add the comment to the mac file too, please)
Comment 17 Stanimir Stamenkov 2011-10-19 14:33:24 PDT
Created attachment 568210 [details] [diff] [review]
Patch v3

Updated per comment 8, comment 9, comment 14 and comment 16.
Comment 18 Stanimir Stamenkov 2011-10-19 14:36:55 PDT
Comment on attachment 568210 [details] [diff] [review]
Patch v3

r+ per comment 8 and comment 14.

Should I ask for superreview or checkin now? (I'm not sure how is the later done.)
Comment 19 Tony Mechelynck [:tonymec] 2011-10-19 15:06:09 PDT
In reply to comment #18:
I don't know if superreview is required in this case.
To have the patch checked in (assuming you don't have permission to push to the repository), just make sure that all patches (if any) which must _not_ be checked-in are marked obsolete, then set the "checkin-needed" keyword on the bug. Someone will come along and check it in. Also it is "good practice" to mention in the patch title who actually gave r+ if it isn't who Bugzilla says that it was.
Comment 20 Stanimir Stamenkov 2011-10-19 15:30:36 PDT
> Also it is "good practice"
> to mention in the patch title who actually gave r+ if it isn't who Bugzilla
> says that it was.

Aha, o.k.  I thought the person who's checking the patch in modifies the comment explicitly.  Should I mention both of Stefan and Philip in the title line of the comment like:

Bug 576402 - Add small icons for Address Book toolbar to Classic theme. r=Stefanh, r=Ratty

?  If no super-review is required I'll just attach a patch with an updated comment (as above) and set the "checkin-needed" keyword.
Comment 21 Stanimir Stamenkov 2011-10-19 15:38:31 PDT
Created attachment 568229 [details] [diff] [review]
Final patch r=stefanh f=Ratty [checked-in Comment 27)

For check-in.
Comment 22 Stanimir Stamenkov 2011-10-19 15:40:21 PDT
Comment on attachment 568229 [details] [diff] [review]
Final patch r=stefanh f=Ratty [checked-in Comment 27)

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

r=Stefanh (comment 8), r=Ratty (comment 14)
Comment 23 Stanimir Stamenkov 2011-10-19 15:43:48 PDT
Requesting check-in of patch in attachment 568229 [details] [diff] [review].

Please discard this request and remove the "checkin-needed" keyword if the patch further needs a super-review.
Comment 24 Stefan [:stefanh] 2011-10-20 01:31:55 PDT
Comment on attachment 568229 [details] [diff] [review]
Final patch r=stefanh f=Ratty [checked-in Comment 27)

# Parent  d18a04c66ee8cdf26b65c2115fa07789d845b742
Bug 576402 - Add small icons for Address Book toolbar to Classic theme. r=Stefanh, r=Ratty

+ Add !important to #id[disabled] rule declarations for Mac.

The person who lands this should probably fix up the commit message.
Comment 25 Philip Chee 2011-10-20 12:10:51 PDT
Sigh, C-C tree is closed.
Comment 26 Stefan [:stefanh] 2011-10-20 13:18:25 PDT
(adding back kw)
Comment 27 Philip Chee 2011-10-23 10:51:29 PDT
Pushed to comm-central (CLOSED TREE rs=Callek)
http://hg.mozilla.org/comm-central/rev/50f2f9fc6789

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