468751 - Vista Options icons and separate aero theme space

Status: VERIFIED FIXED

(Thunderbird :: Preferences, enhancement)

Description

[Tyler Downer [He/Him]]

The UI of the Thunderbird Options window is too thrown together feeling. In Firefox trunk, the options window uses a white  taskbar with new icons. i feel we should move that to the Thunderbird, with a few chages not all the icons could be reused, etc.)

Comment 1

[Mark Banner (:standard8)]

This is perfectly fine on Mac and the same as Firefox trunk.

Comment 2

[Tyler Downer [He/Him]]

OK, then, this is for the win port.

Comment 3

[Tyler Downer [He/Him]]

Attachment: Proposed icons

This is the first start at fixing this bug.

It seems that while pinstripe is more like Firefox, in using a white background, updated icons, etc. Gnomestripe and win did not get updated. This causes a few things that need to be done:
1. Update icons
2. Remove background
3. Change CSS

This is the first step at this. I have simply extracted the icons from preferences from Firefox (the privacy icon here still needs to be changed), and used two (composition and attachments) from the current TB. I need someone who is good with icons to create new ones for that, if deemed necessary. A second set, for Aero, will be forthcoming after those two icons have been replaced. This is really pretty simple.

I will later get a first attempt at a patch that removes the background images, and uses these images in the preferences bar. A second bug will have to be filed on Gnome.

Comment 4

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

Cool, I'm adding Andreas who's working visual improvements.  He already did a set of icons for GNOME.
https://wiki.mozilla.org/Thunderbird:Themes:Supported/Linux#Preferences

I'm not sure what the plan was for Windows but I had assumed we'd look into either these Firefox icons (at least for Vista) or Tango icons.

https://wiki.mozilla.org/Thunderbird:Themes:Supported/Windows#Preferences

Comment 5

[Andreas Nilsson (:andreasn)]

Attachment: new vista preferences icons

These are in one image, rather than chopped up as they are now, as Phil noted in #485371 that this impacts performance, so it will need some css changes as well. Will take care of that and attach later today.

Comment 6

[Tyler Downer [He/Him]]

Should we be using these icons, or the firefox ones? The image I gave uses XP style, we would need another one for Aero. I think using FF as much as possible is best for unity between programs.

Comment 7

[Andreas Nilsson (:andreasn)]

We need new icons for the ones that Firefox don't cover. In attachment #5 [details] the general, security and general icons originate from Firefox trunk. The others are new.
I'm not 100% confident with the Aero style in 32x32 quite yet, so let me know if anything in particular sticks out.

For XP, the plan that me and Bryan discussed previously involved reusing the Tango icons from gnomestripe where possible (as the style is pretty close) and some from Firefox and draw new, customized icons where needed, like for the folder pane for example.

Comment 8

[Andreas Nilsson (:andreasn)]

that is attachment #371856 [details] of course. :)

Comment 9

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

I looked into this today and it looks fairly simple.  I'm not sure how to test on WinXP as I only have Vista at my disposal, however maybe I can get asuth to open up an XP vm to test the changes we have when they are in patch form.

Here's an example of a commit FF used to separate the aero files out from the other win css/image files.
http://hg.mozilla.org/mozilla-central/rev/c449cd45567d

Comment 10

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

getting this into the blocking queue

Comment 11

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

(In reply to comment #5)
> These are in one image, rather than chopped up as they are now, as Phil noted
> in #485371 that this impacts performance, so it will need some css changes as
> well. Will take care of that and attach later today.

For the preferences I don't think this matters as much, it's a performance issue in places like the folder pane or message list when we are using the same icon many times.  So lets keep using separate icons for the prefs since it's easier to manage.

For Vista icons lets add a -aero to the name so we differentiate from the XP icons.  So for example general.png is the XP icon and general-aero.png is the  same icon in Vista.  Firefox has a similar scheme that I'm following.

Andreas: If you can attach a zip of those Vista icons I'll get them into a patch and ready for inclusion.

I'm going to create a similar XP variant of this bug to track that we're bringing the tango icons over.

Comment 12

[Andreas Nilsson (:andreasn)]

Attachment: new icons as separate images

Bryan: Here you are. Note that 'privacy' is now 'security' due to the changes for this category we discussed earlier (can't remember the bug number though) and that we have the new junk icon.

Comment 13

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

Attachment: WIP for Vista options icons

This patch adds the new *-aero.png icons to the preferences directory.

The major change here is following the Firefox lead in creating a separate Vista aero space in the jar.mn.  I'm no expert with manifest files so I'd appreciate some scrutiny here.

here's what I did to the jar.mn:
* added the osversion and os=WINNT checks to the chrome registration
* moved the communicator/smileys.css into a location that made more sense.
* created an aero section using the osversion and WINNT ifdef
* copied all the css/icon rules into the aero definition section

I also added in the junk-aero.png icon for now, even though we're not using it yet.  When bug 482617 lands (fairly soon) I'll need that icon.

I threw bienvenu the easier one earlier so now I'm hoping dmose can help me with this one?  Feel free to toss this around or back to me.  Thanks!

Comment 14

[Andreas Nilsson (:andreasn)]

Attachment: vista general icon

It seems I mistakenly used the XP general icon (the look pretty similar, but the vista one have less blue in it), so here is the Vista one.

Comment 15

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

Attachment: updated patch w/ new icon

here's an updated patch with the new icon from andreas

Comment 16

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

The Vista work should really be set against RC1 instead of beta3.  Readjusting the target milestone.

dmose: if you want to drop the review for now I can re-request when we're in the rc1 timeframe

Comment 17

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

This is really needed to get the Vista theme work going, perhaps I should take the patch on again to make sure it hasn't rotted all this time.

Comment 18

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

Comment on attachment 372629 [details] [diff] [review]
updated patch w/ new icon

taking this off since this thing needs some work and I'll probably get philor to warm up his VMs with this patch afterwards.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

For some reason, I've had problems applying this patch on both Mac and XP (three different hunks fail), and it doesn't appear to be obvious bitrot going on.  It could be something nonobvious that I've missed.

Comment 20

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

strange I just tried this patch again and it seems to have applied cleanly for me.  Though I'm on Linux right now, when I get into windows again I'll try the patch out and see if I get the errors you're seeing.

Comment 21

[Phil Ringnalda (:philor)]

wfm, too, so I think the nonobvious thing is "your tree's busted."

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Except I tried this on two completely separate trees, one of them a fresh checkout.  I wonder if qimportbz.py is munging the patch in some weird way.

Comment 23

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

you've got a lot of busted trees :)

Comment 24

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

Comment on attachment 372629 [details] [diff] [review]
updated patch w/ new icon

This seems to apply just fine so I might as well push it down the pipe as we've got a lot of Vista icons and CSS work building up.  Also I'm hoping that this work will make fixing bug 494430 easier as we'd have a separate winXP css file to work with.

Comment 25

[Phil Ringnalda (:philor)]

(In reply to comment #13)
> * moved the communicator/smileys.css into a location that made more sense.

That location only makes more sense if you intend to ever make any Aero-specific changes to communicator/skin/ (practically speaking, to smileys) or to editor/skin/ (practically speaking, to HTML compose dialogs for things like image or table properties). If so, then the Aero versions should be aero/ed up. If not, rather than repeating the exact same thing twice we should just have one section for each, before we get into the split up parts.

Comment 26

[Phil Ringnalda (:philor)]

Attachment: Mah review comments, let me show you them

Think of this as the equivalent of "r=me with these changes..." where the review request is your chance to say "but, but, but!"

Changes:

* somebody crapped DOS line endings all over the file at some point in the past, which means it's sort of hard to see my changes for all the line ending changes, but that's less painful than bitrotting your patch by fixing it first

* moved communicator/, editor/, and icon.png and preview.png out to a "not different for aero" section at the start with the tree stuff I hope to remove before long, on the assumption that you're not yet planning on having separate smileys or separate table properties dialogs

* removed the blank line and the repeated "classic.jar:", to prove that we understand what Neil meant in bug 422628 comment 7

* added the "os=WINNT osversion<6"/"os!=WINNT"/"os=WINNT osversion>=6" to the messenger-newsblog registration lines - the way it was, there were two unconditional registrations, and since .manifests don't have any particular order it would be pretty random whether you'd get aero on XP, or non-aero on OS/2, or what. The way to tell whether it's working is to look at dist/bin/chrome/classic.manifest, where you want to either have only a single line with no qualifications for a package name, or have three lines, all of them qualified (and to clobber first, since regenerating them doesn't seem to care about removals).

* removed a stray tab in the middle of the junk.png line

* bravely resisted the temptation to use this as an excuse to put the damn thing in an order, any order

Comment 27

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

Comment on attachment 379402 [details] [diff] [review]
Mah review comments, let me show you them

This all makes sense to me.  No but buts... just wow, this is awesomeness.

Comment 28

[Phil Ringnalda (:philor)]

http://build.mozillamessaging.com/mercurial/comm-central/rev/2805b857727c

Comment 29

[Tyler Downer [He/Him]]

OK, V in Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090529 Shredder/3.0b3pre. However, I personally think the blue background in the preferences in ugly, so should we take that out? I think it would be another bug, and I would be willing to do it. But it just does not look very clean.

Comment 30

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

tyler, can you file a bug about that and mark it blocking bug 488060

Comment 31

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

Attachment: how many vista nightly users are there? [checked in]

the newsblog is missing the 'aero' which makes it not want to show up

Comment 32

[Phil Ringnalda (:philor)]

Comment on attachment 381135 [details] [diff] [review]
how many vista nightly users are there? [checked in]

Sigh. I knew I was going to screw something up unrotting after Standard8 beat me to taking out the DOS line endings, so apparently I checked everything else twice, and that part zero times. Thanks.

(As to how many: any time you get over two conditions (Windows user, Vista user, RSS account) you're pushing your luck, and sadly we've somehow convinced many of our nightly users that it's reasonable to wait a few days or a week before filing on little things, I guess to give us time to forget how and where we might have screwed up.)

http://build.mozillamessaging.com/mercurial/comm-central/rev/0d042521df92