Closed
Bug 312275
(nsIMsgSendListener's)
Opened 19 years ago
Closed 17 years ago
nsIMsgSendListener's method onStopSending msgId parameter is always null
Categories
(Thunderbird :: Message Compose Window, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bastiaan, Assigned: mscott)
References
Details
Attachments
(2 files, 7 obsolete files)
18.18 KB,
patch
|
neil
:
review+
mscott
:
superreview-
|
Details | Diff | Splinter Review |
17.38 KB,
patch
|
mscott
:
review+
|
Details | Diff | Splinter Review |
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?
Attachment #201847 -
Flags: review?
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?
Attachment #201847 -
Flags: review? → review?(mscott)
Attachment #201848 -
Flags: review? → review?(mscott)
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 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-
Attachment #201847 -
Attachment is obsolete: true
Attachment #202479 -
Flags: review?(mscott)
Attachment #201848 -
Attachment is obsolete: true
Attachment #202480 -
Flags: review?(mscott)
Updated against current CVS version of TB.
Attachment #202479 -
Attachment is obsolete: true
Attachment #204437 -
Flags: review?(mscott)
Attachment #202479 -
Flags: review?(mscott)
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•19 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•19 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•19 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•19 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•19 years ago
|
||
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•19 years ago
|
||
Any news on my last submitted patch?
Comment 15•19 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•18 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•18 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•18 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•18 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•18 years ago
|
||
Ok. Thanks. I'll fix those issues too.
Assignee | ||
Comment 21•18 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•18 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 22•18 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•18 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•17 years ago
|
||
Comment 25•17 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•17 years ago
|
Attachment #254928 -
Flags: review?(mscott)
Assignee | ||
Comment 26•17 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•17 years ago
|
||
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•17 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•17 years ago
|
||
I've checked this patch in for Ivo.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 30•17 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
You need to log in
before you can comment on or make changes to this bug.
Description
•