Closed Bug 290881 Opened 20 years ago Closed 19 years ago

Implement Add to Address book / Compose mail to context menus

Categories

(SeaMonkey :: MailNews: Message Display, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: iannbugzilla, Assigned: iannbugzilla)

References

Details

(Keywords: fixed-seamonkey1.0, fixed1.8)

Attachments

(1 file, 6 obsolete files)

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
Attached patch Provisional patch v0.1b (obsolete) (deleted) — Splinter Review
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)
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.
Attachment #181082 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Revised Patch v0.1c (obsolete) (deleted) — Splinter Review
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)
Attachment #182864 - Flags: review?(neil.parkwaycc.co.uk) → review?(mnyromyr)
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+
Attached patch Updated patch v0.1d (obsolete) (deleted) — Splinter Review
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+
Attached patch Folded in patch v0.2 (obsolete) (deleted) — Splinter Review
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+
Attachment #193616 - Flags: superreview?(neil.parkwaycc.co.uk)
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 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-
Attachment #193739 - Flags: superreview?(neil.parkwaycc.co.uk)
Attached patch New dtd patch v0.1f (obsolete) (deleted) — Splinter Review
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)
Blocks: 309057
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+
Attached patch Reduced var patch v0.1g (obsolete) (deleted) — Splinter Review
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 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+
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+
Attachment #197631 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
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)
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Verified FIXED using build 2005-10-13-05 on Windows XP SeaMonkey trunk.
Status: RESOLVED → VERIFIED
Attachment #197631 - Flags: approval-seamonkey1.0?
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+
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)
Keywords: fixed1.8
Whiteboard: fixed-seamonkey1.0
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)
Whiteboard: fixed-seamonkey1.0
Blocks: TB2SM
No longer blocks: TB2SM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: