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)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: iannbugzilla, Assigned: iannbugzilla)
References
Details
(Keywords: fixed-seamonkey1.0, fixed1.8)
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
iannbugzilla
:
review+
neil
:
superreview+
kairo
:
approval-seamonkey1.0+
|
Details | Diff | Splinter Review |
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
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•20 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.
Attachment #181082 -
Flags: review?(neil.parkwaycc.co.uk)
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 4•19 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+
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+
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 8•19 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-
Attachment #193739 -
Flags: superreview?(neil.parkwaycc.co.uk)
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)
Comment 10•19 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•19 years ago
|
||
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•19 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•19 years ago
|
||
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•19 years ago
|
Attachment #197631 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Assignee | ||
Comment 14•19 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)
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 16•19 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•19 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)
Updated•19 years ago
|
Whiteboard: fixed-seamonkey1.0
Assignee | ||
Comment 18•19 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)
Updated•19 years ago
|
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0
You need to log in
before you can comment on or make changes to this bug.
Description
•