Implement Add to Address book / Compose mail to context menus

VERIFIED FIXED

Status

SeaMonkey
MailNews: Message Display
--
enhancement
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

({fixed-seamonkey1.0, fixed1.8})

Trunk
fixed-seamonkey1.0, fixed1.8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

12 years ago
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
(Assignee)

Comment 1

12 years ago
Created attachment 181082 [details] [diff] [review]
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.
Assignee: sspitzer → bugzilla
Status: NEW → ASSIGNED
Attachment #181082 - Flags: review?(neil.parkwaycc.co.uk)

Comment 2

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #181082 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 3

12 years ago
Created attachment 182864 [details] [diff] [review]
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
Attachment #181082 - Attachment is obsolete: true
Attachment #182864 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

12 years ago
Attachment #182864 - Flags: review?(neil.parkwaycc.co.uk) → review?(mnyromyr)

Comment 4

12 years ago
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.
Attachment #182864 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 5

12 years ago
Created attachment 193616 [details] [diff] [review]
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=
Attachment #182864 - Attachment is obsolete: true
Attachment #193616 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #193616 - Flags: review+
(Assignee)

Comment 6

12 years ago
Created attachment 193739 [details] [diff] [review]
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
Attachment #193616 - Attachment is obsolete: true
Attachment #193739 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #193739 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #193616 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 7

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

Requesting another review because of the number of changes.
Attachment #193739 - Flags: review+ → review?(mnyromyr)

Comment 8

12 years ago
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?
Attachment #193739 - Flags: review?(mnyromyr) → review-
(Assignee)

Updated

12 years ago
Attachment #193739 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 9

12 years ago
Created attachment 195638 [details] [diff] [review]
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.
Attachment #193739 - Attachment is obsolete: true
Attachment #195638 - Flags: review?(mnyromyr)
(Assignee)

Updated

12 years ago
Blocks: 309057

Comment 10

12 years ago
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.
Attachment #195638 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 11

12 years ago
Created attachment 197251 [details] [diff] [review]
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
Attachment #195638 - Attachment is obsolete: true
Attachment #197251 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #197251 - Flags: review+

Comment 12

12 years ago
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(...)
Attachment #197251 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Comment 13

12 years ago
Created attachment 197631 [details] [diff] [review]
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=
Attachment #197251 - Attachment is obsolete: true
Attachment #197631 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #197631 - Flags: review+

Updated

12 years ago
Attachment #197631 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Comment 14

12 years ago
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
Attachment #197631 - Attachment description: No semi and gather patch v0.1h → No semi and gather patch v0.1h (Checked in)
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Verified FIXED using build 2005-10-13-05 on Windows XP SeaMonkey trunk.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

12 years ago
Attachment #197631 - Flags: approval-seamonkey1.0?

Comment 16

12 years ago
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...
Attachment #197631 - Flags: approval-seamonkey1.0? → approval-seamonkey1.0+
(Assignee)

Comment 17

12 years ago
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
Attachment #197631 - Attachment description: No semi and gather patch v0.1h (Checked in) → No semi and gather patch v0.1h (Checked in trunk/branch)
(Assignee)

Updated

12 years ago
Keywords: fixed1.8

Updated

12 years ago
Whiteboard: fixed-seamonkey1.0
(Assignee)

Comment 18

12 years ago
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
Attachment #197631 - Attachment description: No semi and gather patch v0.1h (Checked in trunk/branch) → No semi and gather patch v0.1h (Checked in trunk/branch 1.8 & 1.8.0)
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0

Updated

11 years ago
Blocks: 360488

Updated

10 years ago
No longer blocks: 360488
You need to log in before you can comment on or make changes to this bug.