482476 - Enable Space/Time policy for autosync

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[David Ascher (:davida)]

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.

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

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.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

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. :(

Comment 3

[David Ascher (:davida)]

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

Comment 4

[David :Bienvenu]

moving to b4

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 6

[rsx11m]

(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).

Comment 7

[Jens Müller (:tessarakt)]

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 ...

Comment 8

[David :Bienvenu]

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.

Comment 9

[rsx11m]

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

Comment 10

[David :Bienvenu]

orthogonal issues shouldn't block each other...

Comment 11

[rsx11m]

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

Comment 12

[David :Bienvenu]

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.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

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.

Comment 14

[David :Bienvenu]

Attachment: preliminary work - checked in

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.

Comment 15

[Charles]

Attachment: Graphical depiction of On-Demand offline option

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 16

[Mark Banner (:standard8)]

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.

Comment 17

[David :Bienvenu]

nits addressed and checked in.

Comment 18

[David :Bienvenu]

Attachment: implement age limits

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.

Comment 19

[Charles]

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.

Comment 20

[Charles]

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?

Comment 21

[David :Bienvenu]

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.

Comment 22

[Charles]

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!

Comment 23

[rsx11m]

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.

Comment 24

[Charles]

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...

Comment 25

[rsx11m]

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.

Comment 26

[Charles]

Ok, thanks, done...

See Bug 508276

Comment 27

[Bryan Clark (DevTools PM) [:clarkbw]]

adding blake for the UI parts of this

Comment 28

[David :Bienvenu]

Attachment: age limit with unit test - checked in.

this adds a unit test to the prev patch.

Comment 29

[Mark Banner (:standard8)]

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.

Comment 30

[David :Bienvenu]

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.

Comment 31

[David :Bienvenu]

bug 510707 filed for options UI.

Comment 32

[David :Bienvenu]

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

Comment 33

[David :Bienvenu]

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.

Comment 34

[David :Bienvenu]

Attachment: make age limit per server

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

Comment 35

[David :Bienvenu]

Attachment: patch with right files

Comment 36

[Mark Banner (:standard8)]

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?

Comment 37

[David :Bienvenu]

(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 38

[Mark Banner (:standard8)]

Comment on attachment 397095 [details] [diff] [review]
patch with right files

I haven't tested this extensively, but this looks fine. r/sr=Standard8

Comment 39

[David :Bienvenu]

fix checked in, with test-case change

Comment 40

[:Gijs (he/him)]

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?

Comment 41

[David :Bienvenu]

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.

Comment 42

[Charles]

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?

Comment 43

[rsx11m]

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.