Closed Bug 440794 Opened 16 years ago Closed 15 years ago

Initial Support for "Leverage Offline capabilities to make sending email appear faster" (aka send in background)

Categories

(MailNews Core :: Backend, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: davida, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [b3ux][experimental support included, lacking error condition handling])

Attachments

(8 files, 7 obsolete files)

22.62 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
26.21 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
9.85 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.66 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
11.47 KB, patch
Details | Diff | Splinter Review
71.85 KB, image/png
clarkbw
: ui-review+
Details
31.24 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
6.25 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
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.
Flags: blocking-thunderbird3+
OS: Mac OS X → All
Hardware: PC → All
Product: Core → MailNews Core
Priority: -- → P1
Blocks: 136871
Assignee: nobody → bugmil.ebirol
Leveraging Frank DiLecce's add-on. No backend support added yet. No smtp connection-caching.
Target Milestone: --- → Thunderbird 3.0b2
Attached patch Draft (obsolete) — Splinter Review
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.
Attachment #340269 - Attachment is obsolete: true
Blocks: 459716
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.
moving to b2, though it would be nice to have this for b1
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
Mark, this is the patch I was talking about this morning.
Emre, if you're actively working on this, let me know, if not I'll try and progress it.
Assignee: bugmil.ebirol → bugzilla
Go ahead Mark, I haven;t done anything for this one for a while now.
Depends on: 129260
Depends on: 473740
Whiteboard: [affects l10n]
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
Status: NEW → ASSIGNED
Version: unspecified → Trunk
How does this affect l10n as said in the status whiteboard?
(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.
Whiteboard: [affects l10n] → [has l10n impact]
Depends on: 476467
Blocks: 476698
Depends on: 477054
Whiteboard: [has l10n impact] → [has l10n impact][needs updated patch]
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
Depends on: 477680
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.
No longer blocks: 476698
Depends on: 476698
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.
Whiteboard: [has l10n impact][needs updated patch] → [no l10n impact][target frozen][at risk]
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
Whiteboard: [no l10n impact][target frozen][at risk] → [no l10n impact]
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Whiteboard: [no l10n impact] → [no l10n impact][b3ux]
Depends on: 459376
Whiteboard: [no l10n impact][b3ux] → [no l10n impact][b3ux][needs core bug 459376 fixing]
Whiteboard: [no l10n impact][b3ux][needs core bug 459376 fixing] → [no l10n impact][b3ux][needs core bug 459376 fixing][m3]
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.
Attachment #341487 - Attachment is obsolete: true
Attachment #368246 - Flags: superreview?(bienvenu)
Attachment #368246 - Flags: review?(bienvenu)
Sorry, the previous version was missing the changes to some comments in mailnews.js that I wanted to include in the first version.
Attachment #368246 - Attachment is obsolete: true
Attachment #368247 - Flags: superreview?(bienvenu)
Attachment #368247 - Flags: review?(bienvenu)
Attachment #368246 - Flags: superreview?(bienvenu)
Attachment #368246 - Flags: review?(bienvenu)
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...
Attachment #368247 - Flags: superreview?(bienvenu)
Attachment #368247 - Flags: superreview+
Attachment #368247 - Flags: review?(bienvenu)
Attachment #368247 - Flags: review+
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
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?
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 :-) )
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 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
Attachment #368247 - Attachment description: Basic send in background (off by default) v2 → [checked in] Basic send in background (off by default) v2
Whiteboard: [no l10n impact][b3ux][needs core bug 459376 fixing][m3] → [b3ux][needs core bug 459376][m3][experimental support comment 20]
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.
(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?
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?
(In reply to comment #30)
> What the case

What about the case...
(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.
Blocks: TB2SM
(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)
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.
(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.
(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.
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.
Attachment #369842 - Flags: superreview?(bienvenu)
Attachment #369842 - Flags: review?(bienvenu)
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.
Attachment #369842 - Flags: superreview?(bienvenu)
Attachment #369842 - Flags: superreview+
Attachment #369842 - Flags: review?(bienvenu)
Attachment #369842 - Flags: review+
Whiteboard: [b3ux][needs core bug 459376][m3][experimental support comment 20] → [b3ux][m3][needs core bug 459376][experimental support comment 20]
Depends on: 215044
(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 on attachment 369842 [details] [diff] [review]
[checked in] Improved send in background

Checked in: http://hg.mozilla.org/comm-central/rev/fd3d5ede674b
Attachment #369842 - Attachment description: Improved send in background → [checked in] Improved send in background
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 :-)
Whiteboard: [b3ux][m3][needs core bug 459376][experimental support comment 20] → [b3ux][m3][needs core bug 459376][experimental support comment 20][at risk]
Whiteboard: [b3ux][m3][needs core bug 459376][experimental support comment 20][at risk] → [b3ux][m5][needs core bug 459376][experimental support comment 20]
Whiteboard: [b3ux][m5][needs core bug 459376][experimental support comment 20] → [b3ux][m6][needs core bug 459376][experimental support comment 20]
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.
Attachment #372608 - Flags: superreview?(bienvenu)
Attachment #372608 - Flags: review?(bienvenu)
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 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.
Attachment #372608 - Flags: superreview?(bienvenu)
Attachment #372608 - Flags: superreview+
Attachment #372608 - Flags: review?(bienvenu)
Attachment #372608 - Flags: review+
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
Attachment #372608 - Attachment description: Hook up to shutdown service part 1 → [checked in] Hook up to shutdown service part 1
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.
Attachment #373738 - Flags: superreview?(bienvenu)
Attachment #373738 - Flags: review?(bienvenu)
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.
Attachment #373738 - Flags: superreview?(bienvenu)
Attachment #373738 - Flags: superreview+
Attachment #373738 - Flags: review?(bienvenu)
Attachment #373738 - Flags: review+
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
Attachment #373738 - Attachment description: Fix sending more than one email in the same session → [checked in] Fix sending more than one email in the same session
Depends on: 489325
Depends on: 489779
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.
Attachment #376780 - Flags: superreview?(bienvenu)
Attachment #376780 - Flags: review?(bienvenu)
Attachment #376780 - Flags: superreview?(bienvenu)
Attachment #376780 - Flags: superreview+
Attachment #376780 - Flags: review?(bienvenu)
Attachment #376780 - Flags: review+
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)
Updated patch, now wait until tasks finished or cancel pressed, and wait 50ms in each loop.
Attachment #376780 - Attachment is obsolete: true
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
Attachment #376986 - Attachment description: Protect against shutdown during sending if quit-application-requested isn't received v2 → [checked in] Protect against shutdown during sending if quit-application-requested isn't received v2
No longer depends on: 459376
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.
Attachment #378315 - Flags: ui-review?(clarkbw)
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?
(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 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.
Attachment #378315 - Flags: ui-review?(clarkbw) → ui-review+
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.
Attachment #378423 - Flags: superreview?(bienvenu)
Attachment #378423 - Flags: review?(bienvenu)
Whiteboard: [b3ux][m6][needs core bug 459376][experimental support comment 20] → [b3ux][experimental support comment 20]
Attached image weird indentation (obsolete) —
Attachment #378613 - Attachment is obsolete: true
Comment on attachment 378613 [details]
weird indentation

This is covered by bug 492953 and isn't affected by my patch.
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.
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 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 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...
Attachment #378423 - Flags: superreview?(bienvenu)
Attachment #378423 - Flags: superreview+
Attachment #378423 - Flags: review?(bienvenu)
Attachment #378423 - Flags: review+
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 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.
Depends on: 494936
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.
Attachment #378674 - Attachment is obsolete: true
Attachment #378710 - Attachment is obsolete: true
Attachment #379743 - Flags: superreview?(bugzilla)
Attachment #379743 - Flags: review?(bugzilla)
Attachment #379743 - Flags: superreview?(bugzilla)
Attachment #379743 - Flags: superreview+
Attachment #379743 - Flags: review?(bugzilla)
Attachment #379743 - Flags: review+
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.
(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.
Attachment #378423 - Attachment description: Improved Activity Manager UI - patch → [checked in] Improved Activity Manager UI - patch
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 on attachment 379743 [details] [diff] [review]
[checked in] patch addressing Standard8's comments

Checked in: http://hg.mozilla.org/comm-central/rev/38c88455ec35
Attachment #379743 - Attachment description: patch addressing Standard8's comments → [checked in] patch addressing Standard8's comments
Depends on: 495527
Depends on: 495776
Depends on: 28211
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?
Depends on: 498298
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)
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0b4
Depends on: 503193
Blocks: 291379
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.
Blocks: sendinbackground
No longer blocks: 136871, 291379, TB2SM, 459716
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
No longer depends on: 28211, 129260, 215044, 489325, 498298, 503193
Flags: in-testsuite+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Resolution: --- → FIXED
Summary: Leverage Offline capabilities to make sending email appear faster → Initial Support for "Leverage Offline capabilities to make sending email appear faster" (aka send in background)
Whiteboard: [b3ux][experimental support comment 20] → [b3ux][experimental support included, lacking error condition handling]
Depends on: 540778
Blocks: 789865
You need to log in before you can comment on or make changes to this bug.