Closed Bug 479844 Opened 11 years ago Closed 11 years ago
Consolidate checking for messages to send later into one function
We currently have various places where we're iterating around identities and trying to work out if they have messages to send in the unsent messages folder. As we've got this code in at least 3 places it seems to make sense to consolidate it into one function (hasUnsentMessages) on nsIMsgSendLater. Attaching patch which does this.
Fixed syntax error in TB (; in the wrong place).
Comment on attachment 363872 [details] [diff] [review] The fix v2 >+ return Components.classes["@mozilla.org/messengercompose/sendlater;1"] >+ .getService(Components.interfaces.nsIMsgSendLater) >+ .hasUnsentMessages(); Maybe I've misunderstood the code, but it seems to me that if you don't pass in an identity then hasUnsentMessages might fail to find the outbox.
(In reply to comment #2) > (From update of attachment 363872 [details] [diff] [review]) > >+ return Components.classes["@mozilla.org/messengercompose/sendlater;1"] > >+ .getService(Components.interfaces.nsIMsgSendLater) > >+ .hasUnsentMessages(); > Maybe I've misunderstood the code, but it seems to me that if you don't pass in > an identity then hasUnsentMessages might fail to find the outbox. hasUnsent messages calls nsMsgSendLater::GetUnsentMessagesFolder which in turn calls GetFolderURIFromUserPrefs , the parameter was nsIMsgSend::nsMsgQueueForLater so in that function, it just gets the mail.default_sendlater_uri" pref and ignores the identity argument. At the moment I've left the identity arguments in for future use, though I would eventually expect a "null" identity to check all outboxes if we implemented it.  http://hg.mozilla.org/comm-central/diff/e4f4569d451a/mailnews/compose/src/nsMsgCompUtils.cpp
Comment on attachment 363872 [details] [diff] [review] The fix v2 OK, I guess I must have misread the code the first time around.
Attachment #363872 - Flags: review?(neil) → review+
Attachment #363872 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 363872 [details] [diff] [review] The fix v2 couple nits, which you can address or not, up to you: + // folder (context menu), so we can use the folder and return true/false + // straight away. + return (folderResource.getTotalMessages(false) > 0); don't need parens here. + // Otherwise, we don't know where we are, so use the current identity and + // find out if we have messages or not via that. + var identity; + var folders = GetSelectedMsgFolders(); + if (folders.length > 0) + identity = getIdentityForServer(folders.server); you could use "let" instead of var here...
Checked in with comments addressed: http://hg.mozilla.org/comm-central/rev/b2161ed4236a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs review neil,bienvenu]
You need to log in before you can comment on or make changes to this bug.