235577 - Implement Add to Addressbook and compose mailto to contextual menu in message body

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Robert Rothenberg]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

Suppose you have an E-mail which has E-mail addresses in it. Mozilla recognizes
that these are E-mail addresses, and one can right-click on them to copy or
compose mail to, but there's no "Add to address book" option.

Reproducible: Always
Steps to Reproduce:
1. Open an E-mail that has an E-mail address in the body
2. Right click on the highlighted address

Comment 1

[Robert Accettura [:raccettura]]

I don't see an obvious dup, and I've got a patch (kind of), so confirming.

If there  is a bug, and I missed it, someone enlighten me (and just dup it to
this one).  Apologies in advance.

Comment 2

[Robert Accettura [:raccettura]]

Attachment: Patch v1 (Partial, doesn't totally work)

Patch, explanation forthcoming.

Comment 3

[Robert Accettura [:raccettura]]

CC--> mscott

I wanted to try this one, and got most of the way there, I'm just short a tiny
bit.  Apparantly I need something between:

+          addresses = textToSubURI.unEscapeNonAsciiURI(characterSet, addresses);

and

+                       {addresses:addresses, displayName:displayName});


Exactly what, I don't know :-(

So I gave it my best shot.  Perhaps someone can fill me in a bit.

Comment 4

[Scott MacGregor]

Robert, this may be easier to do directly in thunderbird's version of
nsContextMenu.js first (mozilla\mail\base) with the suite you don't know that
mail is installed and you don't want to be including this new mail code if the
suite was installed without mail.

As far as the JS part of your patch, did you find where the context menu for add
to address book is implemented for the header pane? You should be able to copy
most of that.

Comment 5

[Robert Accettura [:raccettura]]

(In reply to comment #4)
> Robert, this may be easier to do directly in thunderbird's version of
> nsContextMenu.js first (mozilla\mail\base) with the suite you don't know that
> mail is installed and you don't want to be including this new mail code if the
> suite was installed without mail.

That's easy enough


> As far as the JS part of your patch, did you find where the context menu for add
> to address book is implemented for the header pane? You should be able to copy
> most of that.
I did
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/msgHdrViewOverlay.js#774

Comment 6

[Robert Accettura [:raccettura]]

Attachment: Patch v1

My mistake last night was just stupid, now that I found some more samples, and
understand how it works.  

So anyway, submitted for your approval.

I do wonder if the position on the menu is good, or should it be on the top
with a separator under it (separating it fromt he rest of the options).  Not
quite sure on that though.

Comment 7

[Robert Accettura [:raccettura]]

Attachment: Patch v2

Removed unnecessary variable.

Comment 8

[Scott MacGregor]

Comment on attachment 142497 [details] [diff] [review]
Patch v2

Couple comments:

1) I didn't understand why we look for a question mark and why we look for it
after the mailto portion. Is this so we can ignore any ?subject="foo" stuff?

2) Do we do this same charset unescaping for the case when you click add to
address book from the mail envelope? Just checking to make sure this is the
right thing to do.

3) Instead of hard coding a length of 7 for mailo, try declaring a constant at
the top of the JS file...i..e. kMailtoSchemeLength or something

4) I think we should re-arrange & add some options to this conext menu when you
click on an  email address.

What do you think about something like:

Select All
----------
Add To Address Book...
Compose Mail To
Copy Link Location
Copy Email Address

I wonder if Select All shouldn't be at the bottom in that scenario  with a
separator above it. But I'm guessing it's hard to move it to the bottom for
this case when it is always at the top for other right clicks in the message
pane...

Comment 9

[Robert Accettura [:raccettura]]

(In reply to comment #8)
> (From update of attachment 142497 [details] [diff] [review])
> Couple comments:
> 
> 1) I didn't understand why we look for a question mark and why we look for it
> after the mailto portion. Is this so we can ignore any ?subject="foo" stuff?
Well, I copied it from somewhere else. ;-)

I kept it in because every so often I get an email from a mailinglist or some
other mass mailing (legitimate mass mailing, not spam), that isn't so cleanly
translated from HTML -> text.  So often, there sometimes is a ?subject=foo.  

Should I remove it?

> 
> 2) Do we do this same charset unescaping for the case when you click add to
> address book from the mail envelope? Just checking to make sure this is the
> right thing to do.
Again, it's from copy mail.  I don't think we do it when we copy from mail
envelope.  I take it, we should remove it?  
> 
> 3) Instead of hard coding a length of 7 for mailo, try declaring a constant at
> the top of the JS file...i..e. kMailtoSchemeLength or something
No problem.
> 
> 4) I think we should re-arrange & add some options to this conext menu when you
> click on an  email address.
> 
> What do you think about something like:
> 
> Select All
> ----------
> Add To Address Book...
> Compose Mail To
> Copy Link Location
> Copy Email Address

I like it.  I suppose I should add a bug for adding Add To Address Book?  Unless
I'm blind.
> 
> I wonder if Select All shouldn't be at the bottom in that scenario  with a
> separator above it. But I'm guessing it's hard to move it to the bottom for
> this case when it is always at the top for other right clicks in the message
> pane...
> 
I think it typically has app specific functions, then copy/paste functions. 
Then select all.  Each separated by breaks:

Add To Address Book...
Compose Mail To
----------
Copy Link Location
Copy Email Address
----------
Select All

But that may be to many breaks for the amount of options in the menu.

Comment 10

[Scott MacGregor]

Ok, so let's keep what you got...

1) add the constant declaration

2) Change the context menu order to look like the following for a mailto click:
Add To Address Book...
Compose Mail To
Copy Link Location
Copy Email Address
----------
Select All

3) If it is too hard to move select all for this case, then leave it at the top
(just reverse the order of the items)

Let's do all of that in this bug if you are up for it :)

Comment 11

[Robert Accettura [:raccettura]]

I'm a little bogged down with some stuff right now.  If it eases up (and I can
keep myself from lying in bed watching TV, as I'm still recovering from a bad
cold).  I'll have a patch soon.  Otherwise it may by closer to the end of the week.

Comment 12

[Robert Accettura [:raccettura]]

Sorry for bugspam... 

I take it we are adding "Compose Mail To" in this bug?  Shouldn't be a big deal,
but just wanted to clarify.

Comment 13

[Scott MacGregor]

yes we are adding compose mailto as well in this bug now.  hope you feel better
soon.

Comment 14

[Scott MacGregor]

Comment on attachment 142497 [details] [diff] [review]
Patch v2

minusing since we are going to add some more stuff to this patch

Comment 15

[Robert Accettura [:raccettura]]

Ok, adding a compose was easy as pie.  
I created a function getEmail() for that whole email extraction process, since
we technically use it 3 times (copyEmail, addEmail, and composeEmail).  So clean
up was welcome.  Set mailto's size (7) as a constant.  

All the above took a few minutes.  My first few minutes coding in a few days now
:-)  Feels good.


(In reply to comment #10)
> 2) Change the context menu order to look like the following for a mailto click:
> Add To Address Book...
> Compose Mail To
> Copy Link Location
> Copy Email Address
> ----------
> Select All
> 
> 3) If it is too hard to move select all for this case, then leave it at the top
> (just reverse the order of the items)
> 
> Let's do all of that in this bug if you are up for it :)

Ok, I've got to be honest.  I'm clueless on how I should go about doing this. 
:-/ (or should I say :-\).

Other than that, this patch is mostly complete.

Comment 16

[Robert Accettura [:raccettura]]

Attachment: Patch v3

Captain Smiles once again has arrived.

I managed to get all mission objectives accomplished.

Comment 17

[Robert Accettura [:raccettura]]

Scott,

I guess we're not landing this until 1.7 is out the door and 1.8a opens up. 
This one slipped my radar for a while.  But how about a review when you get a
chance?

Comment 18

[Scott MacGregor]

Thanks for the reminder. I forgot about this bug.

I'm getting an error with this patch when I try to use the context menus:

JavaScript error:
 line 0: gContextMenu.composeEmailTo is not a function

I also have a new version of your patch which makes it so we don't change any
DTD files in mozilla/xpfe. I also changed my mind about having a second select
all menu item that we show for this case.  I'll attach the version of your patch
that I'm running with that I can't seem to get to work right.


I'm not sure why the defintions aren't showing up for the nsContextMenu object

Comment 19

[Scott MacGregor]

Attachment: new patch

Comment 20

[Robert Accettura [:raccettura]]

I just manually put that into the 20040312 build, and it works perfectly.

Comment 21

[Scott MacGregor]

Attachment: updated fix

This is the only way I could get the JS calls to work. I moved them to
mailContextMenus.js which is where one could argue they belong anyway since
they are so mail specific.

I also made sure a separator was visible after Select All when over a mailto
link. I'm sure I broke a case where we are now going to get a separator where
we didn't before :).

Note: we already had entity definitions for these menu items because they are
in the message header context menu. So we didn't need to add duplicate DTD
entities, so I took that part out of the patch too.

Nice work Robert!

Comment 22

[Scott MacGregor]

I checked this last patch in.

Robert, are the methods getEmail, AddToAB, etc. duplicated somewhere else for
the message header context menu item? Maybe both context menus can share the
same functions in mailContextMenus.js so we don't have 2 copies lying around. 

Comment 23

[Robert Accettura [:raccettura]]

Well we have this:
http://lxr.mozilla.org/mozilla/source/mailnews/base/resources/content/msgHdrViewOverlay.js#774

Comment 24

[Mike Cowperthwaite]

Man, I wish I'd been aware of this bug earlier.

Scott says "this last patch" -- I assume this means Attachment 144817 [details] [diff], which has 
no reviews -- was checked in yesterday.  I take it it's in the trunk (now 1.8)?  
I don't know where to look for those builds.

Two questions for Scott:

Why "Compose Mail To"?  That's the action you get with a single click, why 
should the context menu be cluttered with that item as well?  It makes sense 
(sort of) to have that in the context menus for addresses in the envelope, since 
those have deliberately been created as not-really-links.

Why are you suggesting moving Select All?  Is it OK to simply ignore the current 
specs, just because they're out of date?
  http://www.mozilla.org/mailnews/specs/threepane/MailMenus.html
Granted, this spec doesn't have any entry at all for a context menu on an email 
address within a message; but it does show all the context menus on the message 
body with Select All or Copy at the top.  (Incidentally, "Select All" will be 
paired with "Copy" if any text is selected in the message.)

Finally, I question putting the action functions that get run on menu selection 
*in* mailContextMenus.js -- previously, that file, except those two functions at 
the very bottom, had appeared to be only about controlling the show/hide and 
enable/disable of the menu items. Shouldn't these new actions be integrated into 
the command system?  Or is that only for actions on messages?

Comment 25

[Scott MacGregor]

sorry this is a tbird only thing hence no review. didn't realize it was filed
under mailnews. No this is not in 1.8, not it has no bearing on the UI specs for
seamonkey.