Last Comment Bug 452615 - mail.server.default.offline_download when set, it doesn't make discovered imap folders offline by default
: mail.server.default.offline_download when set, it doesn't make discovered ima...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 3.0a3
Assigned To: Emre Birol
:
Mentors:
: 268020 (view as bug list)
Depends on:
Blocks: 436615
  Show dependency treegraph
 
Reported: 2008-08-28 06:43 PDT by Emre Birol
Modified: 2009-03-17 00:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Revision 1 (1.20 KB, text/plain)
2008-08-29 19:00 PDT, Emre Birol
no flags Details
Revision 2 (1.56 KB, patch)
2008-09-01 11:14 PDT, Emre Birol
mozilla: review-
mozilla: superreview-
Details | Diff | Splinter Review
Revision 3 (3.25 KB, patch)
2008-09-08 14:04 PDT, Emre Birol
no flags Details | Diff | Splinter Review
Final (2.53 KB, patch)
2008-09-09 10:51 PDT, Emre Birol
no flags Details | Diff | Splinter Review
Final-2 (3.42 KB, patch)
2008-09-09 11:50 PDT, Emre Birol
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review

Description Emre Birol 2008-08-28 06:43:57 PDT
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 2008-08-29 13:20:53 PDT
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 2008-08-29 19:00:35 PDT
Created attachment 336148 [details]
Revision 1

Makes discovered sub folders and INBOX offline by default if pref is set
Comment 3 Emre Birol 2008-09-01 11:14:59 PDT
Created attachment 336372 [details] [diff] [review]
Revision 2

Revision 1 is not the correct patch.
Comment 4 David :Bienvenu 2008-09-02 09:41:31 PDT
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 2008-09-02 11:35:05 PDT
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 2008-09-08 14:04:27 PDT
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.
Comment 7 David :Bienvenu 2008-09-09 09:19:00 PDT
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 2008-09-09 09:55:50 PDT
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 2008-09-09 10:51:22 PDT
Created attachment 337689 [details] [diff] [review]
Final
Comment 10 David :Bienvenu 2008-09-09 11:08:01 PDT
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 2008-09-09 11:50:01 PDT
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?
Comment 12 David :Bienvenu 2008-09-09 13:05:57 PDT
Comment on attachment 337705 [details] [diff] [review]
Final-2

yay, thx, sorry if I was unclear earlier...
Comment 13 Mark Banner (:standard8) (afk until 26th July) 2008-09-10 03:51:12 PDT
Checked in, changeset id: 303:02875f109cf1
Comment 14 Emre Birol 2008-09-12 09:39:23 PDT
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 (:dmose) 2008-09-12 10:50:59 PDT
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.
Comment 16 Mark Banner (:standard8) (afk until 26th July) 2009-03-17 00:30:25 PDT
*** Bug 268020 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.