452615 - mail.server.default.offline_download when set, it doesn't make discovered imap folders offline by default

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Emre Birol]

mail.server.default.offline_download should make user created folders and discovered folders offline by default. It only works for user created folders.

Comment 1

[Emre Birol]

a snippet from our irc conversation with bienvenu:

bienvenu: my inclinination is to change the meaning of the pref
bienvenu: to mean that folders are configured for offine use by default
bienvenu: and change the pref ui to reflect that
emre: makes sense
bienvenu: and then just set the offline flag whenever we don't have a .msf file, and not just when we've created a folder...
bienvenu: since we're really trying to encourage offline by default
emre: ok, where is the right place to make this change?
bienvenu: you have to use a different string in the options ui dtd and xul files for one
emre: I mean where we check whether there is a msf file or not
bienvenu: doesn't that code already check that?
bienvenu:   a_nsIFolder->GetChildWithURI(uri, PR_TRUE, caseInsensitive, getter_AddRefs(child));
bienvenu: that tells us if it's aNewFolder
bienvenu: one we didn't know about
bienvenu: and the only way we would know about it is if there was a .msf file
emre: ok cool
bienvenu: in nsImapIncomingServer::PossibleImapMailbox
emre: I think there is a setting for inbox on pref dialog saying "Make the messages in my Inbox available when I am working offline"
bienvenu: right, I think that's going away
emre: ok

Comment 2

[Emre Birol]

Attachment: Revision 1

Makes discovered sub folders and INBOX offline by default if pref is set

Comment 3

[Emre Birol]

Attachment: Revision 2

Revision 1 is not the correct patch.

Comment 4

[David :Bienvenu]

Comment on attachment 336372 [details] [diff] [review]
Revision 2

the problem with putting the check here is that this code is executed for every imap inbox on startup, so the user could never turn off sync for the imap inbox...I'm not sure why the user would want to do that, but it doesn't cost us anything to put this code in a place where it would work. My suggestion would be nsImapMailFolder::GetSubFolders where we create the imap inbox if it doesn't exist.

But I wonder how we should handle the upgrade case - do we want to switch existing profiles to have folders configured for offline use by default, in other words, do a one-time switch of all imap folders to be configured for offline use? Bryan?

Comment 5

[Emre Birol]

My first try was something like that in nsImapMailFolder::GetSubFolders()
...
...
GetFolderWithFlags(nsMsgFolderFlags::Inbox, getter_AddRefs(inboxFolder));
      if (!inboxFolder)
      {
        PRInt32 flags = 0;
        //special case: Make INBOX offline by default if offline_download pref is true 
        nsCOMPtr<nsIImapIncomingServer> imapServer;
        GetImapIncomingServer(getter_AddRefs(imapServer));
        if (imapServer)
        {
          PRBool setNewFoldersForOffline = PR_FALSE;
          imapServer->GetOfflineDownload(&setNewFoldersForOffline);
          if (setNewFoldersForOffline)
            flags |= nsMsgFolderFlags::Offline;
        }
      
        // create an inbox if we don't have one.
        CreateClientSubfolderInfo(NS_LITERAL_CSTRING("INBOX"), kOnlineHierarchySeparatorUnknown, flags, PR_TRUE);
      }
...
...


This argument is used to sets the flags on the given folder. But unfortunately when I create a gmail account, this change doesn't make the INBOX of the newly created gmail imap account OFFLINE. Are these flags are overwritten later somewhere  or do I miss something?

Comment 6

[Emre Birol]

Attachment: Revision 3

Assuming that the only time AddSubfolderWithPath is called by CreateClientSubfolderInfo when a new INBOX is created, I propose this patch. 

Maybe we don't even have to touch 
>if ((boxFlags & kNewlyCreatedFolder) || *aNewFolder)
anymore.

Comment 7

[David :Bienvenu]

Comment on attachment 337526 [details] [diff] [review]
Revision 3

If you do this, would it be possible to remove the code in nsImapIncomingServer::PossibleImapMailbox that does the same thing? You'd have to move your new code outside the "    if(isServer && name.LowerCaseEqualsLiteral("inbox"))
" branch, but then it might be able to handle all new folders in one place.

Comment 8

[Emre Birol]

Works fine. The only problem now; when the user renames a folder TB forgets that the folder was offline. Is this another bug?

Comment 9

[Emre Birol]

Attachment: Final

Comment 10

[David :Bienvenu]

Comment on attachment 337689 [details] [diff] [review]
Final

I don't see the removal of the code that sets the offline flag based on the pref in nsImapIncomingServer::PossibleImapMailbox - did that not work?

Comment 11

[Emre Birol]

Attachment: Final-2

>If you do this, would it be possible to remove the code in
>nsImapIncomingServer::PossibleImapMailbox that does the same thing?

Oh, I thought you are talking about the code that the patch is adding. I didn't realize that you are talking about the whole if-block.

Ok. this one removes the block. It works fine with newly added accounts, folders, and discovered folders. My tests cover only stated cases, and don't have any clue what would be the right test coverage to make sure that removing the block doesn't cause any regression. Do you think we need more test here or it is safe to go for it?

Comment 12

[David :Bienvenu]

Comment on attachment 337705 [details] [diff] [review]
Final-2

yay, thx, sorry if I was unclear earlier...

Comment 13

[Mark Banner (:standard8)]

Checked in, changeset id: 303:02875f109cf1

Comment 14

[Emre Birol]

I don't get this change when I update hg. I don't see it at bonsai neither. Can somebody else please confirm.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Bonsai doesn't track hg.  You can follow the hg checkins at <http://hg.mozilla.org/comm-central/pushloghtml>.  I'm not sure why it wouldn't be showing up when you update, though.