290881 - Implement Add to Address book / Compose mail to context menus

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Ian Neal]

The context menu of the header view has context menu options for Add to Address
Book and Compose Mail to - this is the equivalent of TB bug 235577

Comment 1

[Ian Neal]

Attachment: Provisional patch v0.1b

This patch:
* Splits nsContextMenu's copyEmail function into three functions so bits can be
used elsewhere.
* Moves AddNodeToAddressBook, SendMailToNode, CopyEmailAddress and CreateFilter
from msgHdrViewOverlay into mailContextMenus so bits can be used elsewhere.
* Moves popup "emailAddressPopup" from mail3PaneWindowVertLayout.xul,
messageWindow.xul and messenger.xul into mailWindowOverlay.xul as part of
overlay.
* Adds "Add to Address Book" and "Compose Mail to" context menus to message
pane.

Comment 2

[[e-mail]]

Without actually having looked at the patch I'd suggest that emailAddressPopup
more accurately belongs in msgHdrViewOverlay.xul as that contains the widgets
whose bindings in mailWidgets.xml actually reference the popup.

Comment 3

[Ian Neal]

Attachment: Revised Patch v0.1c

Changes since v0.1b
* Moved combined popup into msgHdrViewOverlay.xul instead of
mailWindowOverlay.xul (as suggested by Neil)
* Added "Create Filter from..." to mailnews context menu

Comment 4

[Karsten Düsterloh]

Comment on attachment 182864 [details] [diff] [review]
Revised Patch v0.1c

>Index: xpfe/communicator/resources/content/nsContextMenu.js
>===================================================================
>+const kMailToLength = 7;
[...]
>         var url = this.linkURL();
>         var qmark = url.indexOf( "?" );
>         var addresses;
>         
>-        if ( qmark > 7 ) {                   // 7 == length of "mailto:"
>-            addresses = url.substring( 7, qmark );
>-        } else {
>-            addresses = url.substr( 7 );
>-        }
>+        if ( qmark > kMailToLength )
>+            addresses = url.substring( kMailToLength, qmark );
>+        else
>+            addresses = url.substr( kMailToLength );
> 
>         // Let's try to unescape it using a character set
>         try {
>           var ownerDocument = new XPCNativeWrapper(this.target, "ownerDocument").ownerDocument;
>           var characterSet = new XPCNativeWrapper(ownerDocument, "characterSet").characterSet;
>           const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
>                                          .getService(Components.interfaces.nsITextToSubURI);
>           addresses = textToSubURI.unEscapeURIForUI(characterSet, addresses);

This is pretty complicated. You could just say 

addresses = textToSubURI.unEscapeURIForUI(characterSet,
this.linkURL().match(/^mailto:([^?]+)/)[1]);

at the end of the try-clause and get rid of nearly all the rubbish before the
try, including the global constant.

>Index: mailnews/base/resources/locale/en-US/msgHdrViewOverlay.dtd
>===================================================================
>+<!ENTITY CreateFilterFrom.label            "Create Filter from...">

'From' should start with an upper case F for consistency, and so should the To
in "Add to Address Book...".
'Compose Mail To' should end in ..., too, since it opens the editor.

>Index: mailnews/base/resources/locale/en-US/messenger.dtd
>===================================================================
> <!-- Message Header View Popup -->
> <!ENTITY AddToAddressBook.label "Add to Address Book...">
> <!ENTITY AddToAddressBook.accesskey "B">
> <!ENTITY SendMailTo.label "Compose Mail To">
> <!ENTITY SendMailTo.accesskey "s">
> <!ENTITY CopyEmailAddress.label "Copy Email Address">
> <!ENTITY CopyEmailAddress.accesskey "C">
>-<!ENTITY CreateFilter.label "Create Filter from Message...">
>-<!ENTITY CreateFilter.accesskey "F">
>+<!ENTITY CreateFilterFrom.label "Create Filter from...">
>+<!ENTITY CreateFilterFrom.accesskey "F">
> <!ENTITY BlockAddress.label "Block Address">
> <!ENTITY BlockAddress.accesskey "A">

These definitions overlap with those in msgHdrViewOverlay.dtd. Can't we unify
them?

The context menu of an email address now looks rather unordered:

Currently:			  My suggestion:

 Select All			   Select All
 ----------			   ----------
 Copy Link Location		   Compose Mail To...
 Add to Address Book... 	   Create Filter From...
 Compose Mail To		   Add To Address Book...
 Copy Email Address		   Copy Email Address
 Create Filter from...		   Copy Link Location


r=me with that.

Comment 5

[Ian Neal]

Attachment: Updated patch v0.1d

Changes since v0.1c:
* Unbitrotted
* Altered nsContextMenu.js to use match as suggested
* Capitalised "to" and "from" in entities and added "..." to compose entity
* Removed duplication between messenger.dtd and msgHdrViewOverlay.dtd by adding
the latter as an entity overlay to mailWindowOverlay.xul - in theory
msgHdrViewOverlay.xul could be collapsed into mailWindowOverlay.xul but, if
that is wanted, it is probably a separate bug.

Carrying forward r= and requesting sr=

Comment 6

[Ian Neal]

Attachment: Folded in patch v0.2

Changes from v0.1e:
* Do not create gContextMenu for popup id="emailAddressPopup" instead just
duplicate the clipboard copy code (about 3 lines)
* Corrected now incorrect comments
* Show/hide the new context items *before* figuring out the separators
* Fold msgHdrViewOverlay.xul into mailWindowOverlay.xul and
msgHdrViewOverlay.dtd into messenger.dtd to remove duplication of entities
* Removed unused saveAll.label entity
* Added accesskeys for Detach All and Delete All in attachment context menu

Comment 7

[Ian Neal]

Comment on attachment 193739 [details] [diff] [review]
Folded in patch v0.2

Requesting another review because of the number of changes.

Comment 8

[Karsten Düsterloh]

Comment on attachment 193739 [details] [diff] [review]
Folded in patch v0.2

>Index: xpfe/communicator/resources/content/nsContextMenu.js
>===================================================================
>+    // Generate email address.
>+    getEmail : function () {
>+        // Get the comma-separated list of email addresses only.
>         // There are other ways of embedding email addresses in a mailto:
>         // link, but such complex parsing is beyond us.
>-        var url = this.linkURL();
>-        var qmark = url.indexOf( "?" );
>-        var addresses;
>+        var addresses = this.linkURL().match(/^mailto:([^?]+)/)[1];

This should be inside the following try...catch block, because match may fail.

>         try {
>           var characterSet = this.target.ownerDocument.characterSet;
>           const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
>                                          .getService(Components.interfaces.nsITextToSubURI);
>           addresses = textToSubURI.unEscapeURIForUI(characterSet, addresses);
>         }
>         catch(ex) {

>Index: mailnews/jar.mn
>===================================================================
>-    content/messenger/msgHdrViewOverlay.xul                                    

The negative impact of this folding is definitely not worth the gain: not only
it breaks SMIME in the normal trunk, but it will break _lots_ of MailNews
extensions, e.g. Enigmail and Mnenhy.
Can't you just put the doubled entities into a separate dtd and include that?

Comment 9

[Ian Neal]

Attachment: New dtd patch v0.1f

Changes since v0.1d:
* Do not create gContextMenu for popup id="emailAddressPopup" instead just use
clipboard code already in mailContextMenus.js and remove some duplication.
* Corrected now incorrect comments.
* Show/hide the new context items *before* figuring out the separators.
* Pull duplicated entities into separate dtd file for both
msgHdrViewOverlay.xul and mailWindowOverlay.xul to reference.
* Removed unused saveAll.label entity.
* Added accesskeys for Detach All and Delete All in attachment context menu.

Comment 10

[Karsten Düsterloh]

Comment on attachment 195638 [details] [diff] [review]
New dtd patch v0.1f

Just some final nits:

>Index: xpfe/communicator/resources/content/nsContextMenu.js
>===================================================================
>         try {
>+          addresses = this.linkURL().match(/^mailto:([^?]+)/)[1];
>+
>+          // Let's try to unescape it using a character set
>           var characterSet = this.target.ownerDocument.characterSet;
>           const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
>                                          .getService(Components.interfaces.nsITextToSubURI);
>           addresses = textToSubURI.unEscapeURIForUI(characterSet, addresses);

I'm usually no friend of the "x = e; x = something(x);" pattern, but I agree
that the line with  unEscapeURIForUI would become too wide. Just move the first
addresses assignent directly above the other one.

>Index: mailnews/base/resources/content/mailContextMenus.js
>===================================================================
>+function CopyString(aString)
>+{
>+  var contractid = "@mozilla.org/widget/clipboardhelper;1";
>+  var iid = Components.interfaces.nsIClipboardHelper;
>+  var clipboard = Components.classes[contractid].getService(iid);
>+  clipboard.copyString(aString);
>+}

This function is a mere one-liner on the inside, no need to create all kinds of
vars if you don't check them anyway. But exception handling might be a good
idea then...

try
{
  Components.classes["@mozilla.org/widget/clipboardhelper;1"]
	    .getService(Components.interfaces.nsIClipboardHelper)
	    .copyString(aString);
}
...

r=me with that.

Comment 11

[Ian Neal]

Attachment: Reduced var patch v0.1g

Changes since v0.1f
* In getEmail function moved the first addresses assignent directly above the
other one
* Changed CopyString to be a split one-liner - try/ctach not required as it is
already called from within try/catch.

Carrying forward r= and requesting sr

Comment 12

[[e-mail]]

Comment on attachment 197251 [details] [diff] [review]
Reduced var patch v0.1g

>+        <menuitem id="context-addemail"
>+                  label="&AddToAddressBook.label;"
>+                  accesskey="&AddToAddressBook.accesskey;"
>+                  oncommand="AddEmailToAddressBook(gContextMenu.getEmail(), '');"/>
A wish: it would be nice to have a display name here (maybe using
gatherTextUnder from utilityOverlay.js)...

>+function CopyString(aString)
>+{
>+  Components.classes["@mozilla.org/widget/clipboardhelper;1"]
>+            .getService(Components.interfaces.nsIClipboardHelper);
>+            .copyString(aString);
>+}
Extraneous ; after .getService(...)

Comment 13

[Ian Neal]

Attachment: No semi and gather patch v0.1h (Checked in trunk/branch 1.8 & 1.8.0)

Changes since v0.1g:
* Removed extraneous ;
* Added gContextMenu.linkText() instead of '' in AddEmailToAddressBook call

Carrying forward r=

Comment 14

[Ian Neal]

Comment on attachment 197631 [details] [diff] [review]
No semi and gather patch v0.1h (Checked in trunk/branch 1.8 & 1.8.0)

Checking in
xpfe/communicator/resources/content/nsContextMenu.js; new revision: 1.110;
previous revision: 1.109
mailnews/jar.mn; new revision: 1.100; previous revision: 1.99
mailnews/base/resources/locale/en-US/msgHdrViewOverlay.dtd; new revision: 1.13;
previous revision: 1.12
mailnews/base/resources/locale/en-US/messenger.dtd; new revision: 1.205;
previous revision: 1.204
mailnews/base/resources/locale/en-US/msgHdrViewPopup.dtd; initial revision: 1.1
mailnews/base/resources/content/msgHdrViewOverlay.js; new revision: 1.144;
previous revision: 1.143
mailnews/base/resources/content/msgHdrViewOverlay.xul; new revision: 1.65;
previous revision: 1.64
mailnews/base/resources/content/mailWindowOverlay.xul; new revision: 1.303;
previous revision: 1.302
mailnews/base/resources/content/mail3PaneWindowVertLayout.xul; new revision:
1.109; previous revision: 1.108
mailnews/base/resources/content/messenger.xul; new revision: 1.261; previous
revision: 1.260
mailnews/base/resources/content/messageWindow.xul; new revision: 1.83; previous
revision: 1.82
mailnews/base/resources/content/mailContextMenus.js; new revision: 1.56;
previous revision: 1.55
done

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2005-10-13-05 on Windows XP SeaMonkey trunk.

Comment 16

[Robert Kaiser]

Comment on attachment 197631 [details] [diff] [review]
No semi and gather patch v0.1h (Checked in trunk/branch 1.8 & 1.8.0)

a=me given all those files are really SeaMonkey-only...

Comment 17

[Ian Neal]

Comment on attachment 197631 [details] [diff] [review]
No semi and gather patch v0.1h (Checked in trunk/branch 1.8 & 1.8.0)

Checking in (branch)
xpfe/communicator/resources/content/nsContextMenu.js;
new revision: 1.108.2.3; previous revision: 1.108.2.2
mailnews/jar.mn;
new revision: 1.99.2.2; previous revision: 1.99.2.1
mailnews/base/resources/locale/en-US/msgHdrViewOverlay.dtd;
new revision: 1.12.8.1; previous revision: 1.12
mailnews/base/resources/locale/en-US/messenger.dtd;
new revision: 1.202.2.2; previous revision: 1.202.2.1
mailnews/base/resources/locale/en-US/msgHdrViewPopup.dtd;
new revision: 1.1.4.2; previous revision: 1.1.4.1
mailnews/base/resources/content/msgHdrViewOverlay.js;
new revision: 1.141.2.4; previous revision: 1.141.2.3
mailnews/base/resources/content/msgHdrViewOverlay.xul;
new revision: 1.64.8.1; previous revision: 1.64
mailnews/base/resources/content/mailWindowOverlay.xul;
new revision: 1.298.2.5; previous revision: 1.298.2.4
mailnews/base/resources/content/mail3PaneWindowVertLayout.xul;
new revision: 1.106.2.2; previous revision: 1.106.2.1
mailnews/base/resources/content/messenger.xul;
new revision: 1.258.2.2; previous revision: 1.258.2.1
mailnews/base/resources/content/messageWindow.xul;
new revision: 1.81.2.2; previous revision: 1.81.2.1
mailnews/base/resources/content/mailContextMenus.js;
new revision: 1.55.4.1; previous revision: 1.55
done

Comment 18

[Ian Neal]

Comment on attachment 197631 [details] [diff] [review]
No semi and gather patch v0.1h (Checked in trunk/branch 1.8 & 1.8.0)

Checking in (branch 1.8.0)
xpfe/communicator/resources/content/nsContextMenu.js;
new revision: 1.108.2.2.2.1; previous revision: 1.108.2.2
mailnews/jar.mn;
new revision: 1.99.2.1.2.1; previous revision: 1.99.2.1
mailnews/base/resources/locale/en-US/msgHdrViewOverlay.dtd;
new revision: 1.12.14.1; previous revision: 1.12
mailnews/base/resources/locale/en-US/messenger.dtd;
new revision: 1.202.2.1.4.1; previous revision: 1.202.2.1
mailnews/base/resources/locale/en-US/msgHdrViewPopup.dtd;
new revision: 1.1.6.2; previous revision: 1.1.6.1
mailnews/base/resources/content/msgHdrViewOverlay.js;
new revision: 1.141.2.1.2.3; previous revision: 1.141.2.1.2.2
mailnews/base/resources/content/msgHdrViewOverlay.xul;
new revision: 1.64.14.1; previous revision: 1.64
mailnews/base/resources/content/mailWindowOverlay.xul;
new revision: 1.298.2.2.4.1; previous revision: 1.298.2.2
mailnews/base/resources/content/mail3PaneWindowVertLayout.xul;
new revision: 1.106.2.1.2.1; previous revision: 1.106.2.1
mailnews/base/resources/content/messenger.xul;
new revision: 1.258.2.1.2.1; previous revision: 1.258.2.1
mailnews/base/resources/content/messageWindow.xul;
new revision: 1.81.2.1.2.1; previous revision: 1.81.2.1
mailnews/base/resources/content/mailContextMenus.js;
new revision: 1.55.10.1; previous revision: 1.55
done