385502 - investigate turning on offline imap by default

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect, P1)

Description

[David :Bienvenu]

We should consider defaulting imap folders to be configured for offline use, and we should default autosync_offline_stores to true.

Comment 1

[Magnus Melin [:mkmelin]]

Could be a good win for imap, setting blocking flag so we don't forget this for tb3.

Comment 2

[David Ascher (:davida)]

I think dmose wanted to take this one, if that's ok w/ you david.

Comment 3

[David :Bienvenu]

Yes, that's fine. Let me know if you have any questions, Dan.

One thing that has to happen if you do this, I think, is that we have to turn on auto-compaction of offline stores (and local folders) by default. It would be lovely to do this at shutdown, or in the background.

Comment 4

[WADA:World Anti-bad-Duping Agency]

(Off-Topic, perhaps)
To David Bienvenu:
Before making offline-use default, resolve bug such as Bug 381472 please, in order to avoid unwanted bug reports(==help/support requests. e.g. bugs listed in dependency tree for Bug 381472) and in order to avoid needless problem analysis works due to unwanted bug reports.

Comment 5

[Bob]

I posted this to Bug 333846 but will repost here.  It would be great if there was some action on it...

I am very interested in this feature as well.  I really think that TB needs a
way to "Make message available in ALL folders for offline use" that would work
like the equivalent one for INBOX.  In fact, there should be a per folder
setting for this as well (it could go in the same dialog as you get when right
clicking a folder and selecting "Properties", right below the option "check
this folder for new messages").

I tried the mail.server.serverX.autosync_offline_stores hidden pref and it only
goes half way.  It only sync's if the folder is opened while online.  This does
you little good if you lose your online connection suddenly and you want to
look at an email in a folder you have not opened for a while.  I think the sync
should happen with every check of the IMAP server for new mail.

I realize that these operations would be disk and bandwidth expensive but for a
computer with a big disk and a fat pipe it is not a big deal.  I really want my
local folders to be a complete backup of what is on the IMAP server.  Note that
Eudora has this and many of my colleagues use it, in part, for this feature (it
is a per folder setting only on Eudora).  They are worried that with the
TB/Eudora merge they may lose this very nice feature.  I, on the other hand,
like TB but have to keep remembering to do the "download/sync" think
neurotically.

Comment 6

[David Ascher (:davida)]

Interesting to know that Eudora does this to.  It might be another project we can do with the Penelope folks.  Cc'ing beckley for his enjoyment.

Comment 7

[WADA:World Anti-bad-Duping Agency]

See also Bug 425723 please, for another issue in current autosync_offline_stores design. 

Comment 8

[Jeff Beckley]

Cc'ing Dale Wiggins, as he is the Eudora IMAP guru.

Comment 9

[Bob]

(In reply to comment #6)
> Interesting to know that Eudora does this to.  It might be another project we
> can do with the Penelope folks.  Cc'ing beckley for his enjoyment.

The Eudora code may be useful, but TB already has the code to do the syncing since it already knows how to sync the INBOX.  This code just needs to be applied to other folders, and some options put in to decide which folders to apply it to (including the option to do all of them).

Comment 10

[Nikolay Shopik]

I believe this should depends on bug 428266

Comment 11

[Chris Pepper]

FWIW, I definitely want the ability to only auto-download certain mailboxes. Setting this as default is okay, as long as there's an off switch available (can be a hidden pref).

Comment 12

[Brian Kennelly]

I think that autosync should not default to TRUE until it respects the per-folder offline setting.  

It may also be necessary to make Thunderbird SELECT each folder with changed message counts if it is using the STATUS command to find them.

Comment 13

[Nikolay Shopik]

Problem with autosync_offline_stores is it doesn't download messages to offline until you open this folder. The only solution I come up with is set mail.imap.use_status_for_biff=FALSE which increase overhead and load on server.

Comment 14

[David :Bienvenu]

Shouldn't this be marked fixed/duped with the preemptive imap download bug?

Comment 15

[David Ascher (:davida)]

I think what's left is to actively switch folders to offline on migration, with some answer for folks for whom that won't work.

Comment 16

[David :Bienvenu]

Emre, is this going to make beta 1? Code freeze is Nov 18th; string freeze is Nov. 11th)

Comment 17

[David :Bienvenu]

sorry, string freeze is 11/13

Comment 18

[Emre Birol]

We have two text messages here; the first one is the upgrade message where we tell the user that we set all imap folders to offline, second is the title of the 'tab' containing the message. I am going to ask rebron's opinion on that.

Comment 19

[Emre Birol]

It looks like we are going to announce imap-folder-offline switch on the "What's new" tab. That makes this bug string free.

Switching imap folders to offline automatically is going to put these folders into Auto-Sync Manager's radar. During idle time (not immediately), offline folders will be sync'd with the server automatically (see bug 436615) on background.

- to turn this feature off for a specific folder, you can go and set the folder "not offline". 
- to to turn auto-sync off for all folders, you can set the autosync_offline_stores pref to false.

Comment 20

[Emre Birol]

Attachment: Proposed fix 1

Not sure about the usage of the thread pane ui version.

Comment 21

[David :Bienvenu]

Comment on attachment 347918 [details] [diff] [review]
Proposed fix 1

numFolders isn't need, right?

+        let numFolders = allFolders.Count();
+        for each (let folder in fixIterator(allFolders, Components.interfaces.nsIMsgFolder)) {

I think you can just call setFlag w/o calling getFlag - setFlag is basically a noop if the flag is already set.

to check the type of the server, 

if (server.type != "imap")
  continue;

is cleaner.

finally, should be consistent in braces style
+          if (!(folder.flags & Components.interfaces.nsMsgFolderFlags.Offline))
+            folder.setFlag(Components.interfaces.nsMsgFolderFlags.Offline);

no need for the temp var accountManager here:

+      let accountManager = Components.classes["@mozilla.org/messenger/account-manager;1"].getService(Components.interfaces.nsIMsgAccountManager);
+      let servers = accountManager.allServers;


so the descendents are created by this point?

One thing about using the threadpane ui version is that, if you do, you should add a comment to the other code that upgrades threadpane ui that there's some thread pane versioning code in an other place now (or just set a flag here, and check it in the other thread pane ui upgrading code)

Comment 22

[David :Bienvenu]

I'm not crazy about using the threadpane ui version, but I understand that it's convenient - I think we should just add a comment here that we're using the threadpane ui version as a more general versioning mechanism.

Comment 23

[Emre Birol]

Attachment: Rev 1

>so the descendents are created by this point?

Yes, after LoadPostAccountWizard() is called, it works fine.

Comment 24

[David :Bienvenu]

Comment on attachment 347935 [details] [diff] [review]
Rev 1

I tested this and it works - but I know jminta's going to hate adding an other global, and it occurred to me, maybe we don't need the global at all - this code could just go where the normal thread pane ui upgrade happens. We don't really care if it's a new profile or an existing profile now, because we're not putting up an alert at all. I.e., we'll always set imap folders for offline use when upgrading a profile, or creating a new one. I can make that change and check it in, if you like...

Comment 25

[Emre Birol]

I thought the same thing first but unfortunately UpgradeThreadPaneUI is getting called before the OnLoadMessenger(). In order to make ListDescendents work we need to call it before LoadPostAccountWizard(), otherwise it returns 0. Basically that's why I used the global.

Comment 26

[Emre Birol]

err, I meant _after_ LoadPostAccountWizard()

Comment 27

[David :Bienvenu]

Attachment: get rid of global

same as Emre's last patch, except that we got rid of the global, and moved the setting of the flag directly to the thread pane version code.

Comment 28

[Emre Birol]

Comment on attachment 348024 [details] [diff] [review]
get rid of global

Tested, looks good. Thanks for clarification.

Comment 29

[David :Bienvenu]

fix checked in

Comment 30

[Steffen Wilberg]

Comment on attachment 348024 [details] [diff] [review]
get rid of global

>diff --git a/mail/base/content/msgMail3PaneWindow.js b/mail/base/content/msgMail3PaneWindow.js
>--- a/mail/base/content/msgMail3PaneWindow.js
>+++ b/mail/base/content/msgMail3PaneWindow.js
>@@ -669,7 +669,7 @@
>   // verifyAccounts returns true if the callback won't be called
>   if (verifyAccounts(LoadPostAccountWizard))
>     LoadPostAccountWizard();
>-
>+       
>   window.addEventListener("AppCommand", HandleAppCommandEvent, true);
Hey, I know that line, I just added it last week...
Adding some whitespace before so that it's not so lonely? ;-)

Comment 31

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #23)
> Created an attachment (id=347935) [details]
> Rev 1
> 
> >so the descendents are created by this point?
> 
> Yes, after LoadPostAccountWizard() is called, it works fine.

Is this patch still needed for review, or now obsolete?