Closed Bug 452615 Opened 12 years ago Closed 12 years ago

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

Categories

(Thunderbird :: Preferences, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0a3

People

(Reporter: bugmil.ebirol, Assigned: bugmil.ebirol)

References

Details

Attachments

(1 file, 4 obsolete files)

mail.server.default.offline_download should make user created folders and discovered folders offline by default. It only works for user created folders.
Blocks: 436615
Depends on: 369096
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
Attached file Revision 1 (obsolete) —
Makes discovered sub folders and INBOX offline by default if pref is set
Attachment #336148 - Flags: superreview?(bienvenu)
Attachment #336148 - Flags: review?(bienvenu)
Attached patch Revision 2 (obsolete) — Splinter Review
Revision 1 is not the correct patch.
Attachment #336148 - Attachment is obsolete: true
Attachment #336372 - Flags: superreview?(bienvenu)
Attachment #336372 - Flags: review?(bienvenu)
Attachment #336148 - Flags: superreview?(bienvenu)
Attachment #336148 - Flags: review?(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?
Attachment #336372 - Flags: superreview?(bienvenu)
Attachment #336372 - Flags: superreview-
Attachment #336372 - Flags: review?(bienvenu)
Attachment #336372 - Flags: review-
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?
Attached patch Revision 3 (obsolete) — Splinter Review
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.
Attachment #336372 - Attachment is obsolete: true
Attachment #337526 - Flags: review?(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.
Works fine. The only problem now; when the user renames a folder TB forgets that the folder was offline. Is this another bug?
Attached patch Final (obsolete) — Splinter Review
Attachment #337526 - Attachment is obsolete: true
Attachment #337689 - Flags: superreview?(bienvenu)
Attachment #337689 - Flags: review?(bienvenu)
Attachment #337526 - Flags: review?(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?
Attached patch Final-2Splinter Review
>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?
Attachment #337689 - Attachment is obsolete: true
Attachment #337689 - Flags: superreview?(bienvenu)
Attachment #337689 - Flags: review?(bienvenu)
Attachment #337705 - Flags: superreview+
Attachment #337705 - Flags: review+
Comment on attachment 337705 [details] [diff] [review]
Final-2

yay, thx, sorry if I was unclear earlier...
Attachment #337705 - Attachment is patch: true
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Checked in, changeset id: 303:02875f109cf1
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
I don't get this change when I update hg. I don't see it at bonsai neither. Can somebody else please confirm.
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.
No longer depends on: 369096
Target Milestone: --- → Thunderbird 3.0a3
Duplicate of this bug: 268020
You need to log in before you can comment on or make changes to this bug.