Add small icons for Address Book toolbar to Classic theme

RESOLVED FIXED in seamonkey2.7

Status

SeaMonkey
Themes
--
enhancement
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ian Neal, Assigned: Stanimir Stamenkov)

Tracking

(Blocks: 1 bug, {classic})

Trunk
seamonkey2.7
classic
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Updated

7 years ago
Blocks: 523274

Comment 1

7 years ago
Created attachment 475516 [details]
new image

Comment 2

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

Comment 3

6 years ago
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.
Attachment #566931 - Flags: review?(philip.chee)
(Reporter)

Comment 4

6 years ago
(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

6 years ago
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).

Updated

6 years ago
Assignee: nobody → stanio
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #566931 - Flags: review?(philip.chee)
(Assignee)

Comment 6

6 years ago
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.
Attachment #566931 - Attachment is obsolete: true
Attachment #566944 - Flags: feedback?(stefanh)

Comment 7

6 years ago
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 8

6 years ago
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)

Comment 9

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

Comment 10

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

Comment 11

6 years ago
I've now tried optipng - it says both images in my working copy are already optimized.

Comment 12

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

Comment 13

6 years ago
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

6 years ago
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

6 years ago
Comment on attachment 566944 [details] [diff] [review]
Suggested patch

Oops forgot to set the flag.
Attachment #566944 - Flags: feedback?(philip.chee) → feedback+

Comment 16

6 years ago
(add the comment to the mac file too, please)
(Assignee)

Comment 17

6 years ago
Created attachment 568210 [details] [diff] [review]
Patch v3

Updated per comment 8, comment 9, comment 14 and comment 16.
Attachment #566944 - Attachment is obsolete: true
Attachment #568210 - Flags: review?(stanio)
(Assignee)

Comment 18

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

Comment 20

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

Comment 21

6 years ago
Created attachment 568229 [details] [diff] [review]
Final patch r=stefanh f=Ratty [checked-in Comment 27)

For check-in.
Attachment #568210 - Attachment is obsolete: true
Attachment #568229 - Flags: review?(stanio)
(Assignee)

Comment 22

6 years ago
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+
(Assignee)

Comment 23

6 years ago
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 24

6 years ago
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.

Updated

6 years ago
Attachment #568229 - Attachment description: Final patch → Final patch [checked in] r=Stefanh r=Ratty

Comment 25

6 years ago
Sigh, C-C tree is closed.
Keywords: checkin-needed

Updated

6 years ago
Attachment #568229 - Attachment description: Final patch [checked in] r=Stefanh r=Ratty → Final patch r=Stefanh f=Ratty

Updated

6 years ago
Attachment #568229 - Attachment description: Final patch r=Stefanh f=Ratty → Final patch r=stefanh f=Ratty

Comment 26

6 years ago
(adding back kw)
Keywords: checkin-needed

Comment 27

6 years ago
Pushed to comm-central (CLOSED TREE rs=Callek)
http://hg.mozilla.org/comm-central/rev/50f2f9fc6789
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Attachment #568229 - Attachment description: Final patch r=stefanh f=Ratty → Final patch r=stefanh f=Ratty [checked-in Comment 27)
Attachment #568229 - Flags: feedback+

Updated

6 years ago
Target Milestone: --- → seamonkey2.7

Updated

6 years ago
Keywords: classic
You need to log in before you can comment on or make changes to this bug.