make message sending interfaces scriptable

RESOLVED FIXED in Thunderbird 9.0

Status

MailNews Core
Composition
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

(Depends on: 1 bug)

Trunk
Thunderbird 9.0
x86_64
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
nsIMsgSend is not scriptable, which makes it harder to construct and send messages from extensions like Conversations. There's no good reason for it not to be scriptable. Most of the work is to make an interface for describing attachments that the message sending code can use to build up the attachments for a message.

Since I have to deal with the import code proxying message sending to the ui thread, I might as well fix this at the same time.
(Assignee)

Comment 1

7 years ago
Created attachment 554228 [details] [diff] [review]
wip

this patch includes some of the work to get rid of the sendproxy in the outlook/eudora import code. I'm not sure if it builds since I didn't include the work to get rid of the string bundles in the import code.

All xpcshell and mozmill tests still pass with this patch, and basic user tests seem to work. I had to touch the parts of libmime that deal with attachments, and I drug those parts kicking and screaming out of the early 90's with C++ classes and nsCStrings and nsTArray. I may be able to clean up the patch a bit more, and it's going to need quite a bit more testing.
I'm so glad you just made CreateAndSendMessage scriptable! And... wow... comments \o/

bienvenu++
(Assignee)

Comment 3

7 years ago
Created attachment 554289 [details] [diff] [review]
wip v2

this implements some methods I had left unimplemented, and fixes the case of editing a draft with an inline attachment
Attachment #554228 - Attachment is obsolete: true
David would it make sense to get some crowd testing on this with a try build ?
(Assignee)

Comment 5

7 years ago
(In reply to Ludovic Hirlimann [:Usul] from comment #4)
> David would it make sense to get some crowd testing on this with a try build
> ?

It will, once I've done some more testing, and am closer to the final version of the patch.
(Assignee)

Comment 6

7 years ago
Created attachment 554570 [details] [diff] [review]
fix for review

I'll request a try server build with this patch. Protz, I was hoping you could review the idl changes to make sure they're usable for you, and Neil, I was hoping you could sr the idl changes, and review everything else. Sorry the patch is so big - I decided to use cstrings, and c++ classes and the like to make the code easier to handle. I restrained myself from fixing everything in the libmime files I touched...
Attachment #554289 - Attachment is obsolete: true
Attachment #554570 - Flags: superreview?(neil)
Attachment #554570 - Flags: review?(jonathan.protzenko)
(Assignee)

Comment 8

7 years ago
Created attachment 554669 [details] [diff] [review]
fix line endings

line endings were a bit messed up in prev patch
Attachment #554570 - Attachment is obsolete: true
Attachment #554570 - Flags: superreview?(neil)
Attachment #554570 - Flags: review?(jonathan.protzenko)
Attachment #554669 - Flags: superreview?(neil)
Attachment #554669 - Flags: review?(jonathan.protzenko)
Comment on attachment 554669 [details] [diff] [review]
fix line endings

Review of attachment 554669 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't add anything meaningful here, I'm afraid. However, the changes all look good, and I'm very enthusiastic about getting rid of all the PR_NEWZAP stuff, the SNARF macros, and all the quirky stuff that should've gone long ago. I'm not actively working on compose in a tab right now, but I think this will allow me to use ComposeAndSendMessage directly, which should be enough to get a working replacement going.

However, I remember I had other problems related to this area... They might be fit for that bug, or maybe they belong to another bug, as the patch is pretty big already, I don't know for sure.
- I would like to see nsMsgComposeSendListener scriptable, so that we could get a cheap implementation of various tedious tasks: deleting the previous draft message, displaying a progress window, retrying when it failed... if we are to gradually replace this with a javascript composition process, I think we want to reuse this at least in a first approach.
- I remember having a lot of difficulties getting the URL of the newly saved draft (see nsMsgCompose.cpp:3809), because if you walk up the if blocks, there's an awful lot of conditions that have to be met (I don't remember these exactly). That's probably better to tackle that in another bug, though. Probably when there's someone deep in the code, I only have vague memories of the trouble I had getting this URL.

Note that we still are able to register custom listeners since we can add various types of listeners in the comp fields and the comp params, and the nsMsgComposeSendListener takes care of calling those. So that's not limiting us.

Thanks for this great patch!

::: mailnews/compose/public/nsIMsgSend.idl
@@ +107,5 @@
> +   */
> +  attribute ACString realName;
> +  /**
> +   * If you put a string here, it will show up as the Content-Description
> +   * header. This can be any explanatory text; it's not a file name.

Does that surface anywhere in the UI? I've seen such descriptions in test-generated messages but I can't think of where we would make something useful out of that... may be worth an extra comment?

@@ +111,5 @@
> +   * header. This can be any explanatory text; it's not a file name.
> +   */
> +  attribute ACString description;
> +
> +  /// mac-specific info

If you know what that means exactly, I'd love a more elaborate description. I know this is somehow related to apple-encoded double-attachments whatever, but I've been unable to figure out what this means exactly :-).

@@ +132,5 @@
> +
> +  /// The tmp file in which the (possibly converted) data now resides.
> +  attribute nsILocalFile tmpFile;
> +
> +  /// The type of the data in file_name (not necessarily the same as the type of orig_url.)

Er, why? I don't know if you have the answer, and I understand you might be just copying older comments...

@@ +285,5 @@
> +   *                especially in the case of import.
> +   * @param aUserIdentity identity to send from.
> +   * @param aAccountKey account we're sending message from. May be null.
> +   * @param aFields composition fields from addressing widget
> +   * @param aIsDigest is this a digest message?

what is a digest message?

@@ +291,5 @@
> +   *                     trying to create a message from parts.
> +   * @param aMode delivery mode
> +   * @param aMsgToReplace e.g., when saving a draft over an old draft. May be 0
> +   * @param aBodyType content type of message body
> +   * @param aBody message body text

It is unclear if the body will be picked from the compFields or from the body parameter. Could you clarify that, and also add a remark about the \r\n vs \n if it applies?

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ +1119,5 @@
>    // (e.g. a charset may be specified in HTTP header)
>    //
> +  if (!m_type.IsEmpty() && m_charset.IsEmpty() &&
> +      m_type.LowerCaseEqualsLiteral(TEXT_HTML))
> +    m_charset = nsMsgI18NParseMetaCharset(mTmpFile);

I thought nsCStrings didn't support assignment when the value is '\0'? (I probably misunderstood, this is more for my own educating purposes!)

::: mailnews/compose/src/nsMsgSend.cpp
@@ -549,5 @@
>        for (i = 0; i < m_attachment_count; i++)
>        {
>          nsMsgAttachmentHandler *ma = &m_attachments[i];
>  
> -#undef SNARF

I'm so glad this is going away. That part was scary and pretty unclear to boot.
Attachment #554669 - Flags: review?(jonathan.protzenko) → review+
(Assignee)

Comment 10

7 years ago
Thx for the comments - re nsCString and '\0', I think you might be thinking of nsDependent[C]String not being happy with null pointers.

A digest message is generally a mailing list message where the day's messages are sent out to the list recipients in one big message, instead of individually. I don't think we ever set this to true ourselves.

I don't know much about the x-mac headers - I think they do come from the resource fork of apple files, so it's quite possible we could get rid of these.
(Assignee)

Comment 11

7 years ago
pinging for sr...
(Assignee)

Comment 12

7 years ago
Created attachment 556210 [details] [diff] [review]
de-bitrotted patch

previous patch got bit-rotted
Attachment #554669 - Attachment is obsolete: true
Attachment #554669 - Flags: superreview?(neil)
Attachment #556210 - Flags: superreview?(neil)
Attachment #556210 - Flags: review+
(Assignee)

Comment 13

7 years ago
ping for sr, or should I try to get someone else (standard8?) to do a sr?
Comment on attachment 556210 [details] [diff] [review]
de-bitrotted patch

Sorry for the delay.

nsIMsgAttachedFile appears to be very similar to nsIMsgAttachmentData, but with a few extra attributes, or am I missing something? (If one was derived from the other then this might allow you to replace some or all of the nsIArrays with regular arrays.)

> %{C++
> #include "nsIURL.h"
>+#include "nsString.h"
This C++ stuff belongs either in nsMsgSend.h or in its own .h/.cpp, but at the very least please use nsStringGlue.h instead.

>+    attachment->m_realName = NS_LossyConvertUTF16toASCII(tName);
[Could write this as LossyCopyUTF16toASCII(tName, attachment->mrealName); ]

>       bundle->GetStringFromID(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SAVING, getter_Copies(msg));
>     else
>       bundle->GetStringFromID(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SENDING, getter_Copies(msg));
>-    if (m_real_name && *m_real_name)
>-      printfString = nsTextFormatter::smprintf(msg.get(), m_real_name);
>+    if (!m_realName.IsEmpty())
>+      printfString = nsTextFormatter::smprintf(msg.get(), m_realName.get());
[Isn't this what FormatStringFromID was invented for?]

>-      if (m_attachments[i].m_real_name)
>-        printfString = nsTextFormatter::smprintf(msg.get(), m_attachments[i].m_real_name);
>+      if (m_attachments[i].m_realName.Length())
>+        printfString = nsTextFormatter::smprintf(msg.get(), m_attachments[i].m_realName.get());
>       else
>         printfString = nsTextFormatter::smprintf(msg.get(), "");
This condition makes no sense any more, because m_realName.get() is never null.

+      aAttach->m_realName.Adopt(PR_smprintf("%s.eml", aHdrs->munged_subject));
I'd prefer to see an assign/append here.
(Assignee)

Comment 15

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #14)
thx for the review
> nsIMsgAttachedFile appears to be very similar to nsIMsgAttachmentData, but
> with a few extra attributes, or am I missing something? (If one was derived
> from the other then this might allow you to replace some or all of the
> nsIArrays with regular arrays.)
Yes, I thought about that, but even though they share some attributes, each has attributes that the other does not, so there would need to be a common base interface/class. Neither could derive from the other, so it might make the code more complicated to try to force them into sharing.

Specifically, nsIMsgAttachmentData is a subset of nsIMsgAttachedFile, except that is has two types, desiredType and realType. And the semantics of the encoding are a bit different, I think, as well.
(Assignee)

Comment 16

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #14)
> >       bundle->GetStringFromID(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SAVING, getter_Copies(msg));
> >     else
> >       bundle->GetStringFromID(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SENDING, getter_Copies(msg));
> >-    if (m_real_name && *m_real_name)
> >-      printfString = nsTextFormatter::smprintf(msg.get(), m_real_name);
> >+    if (!m_realName.IsEmpty())
> >+      printfString = nsTextFormatter::smprintf(msg.get(), m_realName.get());
> [Isn't this what FormatStringFromID was invented for?]

FormatStringFromID wants PRUnichar params, and unfortunately, we have char * params.

I have a patch that addresses the rest of the comments which I'll put up after I build...
(In reply to David Bienvenu from comment #16)
> (In reply to comment #14)
> > >       bundle->GetStringFromID(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SAVING, getter_Copies(msg));
> > >     else
> > >       bundle->GetStringFromID(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SENDING, getter_Copies(msg));
> > >-    if (m_real_name && *m_real_name)
> > >-      printfString = nsTextFormatter::smprintf(msg.get(), m_real_name);
> > >+    if (!m_realName.IsEmpty())
> > >+      printfString = nsTextFormatter::smprintf(msg.get(), m_realName.get());
> > [Isn't this what FormatStringFromID was invented for?]
> FormatStringFromID wants PRUnichar params, and unfortunately, we have char *
> params.
Very unfortunately for our international users ;-)
(Assignee)

Comment 18

6 years ago
Created attachment 561008 [details] [diff] [review]
fix more bitrot, address comments, and a couple crashes I came across

I'd like to address the lack of intl support in a new bug, as well as the c++ classes defined in nsIMsgSend.idl, since this patch has bit-rotted a few times and I'd like to get it in.

I may need to break out the crash fixes into a new bug if this doesn't land before Aurora, since I suspect the trunk is a bit broken. I'm doing a build w/o this patch to make sure...
Attachment #556210 - Attachment is obsolete: true
Attachment #556210 - Flags: superreview?(neil)
Attachment #561008 - Flags: superreview?(neil)
Comment on attachment 561008 [details] [diff] [review]
fix more bitrot, address comments, and a couple crashes I came across

(In reply to comment #18)
> I'd like to address the lack of intl support in a new bug,
> as well as the c++ classes defined in nsIMsgSend.idl
Fair enough.
Attachment #561008 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 20

6 years ago
fixed on trunk - http://hg.mozilla.org/comm-central/rev/9b1d6d96451a

Bug 688471 filed for the international users issue
Bug 688475 filed for not defining c++ objects in nsIMsgSend.idl

http://hg.mozilla.org/comm-central/rev/2e9070d54ee6 pushed for linux build bustage
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 9.0

Comment 21

6 years ago
Thanks David!  :-)
Depends on: 714265

Updated

6 years ago
Depends on: 715810
Depends on: 744077

Updated

5 years ago
Depends on: 844647

Comment 22

5 years ago
I have regressed bug 745919 and confirmed that the first nightly build in which it appears is 2011-09-23. That means that it was almost certainly caused by the commit in bug 679476, which landed on 2011-09-22.

That means that the bug has been in the code base for over 18 months, and for over 9 months since it was allegedly fixed by the commit in bug 745919 which didn't actually fix it.

It would be nice if one of the people responsible for writing or reviewing the commit in bug 679476, or one of the people responsible for writing or reviewing the patch in bug 745919, would step up and fix this bug properly.

It's pretty appalling that it's still in the code base after over 18 months.

Comment 23

5 years ago
FYI, I think bug 856456 is also caused by the commit in this bug.
You need to log in before you can comment on or make changes to this bug.