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

RESOLVED FIXED in Thunderbird 3.0a3

Status

Thunderbird
Preferences
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Emre Birol, Assigned: Emre Birol)

Tracking

Trunk
Thunderbird 3.0a3
x86
All

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

9 years ago
mail.server.default.offline_download should make user created folders and discovered folders offline by default. It only works for user created folders.
(Assignee)

Updated

9 years ago
Blocks: 436615
(Assignee)

Updated

9 years ago
Depends on: 369096
(Assignee)

Comment 1

9 years ago
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
(Assignee)

Comment 2

9 years ago
Created attachment 336148 [details]
Revision 1

Makes discovered sub folders and INBOX offline by default if pref is set
Attachment #336148 - Flags: superreview?(bienvenu)
Attachment #336148 - Flags: review?(bienvenu)
(Assignee)

Comment 3

9 years ago
Created attachment 336372 [details] [diff] [review]
Revision 2

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 4

9 years ago
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-
(Assignee)

Comment 5

9 years ago
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?
(Assignee)

Comment 6

9 years ago
Created attachment 337526 [details] [diff] [review]
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.
Attachment #336372 - Attachment is obsolete: true
Attachment #337526 - Flags: review?(bienvenu)

Comment 7

9 years ago
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.
(Assignee)

Comment 8

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

Comment 9

9 years ago
Created attachment 337689 [details] [diff] [review]
Final
Attachment #337526 - Attachment is obsolete: true
Attachment #337689 - Flags: superreview?(bienvenu)
Attachment #337689 - Flags: review?(bienvenu)
Attachment #337526 - Flags: review?(bienvenu)

Comment 10

9 years ago
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?
(Assignee)

Comment 11

9 years ago
Created attachment 337705 [details] [diff] [review]
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?
Attachment #337689 - Attachment is obsolete: true
Attachment #337689 - Flags: superreview?(bienvenu)
Attachment #337689 - Flags: review?(bienvenu)

Updated

9 years ago
Attachment #337705 - Flags: superreview+
Attachment #337705 - Flags: review+

Comment 12

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

yay, thx, sorry if I was unclear earlier...
(Assignee)

Updated

9 years ago
Attachment #337705 - Attachment is patch: true
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Checked in, changeset id: 303:02875f109cf1
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 14

9 years ago
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.