Last Comment Bug 119459 - Option to add a photo/image/picture to each entry of the addressbook
: Option to add a photo/image/picture to each entry of the addressbook
Status: RESOLVED FIXED
[delight]
:
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: P4 enhancement with 36 votes (vote)
: Thunderbird 3.0b4
Assigned To: Josh Geenen (:pi)
:
Mentors:
: 222506 364470 371138 375060 380098 413316 461557 (view as bug list)
Depends on: 513471
Blocks: 29106 271976 469517 469519 496620 496789 475512
  Show dependency treegraph
 
Reported: 2002-01-11 05:06 PST by Andrea Monni
Modified: 2009-09-07 02:03 PDT (History)
47 users (show)
davida: blocking‑thunderbird3-
mkmelin+mozilla: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
The card view pane of a contact with a photo (94.51 KB, image/png)
2009-06-05 05:44 PDT, Josh Geenen (:pi)
no flags Details
business card layout + m/f default images (79.50 KB, image/png)
2009-06-05 20:16 PDT, chrizoo
no flags Details
Work in progress (29.50 KB, patch)
2009-06-06 22:08 PDT, Josh Geenen (:pi)
no flags Details | Diff | Splinter Review
Newer WIP (33.71 KB, patch)
2009-06-08 22:01 PDT, Josh Geenen (:pi)
no flags Details | Diff | Splinter Review
Preview in Windows Vista (36.05 KB, image/jpeg)
2009-06-14 21:09 PDT, Josh Geenen (:pi)
no flags Details
Updated preview (99.58 KB, image/png)
2009-06-24 22:45 PDT, Josh Geenen (:pi)
no flags Details
generic photo graphics (5.71 KB, image/png)
2009-06-25 03:46 PDT, Andreas Nilsson (:andreasn)
no flags Details
Patch w/ radio buttons for photo types (39.79 KB, patch)
2009-07-03 14:10 PDT, Josh Geenen (:pi)
no flags Details | Diff | Splinter Review
Updated preview (Mac OS X) (53.23 KB, image/jpeg)
2009-07-03 16:03 PDT, Nomis101
no flags Details
Patch v1 (39.72 KB, patch)
2009-07-25 10:29 PDT, Josh Geenen (:pi)
clarkbw: ui‑review+
Details | Diff | Splinter Review
Patch v1 in Linux (82.00 KB, image/png)
2009-07-30 20:41 PDT, Josh Geenen (:pi)
no flags Details
Patch v1 in Windows Vista (92.50 KB, image/png)
2009-07-30 20:42 PDT, Josh Geenen (:pi)
no flags Details
Patch v1 in Mac OS X (46.28 KB, image/jpeg)
2009-08-01 13:27 PDT, Nomis101
no flags Details
Patch v1.1 - Adds image from Andreas Nilsson (90.61 KB, patch)
2009-08-16 20:29 PDT, Josh Geenen (:pi)
no flags Details | Diff | Splinter Review
Patch v1.2 - Fixes accesskeys, updates photo menulist (93.57 KB, patch)
2009-08-17 21:42 PDT, Josh Geenen (:pi)
standard8: review+
Details | Diff | Splinter Review
Patch v1.2a - address comments (90.27 KB, patch)
2009-08-26 03:54 PDT, Mark Banner (:standard8)
mozilla: superreview+
Details | Diff | Splinter Review

Description Andrea Monni 2002-01-11 05:06:32 PST
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:0.9.7) Gecko/20011221
BuildID:    

I think that after the Map-it! functionality in addressbook it deserves also the
ability to associate a photo to each entry. What do you think?
Comment 1 xyzzy 2002-01-11 10:23:27 PST
Just to clarify, do you want a photo of the address location or a photo of a person?
Comment 2 Andrea Monni 2002-01-12 07:10:23 PST
> Just to clarify, do you want a photo of the address location or a photo of a
person?

I meant the photo of a person. Sorry if it was unclear but I'm not a native
english-speaking person and sometimes I can't be as clear as I should be. Sorry
again and thanks for asking a clarification.
Comment 3 Ninoschka Baca 2002-01-15 11:00:48 PST
I like the idea, setting to New.
Comment 4 jglick 2002-01-15 11:28:44 PST
If you take a look at the Address Book spec, it was proposed that the "Notes" 
field on the "Other" tab, allow html, including image files. This would cover 
your request as well as allowing html.

http://www.mozilla.org/mailnews/specs/addressbook/#Cardfor
http://www.mozilla.org/mailnews/specs/addressbook/#CardPane

Mind if we expand this bug to cover both?

Make the Notes field a Composer widget. The user can enter html and/or graphics 
(gif, jpg, png) to this field. Stuff entered, appears in the Card Pane Summary 
panel.
Comment 5 Alex Bishop 2002-01-15 12:27:05 PST
Allowing images in the Notes field is nice but the average user is going to balk
at typing <img src="file:///C:/My%20Pictures/Winnie%20%the%20Pooh.jpg"> into the
Notes field.

It would be better if there was a specific UI for choosing the picture (a
standard file path text box and a 'Browse...' button). The picture could still
appear in the Notes field though (in fact, the picture select UI could
automatically add the requisite HTML into the Notes field).

If the picture feature was really smart, it would auto-scale pictures to fit in
the Address Book. An area of X by Y pixels would set aside for the picture. If
the user's picture was too large to fit in the area then it would be scaled to
fit in the area, while remaining as large as possible and still retaining its
aspect ratio. Smaller pictures would never be scaled. Of course, this scaling
would have to be done everytime the picture was loaded in case the user swaps
the image for one with a different size but the same name.
Comment 6 jglick 2002-01-15 12:34:47 PST
The user won't have to know any html. It would be the same as a wysiwyg editor.
http://www.mozilla.org/mailnews/specs/addressbook/images/CardOther2.gif (pic a 
little old).

Having separate fields for "Image" and "Notes" (html) would be a good idea as 
well. With images correctly scaling.
Comment 7 Andrea Monni 2002-01-16 05:58:07 PST
While the notes idea is sweet I agree with Alex that a specific UI would be
better for this photo feature and Jennifer's idea of a specific field for
"Image" (or "Photo") is what I had in mind when I opened the bug.

This also means that next time I'll open a bug I'll have to be more precise :).

Shall we open a bug for the notes feature or is it already in Bugzilla?
Comment 8 Ian Pottinger 2002-04-03 22:18:13 PST
adding self to cc list
Comment 9 (not reading, please use seth@sspitzer.org instead) 2003-05-08 11:57:09 PDT
mass re-assign.
Comment 10 Jo Hermans 2003-10-16 15:22:28 PDT
*** Bug 222506 has been marked as a duplicate of this bug. ***
Comment 11 Mark Stier 2003-10-16 15:31:17 PDT
The pictures should also be displayed on the address book's address lists and
not only in the fields of the address cards.
Comment 12 Robert Markula 2004-01-16 02:41:30 PST
Associating pictures (that may be a photo of a person or a company logo) with
contacts is already part of the vCard standard, so why not implementing it here
as well? Voting.
Comment 13 Andrea Monni 2004-01-16 03:12:36 PST
> ------- Additional Comment #12 From Robert Markula  2004-01-16 02:41 -------

> Associating pictures (that may be a photo of a person or a company logo) with
> contacts is already part of the vCard standard, so why not implementing it here
> as well? Voting.

Robert, your comment makes sense.

How shall we proceed? Shall we open a specific vCard bug?
Comment 14 Robert Markula 2004-01-16 03:52:29 PST
I meant implementing the feature to associate pictures with contacts, not
implementing the vCard standard (although that sounds very interesting as well,
see Bug# 29106).

Sorry for the misunderstanding.
Comment 15 Wout Mertens 2006-03-21 02:42:01 PST
Please add this, you have no idea how nice it is to show the picture of the sender in the mail header of a message.

On top of that, our company has everybody's picture in the company LDAP addressbook, so these pictures could be looked up without having to come up with them yourself.

The Apple AddressBook application has support for pictures, and if the user has an IM account but a missing picture, their IM buddy icon will be stored automatically. Just to give an example of the possibilities...
Comment 16 M.J.G. 2006-05-31 05:57:42 PDT
(In reply to comment #15)
> The Apple AddressBook application has support for pictures, and if the user has
> an IM account but a missing picture, their IM buddy icon will be stored
> automatically. Just to give an example of the possibilities...
> 

Note that MailNews/TB already has buddy icons support. buddy icons are displayed in the address book as well as in the mail pane. It's just that there is no GUI for adding pictures, and the docs are somewhat hidden.

How do we get together buddy icons/LDAP pictures/Faces and XFaces and such? There are too many "standards", none being commonly used.

I'd be willing to work on a GUI/extension for the buddy icon stuff, but not if buddy icons are supposed to be dropped in favour of other picture solutions for the address book.

Michael
Comment 17 Mark Banner (:standard8) 2006-12-20 07:46:31 PST
*** Bug 364470 has been marked as a duplicate of this bug. ***
Comment 18 Magnus Melin 2007-02-21 10:34:51 PST
*** Bug 371138 has been marked as a duplicate of this bug. ***
Comment 19 Mark Banner (:standard8) 2007-03-23 04:43:28 PDT
*** Bug 375060 has been marked as a duplicate of this bug. ***
Comment 20 Mark Banner (:standard8) 2007-05-09 00:00:17 PDT
*** Bug 380098 has been marked as a duplicate of this bug. ***
Comment 21 dharana 2007-12-01 05:36:28 PST
5 years.. I have no hope but still..
Comment 22 amiscorp 2008-01-15 19:25:10 PST
I really think this should be considered as many e-mail clients are supporting pictures in the addressbook. It is becoming a nice feature to have, not just a fancy frivolous feature. Seeing an image of the person is quite useful when you have hundreds of contacts in the addressbook.
Comment 23 M Lopez-Ibanez 2008-01-16 09:13:29 PST
Not sure about it but it seems there is some add-on available for this:
http://blog.plaxo.com/archives/2005/07/thunderbird_has.html
Comment 24 Mark Banner (:standard8) 2008-01-21 01:55:13 PST
*** Bug 413316 has been marked as a duplicate of this bug. ***
Comment 25 Mark Banner (:standard8) 2008-01-21 01:57:21 PST
Added "picture/image" to the title to make it easier to find for dups.
Comment 26 Magnus Melin 2008-10-23 11:52:03 PDT
wanted‑thunderbird3+, I wonder who we can get to take this on...
Comment 27 Wayne Mery (:wsmwk, NI for questions) 2008-10-23 12:08:39 PDT
There's probably many who will find this feature particularly attractive - bling bling and all that.  But if it takes long to deliver ... they won't be so young anymore.
Comment 28 Ronald Killmer 2008-10-23 13:59:35 PDT
A user can apply the MoreFunctionsForAddressbook extension from https://nic-nac-project.de/~kaosmos/index-en.html and get this functionality now in Tb 2.0x. I have not evaluated it against the 3.0b1pre nightlies.  The extension adds a folder in a profile to store copies of images using salted file names. When used the feature has an image sizer to constrain the display size in the contact card.

Reply to comment #26, perhaps Kaosmos would contribute if asked.
Comment 29 Jeffrey King 2008-10-23 17:47:34 PDT
I find that a picture in the address book helps me remember a lot of information that I can not normally store in the address book. I would hope that someone will take this and run with it.
Comment 30 Phil Ringnalda (:philor, back in August) 2008-10-24 12:27:17 PDT
*** Bug 461557 has been marked as a duplicate of this bug. ***
Comment 31 Clint 2008-10-27 08:47:41 PDT
Ronald-

Kaosmos MoreFunctionsForAddressBook extension does add the photo field but since it is not apart of the core TB build, other extensions that handle synchronization (Zindus) to other devices (Google contacts..etc) will not sync this photo attribute.  This would be a nice addition especially since other major email clients and webmail services already offer it.

(In reply to comment #28)
> A user can apply the MoreFunctionsForAddressbook extension from
> https://nic-nac-project.de/~kaosmos/index-en.html and get this functionality
> now in Tb 2.0x. I have not evaluated it against the 3.0b1pre nightlies.  The
> extension adds a folder in a profile to store copies of images using salted
> file names. When used the feature has an image sizer to constrain the display
> size in the contact card.
> 
> Reply to comment #26, perhaps Kaosmos would contribute if asked.
Comment 32 Ronald Killmer 2008-10-27 13:01:55 PDT
(In reply to comment #31)
> Ronald-
> 
> Kaosmos MoreFunctionsForAddressBook extension does add the photo field but
> since it is not apart of the core TB build, other extensions that handle
> synchronization (Zindus) to other devices (Google contacts..etc) will not sync
> this photo attribute.  This would be a nice addition especially since other
> major email clients and webmail services already offer it.
> 

I am aware the photo feature was an expermental addition to the extension.  I mentioned it because it points out the potential of the Tb Abook UI. Being the extension is an OpenSource project it's code could be a readmap for adoption.  The challenge I anticipate is how the image data would have to be packaged for Syncing.
Comment 33 David :Bienvenu 2008-12-11 09:50:10 PST
jcranmer, any interest in doing this as part of your other work? Just storing a url to the picture should be sufficient (e.g., a file url)
Comment 34 Ronald Killmer 2008-12-11 13:54:07 PST
(In reply to comment #33)
> jcranmer, any interest in doing this as part of your other work? Just storing a
> url to the picture should be sufficient (e.g., a file url)

I think more than URL storage would be needed because an image is likely to be too large for the Abook UI. Paolo resolved that in His extension. Also as I noted in Comment #28 He stores copies within the Profile in folder ABPhotos which is insurance against the risk of a rename of the original, thus altering the File:// URI and generating a mismatch to what is stored in Abook.
Comment 35 Giuseppe Tino 2008-12-12 03:49:50 PST
Another improvement should be the possibility to display the photo of the contact in the box that advice that a new message arrived and in the header bar when a message is selected.
Comment 36 Ronald Killmer 2008-12-12 09:35:25 PST
(In reply to comment #35)
> Another improvement should be the possibility to display the photo of the
> contact in the box that advice that a new message arrived and in the header bar
> when a message is selected.

These two ideas will need two new Enhancement requests being filed. The basic policy is bug per report, or one enhancement per request.
Comment 37 Giuseppe Tino 2008-12-13 03:49:13 PST
(In reply to comment #36)
> These two ideas will need two new Enhancement requests being filed. The basic
> policy is bug per report, or one enhancement per request.

Sorry for the mistake.
I thought that this was the correct report because those requests are strictly connected whit the possibility to put the contact's photo in the address book. 
If this function is not yet supported there is not sense to make my requests.
Comment 38 Ronald Killmer 2008-12-13 17:26:26 PST
(In reply to comment #37)
> (In reply to comment #36)
> > These two ideas will need two new Enhancement requests being filed. 
> 
> Sorry for the mistake.
> I thought that this was the correct report because those requests are strictly
> connected whit the possibility to put the contact's photo in the address book. 
> If this function is not yet supported there is not sense to make my requests.

Your Ideas are actually different. They each apply to different parts of the code base that are worked on by different teams of developers.  Each idea deserves to be evaluated by the proper team.  Also, each idea could be developed independently of the other or the one this bug is about. 

This is why I recommended new requests so yours do not get lost when this bug is resolved.
Comment 39 Andrea Monni 2008-12-13 20:47:09 PST
Giuseppe:

You should go to https://bugzilla.mozilla.org/enter_bug.cgi and follow the steps to file a new bug, once you file it and it's been confirmed as a valid enhancement request it will be marked as "New", (eventually) assigned to the relevant developer, and (probably) marked as "Depends on" this bug so that it can be tracked effectively. You might also want to leave a comment on this bug notifying us that you filed some related bugs to this one.

If you want to learn more about a bug life cycle you should read this document: http://www.bugzilla.org/docs/2.18/html/lifecycle.html and feel free to contact me.
Comment 40 Susurrus 2008-12-13 21:13:01 PST
Does this feature include using photos stored in the OS' address book or should using OS-dependent address books' photo capabilities be separate?

Also:
See https://bugzilla.mozilla.org/show_bug.cgi?id=469519 and https://bugzilla.mozilla.org/show_bug.cgi?id=469517 for the suggestions made in comment 35. They've also been added as dependencies of this bug.
Comment 41 klumy 2008-12-14 05:02:50 PST
I also vote for this request
Comment 42 Nomis101 2008-12-18 05:53:59 PST
On Mac OS X you can now use the OS X addressbook in Thunderbird 3. You can add pictures of the person in the Mac OS X addressbook. So, if you include this new feature in Thunderbird 3, please make sure that you get displayed the pictures from the Mac OS X addressbook in the Thunderbird addressbook.
Comment 43 Armond Avanes 2008-12-20 13:58:23 PST
Anyone working on this enhancement request? Any chance we could get it on 3.0b2?
Comment 44 David Ascher (:davida) 2009-01-08 11:01:06 PST
This wouldn't block a tb3 release, but we still want it.
Comment 45 chrizoo 2009-05-19 10:36:34 PDT
I think Thunderbird would get a major popularity boost from this feature. Consider  the establishing of emotional ties between the user and his/her software under the influence of emotional ties between the user and his friends the pictures of whom he/she sees in the Thunderbird address book.

For example: I'm not an Apple user, but most of my friends on Macs (if I understood them correctly), prefer Apple Mail to Thunderbird because of the address book pictures.
Comment 46 Josh Geenen (:pi) 2009-06-05 05:44:43 PDT
Created attachment 381759 [details]
The card view pane of a contact with a photo

I might be able to look into this (allowing contact photos in the Address Book) depending on how complex this feature is going to be and how much spare time I have when my online class starts.  If it doesn't make it into TB 3.0 I will probably write a new extension for it or add it to at least one of mine.

I currently have a rough implementation that adds a "Photo" tab to the new/edit contact dialog with a preview, a textbox and browse button.  It stores the file URL and displays the image at 150x150px in a new vbox on the left side of the card view pane if there is a photo (see the attached screenshot).

Here are my questions so far:

Should it just store the image URL, or copy the image to a folder in the profile folder and store the filename instead?  The latter option would make migrating the profile between computers much simpler than the former.

Does it have to detect the size of the image, or can it scale the image down to 150x150 or so? Alternatively, should there be photo height and width fields?

Does a patch for this bug have to import images from vCards, LDAP entries, and other sources?
Comment 47 Josh Geenen (:pi) 2009-06-05 05:50:36 PDT
One more question:

If a contact does not have a photo, should that vbox be collapsed or should there be a default, generic photo like Gmail has[0]?

[0] https://mail.google.com/mail/contacts/static/images/NoPicture.gif
Comment 48 David Ascher (:davida) 2009-06-05 08:00:58 PDT
Josh -- this would be super cool if you got around to it.

My suggestion:

1) I'd bias towards URLs -- for some people, they'll have local images, but we also want to allow people to use systems like gravatars or other online resources as source of images.  So I would suggest the addressbook store URLs, and that we define an "image getter service" which can be configured to get URLs from a variety of places (locally, system services like on the mac, websites like facebook, generated images like gravatar.com).

2) the UI can figure out what size it wants to use depending on the context and/or preferences.  The system should probably get the largest image available, and we can build resizing either in the UI layer or through an intermediate layer if necessary (which I doubt)

3) having a fallback URL like the NoPicture.gif would be good (but it should be something different)

I'd love to use it in the summary view, as I do w/ gravatars in the add-on shown in https://bug496244.bugzilla.mozilla.org/attachment.cgi?id=381647.

I'd be very happy to discuss APIs for this kind of feature.
Comment 49 Andrew Sutherland [:asuth] 2009-06-05 08:10:50 PDT
(In reply to comment #48)
> 1) I'd bias towards URLs -- for some people, they'll have local images, but we
> also want to allow people to use systems like gravatars or other online
> resources as source of images.  So I would suggest the addressbook store URLs,
> and that we define an "image getter service" which can be configured to get
> URLs from a variety of places (locally, system services like on the mac,
> websites like facebook, generated images like gravatar.com).

Are you proposing that we would then retrieve the image from the URL and save it to the profile?  Because if not, I am proposing that.

Gravatars and other solutions that leak information are fine for prototypes, but a real solution should not be leaking information when it does not have to.  Not to mention offline use-cases.
Comment 50 David Ascher (:davida) 2009-06-05 08:17:10 PDT
(In reply to comment #49)
> Gravatars and other solutions that leak information are fine for prototypes,
> but a real solution should not be leaking information when it does not have to.
>  Not to mention offline use-cases.

Good points.  We should store the original URL as well, though, so images can be refreshed (either on demand or based on some policy).
Comment 51 Ronald Killmer 2009-06-05 09:54:23 PDT
I have been using the extension referenced in Comment #28 and see value in it's image storage scheme. The extension keeps it's own copies with in the profile and salts the filename for privacy/security reasons. This breaks easy association of an image with a Contact so a Hacker would be left guessing which image goes with a card.

By all means, design in a placeholder image. That will make clear to a new user that We support images in the Abook.

I believe the first cut on this bug should be a Cross-Platform solution that can be extended later to tap into OS specific services, such as the Mac that DavidA mentions.
Comment 52 Bryan Clark (DevTools PM) [@clarkbw] 2009-06-05 10:09:31 PDT
Yes, assume that there would be a stock "nobody" image that we ship with. We have some stock generic people icons to draw from already and could create one or two that are missing from other themes.  

I think we would restrict the icon size to being 48px width (in the card display) as that's a fairly common size with a max-width CSS rule and likely some kind of larger height restriction.  Maybe andreas has ideas about this as well.
Comment 53 chrizoo 2009-06-05 10:26:32 PDT
48px, that must be kidding, right ? Compare it with a standard reference, for example Outlook AB which has 148x124 px in card view (if you reserve half the card for the address text) and even for the small picture in the contact details, Outlook still uses 96x72 px.

With 48px, you can use smileys, but no photographs. 

The best solution would be to give the user a bit of autonomy here. I guess, user-defined sizes won't be easy to implement, but what about three layouts (small, medium, large photos+cards) ?
Comment 54 David Ascher (:davida) 2009-06-05 10:30:31 PDT
I'd suggest this bug be split up into a couple of bugs -- at least one for discussing the lower-level APIs, one for the address book UI proposals.

I suspect there are two different populations of people cc'ed on this bug, with different interests.
Comment 55 Ronald Killmer 2009-06-05 10:35:11 PDT
(In reply to comment #52)

> I think we would restrict the icon size to being 48px width (in the card
> display) as that's a fairly common size with a max-width CSS rule and likely
> some kind of larger height restriction.  Maybe andreas has ideas about this as
> well.

I think 48px is too narrow for resizing of JPEG photos. I find that 100px is getting close to the minimum. A great power of Gecko is the image sizing it does when CSS constrains the image display box. To me, Gravitars and Face icons are a fallback if a Photo is not available.

(In reply to comment #54)

Yes, I am getting the same impression. This bug started as a Photo RFE and those needs seem to be clashing with the iconified camp.
Comment 56 Bryan Clark (DevTools PM) [@clarkbw] 2009-06-05 12:24:47 PDT
Sigh, I should probably read the whole bug before spitting out number suggestions.  Lets pretend I was thinking of a different layout than the screenshot.
I created bug 119459 as a space to redesign the card layout with the addition of a photo; follow that bug if you think the nobody photo should default to a purple background. :)
Comment 57 Ronald Killmer 2009-06-05 14:01:57 PDT
The ideas about Gravitar, etc. are great. We now have detected a broader use that the existing Abook probably lacks API to implement. I think a two or three step plan will get Tb to a desired end point. 

First in near term, get photo support using the existing Abook.mab data field. A concurrent design of API would lead to a mid-term goal of avitars. 

The final goal being import of images from both remote sources and system services. This last step including import from other apps that have photo/image support.

Long term, an enhancement should dovetail with the extension empowerment scheme.
Comment 58 Ronald Killmer 2009-06-05 14:14:24 PDT
The correct new bug reference in Comment 56 should be Bug 496620.
Comment 59 chrizoo 2009-06-05 20:08:22 PDT
(In reply to comment #35)
> Another improvement should be the possibility to display the photo of the
> contact in the box that advice that a new message arrived and in the header bar
> when a message is selected.

Have these 2 enhancement requests already been filed ?
Comment 60 chrizoo 2009-06-05 20:16:46 PDT
Created attachment 381895 [details]
business card layout + m/f default images

It would provide for a good user experience to have an "overview view" or a "business card view" where you could see a scrollable sheet of your contact pictures (as seen in the Outlook2007 screenshot).

A good idea would also to add a "male/female" AB field, which - if defined - would cause a default male or female picture to be displayed (as can be seen on the screenshot as well).
Comment 61 chrizoo 2009-06-05 20:28:06 PDT
Sorry, but I forgot to say: It would be great if the user could choose how many photographs he/she wants per page (i.e. how big the business cards should be. maybe small+medium+large cards, resulting in 80/40/16 photographs per page for example).

Should I file two new RFEs for comment comment #60, should I wait for a GUI spin-off of this bug as suggested in comment #54, or should this be kept here?
Comment 62 石庭豐 (Seak, Teng-Fong) 2009-06-06 00:55:49 PDT
(In reply to comment #60)
> A good idea would also to add a "male/female" AB field, which - if defined -
> would cause a default male or female picture to be displayed (as can be seen on
> the screenshot as well).

Some contacts could be neutral, eg some company.  In that case, a "neutral" AB field as well as a neutral picture is needed.
Comment 63 Wout Mertens 2009-06-06 06:16:23 PDT
The Apple AddressBook application has an icon for companies (a stylized citiscape). I'm not sure if I want to go through my address book and say if my contacts are male or female.

If the M/F gets implemented, perhaps a name database could be leveraged to fill in that information mostly automatically?
Comment 64 chrizoo 2009-06-06 06:35:42 PDT
(In reply to comment #63)
> The Apple AddressBook application has an icon for companies (a stylized
> citiscape). I'm not sure if I want to go through my address book and say if my
> contacts are male or female.

you don't have to go through your AB, that's obviously entirely optional (as is adding photographs, for that matter). Those user who like, could do it (I'd love to) and those who don't just leave it. It's as simple as that. 

> If the M/F gets implemented, perhaps a name database could be leveraged to fill
> in that information mostly automatically?

cool idea!
Comment 65 Josh Geenen (:pi) 2009-06-06 22:08:13 PDT
Created attachment 381992 [details] [diff] [review]
Work in progress

This is what I came up with today.  It isn't nearly finished and probably isn't perfect since I've had a migraine for most of the day. ;)  Comments are welcome--especially about the savePhoto function: nsIFileOutputStream.init and the buffer size of 8192 [0].

I'd also like to suggest splitting several of the ideas here into separate bugs (male/female/neutral/business field and default image, overview/business view, etc.).

My current implementation just lets the user type a URI or browse for a file and then sets the src attribute of an HTML img after collapsing the parent element (temporarily).  It has an onload listener that finds the natural size of the image and scales as necessary to fit in the new/edit card dialog.  Once that is finished, it removes the collapsed attribute.

Once the user clicks OK it uses an nsIChannel based on that URI to open an input stream which is then written to a new file (with a unique, random name) in the profile directory.

Right now it saves the entire file URL, but in the future I think it should only store the name of the file so it will work if the profile is moved or copied.  When a contact photo changes the old file is removed (or should it be left alone in case it is used by another contact?).  I'm not worrying about the card view much since it looks like that it is going to be improved in Bug 496620.

I added two attributes for photos: PhotoPath for the local path to the file and PhotoURI for the original location.  Again, PhotoPath should probably change to just the filename (PhotoName, maybe?)

(In reply to comment #48)
> Josh -- this would be super cool if you got around to it.
> 
> My suggestion:
> 
> 1) I'd bias towards URLs -- for some people, they'll have local images, but we
> also want to allow people to use systems like gravatars or other online
> resources as source of images.  So I would suggest the addressbook store URLs,
> and that we define an "image getter service" which can be configured to get
> URLs from a variety of places (locally, system services like on the mac,
> websites like facebook, generated images like gravatar.com).

Could it just open an nsIChannel based on the provided URI and save that stream to a local file like in the savePhoto function in this patch?  Extensions or future patches could then implement custom channels.

> 2) the UI can figure out what size it wants to use depending on the context
> and/or preferences.  The system should probably get the largest image
> available, and we can build resizing either in the UI layer or through an
> intermediate layer if necessary (which I doubt)

Right now it looks like it can be done easily with JavaScript and CSS.  The image can be stored at its native size and scaled down to whatever size is necessary where it is being displayed (resizePhoto in abCommon.js).

> 3) having a fallback URL like the NoPicture.gif would be good (but it should be
> something different)

(In reply to comment #52)
> Yes, assume that there would be a stock "nobody" image that we ship with. We
> have some stock generic people icons to draw from already and could create one
> or two that are missing from other themes.

I found this icon to use for now.  It isn't perfect for this application but is better than nothing.  This icon isn't in Seamonkey, so the 'default' image there is currently blank.

chrome://messenger/skin/icons/identity.png

(In reply to comment #57)
> Long term, an enhancement should dovetail with the extension empowerment
> scheme.

It definitely will; I'm planning on using this in at least one extension myself.  There are several functions to make life easier for extensions developers who want to change the behavior.  Anyone who implements a channel can allow custom URIs or just write an image to the Photos directory and set the PhotoPath attribute of the contact.

[0] Code adapted from the section right above 'More' - https://developer.mozilla.org/en/Code_snippets/File_I%2f%2fO#More
Comment 66 chrizoo 2009-06-06 22:44:26 PDT
Normally, I don't do this to avoid spam, but since the bug was opened in 2002 and a lot of us guys have been waiting for years, I exceptionally want to say a : BIG THANK YOU TO JOSH GREENEN for your programming work! (everyone else, too, of course!)
Comment 67 Roland Felnhofer 2009-06-07 04:23:43 PDT
One question about the backend - The backend will be DB neutral?
So it will work with the local DB as well as with a LDAP backend .(That's actally the only backend I'm intrested in).
Comment 68 Roland Felnhofer 2009-06-07 04:26:01 PDT
One question about the backend - The backend will be DB neutral?
So it will work with the local DB as well as with a LDAP backend .(That's actally the only backend I'm intrested in).
Comment 69 Roland Felnhofer 2009-06-07 04:26:24 PDT
One question about the backend - The backend will be DB neutral?
So it will work with the local DB as well as with a LDAP backend .(That's actally the only backend I'm intrested in).
Comment 70 Roland Felnhofer 2009-06-07 04:29:53 PDT
[submission issues from my smartphone - sorry]
Comment 71 Nomis101 2009-06-07 04:48:41 PDT
I've tested this patch in my own build and it works great. But on Mac OS X it is not possible to add a picture to a contact in the "Mac OS X addressbook", because Thunderbird can only read it, not write into it. So it would be very nice if I also can see the pictures I've added into the Apple addressbook in my Thunderbird addressbook. Or is it better to do this in a followup bug, when this bug is fixed?
Comment 72 Josh Geenen (:pi) 2009-06-07 07:21:50 PDT
(In reply to comment #66)
> Normally, I don't do this to avoid spam, but since the bug was opened in 2002
> and a lot of us guys have been waiting for years, I exceptionally want to say a
> : BIG THANK YOU TO JOSH GREENEN for your programming work! (everyone else, too,
> of course!)
You're welcome, but remember that this is a very small change compared to the improvements others have done.

(In reply to comment #67)
> One question about the backend - The backend will be DB neutral?
> So it will work with the local DB as well as with a LDAP backend .(That's
> actally the only backend I'm intrested in).

Yes, it should work with LDAP and other backends in the future.

(In reply to comment #71)
> I've tested this patch in my own build and it works great. But on Mac OS X it
> is not possible to add a picture to a contact in the "Mac OS X addressbook",
> because Thunderbird can only read it, not write into it. So it would be very
> nice if I also can see the pictures I've added into the Apple addressbook in my
> Thunderbird addressbook. Or is it better to do this in a followup bug, when
> this bug is fixed?

It looks like I forgot to mark the field as read-only for certain address books.  I don't have a Mac so I won't be able to work on that.  Could you make another bug and mark it is dependent on this one?
Comment 73 Nomis101 2009-06-07 08:06:42 PDT
(In reply to comment #72)
> It looks like I forgot to mark the field as read-only for certain address
> books.  I don't have a Mac so I won't be able to work on that.  Could you make
> another bug and mark it is dependent on this one?

OK, this is now Bug 496789.
Comment 74 Ronald Killmer 2009-06-07 14:45:55 PDT
To address a question in Comment 46
I think use of a Max-Width and Max-height in CSS are in order, and should be addressed in the Bug 496620 refresh of the Contact card. How that might impact the WIP is detection of the native size. My feeling is photos need a 'portrait' sizing ratio, yet Icons or other images may be square.

Also from Comment 46
I believe our End-game concept is support of V-card, LDAP and other sources such as WAB or Mac addressbook.  

I think the Channeling idea is brilliant. In my eyes that opens the door for tapping into the toolbx to use auto-update mechanisms. With channeling can we fetch stuff from sources such as Facebook?
Comment 75 Josh Geenen (:pi) 2009-06-07 21:33:46 PDT
Here are my plans for the next patch:
-Split savePhoto into a function that gets an input stream and another that saves the stream to a file in abCommon.js.  This will let extensions create custom streams (like HTTP channels with custom headers) for the photos and avoid duplicating too much code.
-Use the same max photo size for the new/edit contact dialog and view contact pane.
-Only save the photo's filename and change PhotoFile to PhotoName or similar
-Add the attributes to nsIAbCard.idl
-Make the textbox readonly and disable the browse button for read-only directories.

(In reply to comment #74)
> To address a question in Comment 46
> I think use of a Max-Width and Max-height in CSS are in order, and should be
> addressed in the Bug 496620 refresh of the Contact card. How that might impact
> the WIP is detection of the native size. My feeling is photos need a 'portrait'
> sizing ratio, yet Icons or other images may be square.

The newest patch only scales the image to fit into the the tab of the new/edit contact dialog and is closer to landscape.  It is an HTML img element and setting the width through JavaScript didn't work until I set the min-width of the photo to the same value.  The size in the contact view pane was arbitrary (portrait) since that will be changed in the future.  Right now the sizes don't match so I'll try to bring them closer in the next patch.

> Also from Comment 46
> I believe our End-game concept is support of V-card, LDAP and other sources
> such as WAB or Mac addressbook.  
> 
> I think the Channeling idea is brilliant. In my eyes that opens the door for
> tapping into the toolbx to use auto-update mechanisms. With channeling can we
> fetch stuff from sources such as Facebook?

Possibly, but remote sources like Facebook, Google, MySpace, etc. all require authentication to get friends/contacts and some also require authentication to retrieve the photo

Someone could create a channel with a URI like "facebook://john.doe" that would use an existing authorization token if necessary to fetch John Doe's photo through an HTTP channel.  If authorization is required then the HTTP channel would just have to set the header or whatever the API requires.

Things like this may be done in an extension that would bypass the contact dialog and save the photo then set the contact's attributes to match the filename.  I was able to do this with gContactSync fairly easily with an HTTP channel and the custom authorization header.  I may add basic photo functionality in a test version of my extension (TB 2, 3 and Seamonkey) to get some extra feedback.
Comment 76 石庭豐 (Seak, Teng-Fong) 2009-06-08 06:59:18 PDT
(In reply to comment #63)
> The Apple AddressBook application has an icon for companies (a stylized
> citiscape). I'm not sure if I want to go through my address book and say if my
> contacts are male or female.

Hmm, in this case, maybe a fourth "neutral human" picture is needed.  The list is getting longer...

> If the M/F gets implemented, perhaps a name database could be leveraged to fill
> in that information mostly automatically?

The idea is cool, but not very practical.  First, this only works for Western first names.  Second, even for Western first names, it's not possible to have a database of most first names.  Think about first names used in non-Western Europe, and you can get a lot of name not seen in English (not even in Spanish or French).  Third, there are first names which are used for both sexes, eg Jacky, Ronny, Leslie, etc.

My points is, the effort you make to implement this function could only be appreciated by 20% (or even less) of users in the world.
Comment 77 Andreas Nilsson (:andreasn) 2009-06-08 07:32:10 PDT
I'm not sure stylized male and female are a good idea, but I think it's simpler to just go with a stylized sign-language person (that can't really be classified as male or female) *

* A funny side story is that the engineer/illustrator who did a lot of the Swedish signs referred to the "Swimming site road sign" [1] as "swimming girl" in a interview.

1. http://www.transportstyrelsen.se/Vagmarken/Lokaliseringsmarken_for_upplysning_om_serviceanlaggningar_mm/H15/H15-1/H15-1.jpg
Comment 78 Josh Geenen (:pi) 2009-06-08 22:01:31 PDT
Created attachment 382238 [details] [diff] [review]
Newer WIP

This has a few improvements that I previously mentioned:
-Splits the function that saves the input stream to a file into two to allow extensions to create a custom input stream more easily.
-Use the same max photo size ratio (1:1 for now) for the new/edit contact dialog (225x225) and view contact pane (150x150).
-Saves the photo's filename as PhotoName, which should allow moving the profile
-Made the textbox readonly and disabled the browse button for read-only
directories.
-Changed the default image to abcard-large.png, which doesn't work in Seamonkey.
Comment 79 Josh Geenen (:pi) 2009-06-14 21:09:04 PDT
Created attachment 383218 [details]
Preview in Windows Vista

Here is a newer preview using Windows Vista with the default theme and newly checked out and compiled source.  Since the previous patch I have lowered the size limit for the preview in the contact dialog to about 125px by 125px.  The image in the contact view pane is a generic image already in TB that is be used as the default for contacts without photos.

Any comments on the appearance?  And should the label "Photo URI" read "Photo Location", just "Location", or something more friendly?  I see now that I forgot a colon at the end of the label.

The "Update" button isn't necessary.  Browse already updates the photo and I could add an onchange listener to the textbox, but then anyone pasting or typing a URI would have to click somewhere else anyway.
Comment 80 石庭豐 (Seak, Teng-Fong) 2009-06-15 03:59:05 PDT
I'd say "Location" is enough.  "URI" is a too technical term.

I see that there's the label called "Preview" on the upper side of the photo.  Is there somewhere the non-preview photo can be seen?
Comment 81 Bryan Clark (DevTools PM) [@clarkbw] 2009-06-15 08:53:01 PDT
Awesome work Josh!  I wonder if it will be slightly simpler if we have two different entries for a remote url and a local file.  I'm not sure what a better button than ( Update ) would be even for this new layout...

(o) Web
    [ http://...                    ]  ( Update )
( ) On This Computer
    [                               ]  ( Browse )

For the local file it would be good to remove the file URI similar to how the Sound File browse works in the Preferences -> General tab.

To have reset or remove the existing photo we could use a button near the photo like ( Remove Photo ) that clears out the existing photo in case someone wants to reset it.  However I think if we go with this radio layout we could use another radio option for no photo.

(o) No Photo
( ) Web
    [ http://...                    ]  ( Update )
( ) On This Computer
    [                               ]  ( Browse )

And even provide more options than that for extensions and "company photos" with something like this:

(o) [ Default Photo  | v ]
( ) Web
    [ http://...                    ]  ( Update )
( ) On This Computer
    [                               ]  ( Browse )

Where the Default Photo drop down could offer other options as we decide to make them.

(o) [ Default Photo  | v ]
    | Company            |
    | Male               |
    | Female             |
    +--------------------+

And extensions could modify this drop down adding automatic photo options.

(o) [ Default Photo  | v ]
    | *Gravatar*         |
    | Company            |
    | Male               |
    | Female             |
    +--------------------+
Comment 82 Josh Geenen (:pi) 2009-06-15 19:49:43 PDT
(In reply to comment #80)
> I see that there's the label called "Preview" on the upper side of the photo. 
> Is there somewhere the non-preview photo can be seen?

Do you mean the photo at its native size?  If so then not at the moment.

(In reply to comment #81)
> For the local file it would be good to remove the file URI similar to how the
> Sound File browse works in the Preferences -> General tab.

Agreed, I'll store the URI and display the filename.  Once it is stored in the Photos folder should it show the new filename (random numbers)?

> ...
> And even provide more options than that for extensions and "company photos"
> with something like this:
> 
> (o) [ Default Photo  | v ]
> ( ) Web
>     [ http://...                    ]  ( Update )
> ( ) On This Computer
>     [                               ]  ( Browse )
> 
> Where the Default Photo drop down could offer other options as we decide to
> make them.
> 
> (o) [ Default Photo  | v ]
>     | Company            |
>     | Male               |
>     | Female             |
>     +--------------------+
> 
> And extensions could modify this drop down adding automatic photo options.

That would definitely be an improvement, thanks!  I'll work on this for the next patch and move the photo to one side (left maybe).
Comment 83 Bryan Clark (DevTools PM) [@clarkbw] 2009-06-16 01:42:53 PDT
> (In reply to comment #81)
> > For the local file it would be good to remove the file URI similar to how the
> > Sound File browse works in the Preferences -> General tab.
> 
> Agreed, I'll store the URI and display the filename.  Once it is stored in the
> Photos folder should it show the new filename (random numbers)?

eek, that would probably not help people too much.  Maybe we can store the original filename somewhere?  Likely even the original filename isn't that helpful to show so if it's not stored it might make more sense to just not show anything.  I'd worry that showing random numbers will cause some people to think something has gone wrong and since they aren't helpful it just makes more sense to not show them.
Comment 84 Ronald Killmer 2009-06-16 05:29:58 PDT
I think Brian is right about dropping display of the image file name.  I went and looked at how 'MoreFunctionsForAddressbook' handles this, and it's showing a non-useful long string of alpha-numeric characters preceding .jpg. What may be useful to show under the image is it's file size on disk though.

Another display method could be use of a tooltip to show file info. Assuming the images used are filed within the profile, the displayed path could begin with the profile name and the pathway from there.
Comment 85 石庭豐 (Seak, Teng-Fong) 2009-06-16 07:25:04 PDT
(In reply to comment #82)
> (In reply to comment #80)
> > I see that there's the label called "Preview" on the upper side of the photo. 
> > Is there somewhere the non-preview photo can be seen?
> 
> Do you mean the photo at its native size?  If so then not at the moment.

Well, I don't know. It's your idea to put the "preview" label there, see ;)

I just consider myself as a simple/naive user and when I see this "preview" label, I'm wondering what non-preview would mean.
Comment 86 Josh Geenen (:pi) 2009-06-24 22:45:49 PDT
Created attachment 385052 [details]
Updated preview

This is an updated preview based on Bryan Clark's design in comment 81.  It shows the filename after being saved as random numbers.  It is helpful IMO to see the filename after using the browse button.

I dropped the 'Preview' caption for the photo since it didn't really have a good reason for being there.  I didn't attach a patch yet because the JavaScript isn't finished yet.

FYI I'm going to be busy during the next few weeks so I may be slow to reply or attach patches.
Comment 87 Andreas Nilsson (:andreasn) 2009-06-25 03:46:03 PDT
Created attachment 385075 [details]
generic photo graphics

instead of the bird
Comment 88 Peter Lairo 2009-06-25 11:01:15 PDT
Having the photo (hidden) away on a separate tab seems bad. Maybe it would be better to put the photo on the "Contact" tab and only when the user clicks the image does a dialog come up with the UI in the previously posted screenshot.

Apologies if this has been considered before. (my 2-year old is urging me to come to dinner...)
Comment 89 Bryan Clark (DevTools PM) [@clarkbw] 2009-06-26 11:09:12 PDT
Comment on attachment 385052 [details]
Updated preview

josh, this looks awesome!  nice work.

You need some indenting on the options below each radio button.  Just add class="indent" and it should line up correctly.
Comment 90 Bryan Clark (DevTools PM) [@clarkbw] 2009-06-26 11:16:08 PDT
(In reply to comment #89)
> You need some indenting on the options below each radio button.  Just add
> class="indent" and it should line up correctly.

that's class="indent" on the hbox that wraps the input or combo box

(In reply to comment #88)
> Having the photo (hidden) away on a separate tab seems bad. Maybe it would be
> better to put the photo on the "Contact" tab and only when the user clicks the
> image does a dialog come up with the UI in the previously posted screenshot.
> 
> Apologies if this has been considered before. (my 2-year old is urging me to
> come to dinner...)

We could try showing a preview image in the contact tab that when clicked opens the photo tab.  But space is tight in the first tab and I'm not sure how to fit it in there.  We could also do a similar onClick opener from the contact preview pane since I don't think there's an expectation of another action when clicking on the photo...
Comment 91 Josh Geenen (:pi) 2009-06-28 15:44:22 PDT
(In reply to comment #89)
> (From update of attachment 385052 [details])
> josh, this looks awesome!  nice work.

Thanks for the help with it!

> You need some indenting on the options below each radio button.  Just add
> class="indent" and it should line up correctly.

That did the trick, thanks.

Should the menulist under "Generic Photo" be collapsed for now since there is only one menuitem in it for now?  Extensions or future patches can add items and remove the collapsed attribute.

Another possible source of confusion is the "On the Web" radio button.  If that option is selected with a valid URI then the image at that location is saved to a file.  Opening the dialog again will show the same image but with the "On this Computer" option selected.  One option may be selecting the web option with that URI and saving the photo every time the edit contact dialog is opened instead.

(In reply to comment #88)
> Having the photo (hidden) away on a separate tab seems bad. Maybe it would be
> better to put the photo on the "Contact" tab and only when the user clicks the
> image does a dialog come up with the UI in the previously posted screenshot.

That would provide easier access to the photo, but like Bryan said there isn't much room on that tab at the moment.
Comment 92 Josh Geenen (:pi) 2009-07-03 14:10:58 PDT
Created attachment 386764 [details] [diff] [review]
Patch w/ radio buttons for photo types

This is the patch for the screenshot I attached earlier.

I changed the default image to this image which is in Seamonkey and Thunderbird.
chrome://mozapps/skin/profile/profileicon.png

What do we want to do for the default image?  The attachment in comment 87 looks ok, but it appears male to me.
Comment 93 Nomis101 2009-07-03 16:03:43 PDT
Created attachment 386772 [details]
Updated preview (Mac OS X)

FYI, the "Patch w/ radio buttons for photo types" looks like this on Mac OS X.
Comment 94 Josh Geenen (:pi) 2009-07-06 20:48:30 PDT
Updating the target milestone to the future (Beta 4 ATM).

(In reply to comment #93)
> Created an attachment (id=386772) [details]
> Updated preview (Mac OS X)
> 
> FYI, the "Patch w/ radio buttons for photo types" looks like this on Mac OS X.

Thanks, I wonder why the filefield is indented too far there...  It looks like I left a flex="1" for that hbox so maybe that is the problem.
Comment 95 Nomis101 2009-07-25 02:28:17 PDT
The patch doesn't apply anymore because of:

unable to find 'mailnews/addrbook/resources/content/abCardOverlay.xul' for patching
3 out of 3 hunks FAILED -- saving rejects to file mailnews/addrbook/resources/content/abCardOverlay.xul.rej
unable to find 'mailnews/addrbook/resources/content/abCardViewOverlay.xul' for patching
1 out of 1 hunks FAILED -- saving rejects to file mailnews/addrbook/resources/content/abCardViewOverlay.xul.rej

These files are now in mailnews/addrbook/content/, see Bug 490118.
Comment 96 Josh Geenen (:pi) 2009-07-25 10:29:35 PDT
Created attachment 390656 [details] [diff] [review]
Patch v1

This fixes the bitrot mentioned in the previous comment and removes the flex attribute from the filepicker and web photo hboxes.

Before I request review should I include the generic photo from Andreas Nilsson?  It definitely looks better for this purpose than the tiny profileicon.png.
Comment 97 Nomis101 2009-07-30 11:53:15 PDT
(In reply to comment #96)
> It definitely looks better for this purpose than the tiny
> profileicon.png.
Yes, you are right, the profileicon.png is really too small. I also think it would be better to include a picture in the kind of the generic photo from Andreas Nilsson. But this photo is a male, what if my contact is a female? But maybe we can get the main patch landing, so the bugs which depends on this bug can be started, and we discuss the "generic photo problem" in a followup(?) ?
Comment 98 Bryan Clark (DevTools PM) [@clarkbw] 2009-07-30 14:41:04 PDT
(In reply to comment #96)
> Before I request review should I include the generic photo from Andreas
> Nilsson?  It definitely looks better for this purpose than the tiny
> profileicon.png.

Thanks for updating this!  Lets land it with the current profile photo and then we can spin off another bug for investigating a new generic photo.  Icons and the like are prone to lots of bike shedding so it would be better to do it in another area than this area with the code and an existing icon.  Awesome work josh!
Comment 99 Josh Geenen (:pi) 2009-07-30 20:41:49 PDT
Created attachment 391785 [details]
Patch v1 in Linux

A screenshot of the photo tab in Linux (Gentoo with GNOME 2.26).
Comment 100 Josh Geenen (:pi) 2009-07-30 20:42:25 PDT
Created attachment 391786 [details]
Patch v1 in Windows Vista

A screenshot of the photo tab in Windows Vista.
Comment 101 Nomis101 2009-08-01 13:27:14 PDT
Created attachment 392110 [details]
Patch v1 in Mac OS X

A screenshot of the photo tab in Mac OS X (10.5.7).
(looks very similar to my last screenshot)
Comment 102 Bryan Clark (DevTools PM) [@clarkbw] 2009-08-03 17:50:56 PDT
Comment on attachment 390656 [details] [diff] [review]
Patch v1

I didn't test this on Linux or Windows but I think I have a fix for the alignment issues on the mac.

The mac filefield has a margin-left:23px which is moving it over.  Set that to be margin-left:2px (or more correctly -moz-margin-strat:2px;) and it will line up correctly.

Also I added flex=1 to the filefield element so that it takes up all the space and looks just like the text entry below.

Otherwise it looks great!  If you add in the default image from Andreas to those fixes above it's ui-r+ from me.
Comment 103 Josh Geenen (:pi) 2009-08-12 21:56:17 PDT
(In reply to comment #102)
> (From update of attachment 390656 [details] [diff] [review])
> I didn't test this on Linux or Windows but I think I have a fix for the
> alignment issues on the mac.
> 
> The mac filefield has a margin-left:23px which is moving it over.  Set that to
> be margin-left:2px (or more correctly -moz-margin-strat:2px;) and it will line
> up correctly.

OK, I'll add that to the next patch.

> Also I added flex=1 to the filefield element so that it takes up all the space
> and looks just like the text entry below.

Done, that does look better.

> Otherwise it looks great!  If you add in the default image from Andreas to
> those fixes above it's ui-r+ from me.

OK, I have a patch with that image + a 16 x 16px version but I haven't tried it in Windows yet.  It works with gnomestripe in TB and Seamonkey so far.
Comment 104 Josh Geenen (:pi) 2009-08-16 20:29:16 PDT
Created attachment 394762 [details] [diff] [review]
Patch v1.1 - Adds image from Andreas Nilsson 

This patch adds the image from Andreas Nilsson along with a 16x16 version to all TB and Seamonkey themes. (Thanks for the image, btw!)

It also adds the flex="1" and -moz-margin-start to the filepicker.

I was only able to test this for Thunderbird in Linux (gnomestripe) & Windows Vista (qute/aero) and Seamonkey (classic & modern) in Linux.  That leaves Mac OS/pinstripe.

This patch currently results in warnings like this:
Warning: XUL box for hbox element contained an inline img child, forcing all its children to be wrapped in a block.
Source File: chrome://messenger/content/addressbook/abEditCardDialog.xul
Line: 0
(also happens for addressbook.xul and abNewCardDialog.xul)

Is there a way to avoid these warnings or is it unavoidable?  Like the warning says, it's just an HTML img element inside of an hbox.
Comment 105 Phil Ringnalda (:philor, back in August) 2009-08-16 21:05:38 PDT
Why is it an HTML img instead of an XUL image?
Comment 106 Josh Geenen (:pi) 2009-08-16 21:53:02 PDT
(In reply to comment #105)
> Why is it an HTML img instead of an XUL image?

I originally used the naturalWidth and naturalHeight in JavaScript to manually scale the image.  Now I use CSS (max-width: 10ch; max-height: 10ch; min-width: 1ch;) to take care of the image size.  An HTML img scales the image and keeps the original aspect ratio for images that are too large while the XUL image doesn't with the style I use.
Comment 107 Vlado Valastiak [:wladow] @ Mozilla.sk 2009-08-17 01:43:13 PDT
Comment on attachment 394762 [details] [diff] [review]
Patch v1.1 - Adds image from Andreas Nilsson 

>+++ b/mailnews/addrbook/content/abCardOverlay.xul
>+                  <button onclick="updatePhoto('web');" id="UpdatePhoto"
>+                          label="Update"/>
This needs to be moved to locales

>+++ b/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
>+
>+<!ENTITY Photo.tab                      "Photo">
>+<!ENTITY Photo.accesskey                "o">

You need to revise several accesskeys in other New contact window tabs, otherwise you're adding accesskey collisions. I'd suggest:

Contact tab:
<!ENTITY CellularNumber.accesskey "M"> -> "b"
<!ENTITY HomePhone.accesskey "o"> -> "m"

Work tab:
<!ENTITY Company.accesskey "O">  -> "n"

With this all the collisions should be resolved

>+<!ENTITY BrowsePhoto.label              "Browse">
This button needs an accesskey as well. The same applies to Update button (see above)
Comment 108 Nomis101 2009-08-17 13:32:06 PDT
With Patch v1.1 the filefield lines now up correctly on Mac OS X and looks very good now. :) And I also see your describet warnings on Mac OS X.
Comment 109 Josh Geenen (:pi) 2009-08-17 21:42:34 PDT
Created attachment 394996 [details] [diff] [review]
Patch v1.2 - Fixes accesskeys, updates photo menulist

This patch fixes the accesskeys, adds the Update button text to abCardOverlay.dtd, changes the 'Default Photo' menuitem to 'Default'.

I removed the width from the menu list but Vista still cuts off the end of the text whether it is 'Default' or 'Default Photo' and is probably offset by the width of the image. (see Comment 100)  Is there a good way to avoid that problem?

Should the menulist be hidden by default since there is only one option?  Future patches or extensions could easily display it and add more options.

I just noticed that the Seamonkey String Freeze is on the 27th with a code and feature freeze on the 31st.  I'm moving out of my apartment and back home this week and from home to college next week to start on the 31st so I'm not sure what the next step is...

(In reply to comment #107)
> (From update of attachment 394762 [details] [diff] [review])
> >+++ b/mailnews/addrbook/content/abCardOverlay.xul
> >+                  <button onclick="updatePhoto('web');" id="UpdatePhoto"
> >+                          label="Update"/>
> This needs to be moved to locales

Done, not sure how I missed that.

> You need to revise several accesskeys in other New contact window tabs,
> otherwise you're adding accesskey collisions. I'd suggest:
> 
> Contact tab:
> <!ENTITY CellularNumber.accesskey "M"> -> "b"
> <!ENTITY HomePhone.accesskey "o"> -> "m"
> 
> Work tab:
> <!ENTITY Company.accesskey "O">  -> "n"
> 
> With this all the collisions should be resolved

Those keys worked, thanks!

> >+<!ENTITY BrowsePhoto.label              "Browse">
> This button needs an accesskey as well. The same applies to Update button (see
> above)
Done - r

(In reply to comment #108)
> With Patch v1.1 the filefield lines now up correctly on Mac OS X and looks very
> good now. :) And I also see your describet warnings on Mac OS X.

OK, thanks for testing it. :)
Comment 110 Josh Geenen (:pi) 2009-08-18 05:43:34 PDT
Comment on attachment 394996 [details] [diff] [review]
Patch v1.2 - Fixes accesskeys, updates photo menulist

Requesting review as discussed in #maildev.
Comment 112 Mark Banner (:standard8) 2009-08-26 03:35:21 PDT
Comment on attachment 394996 [details] [diff] [review]
Patch v1.2 - Fixes accesskeys, updates photo menulist

>+  var fp = Components.classes["@mozilla.org/filepicker;1"]
>+	                   .createInstance(nsIFilePicker);

nit: the dots should align.

>+                <hbox style="min-width: 10ch; max-width: 10ch;">
>+                  <spacer flex="1"/>
>+                  <html:img align="center" src="" id="cvPhoto"
>+                            style="max-width: 10ch; max-height: 10ch; min-width: 1ch;"/>
>+                  <spacer flex="1"/>
>+                </hbox>

I think the styles should really be in the theme files, but I think we can get this in without the change and move it out in a separate bug.

>+                <hbox class="indent">
>+                  <menulist id="GenericPhotoList" onclick="updatePhoto('generic');">

Should be using oncommand rather than onclick.

>+                <radio id="file" label="&PhotoFile.label;"
>+                       command="PhotoCmd"
>+                       accesskey="&PhotoFile.accesskey;"/>
>+                <hbox class="indent">
>+                  <filefield id="PhotoFile" maxlength="255" flex="1" disabled="1"
>+                             style="-moz-margin-start:2px;"/>

disable="true"

>+                  <button onclick="browsePhoto();" id="BrowsePhoto"
>+                          label="&BrowsePhoto.label;"
>+                          accesskey="&BrowsePhoto.accesskey;"/>

oncommand here as well and on the other button.

This is certainly nice to have, I can see we could have some places to integrate it into the app more, but this is a good improvement for starters. r=Standard8
Comment 113 Mark Banner (:standard8) 2009-08-26 03:54:48 PDT
Created attachment 396697 [details] [diff] [review]
Patch v1.2a - address comments

As I know Josh isn't around for a few days, here's an updated patch with my comments addressed.

Neil would you be able to sr this before the SM string freeze, or at least give an OK for it and I'll get an sr from bienvenu?
Comment 114 Mark Banner (:standard8) 2009-08-27 13:47:23 PDT
Comment on attachment 396697 [details] [diff] [review]
Patch v1.2a - address comments

Neil doesn't seem to be around, and I'd like to get this in before SeaMonkey's string (and subsequent feature) freeze. David, could you do the sr maybe?
Comment 115 David :Bienvenu 2009-08-27 13:58:35 PDT
Comment on attachment 396697 [details] [diff] [review]
Patch v1.2a - address comments

sr=me for the shared parts.

I'd have preferred lets for the local vars but I can let go of that for now :-)
Comment 116 Mark Banner (:standard8) 2009-08-27 14:30:18 PDT
Checked in: http://hg.mozilla.org/comm-central/rev/5c26af85988a

Thanks to Josh for doing this patch.

Please file any issues in new bugs (after searching the existing ones).
Comment 117 neil@parkwaycc.co.uk 2009-08-27 14:59:16 PDT
Comment on attachment 396697 [details] [diff] [review]
Patch v1.2a - address comments

I haven't really had a chance to look at this properly but I thought I'd just comment on anything that looked unusual.

>+      <vbox id="abPhotoTab" >
>+        <hbox flex="1">
Just write <hbox id="abPhotoTab">

>+          <vbox align="left">
Do you mean align="start"?

>+            <spacer flex="1"/>
And pack="center" too, no doubt?

>+              <html:img align="center" src="" id="photo"
>+                        style="max-width: 25ch; max-height: 25ch; min-width: 1ch;"/>
Does this need to be an <html:img> rather than an <image>? (I know there are some obscure differences between the two and this might be one of them.)
Nit: ch are width units but not height units; if you wanted the preview area to be square then I'd prefer to misuse em units for the width instead. I'm also not sure how useful this is given the surrounding XUL layout.

>+            <command id="PhotoCmd" oncommand="updatePhoto();"/>
I'm guessing here that you mean
<radiogroup onselect="updatePhoto(this.value);">
(although note that this also fires when code changes the selection).

>+                <radio id="generic" label="&GenericPhoto.label;"
>+                       command="PhotoCmd"
>+                       accesskey="&GenericPhoto.accesskey;"
>+                       selected="true"/>
Better style is to use <radio value="generic"> (see below).

>+                      <menuitem label="&DefaultPhoto.label;" selected="true"
>+                                value="chrome://messenger/skin/addressbook/icons/contact-generic.png"
>+                                image="chrome://messenger/skin/addressbook/icons/contact-generic-tiny.png"/>
I really hate seeing skin URLs hardcoded into content like this :-( Also our support for images in menulists used to be awful, has it improved?

>+                             style="-moz-margin-start:2px;"/>
Definitely belongs in the theme.

>+                  <textbox id="PhotoURI" maxlength="255" style="width: 45ch;"
What was wrong with flex="1"?

>+                           class="AddressCardEditWidth"/>
Ironically this class exists purely to provide a width style...

>+            <spacer flex="1"/>
Another pack="center" candidate?

> var gOnSaveListeners = new Array();
> var gOkCallback = null;
> var gHideABPicker = false;
>+var originalPhotoURI = "";
gOriginalPhotoURI

>       if (directory.readOnly) 
>       {
>+        // Disable the photo field and buttons
>+        document.getElementById("generic").disabled          = true;
>+        document.getElementById("GenericPhotoList").disabled = true;
>+        document.getElementById("file").disabled             = true;
>+        document.getElementById("web").disabled              = true;
>+        document.getElementById("PhotoURI").readOnly         = true;
>+        document.getElementById("PhotoURI").emptyText        = "";
>+        document.getElementById("BrowsePhoto").disabled      = true;
>+        document.getElementById("UpdatePhoto").disabled      = true;
>         // Set all the editable vcard fields to read only
If you read all of the comments, you'll see that the best place to disable the photo fields is before or after the birthday fields.

>+  document.getElementById("PhotoType").selectedItem =
>+    document.getElementById(type ? type : "generic");
By giving the <radio> elements values instead of ids this simply becomes
document.getElementById("PhotoType").value = type || "generic";
(Or you can even get away without the || "generic" if you don't mind reading it back as an empty type.)

>+    var file = Components.classes["@mozilla.org/network/io-service;1"]
>+                         .getService(Components.interfaces.nsIIOService)
>+                         .newURI(originalPhotoURI, null, null)
>+                         .QueryInterface(Components.interfaces.nsIFileURL)
>+                         .file;
You can also do this through the file protocol handler; unfortunately I don't know which way is preferred.

>+    if (file) {
The variable "file" will surely have a non-null value at this point... if anything went wrong above then an exception would have been thrown.

>+  else if (type == "web") {
Could this possibly have been better written as a switch statement?

>+  var type = document.getElementById("PhotoType").selectedItem.id;
... and this becomes document.getElementById("PhotoType").value

>+    photoURI = "file://" + document.getElementById("PhotoFile").file.path;
No, that's definitely not how to convert a file into a URI. Again, either the IO service or the file protocol handler can do this correctly for you.

>+    if (type != "generic") {
>+      cardproperty.setProperty("PhotoType", "file");
What happened to "web"?

>+    value = file ? "file://" + file.path : "";
Ditto. (I wish we'd realised that it would be really useful if <filefield> could also work with file URIs and implemented it for 1.9.1 ...)

>+  document.getElementById("photo").setAttribute("src", value ? value
>+                                                             : defaultPhotoURI);
value || defaultPhotoURI
Although if you're using a XUL image, you can of course style the default image (for when there is no src) in the skin CSS like a good application would...

>+  try {
>+    file.append(aName);
>+  }
>+  catch (e) {
>+    return false;
>+  }
>+  if (file.exists()) {
>+    try {
>+      file.remove(false);
>+      return true;
>+    }
>+    catch (e) {}
>+  }
>+  return false;
You could just put this all in one try block i.e.
try {
  file.append(aName);
  file.remove(false);
  return true;
} catch (e) {}
}
return false;

>+  // Add All Files & Image Files filters and select the latter
>+  fp.appendFilters(nsIFilePicker.filterAll | nsIFilePicker.filterImages);
Annoyingly for what looks like a bitfield you're actually better off calling appendFilters separately for each filter so that filterImages appears first in the list which is what the user would expect.

>+  if (!(aFile instanceof Components.interfaces.nsIFile))
>+    throw "Invalid file passed to saveStreamToFile";
[Don't need this, xpconnect will complain when you try to init the fstream.]

>+  buffer.writeFrom(aIStream, aIStream.available());
I'm not sure that this is guaranteed unless you're reading from cache, which you want to be doing anyway, to avoid hitting the network. (I think there is a flag you can set on the channel to require a cache read.)

>+      aChannel.QueryInterface(Components.interfaces.nsIHttpChannel);
>+      var header = aChannel.getResponseHeader("Content-Type");
A channel already has a contentType attribute (with the ;parameters removed).

>+      switch (type.toLowerCase()) {
>+        case "image/png":
>+          return ".png";
>+        case "image/jpeg":
>+          return ".jpg";
>+        case "image/gif":
>+          return ".gif";
We have a MIME service to do this, I think.

>+  var index = aUri ? aUri.lastIndexOf(".") : -1;
If you passed the actual URI object from the IO service, and that URI object implemented nsIURL, then you could reliably get the extension from the URL, rather than guessing using string manipulation. (If the URI object does not implement nsIURL then you might give up or you might look for a nested URI.)
Comment 118 neil@parkwaycc.co.uk 2009-09-07 02:03:53 PDT
I used bug 513471 to fix most of my nits except the following:

(In reply to comment #117)
>>+              <html:img align="center" src="" id="photo"
>>+                        style="max-width: 25ch; max-height: 25ch; min-width: 1ch;"/>
>Does this need to be an <html:img> rather than an <image>?
[Yes it does.]

>Nit: ch are width units but not height units; if you wanted the preview area to
>be square then I'd prefer to misuse em units for the width instead. I'm also
>not sure how useful this is given the surrounding XUL layout.
[I forgot to do this.]

>>+                      <menuitem label="&DefaultPhoto.label;" selected="true"
>>+                                value="chrome://messenger/skin/addressbook/icons/contact-generic.png"
>>+                                image="chrome://messenger/skin/addressbook/icons/contact-generic-tiny.png"/>
>I really hate seeing skin URLs hardcoded into content like this :-( Also our
>support for images in menulists used to be awful, has it improved?
[Haven't worked out how to do this yet.]

>>+                             style="-moz-margin-start:2px;"/>
>Definitely belongs in the theme.
[I removed this from the content, but I didn't adjust any themes.
 Maybe the correct thing to do is to adjust the filefield margins.]

>>+    var file = Components.classes["@mozilla.org/network/io-service;1"]
>>+                         .getService(Components.interfaces.nsIIOService)
>>+                         .newURI(originalPhotoURI, null, null)
>>+                         .QueryInterface(Components.interfaces.nsIFileURL)
>>+                         .file;
>You can also do this through the file protocol handler; unfortunately I don't
>know which way is preferred.
[I didn't change this.]

>>+  if (!(aFile instanceof Components.interfaces.nsIFile))
>>+    throw "Invalid file passed to saveStreamToFile";
>[Don't need this, xpconnect will complain when you try to init the fstream.]
[I didn't change this.]

>>+  buffer.writeFrom(aIStream, aIStream.available());
>I'm not sure that this is guaranteed unless you're reading from cache, which
>ou want to be doing anyway, to avoid hitting the network. (I think there is a
>flag you can set on the channel to require a cache read.)
[I haven't worked out what the best thing to do here is.]

Note You need to log in before you can comment on or make changes to this bug.