483777 - tango icons for message compose window

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Andreas Nilsson (:andreasn)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.7) Gecko/2009030422 Ubuntu/8.10 (intrepid) Firefox/3.0.7 Ubiquity/0.1.5
Build Identifier: 

bug to track the progress of compose window icons

Reproducible: Always

Comment 1

[Andreas Nilsson (:andreasn)]

Attachment: icons and modified css file

Comment 2

[Andreas Nilsson (:andreasn)]

Attachment: removed #button-security

Seems like #button-security have to be removed from msgCompSMIMEOverlay.css in order for the security icon to show.

Comment 3

[Tyler Downer [He/Him]]

Could you please attach those patches as diffs?

Comment 4

[Magnus Melin [:mkmelin]]

Would be glad to help you set up hg to do patches, if you want. (hg patches can include images, so they make it quite easy)

Comment 5

[Andreas Nilsson (:andreasn)]

Attachment: removal of #button-security from msgCompSMIMEOverlay

patch against latest nightly

Comment 6

[Andreas Nilsson (:andreasn)]

Attachment: messengercompose modifications

patch against latest nightly

Comment 7

[Andreas Nilsson (:andreasn)]

Attachment: messenger icons as a separate patch

Comment 8

[Chinmay Patel]

In Reply to Comment #4:

I am planning to make an HG patch, which will include images as well.

Comment 9

[Chinmay Patel]

Attachment: HG patch including both files and images

I have tried the patch and it worked pretty well. I just have to edit the mn.jar file in the gnomestripe theme. 

Moreover, I have moved all the icons in the icon folder. For moving that, I need to make some changes in the patch. 

Here is the final patch with all the changes.

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 369125 [details] [diff] [review]
HG patch including both files and images

make sure to do an 'hg add $image' and then recreate the patch.  right now the images aren't included.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

that is run 'hg add $image' for each $image that should be included in the patch

Comment 12

[Andreas Nilsson (:andreasn)]

Attachment: updated icons including smaller sizes and disabled states

Missed the small toolbar sizes in my previous batch.

Comment 13

[Andreas Nilsson (:andreasn)]

Attachment: updated css, refferring to the newer icons

Comment 14

[Chinmay Patel]

Attachment: PATCH 2: Including small images

In this patch, I have included small images as well.


Note: I just want to make sure that I am on the correct path. To create the patch (patch that includes images), I have followed this procedure. Let me know, if I am mistaken anywhere.

Step 1: Move all the images to the required folder. In our case move to "mail/themes/gnomestripe/mail/icons/"

Step 2: Apply the necessary changes. In our case, replace the messengercompose.css file with the one provided by Andreas. Moreover, Remove the #button-security part from msgCompSMIMEOverlay.css.

Step 3: Build with all the changes. Check if, it works.

Step 4: Add all images in HG by "hg add <imageFile>" for each image file. In our case, there are 31 image files.

Step 5: create a patch using "hg diff -p -U 8 mail > <patchname>"



step 6: Upload the patch.

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

I'm not sure why, but your patch still doesn't seem to have the images inline.  Anyway, it's all the heavy lifting and looks good so I'll see if there are any cleanups I can do.  Then we'll push this through for review.

In the meantime I have the following in my ~/.hgrc file, perhaps you want to add this as well, I think it will make your patching easier.

[diff]
git = true

[defaults]
diff=-p -U 8

Comment 16

[Chinmay Patel]

Attachment: With the images in BInary

Hey Bryan,

I guess, that's what you needed exactly?

My .hgrc file was not configured properly. Thanks for your ~/.hgrs file configuration.

Let me know, if anything else is remaining.

Comment 17

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 370686 [details] [diff] [review]
With the images in BInary

Awesome, thanks!

Everything looks good to me, the only thing I'd change is these two gtk-save-as to gtk-save

> #button-save {
>-  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar.png");
>-  -moz-image-region: rect(0px 144px 24px 120px);
>+  list-style-image: url("moz-icon://stock/gtk-save-as?size=toolbar");
> }
> 
>-#button-save:hover {
>-  -moz-image-region: rect(24px 144px 48px 120px);
>-} 
>-
> #button-save[disabled="true"] {
>-  -moz-image-region: rect(48px 144px 72px 120px) !important;
>-} 
>+  list-style-image: url("moz-icon://stock/gtk-save-as?size=toolbar&state=disabled");
>+}

If you can just up a new patch with those changes we can move this over to a code review.  (where things get really tough! :)

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 369688 [details] [diff] [review]
PATCH 2: Including small images

just removing this old one

Comment 19

[Andreas Nilsson (:andreasn)]

clarkbw: agreed, it should be gtk-save (that it seems I correctly set the smaller size to). Sorry for the mess.

Comment 20

[Chinmay Patel]

Attachment: "save-as" to only "save"

It's done.
Whom should I ask for "review"?

Comment 21

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 370760 [details] [diff] [review]
"save-as" to only "save"

I think we'll put magnus on the review for this since he's back in action now and I'm waiting on philor for the addressbook changes.

However, before we call in the big guns... I just noticed a section of code where you comment out an area.

+/*
 #FormatToolbar > toolbarbutton {
   list-style-image: url("chrome://messenger/skin/messengercompose/format-buttons.png");
 }
+*/

I'd suggest removing that selector (and the commenting) all together.  

Also you'll want to remove the image it's referring to as well.

I usually do a search using the mxr tool to make sure that nothing else is referring to the image (at least inside the theme for this case).

http://mxr.mozilla.org/comm-central/search?find=%2Fmail%2Fthemes%2Fgnomestripe%2F&string=format-buttons.png

So you can see from that search that the jar.nm file still refers to this image.  You'll want to remove that reference in the jar.nm file.

Then inside the icons directory do an 'hg remove format-buttons.png'.  That will tell mercurial to remove the icon file and it should be included in your next patch with the way you're building your diffs.

One more patch and I think you're ready for the code review!  You can put magnus' email in the review (?|v) [mkmelin...] for the next patch.

Comment 22

[Chinmay Patel]

Attachment: format-buttons.png , compose-toolbar.png and compose-toolbar-small.png removed.

In this patch, I have removed format-buttons, compose-toolbar.png and compose-toolbar-small.png images.

I serached in mxr and found that I don't need these 3 images at all. I have also removed relevant code.

I haven't asked for review for this patch yet because I thought that it would be better if Bryan Clark looks at this code before asking for review.

Comment 23

[Bryan Clark (DevTools PM) [:clarkbw]]

Awesome work. Thanks Chinmay!

(In reply to comment #13)
> Created an attachment (id=369526) [details]
> updated css, refferring to the newer icons

Andreas, do you have a compose-send-small-disabled.png ? 

The selector still refers to the compose-toolbar-small here, other than that I think Chinmay's patch is ready.

 toolbar[iconsize="small"] #button-send[disabled] {
   -moz-image-region: rect(32px 16px 48px 0px) !important;
 }

Comment 24

[Andreas Nilsson (:andreasn)]

Attachment: compose-send-small-disabled

clarkbw: here you are!

Comment 25

[Chinmay Patel]

Attachment: Compose-send-small-disabled.png added.

Compose-send-small-disabled.png is added in this patch.

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 371708 [details] [diff] [review]
Compose-send-small-disabled.png added.

Looks very tango, thx!

I'm not sure why you moved #button-security out of msgCompSMIMEOverlay.css. It should stay there, and the css seemed to work equally well from there, from a quick test.

>-#spellingButton:hover {
>-  -moz-image-region: rect(24px 72px 48px 48px);
>+  list-style-image: url("moz-icon://stock/gtk-spell-check?size=toolbar");
> }
> 
> #spellingButton[disabled="true"] {
>-  -moz-image-region: rect(48px 72px 72px 48px) !important;
>+  list-style-image: url("moz-icon://stock/tools-check-spelling?size=toolbar&state=disabled");
> }

Hm, this is a bit odd. Why isn't the disabled spelling button also gtk-spell-check?
(BTW, can't find any documentation of a tools-check-spelling though it looks like it works.)

>+#button-security [disabled="true"]{
>+  list-style-image: url("chrome://messenger/skin/icons/compose-security-disabled.png"); 
>+}

Should be #button-security[disabled="true"] {
There is also trailing space there.

> #AlignPopupButton {
>-  -moz-image-region: rect(0px 176px 16px 160px);
>+  list-style-image: url("moz-icon://stock/gtk-justify-left?size=menu");
> }
> #AlignPopupButton[disabled="true"] {
>-  -moz-image-region: rect(32px 176px 48px 160px) !important;
>+  list-style-image: url("moz-icon://stock/gtk-justify-left?size=menu&state=disabled");
> }

How about gtk-justify-center for this?
That's more how it used to look; and a bit more neutral thinking of our ltr locales.

There are trailing spaces and tabs in a few places, please remove those.

The one bigger thing to change is to fold the single images into single larger files, like it used to be - e.g. compose-toolbar.png, compose-toolbar-small.png etc. and use -moz-imgage-region. Just for performance. In bug 483761 there's a script that should help you do that easier.

Comment 27

[Chinmay Patel]

Attachment: All the icons are merged

Task Done:
> Merging all the images into 3 images.
> I have left "gtk-justify-left" as it is and I am asking UI person, clarkbw, to come up with the solution.
Let me know, if I am missing anything! 

Thank you very much for providing scripts to merge images. It saves a lot of time.

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 373585 [details] [diff] [review]
All the icons are merged

The patch doesn't apply cleanly to hg tip 
(--fuzz=5 helps for patch though...)

> +#button-security [disabled="true"]{

No space in between please.

> +  //list-style-image: url("chrome://messenger/skin/messengercompose/format-buttons.png");

Remove these comments. Or, well, they aren't even css comments.

You can keep the comments the script outputs.

Comment 29

[Chinmay Patel]

Attachment: Revised

>The patch doesn't apply cleanly to hg tip 
>(--fuzz=5 helps for patch though...)

I have checked the patch by applying to a fresh repository. For applying the patch I use "hg import --no-commit -p1 FILENAME". Let me know, if this is not the correct method.


> +#button-security [disabled="true"]{
>No space in between please.

#button-security[disabled="true"] {

> //list-style-image: url("chrome://messenger/skin/messengercompose/format-buttons.png");
> Remove these comments. Or, well, they aren't even css comments.

All the comments are removed.


Moreover, I just realized that my first patch was not compatible with the icon size. It was having 16x16 for regular icons' size, which should be 22x22. I have fixed that in this patch. 

Let me know, if anything else is remaining...

Comment 30

[Magnus Melin [:mkmelin]]

Hm, this is odd. I'm not sure why this doesn't apply but this is what I get.

hg import --no-commit /opt/review/attachment374006 [details] [diff] [review].patch
applying /opt/review/attachment374006 [details] [diff] [review].patch
: No such file or directoryl/compose/compose-toolbar-small.png
: No such file or directoryl/compose/compose-toolbar.png
: No such file or directoryl/compose/format-buttons.png
: No such file or directoryl/compose/messengercompose.css
: No such file or directoryl/smime/msgCompSMIMEOverlay.css
** unknown exception encountered, details follow
** report bug details to http://www.selenic.com/mercurial/bts
** or [e-mail]
** Mercurial Distributed SCM (version 1.0.1)
Traceback (most recent call last):
  File "/usr/bin/hg", line 20, in <module>
    mercurial.dispatch.run()
  File "/var/lib/python-support/python2.5/mercurial/dispatch.py", line 20, in run
    sys.exit(dispatch(sys.argv[1:]))
  File "/var/lib/python-support/python2.5/mercurial/dispatch.py", line 29, in dispatch
    return _runcatch(u, args)
  File "/var/lib/python-support/python2.5/mercurial/dispatch.py", line 45, in _runcatch
    return _dispatch(ui, args)
  File "/var/lib/python-support/python2.5/mercurial/dispatch.py", line 364, in _dispatch
    ret = _runcommand(ui, options, cmd, d)
  File "/var/lib/python-support/python2.5/mercurial/dispatch.py", line 417, in _runcommand
    return checkargs()
  File "/var/lib/python-support/python2.5/mercurial/dispatch.py", line 373, in checkargs
    return cmdfunc()
  File "/var/lib/python-support/python2.5/mercurial/dispatch.py", line 356, in <lambda>
    d = lambda: func(ui, repo, *args, **cmdoptions)
  File "/var/lib/python-support/python2.5/mercurial/commands.py", line 1558, in import_
    files=files)
  File "/var/lib/python-support/python2.5/mercurial/patch.py", line 229, in patch
    return internalpatch(patchname, ui, strip, cwd, files)
  File "/var/lib/python-support/python2.5/mercurial/patch.py", line 292, in internalpatch
    ret = applydiff(ui, fp, files, strip=strip)
  File "/var/lib/python-support/python2.5/mercurial/patch.py", line 1015, in applydiff
    for state, values in iterhunks(ui, fp, sourcefile):
  File "/var/lib/python-support/python2.5/mercurial/patch.py", line 936, in iterhunks
    current_hunk.extract(fp)
  File "/var/lib/python-support/python2.5/mercurial/patch.py", line 768, in extract
    dec.append(base85.b85decode(line[1:-1])[:l])
ValueError: Bad base85 character at position 66

Comment 31

[Chinmay Patel]

I am not even sure why it is not working. However, you have tried with "hg import --no-commit /opt/review/attachment374006 [details] [diff] [review].patch" in which you are missing "-p1". 

Would you mind to try with "-p1" option and let me know how it goes. It will be something like 

hg import --no-commit -p1 FILENAME (notice -p1)

If it doesn't work, let me know, what command do you use to generate patches. I will try that one to create patches. 


PS: I use " hg diff -p -U 8 mail > FILENAME " to create patches.

Comment 32

[Magnus Melin [:mkmelin]]

Ok, I'll try that later.

BTW, to make it easier, in your .hgrc you can put 

[defaults]
diff =-p -U 9

After that, generate patches as easily as hg diff > bugXXX_foo.patch

Comment 33

[Magnus Melin [:mkmelin]]

Nope, doesn't work with hg import --no-commit -p1 either

I notice patch complains about trailing CRs, don't know if it's related.

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 374006 [details] [diff] [review]
Revised

Cancelling r for now, I hear Bryan couldn't get the patch to apply either.

Comment 35

[Bryan Clark (DevTools PM) [:clarkbw]]

Just tested out the latest version of this patch from Chinmay via IRC and it looks good to me so far except that it's missing a couple icons from the Insert Menu.

Andreas: do you have tango versions of these icons?

The ones that appear to be missing are the 5 icons starting from the right of the last align type icon.

http://mxr.mozilla.org/comm-central/source/mail/themes/gnomestripe/mail/compose/format-buttons.png

These icons are used in following CSS rule and should like be also used in the menu bar menus.

/* ..... insert menu ..... */

#InsertPopup > menuitem {
  list-style-image: url("chrome://messenger/skin/messengercompose/format-buttons.png");
}

#InsertLinkItem {
  -moz-image-region: rect(0px 224px 16px 208px);
}

#InsertAnchorItem {
  -moz-image-region: rect(0px 240px 16px 224px);
}

#InsertImageItem {
  -moz-image-region: rect(0px 256px 16px 240px);
}

#InsertHRuleItem {
  -moz-image-region: rect(0px 272px 16px 256px);
}

#InsertTableItem {
  -moz-image-region: rect(0px 288px 16px 272px);
}

Comment 36

[Chinmay Patel]

Attachment: [checked in] Appllicable Patch (part1)

Here is the patch that Bryan mentioned in Comment#35. 

I also look forward to add those five remaining icons in the tree, if the icons are provided.

Comment 37

[Andreas Nilsson (:andreasn)]

Attachment: fixed format buttons

here they are!

Comment 38

[Magnus Melin [:mkmelin]]

Comment on attachment 375387 [details] [diff] [review]
[checked in] Appllicable Patch (part1)

This part looks good, thx!
I've checked it in for you

changeset:   2546:d19ebdd62dbf
http://hg.mozilla.org/comm-central/rev/d19ebdd62dbf

Comment 39

[Chinmay Patel]

Attachment: Adding Format Buttons

Thank you very much Magnus, Bryan, and Andreas!

Here is a patch that includes more format buttons provided by Andreas.

Comment 40

[Chinmay Patel]

Attachment: Format Button preview

This is how last patch looks like.

Comment 41

[Stefan Plewako [:stef]]

Please change the send icon, use sth more common (well-defined, distinct, explicit, clear, ..) instead the sent folder sign.

Spellcheck and quote icons should be improved also, since they are not really legible.

Comment 42

[Magnus Melin [:mkmelin]]

Comment on attachment 376109 [details] [diff] [review]
Adding Format Buttons


> #AlignLeftItem {
>-  -moz-image-region: rect(0px 160px 16px 144px);
>+  list-style-image: url("moz-icon://stock/gtk-justify-left?size=menu") !important;
> }

These importants aren't needed. 

>-
>+c
> #InsertImageItem {

Hm :/

I'll fix those and check this in for you. r=mkmelin

Comment 43

[Magnus Melin [:mkmelin]]

Attachment: [checked in] Adding Format Buttons (as checked in)

changeset:   2594:8cd0ec2d6de2
http://hg.mozilla.org/comm-central/rev/8cd0ec2d6de2

Comment 44

[Magnus Melin [:mkmelin]]

Re comment 41: Stefan, spellcheck uses the gtk stock icon for spelling. If it looks bad for you, try changing os theme...