Bug 312275 (nsIMsgSendListener's)

nsIMsgSendListener's method onStopSending msgId parameter is always null

RESOLVED FIXED

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: bastiaan, Assigned: Scott MacGregor)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

18.18 KB, patch
neil@parkwaycc.co.uk
: review+
Scott MacGregor
: superreview-
Details | Diff | Splinter Review
17.38 KB, patch
Scott MacGregor
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

The listener interface nsIMsgSendListener has the method  "void onStopSending (
char* msgID , nsresult status , PRUnichar* msg , nsIFileSpec returnFileSpec )",
but it seems to return null values every time for msgID, msg and returnFileSpec
when using it in javascript. Documentation says it should return the sent
e-mails messageId, it's send status, status message and "an nsIFileSpec which
will specify the file that was created (this is used if the dont_deliver_p
argument is set to PR_TRUE)". According to these descriptions msg and
returnFileSpec could be null, but msgID should be the messages messageID. 
(Parameter description taken from
http://www.mozilla.org/mailnews/arch/compose-backend.html#NSIMSGSENDLISTENER)

Reproducible: Always

Steps to Reproduce:
1. Add a nsIMsgSendListener, which is not so easy as there is no
addMsgSendListener method, but can be done using a
nsIMsgComposeRecyclingListener, on the method onReopen the parameter
nsIMsgComposeParams are passed which contain a field for a nsIMsgSendListener.
Set your nsIMsgSendListener there. (Note that this will only work after the
compose window has been opened and closed 1 time)
2. Make sure added listener implementation contains some feedback methods on the
parameters given in onStopSending.
3. Send an e-mail.
4. The onStopSending method gets called but it's msgID parameter is null.
Actual Results:  
The msgID parameter returned is null.

Expected Results:  
The msgID parameter should be the sent mails messageId.

nsIMsgSendListener:
  contains the method onStopSending which msgID parameter has an incorrect value.
nsIMsgComposeParams: 
  contains a property for nsIMsgSendListener, shouldn't there be an add/remove
MsgSendListener method?
(Reporter)

Comment 1

12 years ago
Created attachment 201847 [details] [diff] [review]
Returns message id in the onstopsending method when an email message has been sent succesfully
Attachment #201847 - Flags: review?
(Reporter)

Comment 2

12 years ago
Created attachment 201848 [details] [diff] [review]
Added Add/RemoveMsgSendListener methods to nsMsgCompose for easier use of nsIMsgSendListener

Added methods for easier use of the nsIMsgSendListener (see bug 308798). I've based the changes on code from (Un)RegisterStateListener. I left the existing mExternalSendListener property in for backwards compatibility.
Attachment #201848 - Flags: review?
(Reporter)

Updated

12 years ago
Attachment #201847 - Flags: review? → review?(mscott)
(Reporter)

Updated

12 years ago
Attachment #201848 - Flags: review? → review?(mscott)

Comment 3

12 years ago
Comment on attachment 201847 [details] [diff] [review]
Returns message id in the onstopsending method when an email message has been sent succesfully

>Index: src/nsMsgSend.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/compose/src/nsMsgSend.cpp,v
>retrieving revision 1.375
>diff -u -8 -p -r1.375 nsMsgSend.cpp
>--- src/nsMsgSend.cpp   13 Aug 2005 18:50:37 -0000      1.375
>+++ src/nsMsgSend.cpp   4 Nov 2005 15:03:13 -0000
>@@ -3765,20 +3771,24 @@ nsMsgComposeAndSend::DoDeliveryExitProce
>+  // Get the message id and add it as a parameter to the OnStopSending notifier
>+  const char *msgId = nsnull;
>+  msgId = mCompFields->GetMessageId();
>+
>   //
>   // Tell the listeners that we are done with the sending operation...
>   //
>-  NotifyListenerOnStopSending(nsnull, aExitCode, nsnull, nsnull);
>+  NotifyListenerOnStopSending( msgId, aExitCode, nsnull, nsnull );

why are you changing the whitespace of this stuff?
i wouldn't bother with a local variable, just:

  NotifyListenerOnStopSending(mCompFields->GetMessageId(),
                              aExitCode,
                              nsnull,
                              nsnull);

possibly wrapped if it doesn't fit in 80 cols, which it probably doesn't
Attachment #201847 - Flags: review?(mscott) → review-

Comment 4

12 years ago
Comment on attachment 201848 [details] [diff] [review]
Added Add/RemoveMsgSendListener methods to nsMsgCompose for easier use of nsIMsgSendListener

technically idl methods should be interCaps, not InitialCaps
generally new methods should go at the *end* of an interface, not in the middle
and when you change an interface, you need to bump the iid


+  nsISupportsArray getMsgSendListeners();
could be written:
readonly attribute nsISupportsArray msgSendListeners;
although i have no clue what a msgSendListener is


i'd probably only offer enumerator instead of an array
if i bothered exposing anything


+nsresult nsMsgCompose::AddMsgSendListener( nsIMsgSendListener *msgSendListener ) {

that should be NS_IMETHODIMP not nsresult
and it should be aMsgSendListener, not msgSendListener


+  nsCOMPtr<nsISupports> iSupports = do_QueryInterface( msgSendListener, &rv );
normally people just use nsCOMPtr<nsISupports> supports
that qi should *never* fail
i'd rather not have code that checks
also, you shouldn't need to QI to nsISupports at all (cast at most)


+  // note that this return value is really a PRBool, so be sure to use
+  // NS_SUCCEEDED or NS_FAILED to check it.
+  return mMsgSendListeners->AppendElement( iSupports );
you can't do that, nsresults are bitfields, PRBools aren't
you have to convert failure to NS_ERROR_OUT_OF_MEMORY, otherwise return NS_OK

+  if ( !mMsgSendListeners ) {
+       return ( nsresult ) PR_FALSE;      // yeah, this sucks, but I'm emulating the behaviour of


i'm not sure what indentation style that is, but style should either be 2-2 or 4-4, not 2-8


+  *_retval = mMsgSendListeners;
+  NS_IF_ADDREF( *_retval );
you can write that as:
NS_IF_ADDREF(*_reval = mMsgSendListeners);
(again, that's only if the attribute is needed at all..)


i'd rather you use nsCOMArray and maybe expose an nsISimpleEnuemrator if necessary


+    compose->GetMsgSendListeners( getter_AddRefs( mMsgSendListeners ) );
+       if (!mMsgSendListeners) {
bad indent
+    compose->GetMsgSendListeners( getter_AddRefs( mMsgSendListeners ) );
+    if (!mMsgSendListeners) {
+      return rv;    // maybe there just aren't any.


what rv are you returning?
Attachment #201848 - Flags: review?(mscott) → review-
(Reporter)

Comment 5

12 years ago
Created attachment 202479 [details] [diff] [review]
Changed according to comments from timeless
Attachment #201847 - Attachment is obsolete: true
Attachment #202479 - Flags: review?(mscott)
(Reporter)

Comment 6

12 years ago
Created attachment 202480 [details] [diff] [review]
Changed according to comments from timeless
Attachment #201848 - Attachment is obsolete: true
Attachment #202480 - Flags: review?(mscott)
(Reporter)

Comment 7

12 years ago
Created attachment 204437 [details] [diff] [review]
updated patch file for msg_id.diff

Updated against current CVS version of TB.
Attachment #202479 - Attachment is obsolete: true
Attachment #204437 - Flags: review?(mscott)
Attachment #202479 - Flags: review?(mscott)
(Reporter)

Comment 8

12 years ago
Created attachment 204438 [details] [diff] [review]
sendlistener.diff: added Add/RemoveMsgSendListener methods to nsMsgCompose for easier use of nsIMsgSendListener

Updated for current CVS version of Thunderbird.
Attachment #202480 - Attachment is obsolete: true
Attachment #204438 - Flags: review?(mscott)
Attachment #202480 - Flags: review?(mscott)

Comment 9

12 years ago
I wasn't able to get it to work because I couldn't decide what to do about nsMsgSendListener::OnSendNotPerformed and nsMsgSendListener::OnStopSending methods but the other methods, particularly OnGetDraftFolderURI, were easily simplified using the following approach:
1. Declare an nsCOMArray<nsIMsgSendListener> mExternalSendListeners;
2. Append the original send listener from params
3. Implement methods to add/remove additional listeners
4. Add nsIMsgSendListener to the implementation of nsMsgCompose
5. Implement nsIMsgSendListener e.g.
NS_IMETHODIMP nsMsgCompose::OnGetDraftFolderURI(const char *aFolderURI)
{
  m_folderName = aFolderURI;

  for (PRInt32 i = 0; i < mExternalSendListeners.Count(); i++)
    mExternalSendLsiteners[i]->OnGetDraftFolderURI(aFolderURI);
}
6. Stub out nsMsgComposeSendListener methods e.g.
NS_IMETHODIMP nsMsgComposeSendListener::OnGetDraftFolderURI(const char *aFolderURI)
{
  nsCOMPtr<nsIMsgSendListener> compose = do_QueryReferent(mWeakComposeObj);
  if (compose)
    compose->OnGetDraftFolderURI(aFolderURI);
}
(Assignee)

Comment 10

12 years ago
Comment on attachment 204438 [details] [diff] [review]
sendlistener.diff: added Add/RemoveMsgSendListener methods to nsMsgCompose for easier use of nsIMsgSendListener

minusing as Neil had some good review comments.

Is there a reason why you keep making the patch two separate attachments? Less e-mail headache for me if you just had a single patch for review.
Attachment #204438 - Flags: review?(mscott) → review-
(Assignee)

Comment 11

12 years ago
Comment on attachment 204437 [details] [diff] [review]
updated patch file for msg_id.diff

see my other comment
Attachment #204437 - Flags: review?(mscott) → review-
(Reporter)

Comment 12

12 years ago
Thought it would be easier to review if seperated as the small one is a fix and the other is an added feature(for extensions). But I'll put them together next time. I'll get working on those comments.
Alias: nsIMsgSendListener's
(Reporter)

Comment 13

12 years ago
Created attachment 205226 [details] [diff] [review]
Returns message id in the onstopsending method when an email message has been sent succesfully and adds add/remove methods for external sendlisteners

All changed according to comments.
Attachment #204437 - Attachment is obsolete: true
Attachment #204438 - Attachment is obsolete: true
Attachment #205226 - Flags: review?(mscott)
(Reporter)

Comment 14

12 years ago
Any news on my last submitted patch? 

Comment 15

12 years ago
Bastiaan has gone back to study for a while and he has appointed me as his successor :)

Scott, do you have any news to make me happy? Have you looked at the latest attachment?
(Assignee)

Comment 16

12 years ago
Comment on attachment 205226 [details] [diff] [review]
Returns message id in the onstopsending method when an email message has been sent succesfully and adds add/remove methods for external sendlisteners

adding neil to the review request line since he was providing some feedback on the patch earlier.
Attachment #205226 - Flags: superreview?(mscott)
Attachment #205226 - Flags: review?(neil)
Attachment #205226 - Flags: review?(mscott)

Comment 17

12 years ago
Comment on attachment 205226 [details] [diff] [review]
Returns message id in the onstopsending method when an email message has been sent succesfully and adds add/remove methods for external sendlisteners

>-NS_IMPL_ISUPPORTS2(nsMsgCompose, nsIMsgCompose, nsISupportsWeakReference)
>+NS_IMPL_THREADSAFE_ADDREF(nsMsgCompose)
>+NS_IMPL_THREADSAFE_RELEASE(nsMsgCompose)
Not sure why this was used - I was expecting NS_IMPL_ISUPPORTS3(nsMsgCompose, nsIMsgCompose, nsIMsgSendListener, nsISupportsWeakReference)

>+  AddMsgSendListener( externalSendListener );
Nit: although style varies across modules this one seems to be one of those with compact spacing.

>+  if ( !aMsgSendListener )
>+  {
>+    return NS_ERROR_NULL_POINTER;
>+  }
Nit: use NS_ENSURE_ARG_POINTER (both times)

>+  nsresult rv = mExternalSendListeners.AppendObject( aMsgSendListener );
>+  if ( NS_FAILED( rv ) )
Except it's a PRBool, so you should just use return and ?: (unless mscott prefers if/else).

>+  mExternalSendListeners.RemoveObject( aMsgSendListener );
return ? NS_OK : NS_ERROR_FAILURE; perhaps?

>+NS_IMETHODIMP nsMsgCompose::OnSendNotPerformed(const char *aMsgID, nsresult aStatus)
>+{
>+  for (PRInt32 i = 0; i < mExternalSendListeners.Count(); i++)
>+  {
>+    mExternalSendListeners[i]->OnSendNotPerformed(aMsgID, aStatus);
.+  }
>+  return NS_OK;
>+}
I wonder whether it's worth calling NotifyStateListeners(eComposeProcessDone, aStatus); here instead of in nsMsgSendListener.

The rest looks OK to me. But I'm tempted to suggest saving a few bytes and cycles by making nsIMsgCompose inherit from nsIMsgSendListener (this would also simplify OnStopSending) ;-)
Attachment #205226 - Flags: review?(neil) → review+

Comment 18

12 years ago
Okay, thanks for your comments. I'll take a look at your comments and send you an updated patch soon.
Would it have any unexpected side-effects when nsIMsgCompose inherits from nsIMsgSendListener?
(Assignee)

Comment 19

12 years ago
Comment on attachment 205226 [details] [diff] [review]
Returns message id in the onstopsending method when an email message has been sent succesfully and adds add/remove methods for external sendlisteners

In addition to Neil's comments, I'd add a couple minor ones.

0)
-  params->GetSendListener(getter_AddRefs(mExternalSendListener));
+  nsCOMPtr<nsIMsgSendListener> externalSendListener;
+  params->GetSendListener(getter_AddRefs(externalSendListener));
+  AddMsgSendListener( externalSendListener );

externalSendListener from the params object could be null, and it probably isn't an error for it to be null either, it just means someone didn't set an external send listener.

1) This:
+  if ( !aMsgSendListener )
+  {
+    return NS_ERROR_NULL_POINTER;
+  }
+
+  nsresult rv = mExternalSendListeners.AppendObject( aMsgSendListener );
+  if ( NS_FAILED( rv ) )
+  {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+  else
+  {
+    return NS_OK;
+  }
can be written simply as 
NS_ENSURE_ARG_POINTER(aMsgSendListener);
nsresult rv = ....
return rv;

2) nsMsgCompose::RemoveMsgSendListener should use NS_ENSURE_ARG_POINTER too and shouldn't it be:
return mExternalSendListeners.RemoveObject( aMsgSendListener );
instead of return NS_OK?

3) I typically don't like to see {} around single line statements like:

+  for (PRInt32 i = 0; i < mExternalSendListeners.Count(); i++)
+  {
+    mExternalSendListeners[i]->OnStartSending(aMsgID, aMsgSize);
+  }

Comment 20

12 years ago
Ok. Thanks. I'll fix those issues too.
(Assignee)

Comment 21

12 years ago
Matthijs if you attach a new patch that fixes the minor nits I listed above, I'll sr it and we'll get it checked in as soon as its ready.
(Assignee)

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 22

12 years ago
Sorry. Haven't had the time yet, but will nag boss for that. Unfortunately (from the bug's perspective) I'm on a vacation the next two weeks...
(Assignee)

Comment 23

11 years ago
Comment on attachment 205226 [details] [diff] [review]
Returns message id in the onstopsending method when an email message has been sent succesfully and adds add/remove methods for external sendlisteners

minusing the sr until we get the comments I made in comment 19 fixed.
Attachment #205226 - Flags: superreview?(mscott) → superreview-

Comment 24

11 years ago
Created attachment 254928 [details] [diff] [review]
Patch; updated to current trunk with comments from comment 19 added.

Comment 25

11 years ago
Hi,

Its been a while, but we would still like to see this patch added to the source. I've discussed this with Matthijs and updated the patch to the current trunk and worked in the comments made by scott in #19.
If there are any more changes that need to be made, please let me know.

Updated

11 years ago
Attachment #254928 - Flags: review?(mscott)
(Assignee)

Comment 26

11 years ago
Comment on attachment 254928 [details] [diff] [review]
Patch; updated to current trunk with comments from comment 19 added.

-[scriptable, uuid(0b28cc56-1dd2-11b2-bbe4-99e6a314f8ba)]
+[scriptable, uuid(69A74559-7397-489a-9596-142773F8E59C)]

The interface ID is being changed on the wrong interface.

+  if (composeSendlistener)
+  {
+    composeSendlistener->OnSendNotPerformed(aMsgID, aStatus);
   }

can you remove the braces around this single line if statement?

same here:

+  if (composeSendlistener)
+  {
+    composeSendlistener->OnStopSending(aMsgID, aStatus, aMsg, returnFileSpec);
   }

and here:

+  if (composeSendlistener)
   {
-    compose->SetSavedFolderURI(aFolderURI);
-
-    nsCOMPtr<nsIMsgSendListener> externalListener;
-    compose->GetExternalSendListener(getter_AddRefs(externalListener));
-    if (externalListener)
-      externalListener->OnGetDraftFolderURI(aFolderURI);
+    composeSendlistener->OnGetDraftFolderURI(aFolderURI);
   }

Thanks!
Attachment #254928 - Flags: review?(mscott) → review-

Comment 27

11 years ago
Created attachment 256449 [details] [diff] [review]
Updated patch with scotts comments (#26)

Worked in scotts comments:
- updated the right interface id (stupid mistake :))
- Removed all the braces around single line statements
Attachment #254928 - Attachment is obsolete: true
Attachment #256449 - Flags: review?(mscott)
(Assignee)

Comment 28

11 years ago
Comment on attachment 256449 [details] [diff] [review]
Updated patch with scotts comments (#26)

great! Thanks for the patch.
Attachment #256449 - Flags: review?(mscott) → review+
(Assignee)

Comment 29

11 years ago
I've checked this patch in for Ivo.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 372706

Comment 30

11 years ago
One question. I saw the patch was included in the trunk. Will this be part of a following release (2.0)? Otherwise is there a way to make this happen?

Thanks in advance

Updated

11 years ago
Depends on: 387579
You need to log in before you can comment on or make changes to this bug.