Closed Bug 576402 Opened 14 years ago Closed 13 years ago

Add small icons for Address Book toolbar to Classic theme

Categories

(SeaMonkey :: Themes, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.7

People

(Reporter: iannbugzilla, Assigned: stanio)

References

(Blocks 1 open bug)

Details

(Keywords: classic)

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 523274
Attached image new image
Attached patch Preliminary patch (obsolete) — Splinter Review
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.
Attachment #566931 - Flags: review?(philip.chee)
(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 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).
Assignee: nobody → stanio
Status: NEW → ASSIGNED
Attachment #566931 - Flags: review?(philip.chee)
Attached patch Suggested patch (obsolete) — Splinter Review
Adds the necessary (correct I hope) changes to "suite/themes/classic/jar.mn".

Requesting feedback on the Mac changes.
Attachment #566931 - Attachment is obsolete: true
Attachment #566944 - Flags: feedback?(stefanh)
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.
Attachment #566944 - Flags: feedback?(stefanh) → review?(stefanh)
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+)
Attachment #566944 - Flags: review?(stefanh)
Attachment #566944 - Flags: review+
Attachment #566944 - Flags: feedback?(philip.chee)
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.
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.
I've now tried optipng - it says both images in my working copy are already optimized.
(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
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 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 on attachment 566944 [details] [diff] [review]
Suggested patch

Oops forgot to set the flag.
Attachment #566944 - Flags: feedback?(philip.chee) → feedback+
(add the comment to the mac file too, please)
Attached patch Patch v3 (obsolete) — Splinter Review
Updated per comment 8, comment 9, comment 14 and comment 16.
Attachment #566944 - Attachment is obsolete: true
Attachment #568210 - Flags: review?(stanio)
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.)
Attachment #568210 - Flags: review?(stanio) → review+
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.
> 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.
For check-in.
Attachment #568210 - Attachment is obsolete: true
Attachment #568229 - Flags: review?(stanio)
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)
Attachment #568229 - Flags: review?(stanio) → review+
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.
Keywords: checkin-needed
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.
Attachment #568229 - Attachment description: Final patch → Final patch [checked in] r=Stefanh r=Ratty
Sigh, C-C tree is closed.
Keywords: checkin-needed
Attachment #568229 - Attachment description: Final patch [checked in] r=Stefanh r=Ratty → Final patch r=Stefanh f=Ratty
Attachment #568229 - Attachment description: Final patch r=Stefanh f=Ratty → Final patch r=stefanh f=Ratty
(adding back kw)
Keywords: checkin-needed
Pushed to comm-central (CLOSED TREE rs=Callek)
http://hg.mozilla.org/comm-central/rev/50f2f9fc6789
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Attachment #568229 - Attachment description: Final patch r=stefanh f=Ratty → Final patch r=stefanh f=Ratty [checked-in Comment 27)
Attachment #568229 - Flags: feedback+
Target Milestone: --- → seamonkey2.7
Keywords: classic
You need to log in before you can comment on or make changes to this bug.