The default autosync policy downloads aggressively, all messages. For many users, that's not optimal, for bandwidth or disk space reasons. I forget the UI that we'd talked about w/ Emre, but I think it was either - download last [ ] Megabytes of email or - download last [ ] days of email Both bandwidth and disk space are systemic issues rather than per account problems, so I suspect the core UI for this needs to be in the general Preferences dialog. Assigning to Bryan for his UX take.

I'm not going to likely have time for this, however we do need something like this to land. If I recall correctly the problem was that we needed a cascading determination of how to use space. Plus we need to be sure to set this (personal space management) aside from server space management; even though they may share similar interface components they need to be visually different. A static interface that looks something to the effect of this: Keep the last [ 3 ] ( months | v ) of email and use less than [ 3 ] ( gigabytes | v ) of disk space. While the experience ensures users understand what the consequences of their changes in this interface mean. I'm not sure if we need some kind of read out of the number of messages and the space being consumed and then the difference in any change. This will take some experiments to get right.

whoops, didn't mean to assign to nobody. I'm going to need some help finding someone who could do this patch... I'm looking in davida's direction, but he's not here right now. :(

pushing out to m5, but given that it's not speced yet, that's at risk.

moving to b4

See bug 497833 comment 4 for some use cases that are likely worth taking into account.

(In reply to comment #1) > A static interface that looks something to the effect of this: > > Keep the last [ 3 ] ( months | v ) of email and use less than [ 3 ] ( gigabytes > | v ) of disk space. According to the discussions there, this would need to be extended by a 2nd set of choices to limit the rate of the download (e.g., for slow/costly connections or servers limiting the amount of data over time before blocking an account).

Could this also be based on active network connections? Using a (bad) UMTS or even a GPRS connection for caching mails is not so nice, and might be expensive depending on the plan ...

rate of download is an orthogonal issue and I'd rather that be talked about in a different bug to avoid complicating this bug, which is going to be complicated enough - this is more about which messages to download, and retention policy of already downloaded messages (i.e., which already downloaded messages to remove in favor of newer messages). Auto-compact of offline stores is very closely related, since, if the main goal is to limit the amount of disk space used, we'll need to compact offline stores.

Spun off bug 506024 for download policy per comment #8.

orthogonal issues shouldn't block each other...

Sorry, sneaked in when cloning the bug, certainly can be solved independently.

Bryan, what does "v" mean in this context? Keep the last [ 3 ] ( months | v ) I guess we're thinking that users who want to limit their disk space usage probably either won't turn on gloda, or won't care that their older messages don't get body text indexed by gloda. In the new profile w/ existing imap server case, if you say "Keep the last 3 months of mail", I'm not going to download the older mail at all, so gloda won't see the body text. If download limits are set, we'll need to make sure that autosync looks at the highest priority folders first across all accounts, before downloading lower priority folders (e.g., we shouldn't be downloading messages in trash folders until higher priority folders have been downloaded). I'm thinking I'll use the existing retention policy application code to mark messages in offline stores for cleanup. That code runs when a folder is open, and in the background, on a timer.

The 'v' was indicating a menulist with options for other units of time like days, years. Yes, that was my assumption as well. We'll have to make a note about that in the knowledge base somewhere but I think that will be fine.

The meat of this patch is the pendingRemoval stuff, which will be used by the retention code to mark messages as needing removal from the offline store. I've augmented the offline store compact test to test this. I've also made the compact this folder menu item for an imap folder also compact the offline store, since that seems to be what the user expects. I had to do a little rearrangement to make that work. There's some other miscellaneous cleanup, and I've added a msgWindow argument to the expunge command so that there's feedback on which folders are getting compacted.

I think a 3rd (or maybe '1st'), much simpler option should be added: [ ] download only on-demand This should be a sub-option of the main 'Synchronization & Storage' option (see attachment for graphical depiction)

Comment on attachment 390947 [details] [diff] [review] preliminary work - checked in > nsresult > nsOfflineStoreCompactState::InitDB(nsIMsgDatabase *db) > { > db->ListAllOfflineMsgs(&m_keyArray); >+ // filter out msgs that have the "pendingRemoval" attribute set. >+ nsCOMPtr<nsIMsgDBHdr> hdr; Can you add a comment to the ListAllOfflineMsgs line to the effect of "Get the list of messages to keep.". I've just taken a while working out that is what is happening, so I think it may be worth adding just as clarification. nit: Capital F for Filter please. >+ // Turn off offline flag for message, since after the compact is completed, >+ // we won't have the message in the offline store. >+ PRUint32 resultFlags; nit: incorrect indentation. >+nsresult nsOfflineStoreCompactState::CopyNextMessage() >+{ >+ m_messageUri.SetLength(0); // clear the previous message uri >+ nsresult rv = BuildMessageURI(m_baseMessageUri.get(), m_keyArray[m_curIndex], >+ m_messageUri); nit: suggest indenting m_messageUri under the m_baseMessageUri. >+NS_IMETHODIMP nsImapMailFolder::Expunge(nsIUrlListener *aListener, >+ nsIMsgWindow *aMsgWindow) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIImapService> imapService = do_GetService(NS_IMAPSERVICE_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv,rv); nit: space after comma. >+ >+ return imapService->Expunge(m_thread, this, aListener, aMsgWindow, nsnull); nit: only one space after return. >+ if (header instanceof Components.interfaces.nsIMsgDBHdr) >+ { >+ if (header.flags & Ci.nsMsgMessageFlags.Offline) afaik you should be able to combine these two steps. >+ { >+ let fileStream = gIMAPInbox.getOfflineFileStream(header.messageKey, offset, size); >+ fileStream.close(); you can drop the intermediate variable if you want. >+ function testPendingRemoval() { >+ let msgHdr = gIMAPInbox.msgDatabase.getMsgHdrForMessageID(gMsgId2); >+ gMsgImapInboxFolder.markPendingRemoval(msgHdr);; nit: double ; r/sr=Standard8 with those fixed.

nits addressed and checked in.

this implements the age limit backend - if you set mail.autosync.max_age_days to a > 0 value, then auto sync will not download a message older than that value, and the normal retention setting stuff will mark imap messages older than that for pending removal. When compaction happens, then the space used by messages pending removal will be reclaimed. This pref is specific to auto-sync download and doesn't apply to the other ways of downloading messages for offline use. E.g., if you click on an older message, we will put it in the offline store, though it will get eventually marked for pending removal. I'm going to run with this patch for a bit, and try to come up with some unit tests for it.

Would appreciate a comment on my suggestion for a 'Sync on Demand' only option. This would - for me - be the most useful and most intuitive way for this to work. I'm not against these other options, but they - for me - are way too complicated. I just want TB to only download the headers the first time I load a folder on a new client install, then just d/l the bodies/attachments on demand (and cache them until the message is deleted or the max_age_days expires.

David said: "if you set mail.autosync.max_age_days to a > 0 value, then auto sync will not download a message older than that value, and the normal retention setting stuff will mark imap messages older than that for pending removal." When you say 'pending removal', you mean just from the local cache, not from the IMAP server, right?

Yes, just pending removal from the offline store/local cache, NOT the imap server. Re On Demand offline storage, I commented somewhere (an other bug, perhaps), you can do that today by individually selecting all the folders you want us to store messages offline, and *don't* check the "keep messages for this account offline" button. I don't think your usage model is common enough to need a top-level UI other than that - it would probably just confuse people. But Clarkbw can chime in since it's a UI question.

Hmmm... ok, this is just for TB3+, not TB2 then? It certainly doesn't work that way now with 2.0.0.22 (I'm still using 2.0.0.22 for daily use, just periodically testing stuff for TB3 with a separate profile)... If it does indeed work that way now by simply *not* checking the option at all, then that will be plenty good enough for my needs... Thanks!

That preference and the respective backend is not implemented in the TB 2.0 branch, thus naturally it won't do anything there. Also keep in mind that the patches presented here thus far are providing for adding backend support for the offline policies, this is not yet at the stage for defining UI elements.

No problem, and thanks for the clarification. Since the behavior I want is already implemented in the current TB3 code, I'm happy. :) But, I do think this behavior should be defined/reflected in the GUI... meaning, the option should be as I depicted in my attached screenshot as the default when a new IMAP account is added. So, a user would have to UNcheck the 'On-Demand' sub-option to get full sync. Also, I'd like to see this behavior be fine-grained - meaning, I'd like to be able to set certain folders to 'Sync All', and other to only 'On-Demand', which require another checkbox column under the 'Download' column in the Advanced options: All & On-Demand...

That's all download policy though, not space/time. Per comment #8 this was spun off to bug 506024. What you are asking for may be orthogonal to either topic, thus potentially better served by a separate bug specific to that preference.

Ok, thanks, done... See Bug 508276

adding blake for the UI parts of this

this adds a unit test to the prev patch.

Comment on attachment 393096 [details] [diff] [review] age limit with unit test - checked in. >+NS_IMETHODIMP nsImapMailFolder::ApplyRetentionSettings() >+ nsCOMPtr<nsIMsgDatabase> holdDBOpen; >+ if (numDaysToKeepOfflineMsgs > 0) >+ { >+ PRBool dbWasCached = mDatabase != nsnull; >+ nsresult rv = GetDatabase(); >+ NS_ENSURE_SUCCESS(rv, rv); >+ nsCOMPtr <nsISimpleEnumerator> hdrs; >+ rv = mDatabase->EnumerateMessages(getter_AddRefs(hdrs)); Missing check for success (and no space before <). r/sr=Standard8 with that fixed.

Blake, I'll file a new bug for the UI part of the date constraint pref, and assign to you. Bryan has some ideas for the UI.

bug 510707 filed for options UI.

this has no l10n impact - the options UI bug does.

I'm going to resolve this fixed, in order to get it off the blocker list, since the remaining work is in the options UI. I'm going to file a separate bug (non-blocking) for doing space constraints.

this makes the limit per-server, because Bryan and Blake felt that was a more consistent UI.

Comment on attachment 397095 [details] [diff] [review] patch with right files > NS_IMETHODIMP nsDefaultAutoSyncMsgStrategy::IsExcluded(nsIMsgFolder *aFolder, > nsIMsgDBHdr *aMsgHdr, PRBool *aDecision) > { > NS_ENSURE_ARG_POINTER(aDecision); > NS_ENSURE_ARG_POINTER(aMsgHdr); >+ NS_ENSURE_ARG_POINTER(aFolder); >+ nsCOMPtr<nsIMsgIncomingServer> server; >+ >+ nsresult rv = aFolder->GetServer(getter_AddRefs(server)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ nsCOMPtr<nsIImapIncomingServer> imapServer(do_QueryInterface(server, &rv)); >+ PRInt32 offlineMsgAgeLimit = -1; >+ imapServer->GetAutoSyncMaxAgeDays(&offlineMsgAgeLimit); >+ NS_ENSURE_SUCCESS(rv, rv); > PRInt64 msgDate; > aMsgHdr->GetDate(&msgDate); >- *aDecision = m_offlineMsgAgeLimit > 0 && >- msgDate < MsgConvertAgeInDaysToCutoffDate(m_offlineMsgAgeLimit); >+ *aDecision = offlineMsgAgeLimit > 0 && >+ msgDate < MsgConvertAgeInDaysToCutoffDate(offlineMsgAgeLimit); > return NS_OK; > } The patch in general looks good. However I'm concerned about this change especially for processing folders with a large number of messages in them. Is this likely to be an issue much here? If so could we pass an optional parameter to reduce the number of extrs calls needed?

(In reply to comment #36) > The patch in general looks good. However I'm concerned about this change > especially for processing folders with a large number of messages in them. Is > this likely to be an issue much here? If so could we pass an optional parameter > to reduce the number of extrs calls needed? If we were concerned about this, what I could do is cache a weak ptr to the last folder and age limit pair result, but I don't think it's really needed. The auto sync msg strategy object is supposed to be generic, so that other strategies can be plugged in, and adding a new optional parameter doesn't feel quite right. This function only gets called if we don't have the msg offline, so for the normal case, it will only get called once per message, ever. For the disk limited case, it will get called when new messages are added to a folder, and when we're doing the background sweep on idle. In the big scheme of things, I don't think it's an issue, though it bugged me at the time I was writing the patch.

Comment on attachment 397095 [details] [diff] [review] patch with right files I haven't tested this extensively, but this looks fine. r/sr=Standard8

fix checked in, with test-case change

I have a question about this feature: in bug 510707 comment 0, David Bienvenu wrote that enabling the limit will both mark the already downloaded messages that are old for removal when compacting folders, and limit what is downloaded/checked/fetched. I'm not sure I'm a typical user, but at this point I have some 50,000 messages in GMail's All Mail folder. Looking at an imap log (generated for bug 514386), I see a FETCH command for every single one of them - in All Mail, but also for all the copies in the labels (folders - thanks, GMail). All in all, some 100,000 FETCH calls. This makes my network and machine quite unhappy. What I'd like is for TB to check only for stuff that came in in the last weeks/months, but to leave everything older alone - it will have been downloaded, so I'm happy, I can read it on my machine. However, I don't want the stored messages to go away if/when I (or TB automatically) compact(s) folders. The latter seems to be the behaviour that is currently implemented. Is that analysis correct? Would it be useful to open another bug to separate the issues of offline storage and network usage, or will that be a WONTFIX?

Your request is a valid one; I believe there's already a bug open on the issue of network usage, though it's phrased somewhat differently - see bug 506024 - because we didn't implement a total disk space used constraint for TB 3.0, the age based constraint needs to stand-in for both at the momement.

I was reviewing this bug and had a question regarding 'offline storage' vs 'cache'... When I set up TBird to only sync messages on demand per comment #21, it will store these messages in offline storage (in the IMAP folders for that account), NOT in the local cache, correct?

Cache and offline store are two different things. The disk or memory cache is utilized as source of recently accessed messages which were not synchronized to the offline store. The preference introduced here limits what ends up in the offline copies, but has no impact on caching of the messages.