485371 - New icons for address book

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Andreas Nilsson (:andreasn)]

Attachment: address book icons

New icons for address book

Comment 1

[Andreas Nilsson (:andreasn)]

Attachment: modified css

Comment 2

[Andreas Nilsson (:andreasn)]

As abcard is now a dude rather than a address card, because of the changes in  Bug 484409 , this will also affect the icon shown in the message compose title.

Comment 3

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

Attachment: initial patch with icons and css changes

wow, big patch.  Here's the hg version of all the work andreas did plus some small additions.

I created an icons directory in the addrbook directory because it was getting annoying mixed in with all the CSS files.  I took the new compose icons and moved them into the generic icons directory since they're being used by a number of other places and not just the addressbook.

Changes to the addressbook.xul so I could get the menu item icons to show up properly.  I tried to cleanup the areas I touched into the right alignment.

I rearranged the File menu so the print items could use the print icons and I think they make more sense separated by type instead of action.

So now it looks like this:

Print Preview Contact...
Print Contact...
--------------------
Page Setup...
--------------------
Print Preview Address Book...
Print Address Book...

changed primaryToolbar.css so that we'd use the new Write button icon.

added an id to messagengercompose.xul so I could select it in the css file.
changed messaengercompose.css so we'd use the new contact and addressbook icons in the menus.

the jar.nm file is wrapping a little long right now.

Phil, can you take a look at this patch?

Comment 4

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

Attachment: screenshot of partos of the patch in action

Comment 5

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

phil: do you want me to move this over to magnus?  I know you're working on the autoconfig bug which is a blocker and a bit bigger

magnus: do you want to take this?

Comment 6

[Phil Ringnalda (:philor)]

Yeah, sorry, I didn't realize autoconfig was going to eat a whole weekend before I even got to the code.

The one thing that worries me here (and elsewhere) is switching from -moz-image-region to a lot of separate images. I can't find the bug right now, but I remember fairly recently someone running a patch through Firefox's perf tests, and finding that even combining just a few images into one was a huge difference. Having done it, I know assembling a single combined image is an annoying fiddly task, but it's worth doing.

Comment 7

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

ugh, I was afraid this was going to come up. The image region code is a real pain, but I might have recently noticed some effects of the individual images with the folder pane patch. Images in collapsed children only appeared to load after they were expanded, not sure if that is the same issue.  I'll see if I can find that bug.

Comment 8

[David Ascher (:davida)]

the whole image splicing process feels to me like something that really ought to be part of the build process somehow.  It's fiddly, as you say, and yet should be automateable once-and-for-all.  Not sure how to make that happen though...

Comment 9

[Phil Ringnalda (:philor)]

Yeah, not having icons for hidden things preloaded is another aspect of it: with combined images you pay that cost up front, but the marginal cost of another 16x16 or 64x64 pixels is absolutely nothing compared to the cost of dragging something out of a jar on disk.

Comment 10

[Andreas Nilsson (:andreasn)]

Attachment: combined address book toolbar

Oh, I had no idea of this issue, but if it impacts a lot on performance I'll rework it. Here are the toolbar icons combined into one. Only active and disabled states, since the icons don't change on hover (Firefox only does two states as well). No delete icon, since we'll use a GTK+ stock icon for that. Will rework the css and attach that as well.

Comment 11

[Andreas Nilsson (:andreasn)]

Attachment: address book small toolbar icons

Comment 12

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

Attachment: updated patch using moz-image-region

here's a patch with all the same updates but using moz-image-region instead of the individual files.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 373213 [details] [diff] [review]
updated patch using moz-image-region

Looks really nice!

+#menu_properties {
+  list-style-image: url("chrome://messenger/skin/addressbook/icons/addressbook-toolbar-small.png");
+  -moz-image-region: rect(96px 16px 112px 0px); /* contact-properties-small.png */
+}

There's a gtk stock icon for properties. (And we use it for right-click contact | properties). Maybe it would be best to use that? Or at least make it consistent.
http://library.gnome.org/devel/gtk/2.16/gtk-Stock-Items.html#GTK-STOCK-PROPERTIES--CAPS

+  list-style-image: url("chrome://messenger/skin/addressbook/icons/addressbook-toolbar.png");	

trailing tab, x2

Comment 14

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

Attachment: updated patch w/o tabs

(In reply to comment #13)
> (From update of attachment 373213 [details] [diff] [review])
> Looks really nice!
> 
> +#menu_properties {
> +  list-style-image:
> url("chrome://messenger/skin/addressbook/icons/addressbook-toolbar-small.png");
> +  -moz-image-region: rect(96px 16px 112px 0px); /*
> contact-properties-small.png */
> +}
> 
> There's a gtk stock icon for properties. (And we use it for right-click contact
> | properties). Maybe it would be best to use that? Or at least make it
> consistent.
> http://library.gnome.org/devel/gtk/2.16/gtk-Stock-Items.html#GTK-STOCK-PROPERTIES--CAPS

Ah right, I'm going to go with the properties/info icon we have instead of the gtk-stock icon because it feels more like a contact info icon than the stock one does.

> +  list-style-image:
> url("chrome://messenger/skin/addressbook/icons/addressbook-toolbar.png");    
> 
> trailing tab, x2

wtf!  Where did I get tabs from? :)

I also added in some popup menu properties that I missed on the first pass.  Now the right click menus use the same icons as the file/view/edit menus.

Thanks!

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 374307 [details] [diff] [review]
updated patch w/o tabs

r=mkmelin

Comment 16

[Magnus Melin [:mkmelin]]

changeset:   2466:3fe3e732c210
http://hg.mozilla.org/comm-central/rev/3fe3e732c210

->FIXED

Comment 17

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

I think the older patch got in, I just noticed some of the changes to the popup menu didn't get through.

http://hg.mozilla.org/comm-central/rev/3fe3e732c210 looks like attachment 373213 [details] [diff] [review] instead of attachment 374307 [details] [diff] [review]

Comment 18

[Magnus Melin [:mkmelin]]

hg export 2466 | patch -R -p1
then applied the other patch again with patch

changeset:   2488:4395ed099cde
http://hg.mozilla.org/comm-central/rev/4395ed099cde

->FIXED again. Sorry for the mess :(