Last Comment Bug 440794 - Initial Support for "Leverage Offline capabilities to make sending email appear faster" (aka send in background)
: Initial Support for "Leverage Offline capabilities to make sending email appe...
Status: RESOLVED FIXED
[b3ux][experimental support included,...
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: P1 normal with 12 votes (vote)
: Thunderbird 3.0b4
Assigned To: Mark Banner (:standard8)
:
Mentors:
: 82998 126140 218557 228410 477854 (view as bug list)
Depends on: 473740 476467 476698 477054 477680 489779 494936 495527 495776 540778
Blocks: sendinbackground 440793 789865
  Show dependency treegraph
 
Reported: 2008-06-20 10:27 PDT by David Ascher (:davida)
Modified: 2014-09-03 01:51 PDT (History)
44 users (show)
standard8: blocking‑thunderbird3-
standard8: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Quick and Ugly prototype to test the UX (4.26 KB, patch)
2008-09-24 17:58 PDT, Emre Birol
no flags Details | Diff | Splinter Review
Draft (10.61 KB, patch)
2008-10-02 12:52 PDT, Emre Birol
no flags Details | Diff | Splinter Review
Basic send in background (off by default) (22.45 KB, patch)
2009-03-19 06:43 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
[checked in] Basic send in background (off by default) v2 (22.62 KB, patch)
2009-03-19 06:52 PDT, Mark Banner (:standard8)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
[checked in] Improved send in background (26.21 KB, patch)
2009-03-28 14:21 PDT, Mark Banner (:standard8)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
[checked in] Hook up to shutdown service part 1 (9.85 KB, patch)
2009-04-14 07:02 PDT, Mark Banner (:standard8)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
[checked in] Fix sending more than one email in the same session (1.66 KB, patch)
2009-04-20 14:22 PDT, Mark Banner (:standard8)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
Protect against shutdown during sending if quit-application-requested isn't received. (11.48 KB, patch)
2009-05-11 14:19 PDT, Mark Banner (:standard8)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
[checked in] Protect against shutdown during sending if quit-application-requested isn't received v2 (11.47 KB, patch)
2009-05-12 13:30 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Improved Activity Manager UI (71.85 KB, image/png)
2009-05-19 05:54 PDT, Mark Banner (:standard8)
clarkbw: ui‑review+
Details
[checked in] Improved Activity Manager UI - patch (31.24 KB, patch)
2009-05-19 13:45 PDT, Mark Banner (:standard8)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
weird indentation (27.86 KB, image/jpeg)
2009-05-20 08:24 PDT, David :Bienvenu
no flags Details
move the start sending call to after the point we have the identity (2.07 KB, patch)
2009-05-20 13:37 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
or set the identity key on the outbox msg hdr (6.09 KB, patch)
2009-05-20 15:41 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
[checked in] patch addressing Standard8's comments (6.25 KB, patch)
2009-05-26 13:20 PDT, David :Bienvenu
standard8: review+
standard8: superreview+
Details | Diff | Splinter Review

Description David Ascher (:davida) 2008-06-20 10:27:45 PDT
Sending email should use the offline capabilities (dump the mail in Unsent Folders, schedule a background send), so that the user can move on immediately to the next task.
Comment 1 Jesse Ruderman 2008-06-27 18:18:36 PDT
*** Bug 228410 has been marked as a duplicate of this bug. ***
Comment 2 Jesse Ruderman 2008-06-27 18:18:40 PDT
*** Bug 218557 has been marked as a duplicate of this bug. ***
Comment 3 Emre Birol 2008-09-24 17:58:51 PDT
Created attachment 340269 [details] [diff] [review]
Quick and Ugly prototype to test the UX

Leveraging Frank DiLecce's add-on. No backend support added yet. No smtp connection-caching.
Comment 4 Emre Birol 2008-10-02 12:52:11 PDT
Created attachment 341487 [details] [diff] [review]
Draft

This is a work-in-progress patch. It includes minimum changes to the back-end.
Doesn't check the pending messages at start-up, and gives up if one of the send task fails.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2008-10-21 08:44:16 PDT
This work should automatically retry to send the message, at least once,
and perhaps a few times, periodically, in the event of certain network 
failures, especially if they occur before SMTP authentication is successful
(unless the failure is due to bad password or other invalid credentials).
See bug 459716.
Comment 6 David :Bienvenu 2008-11-06 18:00:10 PST
moving to b2, though it would be nice to have this for b1
Comment 7 David Ascher (:davida) 2009-01-08 12:05:15 PST
Mark, this is the patch I was talking about this morning.
Comment 8 Mark Banner (:standard8) 2009-01-08 15:15:28 PST
Emre, if you're actively working on this, let me know, if not I'll try and progress it.
Comment 9 Emre Birol 2009-01-08 15:37:37 PST
Go ahead Mark, I haven;t done anything for this one for a while now.
Comment 10 Bryan Clark (DevTools PM) [@clarkbw] 2009-01-27 12:27:49 PST
here are some quick notes on the ux from our discussion; I hope these aren't too cryptic.

* send errors: after hitting send, message in outbox attempting to be sent
** Not Connected - Don't try to send, indicate in status bar, send when reconnected
** Login Error - Retry in 30 sec - 1 min, add 1 min to every further failure; indicate in status bar we are resending.  At $X number of retries, fail and show pop-tart
** Generic Send Errors - treat like login errors

* shutdown: on quit of TB indicate unsent messages with a dialog
** Sending Messages - In the process of sending messages
*** Indicate "In process of sending" messages with ability to Cancel
** UnSent Mesages - Messages scheduled for later
*** Indicate how many unsent messages with ability to Cancel
** Sending + UnSent Message - In the process of sending a number of messages
*** Indicate Sending + how many unsent messages with ability to Cancel
Comment 11 Jesse Ruderman 2009-01-30 16:58:48 PST
*** Bug 126140 has been marked as a duplicate of this bug. ***
Comment 12 Wayne Mery (:wsmwk, NI for questions) 2009-01-31 05:05:03 PST
*** Bug 82998 has been marked as a duplicate of this bug. ***
Comment 13 Simon Paquet [:sipaq] 2009-02-02 00:46:33 PST
How does this affect l10n as said in the status whiteboard?
Comment 14 Mark Banner (:standard8) 2009-02-02 01:08:44 PST
(In reply to comment #13)
> How does this affect l10n as said in the status whiteboard?

I'm expecting to need to add/change strings for status updates.
Comment 15 Simon Paquet [:sipaq] 2009-02-09 03:49:17 PST
What is the ETA for this bug? 

Tomorrow at 23:59 PST is our string freeze and it would be great if this blocking bug could be fixed by that time as mentioned in https://wiki.mozilla.org/Thunderbird/Driver_Meetings/2009-02-03
Comment 16 Mark Banner (:standard8) 2009-02-09 16:10:12 PST
I've done some hooking up of the UI to the activity manager in bug 476698 - I need to refine that patch tomorrow but it gives us a good basic idea of what is happening in the send later service and tracking of what has happened, which I think will be important for this bug.

I believe there's no other strings we need for beta 2. I am not quite sure if this bug will fully make it for beta 2, but I'd like a good go, and any work I am doing is improving the send later service anyway ;-)

I'll have another think in the morning to confirm this though - some of the dialogs may need extra strings, but I think we can get away with re-using ones we already have at the moment.
Comment 17 Mark Banner (:standard8) 2009-02-10 12:11:15 PST
Bug 476698 has just landed the strings I think we'll need for this. The rest of it should just be back end changes. Updating summary.
Comment 18 rsx11m 2009-02-12 07:46:55 PST
*** Bug 477854 has been marked as a duplicate of this bug. ***
Comment 19 Mark Banner (:standard8) 2009-02-14 05:10:07 PST
This isn't going to make beta 2 as there are still some significant issues that I haven't had time to address:

- Implement auto-send policy
- Shutdown during send protection
Comment 20 Mark Banner (:standard8) 2009-03-19 06:43:25 PDT
Created attachment 368246 [details] [diff] [review]
Basic send in background (off by default)

This patch implements basic support for send in background:

- mailnews.sendInBackground controls whether or not we send in background and it is off by default. It requires a restart to be activated.
- When the pref is set, the (TB) compose window is reworked to make "Send" act like "Send Later". If we enable this by default, we'll probably want to redo the chrome code a bit later and maybe remove the unnecessary options.
- When the pref is set, if a message is put into the outbox then it will be sent a second later.
- Testcase for the pref being set to check a message is sent.

Limitations:
- Progress and status is currently only shown on the activity manager window.
- No retry on fail / limited error handling.
- No protection for TB being shut down whilst sending.
Comment 21 Mark Banner (:standard8) 2009-03-19 06:52:33 PDT
Created attachment 368247 [details] [diff] [review]
[checked in] Basic send in background (off by default) v2

Sorry, the previous version was missing the changes to some comments in mailnews.js that I wanted to include in the first version.
Comment 22 David :Bienvenu 2009-03-19 10:00:21 PDT
with this patch, there's no file | send later menu item - some users like to get several messages ready for sending using send later, then send them all at once. I don't think this feature replaces that usage model.

Other than that, it seems OK...
Comment 23 David :Bienvenu 2009-03-19 10:01:42 PDT
Comment on attachment 368247 [details] [diff] [review]
[checked in] Basic send in background (off by default) v2

r/sr = me, modulo the missing send later item
Comment 24 David Ascher (:davida) 2009-03-19 10:14:56 PDT
I'm not sure what the right UX is.  

One take on it: 
 - Send effectively acts as Send Later (but doesn't change name)
 - by default we do "send unsent messages" automatically periodically w/ a short period (w/ progressive falloff to deal w/ connectivity issues)

What I'm not sure about is how to deal with people for whom sending should be batched (either because it's a modem dialup thing, or from a work habit POV).  Maybe there's a UI to say "don't send unsent messages automatically"?

Would that cover the use case you're talking about david?

Bryan, your thoughts?
Comment 25 David :Bienvenu 2009-03-19 10:29:32 PDT
The modem dialup thing I think can be dealt with by auto detection of online state, or failing that, the user going into offline mode.

The work flow thing is more problematic. My inclination is to eschew additional ui and just hide the fact that we're using unsent messages for file | send from the user. When we send from unsent messages because the user did a file | send, we should only send the messages that were there from a file | send, not a file | send later. This would require the ability to selectively send from unsent messages, which isn't a huge deal (i.e, Standard8 would be doing it, not me :-) )
Comment 26 Mark Banner (:standard8) 2009-03-19 12:32:39 PDT
I think we can provide some sort of filtering/adjustment. Once Bryan comes back I'll probably file a separate patch as is.

I'm going to push the patch as it currently is, as it doesn't affect anything unless the pref is flipped. Its experimental so I'd like to get it in to builds so we can tart to get a idea of what it is like day-to-day.
Comment 27 Mark Banner (:standard8) 2009-03-19 12:53:24 PDT
Comment on attachment 368247 [details] [diff] [review]
[checked in] Basic send in background (off by default) v2

Checked in: http://hg.mozilla.org/comm-central/rev/1d0fcdefa375
Comment 28 Joe Sabash [:JoeS1] 2009-03-19 15:58:22 PDT
Well, as a nightly tester this bug gets me a bit nervous.
I noted the getBoolPref("mailnews.sendInBackground"), and will be sure to avoid it, but some folks use the outbox for purposes other than to "send later"

Use case:
An easy way to save "incremental" stages of a composition.
Saving as draft overwrites the older copy, whereas File>>Send later, just saves another copy of the composition, so you can easily undo your mistakes.

I would be quite dismayed if a couple years worth of compositions would be suddenly sent.

Maybe it's time for "spring cleaning" on that folder, but my point is that it should be clear to the user what will happen to the contents of the outbox.
Comment 29 Mark Banner (:standard8) 2009-03-20 02:02:08 PDT
(In reply to comment #28)
> Well, as a nightly tester this bug gets me a bit nervous.
> I noted the getBoolPref("mailnews.sendInBackground"), and will be sure to avoid
> it, but some folks use the outbox for purposes other than to "send later"

Out of interest, can you point me to examples other than the way you do it?

> Maybe it's time for "spring cleaning" on that folder, but my point is that it
> should be clear to the user what will happen to the contents of the outbox.

I would have thought that the name "outbox" would make it clear to people that if you put items into this folder then they may get sent.

However, it would be a change in behavior from 2.x so maybe caution is better here.

My current thoughts are (assuming mailnews.sendInBackground is true):

- When we "Send Now" we put the message in the outbox and add an extra header: X-Mozilla-Send: auto (or set a flag on it, I'm not sure which yet).
- When we "Send Later" we don't add the extra header.
- When the automated code comes along it looks for the presence of the X-Mozilla-Send: auto header, and if found it sends the mail.
- For emails without the header included, they don't get sent unless you do "Send Unsent Messages".

The potential issues:

- Users wonder why some emails get auto-sent and others don't.
- Users want to auto send after a period of time (there is at least one extension I've seen that does this).
- We may want to add UI in the outbox to indicate that a message is going to be automatically sent (in the event of message send failure).

Thoughts?
Comment 30 Karsten Düsterloh 2009-03-20 02:20:09 PDT
What the case of "I hit 'Send Now' and this very large mail apparently got sent immediately, so I can shut down the app right now."
IOW, how do you want to deal with application shutdown before sending mail which already was claimed to be sent?
Comment 31 Karsten Düsterloh 2009-03-20 02:21:16 PDT
(In reply to comment #30)
> What the case

What about the case...
Comment 32 Mark Banner (:standard8) 2009-03-20 02:35:36 PDT
(In reply to comment #30)
> What the case of "I hit 'Send Now' and this very large mail apparently got sent
> immediately, so I can shut down the app right now."

Please see comment 10 for the main thoughts on this so far. It depends on bug 459376, and it is something that we will be experimenting with once we sort out the initial work for sending in background.
Comment 33 David Ascher (:davida) 2009-03-20 08:06:14 PDT
(In reply to comment #29)
> (In reply to comment #28)

> > Maybe it's time for "spring cleaning" on that folder, but my point is that it
> > should be clear to the user what will happen to the contents of the outbox.
> 
> I would have thought that the name "outbox" would make it clear to people that
> if you put items into this folder then they may get sent.
> 
> However, it would be a change in behavior from 2.x so maybe caution is better
> here.

It should be possible to detect un-empty Outboxes on first-start of a build that has the pref set to true, and ask people whether they want to move the contents to Drafts, Trash, or send them.  I don't think this (relatively obscure use case) should trump making sending email faster for millions =).

> My current thoughts are (assuming mailnews.sendInBackground is true):
> 
> - When we "Send Now" we put the message in the outbox and add an extra header:
> X-Mozilla-Send: auto (or set a flag on it, I'm not sure which yet).
> - When we "Send Later" we don't add the extra header.
> - When the automated code comes along it looks for the presence of the
> X-Mozilla-Send: auto header, and if found it sends the mail.
> - For emails without the header included, they don't get sent unless you do
> "Send Unsent Messages".

> > The potential issues:
> 
> - Users wonder why some emails get auto-sent and others don't.
> - Users want to auto send after a period of time (there is at least one
> extension I've seen that does this).
> - We may want to add UI in the outbox to indicate that a message is going to be
> automatically sent (in the event of message send failure).
> 
> Thoughts?

This feels really complex, both for the users and from a code complexity POV (what do add-ons have to learn to do, etc.).  

Can we not have a simpler mode switch which boils down to "old way vs. new way of emptying outbox"?

(aside: gmail labs blogged about 'undo send' yesterday, which is basically an outbox w/ a 5 second fuse)
Comment 34 David :Bienvenu 2009-03-20 13:53:25 PDT
we already have a message flag,  const nsMsgMessageFlagType Queued - to use it, you would just have to make sure we don't set it when doing send later for speed, instead of because the user asked us to. When we're doing a background send, we would just ignore queued messages, i.e., ones the user explicitly said to send later.
Comment 35 Joe Sabash [:JoeS1] 2009-03-20 15:20:52 PDT
(In reply to comment #29)

> > it, but some folks use the outbox for purposes other than to "send later"
> 
> Out of interest, can you point me to examples other than the way you do it?
> 

No, other than the fact that "if it can be done, someone is going to do it"

> 
> I would have thought that the name "outbox" would make it clear to people that
> if you put items into this folder then they may get sent.

Well it wasn't called the "outbox" until relatively recently.
And of course auto send wasn't in the picture for "unsent" 
> However, it would be a change in behavior from 2.x so maybe caution is better
> here.
>

Maybe a "sanity check" For instance, it would make no sense for me to have set up 3,492 messages, many of which have duplicate subjects, for sending later.

> My current thoughts are (assuming mailnews.sendInBackground is true):
> 
> - When we "Send Now" we put the message in the outbox and add an extra header:
> X-Mozilla-Send: auto (or set a flag on it, I'm not sure which yet).
> - When we "Send Later" we don't add the extra header.
> - When the automated code comes along it looks for the presence of the
> X-Mozilla-Send: auto header, and if found it sends the mail.
> - For emails without the header included, they don't get sent unless you do
> "Send Unsent Messages".
> 

I think the 2 functions are fundamentally different.
Save a message for sending later.(when I want to)
And save message to send now in background.

So yes, I think distinct flags are necessary.
(In reply to comment #33)
> (In reply to comment #29)
> > (In reply to comment #28)
> 

> 
> It should be possible to detect un-empty Outboxes on first-start of a build
> that has the pref set to true, and ask people whether they want to move the
> contents to Drafts, Trash, or send them.  I don't think this (relatively
> obscure use case) should trump making sending email faster for millions =).
> 

David, a comment is not an objection, I think this is a very good idea.
Just pointing out that the "Outbox" had no auto send functionality before, and could have messages the user does not want sent.
Unexpected behavior is mostly "bad behavior"
How about this scenario:
"I was going to send in my resignation, but decided not to. But Thunderbird sent it anyway."
Also, see my "sanity check" comment to Mark, above.
Comment 36 Mark Banner (:standard8) 2009-03-21 11:44:00 PDT
(In reply to comment #33)
> > - When we "Send Now" we put the message in the outbox and add an extra header:
> > X-Mozilla-Send: auto (or set a flag on it, I'm not sure which yet).
> > - When we "Send Later" we don't add the extra header.
> > - When the automated code comes along it looks for the presence of the
> > X-Mozilla-Send: auto header, and if found it sends the mail.
> > - For emails without the header included, they don't get sent unless you do
> > "Send Unsent Messages".
>
> This feels really complex, both for the users and from a code complexity POV
> (what do add-ons have to learn to do, etc.).  
> 
> Can we not have a simpler mode switch which boils down to "old way vs. new way
> of emptying outbox"?

It depends what you mean. If you're saying what I think you are saying, then the old way would be manual send via the "send unsent messages" and the new way would be automatically send everything that goes into that folder.

This would then mean you loose the Send Later (and the possibility for send at specific time via extensions) functionality that people use.

> (aside: gmail labs blogged about 'undo send' yesterday, which is basically an
> outbox w/ a 5 second fuse)

I think that's what most people who currently use Send Later use it for. There's also the ones that like to batch it up.
Comment 37 Mark Banner (:standard8) 2009-03-28 14:21:44 PDT
Created attachment 369842 [details] [diff] [review]
[checked in] Improved send in background

Revised change:

- Reverts the UI changes by the previous patch (so keep Send Now and Send Later).
- Automated send now only sends messages that don't have the Queued flag set.
- Implemented new send flag for deliver in background - this echos queue for later, but doesn't set the queued flag on the message.
- Updated unit test to take account of send and send later flags.
Comment 38 David :Bienvenu 2009-03-30 08:33:12 PDT
Comment on attachment 369842 [details] [diff] [review]
[checked in] Improved send in background

out of curiousity, is there a reason you switched to var from let? It doesn't look like you try to access the variables outside the try scope.

+    var sendButton = document.getElementById("button-send");
+    var sendNowMenuItem = document.getElementById("menu-item-send-now");


This looked relatively painless - I'm glad!

One question - my recollection was that before we used to simply delete the Unsent Messages folder and recreate it, once sending was complete, so we wouldn't have to worry about compacting the sent+deleted messages. We're not doing that, at least in the test case I was worried about (send later explicit, send later implicit + send). Is that code simply gone or are we not hitting it anymore? I noticed even after doing the explicit send of unsent messages, the unsent messages folder is still not 0 bytes.
Comment 39 Mark Banner (:standard8) 2009-03-31 01:54:21 PDT
(In reply to comment #38)
> (From update of attachment 369842 [details] [diff] [review])
> out of curiousity, is there a reason you switched to var from let? It doesn't
> look like you try to access the variables outside the try scope.

The changes in MsgComposeCommands.js were a based on a straight backout of changes in the first patch plus the mod to use the new background flag - so they just revert back to the way it was - I'll do a follow up patch to fix that function, we really shouldn't have a try/catch like it round it though.

> One question - my recollection was that before we used to simply delete the
> Unsent Messages folder and recreate it, once sending was complete, so we
> wouldn't have to worry about compacting the sent+deleted messages. We're not
> doing that, at least in the test case I was worried about (send later explicit,
> send later implicit + send). Is that code simply gone or are we not hitting it
> anymore? I noticed even after doing the explicit send of unsent messages, the
> unsent messages folder is still not 0 bytes.

I've looked through nsMsgSendLater and nsMsgOfflineService and can't find any references to doing that. Its a good point - should we auto-compact it after sending messages?
Comment 40 Mark Banner (:standard8) 2009-03-31 02:01:59 PDT
Comment on attachment 369842 [details] [diff] [review]
[checked in] Improved send in background

Checked in: http://hg.mozilla.org/comm-central/rev/fd3d5ede674b
Comment 41 David :Bienvenu 2009-03-31 16:11:50 PDT
I would hope that we'll do auto-compaction in general for local folders, but it might behoove us to empty out the unsent messages when we've done a send and there are no unsent messages in it, since that will be the common case. Though auto-compaction of an empty folder is pretty quick too :-)
Comment 42 Mark Banner (:standard8) 2009-04-14 07:02:22 PDT
Created attachment 372608 [details] [diff] [review]
[checked in] Hook up to shutdown service part 1

This starts to hook us up to the shutdown service - if a message is in the process of being sent (either via Send Unsent Messages or an automated Send In Background), and we get a request to shutdown (subject to bug 459376), we'll now pop up a dialog and wait until sending finishes.

The progress dialog isn't pretty, but is part of the existing shutdown service - I'm wanting to land this patch so that Bryan and others can easily look at it and test it. I'm proposing we land the task name un-localised as I expect we'll be wanting a few more strings and maybe to change some other strings.
Comment 43 David :Bienvenu 2009-04-15 14:33:48 PDT
this all looks good, but when I tried it, I got something along the lines of  "sending failed - check account settings".

By "try it", I mean send a large message and then shutdown, with offline send pref set. Oh, duh, I'm shutting down by closing the last window. I'll try it with file | exit.
Comment 44 David :Bienvenu 2009-04-15 14:45:58 PDT
Comment on attachment 372608 [details] [diff] [review]
[checked in] Hook up to shutdown service part 1

ok, it works if I do file | exit.

I think this needs some UI work, though. There's no feedback while the message sending happens. The shutdown service does have a method to let clients poke the status text in the shutdown dialog:

nsMsgShutdownService::SetStatusText

One thought is to forward the send progress strings to the shutdown service, when appropriate. Or augment the shutdown service so that it can hook directly into generic progress/status updates.
Comment 45 Mark Banner (:standard8) 2009-04-17 03:39:20 PDT
Comment on attachment 372608 [details] [diff] [review]
[checked in] Hook up to shutdown service part 1

I agree this still needs work, and I'm actually pondering moving where we do the hook, however, for now I've checked this in as it will make testing the core bug 459376 easier, and my next patch which is looking at redoing a lot of the notifications is also based on this.

http://hg.mozilla.org/comm-central/rev/8ef97925ead2
Comment 46 Mark Banner (:standard8) 2009-04-20 14:22:25 PDT
Created attachment 373738 [details] [diff] [review]
[checked in] Fix sending more than one email in the same session

When I added the timer, I neglected to check the case where we send more than one email in the same session - hence it was broken. This modifies the code so that we'll now do the right thing - don't reset it if we're just adding a second message, but reset it correctly even if the timer already exists.
Comment 47 David :Bienvenu 2009-04-20 14:28:19 PDT
Comment on attachment 373738 [details] [diff] [review]
[checked in] Fix sending more than one email in the same session

I was able to send two different messages w/ this patch, and receive both.
Comment 48 Mark Banner (:standard8) 2009-04-21 01:52:20 PDT
Comment on attachment 373738 [details] [diff] [review]
[checked in] Fix sending more than one email in the same session

Checked in: http://hg.mozilla.org/comm-central/rev/2e28575d773c
Comment 49 Mark Banner (:standard8) 2009-05-11 14:19:41 PDT
Created attachment 376780 [details] [diff] [review]
Protect against shutdown during sending if quit-application-requested isn't received.

This patch extends the shutdown service so that if the quit-application-requested notification isn't received, it will do the checks on quit-application notification instead.

I've also tidied up folder the folder listener subscription options - its a bad idea to call GetUnsentMessagesFolder at xpcom-shutdown because this ends up calling into account manager and essentially re-initialising it, which isn't a good idea at that stage. Hence we save the folder in the local member variable and then we can remove the listener direct at the shutdown stage.

There's also a bit of tidy up so that we free various items we no longer need once we've sent some messages.
Comment 50 David :Bienvenu 2009-05-11 16:30:43 PDT
Comment on attachment 376780 [details] [diff] [review]
Protect against shutdown during sending if quit-application-requested isn't received.

this code probably warrants a comment - we're waiting for 10 seconds, a millisecond at a time?

+      PRInt32 loopCount = 0;
+      mReadyToQuit = PR_FALSE;
+      while (loopCount++ < 100000 && !mReadyToQuit)
Comment 51 Mark Banner (:standard8) 2009-05-12 13:30:57 PDT
Created attachment 376986 [details] [diff] [review]
[checked in] Protect against shutdown during sending if quit-application-requested isn't received v2

Updated patch, now wait until tasks finished or cancel pressed, and wait 50ms in each loop.
Comment 52 Mark Banner (:standard8) 2009-05-13 01:59:04 PDT
Comment on attachment 376986 [details] [diff] [review]
[checked in] Protect against shutdown during sending if quit-application-requested isn't received v2

Checked in: http://hg.mozilla.org/comm-central/rev/f2ad961aefcb
Comment 53 Mark Banner (:standard8) 2009-05-19 05:54:49 PDT
Created attachment 378315 [details]
Improved Activity Manager UI

I'll attach the patch for this later - I've been reworking the notification system so that:

- When we send emails they can be grouped by identity and have subject, so that it is not just "Sent Message".
- We have a better base on which to try and sit the automated retry on error system and any other improvements we need (I'm sure there are some, just forgotten them at the moment).

This image is the new version of the UI - Failures & Sends are grouped under the identity - processes for sending are grouped under a generic "Sending Messages" section. I can't get at progress information for copying messages to sent folder at the moment which is why that progress bar is in the undetermined state.
Comment 54 Bryan Clark (DevTools PM) [@clarkbw] 2009-05-19 12:47:45 PDT
Comment on attachment 378315 [details]
Improved Activity Manager UI

looks good.

I'm only confused by the grouping of 'Sent' messages.  Is that going to stay grouped / how do they scroll away over time?
Comment 55 Mark Banner (:standard8) 2009-05-19 13:08:43 PDT
(In reply to comment #54)
> (From update of attachment 378315 [details])
> looks good.
> 
> I'm only confused by the grouping of 'Sent' messages.  Is that going to stay
> grouped / how do they scroll away over time?

I was thinking they would stay grouped under identity - that way you can check that the messages from an identity were sent.

I hadn't thought about expiry - we could drop them after 12 hours or something? (I guess we could address that in a different bug though).
Comment 56 Bryan Clark (DevTools PM) [@clarkbw] 2009-05-19 13:18:53 PDT
Comment on attachment 378315 [details]
Improved Activity Manager UI

Ok, I think the sent messages were originally designed to float individually as activity events but this might work well.  

I like the idea of having successfully sent messages and failures near each other; that will give good context to the failed message.
Comment 57 Mark Banner (:standard8) 2009-05-19 13:45:07 PDT
Created attachment 378423 [details] [diff] [review]
[checked in] Improved Activity Manager UI - patch

Patch for the improved UI. This patch implements what I said in comment 53, and additionally note the following:

- Activity manager updated so that a process shows an undetermined progress bar if we don't know how long a process will take (I haven't been able to dig out the copy progress % yet).
- nsMsgSendLater has been slightly reorganised so that we're not doing some work in SendOperationListener and some in nsMsgSendLater - nsMsgSendLater now controlls everything.
Comment 58 David :Bienvenu 2009-05-20 08:24:05 PDT
Created attachment 378613 [details]
weird indentation
Comment 59 Mark Banner (:standard8) 2009-05-20 08:27:32 PDT
Comment on attachment 378613 [details]
weird indentation

This is covered by bug 492953 and isn't affected by my patch.
Comment 60 David :Bienvenu 2009-05-20 13:37:11 PDT
Created attachment 378674 [details] [diff] [review]
move the start sending call to after the point we have the identity

this goes on top of your patch - the problem was that you were sending the start send notification before we parsed the headers of the message we were sending and determined the identity key. This is worthy of a nice big comment, since it's weird to send the start notification in the middle of our on data available handler, but that's the point where we know the identity.

Maybe you can find a better place for this code, but I think that would require some pre-parsing of the message, or setting the identity key property on the msg hdr in the db when putting the message in the outbox in the first place - actually, I wonder why we don't do that. I'll go look for the code that adds the message to the outbox, and presumably creates a message header in the db.
Comment 61 David :Bienvenu 2009-05-20 15:41:32 PDT
Created attachment 378710 [details] [diff] [review]
or set the identity key on the outbox msg hdr

this is a slightly bigger change, but it results in ultimately cleaner code, I think. But I leave it up to you. This makes us set the identity key in the msg hdr when copying to the outbox.
Comment 62 David :Bienvenu 2009-05-20 15:58:16 PDT
Comment on attachment 378423 [details] [diff] [review]
[checked in] Improved Activity Manager UI - patch

I see things like this in several places:

+      if (this._sendProcess.state != Ci.nsIActivityProcess.STATE_INPROGRESS)
+        this._sendProcess.state = Ci.nsIActivityProcess.STATE_INPROGRESS;

Unless I'm missing something, it looks to me like nsActProcess handles set state getting the already set state, so the code would be cleaner without the if.

+   * @param aMessageHeader       The header information for the message that is
+   *                             being sent.

Instead of "information", I'd say something like msg header object

-  NS_ADDREF(this);
+  AddRef();
 
You'd probably know better than I do, but I thought we did this for the ref count balancing stuff.
Comment 63 David :Bienvenu 2009-05-20 15:59:25 PDT
Comment on attachment 378423 [details] [diff] [review]
[checked in] Improved Activity Manager UI - patch

r/sr=bienvenu, with those comments addressed, and a choice made about how to get the right identity...
Comment 64 David :Bienvenu 2009-05-22 17:00:00 PDT
Running with what's checked in, and the pref turned on, I did a file | send link, which launched TB and brought up the compose window. When I clicked send, the compose window closed and the app shut down, so that the next time I launched TB, it asked me if I wanted to send unsent messages. Aplogies if this is a known issue...
Comment 65 Mark Banner (:standard8) 2009-05-25 05:45:06 PDT
Comment on attachment 378710 [details] [diff] [review]
or set the identity key on the outbox msg hdr

I think this is the better choice, though it doesn't quite work. m_identity is actually set when the compose window is initialised and therefore is the identity Thunderbird chooses when doing new message or on replying to the email - its not replaced by the identity that the user then chooses in the compose window :-(

I haven't yet worked out where we can get the correct identity from (or if it would be safe to set m_identity).

> NS_IMETHODIMP nsMsgCompose::RememberQueuedDisposition()
> {
>   // need to find the msg hdr in the saved folder and then set a property on
>   // the header that we then look at when we actually send the message.
>+
>+  const char *dispositionSetting;

I think we should be setting this to nsnull.

>+
>   if (mType == nsIMsgCompType::Reply ||
>     mType == nsIMsgCompType::ReplyAll ||
>     mType == nsIMsgCompType::ReplyToList ||
>     mType == nsIMsgCompType::ReplyToGroup ||
>     mType == nsIMsgCompType::ReplyToSender ||
>     mType == nsIMsgCompType::ReplyToSenderAndGroup ||
>     mType == nsIMsgCompType::ForwardAsAttachment ||
>     mType == nsIMsgCompType::ForwardInline)
>   {

>+      dispositionSetting =(mType == nsIMsgCompType::ForwardAsAttachment ||
>+        mType == nsIMsgCompType::ForwardInline) ? "forwarded" : "replied";
>+  }

The big if statement can then be split into two to set these as appropriate, rather than checking twice in the forward case.
Comment 66 Mark Banner (:standard8) 2009-05-26 12:52:37 PDT
Comment on attachment 378674 [details] [diff] [review]
move the start sending call to after the point we have the identity

Marking obsolete, the other patch is the one we want.
Comment 67 David :Bienvenu 2009-05-26 13:20:11 PDT
Created attachment 379743 [details] [diff] [review]
[checked in] patch addressing Standard8's comments
Comment 68 Mark Banner (:standard8) 2009-05-27 03:20:48 PDT
Comment on attachment 379743 [details] [diff] [review]
[checked in] patch addressing Standard8's comments

I'll check this in with the other patches we need.
Comment 69 Mark Banner (:standard8) 2009-05-27 13:39:35 PDT
(In reply to comment #62)
> (From update of attachment 378423 [details] [diff] [review])
> I see things like this in several places:
> 
> +      if (this._sendProcess.state != Ci.nsIActivityProcess.STATE_INPROGRESS)
> +        this._sendProcess.state = Ci.nsIActivityProcess.STATE_INPROGRESS;
> 
> Unless I'm missing something, it looks to me like nsActProcess handles set
> state getting the already set state, so the code would be cleaner without the
> if.

Unfortunately, it doesn't quite handle it as you expect - it throws if you try and set the same state.

> +   * @param aMessageHeader       The header information for the message that
> is
> +   *                             being sent.
> 
> Instead of "information", I'd say something like msg header object

I forgot to fix this, so I've added it to the next patch.

> -  NS_ADDREF(this);
> +  AddRef();
> 
> You'd probably know better than I do, but I thought we did this for the ref
> count balancing stuff.

NS_ADDREF(this) equates to this->AddRef(); so ref count wise it is exactly the same.
Comment 70 Mark Banner (:standard8) 2009-05-27 13:40:22 PDT
Comment on attachment 378423 [details] [diff] [review]
[checked in] Improved Activity Manager UI - patch

Checked in: http://hg.mozilla.org/comm-central/rev/33d632a3a739
Comment 71 Mark Banner (:standard8) 2009-05-27 13:40:58 PDT
Comment on attachment 379743 [details] [diff] [review]
[checked in] patch addressing Standard8's comments

Checked in: http://hg.mozilla.org/comm-central/rev/38c88455ec35
Comment 72 Peter Weilbacher 2009-06-12 01:43:01 PDT
Comment on attachment 369842 [details] [diff] [review]
[checked in] Improved send in background

Was the "deliver mode" printf left in the code (nsMsgCompose::_SendMsg) on purpose?
Comment 73 Ittay Dror 2009-06-30 04:58:53 PDT
will there be a way to define the delay of sending. this is useful for "oops" cases (sending and then going 'oops' and being able to delete/edit the message)
Comment 74 John David Galt 2009-07-22 20:01:57 PDT
*** Bug 291379 has been marked as a duplicate of this bug. ***
Comment 75 Mark Banner (:standard8) 2009-08-18 04:06:14 PDT
As there is still a significant amount of work still to do to rework the mailnews listeners to support error conditions (e.g. send fail, copy fail etc), we have decided to move this work out to after Thunderbird 3 and not delay Thunderbird 3 on it.

As I have done a lot of work in this bug, I'd like to close this as fixed for the initial (experimental) support and I've opened a clone - bug 511079 that will be used as a meta bug to complete the work for send in background.

Note You need to log in before you can comment on or make changes to this bug.