Consolidate checking for messages to send later into one function

RESOLVED FIXED in Thunderbird 3.0b3

Status

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
Thunderbird 3.0b3
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 363747 [details] [diff] [review]
The fix

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.
Attachment #363747 - Flags: superreview?(bienvenu)
Attachment #363747 - Flags: review?(neil)
(Assignee)

Comment 1

10 years ago
Created attachment 363872 [details] [diff] [review]
The fix v2

Fixed syntax error in TB (; in the wrong place).
Attachment #363747 - Attachment is obsolete: true
Attachment #363872 - Flags: superreview?(bienvenu)
Attachment #363872 - Flags: review?(neil)
Attachment #363747 - Flags: superreview?(bienvenu)
Attachment #363747 - Flags: review?(neil)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
(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 [1], 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.

[1] http://hg.mozilla.org/comm-central/diff/e4f4569d451a/mailnews/compose/src/nsMsgCompUtils.cpp
(Assignee)

Updated

10 years ago
Whiteboard: [needs review neil,bienvenu]

Comment 4

10 years ago
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+

Updated

10 years ago
Attachment #363872 - Flags: superreview?(bienvenu) → superreview+

Comment 5

10 years ago
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[0].server);
 
you could use "let" instead of var here...
(Assignee)

Comment 6

10 years ago
Checked in with comments addressed: http://hg.mozilla.org/comm-central/rev/b2161ed4236a
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review neil,bienvenu]
You need to log in before you can comment on or make changes to this bug.