Closed Bug 541209 Opened 14 years ago Closed 14 years ago

Profile Update should honor existing offline folder settings

Categories

(Thunderbird :: Account Manager, defect)

Product:
Thunderbird
Component:
Account Manager
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: tanstaafl, Assigned: bwinton)

References

Details

(Whiteboard: [fixed as part of bug 569161])

Attachments

(1 obsolete file) 

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7
Build Identifier: 3.0.1 final

When updating a profile from Thunderbird 2, the current offline folder settings (from 'Offline and Disk Space' > 'Select folders for offline use') should be honored. Currently, if you simply close all of the Tabs but the mail folders Tab, it simply changes them all to full offline mode.

Reproducible: Always

Steps to Reproduce:
1. Start with a Thunderbird 2 installation and profile with at least one IMAP account in it with some folders set to offline mode and some not.

2. Update Thunderbird to v 3.0.1, launch Thunderbird

3. Close all of the informational tabs, leaving only the mail folders tab open

4. The offline settings for all folders in all accounts are set to full offline mode, even though they weren't prior to upgrading.
Actual Results:  
All folders set to full offline mode, regardless of previous settings in TB2 profile.

Expected Results:  
The update should honor my carefully chosen offline folder settings.

This isn't a very big problem if you only have one account and a few folders, but in my case, it is a *huge* problem, because I have 16+ IMAP accounts, some with *hundreds* of folders, and only a few, select folders set to full offline mode with the rest *not*.
Oh - and I neglected to say that most of my IMAP accounts have multiple GB of messages, some with 10+GB...

The way I have tuned my offline folder settings gives me a nice balance - considerably less than a GB of local storage for the mail lists and things that don't have a lot of large binary attachments, but lots of messages, for fast searching...

I spent more than an hour configuring the offline settings before I got frustrated and switched back to TB2.
Tested migration:
From: version 2.0.0.23 (20090812)
To: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.1.7) Gecko/20100111 Thunderbird/3.0.1

I created in TB v2 a new Profile with an IMAP Gmail account for this test.
This new profile has no extension.
On Gmail account 2 folders:
- folder "TEST" with "Select folders for offline use" not set
- folder "BUG" with "Select folders for offline use" set to X

I started this profile with TB v3.0.1.
Checked Gmail 2 folders:
- folder "TEST" with "Select folders for offline use" set to X => NOT CORRECT
- folder "BUG" with "Select folders for offline use" set to X => Correct
I try to start in Safe Mode => no change

I update this bug to NEW

***********
I changed COMPONENT to Account Manager based on
https://wiki.mozilla.org/QA/Community/Bug_Watchers#Thunderbird_Components_-_Default_QA_Contact
**********
Status: UNCONFIRMED → NEW
Component: Preferences → Account Manager
Ever confirmed: true
QA Contact: preferences → account-manager
Thanks cantalou...

Hmmm... maybe there should be a prompt during a profile upgrade when there are IMAP accounts defined, that allows the user to choose one of three things:

1. Honor existing offline settings

2. Enable offline for all

3. Disable offline for all

I personally think this should be a separate prompt *before* it connects to the accounts and starts downloading anything, as opposed to just showing up on the Migration Tab.

Side note: I'm still working on getting the UI configured the way I like it, but once some of these bugs/issues (like this one) are fixed up, TB3 will be pretty sweet... :)
The migration code introduced by bug 385502 explicitly iterates over all folders when migrating from a 2.0 or earlier profile to mark them all for offline use, so that's apparently overdoing it.

> (In reply to comment #3)... a separate prompt *before* it connects to the
> accounts and starts downloading anything, as opposed to just showing up on the
> Migration Tab.

That problem is related to bug 522631 - connecting to the IMAP server is currently done at the same time as the migration assistant is shown, thus starting synchronization already while the user is still picking options...
Blocks: 385502
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk
Is this bug going to get fixed for 3.1?

I am warning all TB devs - if this bug isn't addressed, and serious IMAP users who have carefully defined their offline settings (like I did) upgrade and see their carefully selected offline settings arrogantly demolished, you will hear screaming and a gnashing of teeth.

Please, don't make this mistake. Fix this serious BUG.
> Bug summary: Profile Update should honor existing offline folder setting

FYI.
A way for this will be provided by fix of bug 562589.
  User puts mail.server.serverX.offline_download=false in prefs.js
  before first use of Tb 3, if user want offline-use never being touched.
This is natural enhancement of meaning of offline_download setting.
If migration assistant asks user for offline-use setting of IMAP folders, and if migration assistant sets mail.server.serverX.offline_download=false when user doesn't want "offline use" setting being touched, offline use setting is not altered upon upgrade from Tb 2 to Tb 3 by change of bug 562589.
Hi WADA,

I'm not sure I understand what you are saying...

If you mean that the user will have to know *in* *advance* to make a *manual* edit to prefs.js *before* TBs first run after upgrading, that is absolutely, positively *not* even *close* to a satisfactory resolution of this bug.

This *must* be a clear, unequivocal option *in* *the* *migration* *assistant* *GUI*, as described in Comment 3.

If that is what you meant, then - yaay!
(In reply to comment #7)
> Hi WADA,
> I'm not sure I understand what you are saying...
> If you mean that the user will have to know *in* *advance* to make a *manual*
> edit to prefs.js *before* TBs first run after upgrading, that is absolutely,
> positively *not* even *close* to a satisfactory resolution of this bug.
> This *must* be a clear, unequivocal option *in* *the* *migration* *assistant*
> *GUI*, as described in Comment 3.
> If that is what you meant, then - yaay!

I merely have let you notify about next.
  If you set a preference setting which was newly introduced by Tb3.0
  before first use of your Tb3, the new setting of Tb 3 in prefs.js
  will be respected by migration code change of Tb 3 by fix of bug 562589.
It's sufficient for usual corporate user who wants similar behaviour to Tb2 with Tb 3 after migration to Tb 3 from Tb 2.
I never say user has to know it. I simply say that we will get very simple/easy workaround of problem which you stated in this bug summary, if bug 562589 will be fixed, with expecting that you will ask developers for enhancement of Migration Assistance based on change by bug 562589 :-)
The migration assistant now offers the user a specific choice about what to do here.  Charles, would you be willing to try upgrading a Tb2 profile to Tb 3.1b2 and see how well that experience addresses your concerns?
(In reply to comment #9)
> The migration assistant now offers the user a specific choice about what to do here.

"Change to offline-use=on for all IMAP folder" is executed before Migration Assistant is called and invoked. See bug 562589 #23, please.
Dan Mosedale, does Migration Assistance change back to original offline-use or change to offline-use=off if user requests TB not to change offline-use setting"?
(In reply to comment #9)
> The migration assistant now offers the user a specific choice about what to do
> here.  Charles, would you be willing to try upgrading a Tb2 profile to Tb 3.1b2
> and see how well that experience addresses your concerns?

Absolutely! :)

I'll have to go create a 2.0 profile to upgrade to test it, though, and I'm headed out of town this weekend, so it will have to wait until next week some time.

(In reply to comment #10)
> (In reply to comment #9)
>> The migration assistant now offers the user a specific choice about what
>> to do here.

> "Change to offline-use=on for all IMAP folder" is executed before Migration
> Assistant is called and invoked. See bug 562589 #23, please.

Potential 'ouch'...

> Dan Mosedale, does Migration Assistance change back to original offline-use or
> change to offline-use=off if user requests TB not to change offline-use
> setting"?

Inquiring minds want to know... :)

WADA - sorry for my tone... I'm having a little trouble understanding your meaning sometimes...
(In reply to comment #10)
> "Change to offline-use=on for all IMAP folder" is executed before Migration
> Assistant is called and invoked. See bug 562589 #23, please.
> Dan Mosedale, does Migration Assistance change back to original offline-use or
> change to offline-use=off if user requests TB not to change offline-use
> setting"?

More precisely - does it revert the individual folder offline settings to what they were *originally*? Or just change them *all* to off?
One thing 3.0 does is change the default for newly discovered folders to be offline, before the migration assistant runs, but it doesn't poke each existing folder and configure it for offline.
? What do you mean by 'newly discovered folders'? When upgrading, all folders already exist, so what 'new' folders could it 'discover'?

And are you talking about the *current* 2.0 > 3.0 behavior? Or the new 3.1 behavior when upgrading a 3.0 profile?

Sorry, I'm just not clear on what you mean...
I'm saying when you create a new profile, or add an imap account, we default to offline for those folders. I believe that's what WADA was referring to.
(In reply to comment #15)
> I'm saying when you create a new profile, or add an imap account, we default to
> offline for those folders. I believe that's what WADA was referring to.

But you said 'when the migration assistant runs'... the migration assistant only runs immediately following an *upgrade*, no? Why would it ever run when adding a new account or even creating a new profile? There's nothing to 'migrate'...
(In reply to comment #16)
> (In reply to comment #15)
> > I'm saying when you create a new profile, or add an imap account, we default to
> > offline for those folders. I believe that's what WADA was referring to.
> 
> But you said 'when the migration assistant runs'

No, I said "before", though I was replying to WADA's comment. I'm really not sure what WADA was referring to, but I'm trying to shed light on a potential issue.
David, what I wanted to refer was Bug 562589 comment #23 and Bug 562589 comment #25, and attachment 443536 [details] [diff] [review] (patch for that bug). Sorry for wrong linkify.
Yeah, this code needs to go away - http://mxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#775

marking as blocker - is this not covered by some other bug?
blocking-thunderbird3.1: --- → rc1+
Sounds awfully similar to bug 562589.  I think we kind of want the folders to be marked offline, but let's ask the person who committed it.  :)

The changeset is http://hg.mozilla.org/releases/comm-1.9.2/rev/9346f93d9620
fixing bug 385502.

Emre, any thoughts?

Thanks,
Blake.
bug 562589 stated purpose was:
investigate turning on offline imap by default
Somehow that "investigation" became a reality causing much pain for upgraders from tb2.0
While the concept is a big gain in performance, it comes with a cost, and should be an option for the user.
Assuming the user has the Hd space and bandwidth to implement this is just..wrong.
(In reply to comment #19)
> is this not covered by some other bug?

Complaint of "all my IMAP folders was changed to offline-use=on by upgrade to Tb 3.0!" can be seen in many bug reports at B.M.O, at many forums, and at many Web pages. Typical one is comments in Bug 505759 started on 2010-01-21 after five months of WONTFIX close of Bug 505759(on 2009-08-12).
AFAIK, bug which clearly focused on the complaint was this bug and bug 562589 only, and I think first analysis of the problem is Comment 4 by rsx11m.

As Migration Assistant is disabled by user on purpose in bug 562589's case, next is never problem in that bug.
  - Migration Assistant can do nothing on offline-use=on
    which is already set by migration code.
For problem of "offline-use=on is always set" when Migration Assisant=disabled, simplest/safest patch to bypass the problem was created by Blake Winton in bug 562589. The patch is already landed on Tb 3.1pre/Tb 3.2pre, and approval for Tb 3.0.5 has been requested.
(In reply to comment #21)
Joe Sabash, there is no need to add any comment which is similar to comments by Charles Marcus started from 2010-01-21(==open date of this bug) in Bug 505759. Please read conversation between him and other peoples from 2010-01-21 in Bug 505759.
(In reply to comment #13)
> One thing 3.0 does is change the default for newly discovered folders to be
> offline, (snip)

David, does it mean that mail.server.serverX.offline_download=false which is put in prefs.js or user.js prior to migration by user is ignored by migration code of Tb3?
If so, patch of bug 562589 can do nothing...
Or merely next?
- no offline_download    in prefs.js =
    Tb2: false, or ignore                 Tb3: true (default is true)
- offline_download=true  in prefs.js =
    Tb2: keep true, or ignore             Tb3: reset it, as default
- offline_download=false in prefs.js =
    Tb2: reset it, as defaut, or ignore   Tb3: keep false
(In reply to comment #24)
> (In reply to comment #13)
> > One thing 3.0 does is change the default for newly discovered folders to be
> > offline, (snip)
> 
> David, does it mean that mail.server.serverX.offline_download=false which is
> put in prefs.js or user.js prior to migration by user is ignored by migration
> code of Tb3?

No, that patch works if the user knows to do that before migration, but normal users aren't going to know to do that. But I guess this is all working as designed, false alarm on my part - I think I would have preferred the migration assistant do all the work, instead of having to undo the work the ui version code does, but that's probably water under the bridge.
blocking-thunderbird3.1: rc1+ → ---
(In reply to comment #17)
> No, I said "before", though I was replying to WADA's comment. I'm really not
> sure what WADA was referring to, but I'm trying to shed light on a potential
> issue.

Ah, I see... but you are wrong in your comment 13:

(In reply to comment #13)
> One thing 3.0 does is change the default for newly discovered folders to
> be offline, before the migration assistant runs, but it doesn't poke each
> existing folder and configure it for offline.

??? Upgrading from 2.0 to 3.0 absolutely *does* poke each and every folder for *existing* accounts and indiscriminately sets them *all* to offline-use=on.

That's what happened to me, and what this bug is all about, so I don't see why you would make such a comment.
(In reply to comment #26)
> 
> ??? Upgrading from 2.0 to 3.0 absolutely *does* poke each and every folder for
> *existing* accounts and indiscriminately sets them *all* to offline-use=on.
> 
> That's what happened to me, and what this bug is all about, so I don't see why
> you would make such a comment.

Yes, as I pointed out in comment 19, it does poke all the folders, so I already pointed at the code that showed I was wrong, but thx for reiterating :-)
(In reply to comment #27)
> Yes, as I pointed out in comment 19, it does poke all the folders, so I already
> pointed at the code that showed I was wrong, but thx for reiterating :-)

Oh... ok, well... since ianap, I had no idea what that comment meant... ;)
(In reply to comment #9)
> The migration assistant now offers the user a specific choice about what to do

Just as an information for people following the bug here: The new migration assistant has a full page to enable or disable synchronization, and also calculates the space needed/available for the local copy. So, that's sure an improvement. Unfortunately, the third option to individually select folders
to be synchronized didn't work for me and ignored my choices.

I've filed bug 569161 for this. I had no customizations in that 2.0.0.23 profile for offline usage, maybe that was contributing, but it seems to be simply the choices made not being correctly registered after closing.

Expanding on this, the third option could be initialized with the current settings in the 2.0 profile found, thus retaining what the user has picked.
This would involve removing the folder-properties poking first.
WADA reminded me that he discussed this proposal in comment #10 already, so sorry for the duplicate, but I think it's a valid suggestion. I can't quite follow bug 562589 comment #25, not executing UpgradeProfileAndBeUglyAboutIt() for the folder properties wouldn't be a problem given that setSyncStatus() is doing it later anyway. Even if the Migration Assistant is disabled, it would simply mean that the folders aren't changed, right? So, where is the harm done?
(In reply to comment #30)
> not executing UpgradeProfileAndBeUglyAboutIt() for the folder properties
> wouldn't be a problem given that setSyncStatus() is doing it later anyway.

Does it mean that UpgradeProfileAndBeUglyAboutIt() is already not called any more? If yes, and if mail.server.serverX.offline_download=false (and mail.server.serverX.autosync_offline_stores=false too, if possible) is respected by calling of setSyncStatus() when Migration Assistance is not used, there is no problem. If yes, and if setSyncStatus() is called by Migration Assistance only, there is no problem too.  

> Even if the Migration Assistant is disabled, it would simply mean that the folders aren't changed, right?

If UpgradeProfileAndBeUglyAboutIt() is still called and current code will be  kept, answer is No, because change to offline-use=off of all folders is done in UpgradeProfileAndBeUglyAboutIt(). Bug 562589 is to skip it by putting mail.server.serverX.offline_download=false in prefs.js prior to migration(prior to first use of Tb 3).
Ok, I think I figured where the catch is. Not overwriting the folder-specific settings if the advanced sync dialog is called (as solved by bug 569161) is a prerequisite, but won't be the full solution to the problem here.

> Does it mean that UpgradeProfileAndBeUglyAboutIt() is already not called any

It is called before the Migration Assistant, and that ensures that it shows the "Synchronize" option as the default. If UpgradeProfileAndBeUglyAboutIt() was omitted, it would show the default choice based on the current settings, thus likely propose to not synchronize (against the design decision made). Since all folders are set to sync upon profile upgrade, the desired default is secured.

In order to get rid of the current "BeUglyAboutIt()" function, the Migration Assistant would have to take care of the profile upgrade itself, thus recognize that is was called in a first-run situation, then propose "Synchronization" but without actually executing setSyncStatus() already to give the user a choice to pick the "some" option. In that case, the patch in bug 569161 would ensure that the previously present selections are retained without further customizations.
Depends on: 569161
I don't know how to exactly implement this as it may involve some timing of events in the proper order, but here some suggestion based on comment #32:

 - Replace "BeUglyAboutIt()" with "BeNiceToTheUser()" and remove the code to
   set the offline flag for all folders already. Instead, set some temporary
   preference as a reminder that a profile upgrade has been performed; nothing
   would change with respect to calling featureConfigurator().

 - When entering the autosync page of the Migration Assistant, check if that
   pref is set. If yes, use "all" ("Synchronize") as the default setting to get
   the desired display for the user, not calling setSyncStatus() yet. If it is
   not set, determine the default from the current settings as already done.

 - If the user just clicks "Next" with the preference set, call setSyncStatus()
   now to set all folder properties to offline use, then remove the migration
   preference as the user has made his or her decision to accept the change.

 - If the user selects any of the other options or clicks on an account button,
   remove the preference and proceed with the current implementation. In this
   case, setSyncStatus() shouldn't be called again on clicking "Next".

The reason for making it a temporary preference would be to allow the first-run information to persist until the Migration Assistant is actually called. In the normal case, that's right after the profile upgrade. However, if it is not to be called during migration, it may be invoked by the user manually later, in which case that information is retained until some user decision has been made.
(In reply to comment #33)
> I don't know how to exactly implement this as it may involve some timing of
> events in the proper order, but here some suggestion based on comment #32:

<snip>

>  - If the user just clicks "Next" with the preference set, call 
>    setSyncStatus() now to set all folder properties to offline use,
>    then remove the migration preference as the user has made his or
>    her decision to accept the change.

Bad... my understanding was that there would be *no* 'default' behavior - ie, no 'Next' button here - and that the user would have to explicitly choose the desired behavior.

I disagree vehemently with allowing the user to screw their folder settings because they clicked 'Next' too soon (as many users are apt to do).

Please don't do this - make the user intentionally select one of the choices before the Next button is available.
(In reply to comment #34)
> Bad... my understanding was that there would be *no* 'default' behavior - ie,
> no 'Next' button here - and that the user would have to explicitly choose the
> desired behavior.

I don't see any agreement (or even proposal) stated here that this page of the Migration Assistant should open without anything checked (all other pagess have some default checked, e.g., for the toolbar buttons). This would simplify the issue by not needing to keep track of a first-use condition, only the "Next" button would have to be disabled until a choice was made.
It wasn't in this bug discussion, it was in the long heated discussion in bug 50579, where I vented a lot of extreme frustration about this issue - because it caused me an extreme amount of pain and no one seemed to get it - and other imho bad decisions that were made for 3.0 defaults.
Attached patch A first cut at the patch. (obsolete) — — Splinter Review 
Okay, here's a patch (against comm-central) that I think fixes this.

Play around with it, and let me know what you think.

(Also, I forget whether I was going to ask asuth or bienvenu for the review on this one, so if I got it wrong, please feel free to re-assign the request, Andrew.)

Thanks,
Blake.
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #449299 - Flags: ui-review?(clarkbw)
Attachment #449299 - Flags: review?(bugmail)
This would obsolete the patch in bug 569161, right?

I didn't test it, just had a brief look at it but noticed the following:
> +          if (aSyncStatus == "some") {
> +            // Restore the state of the folders that we saved from before.
> +            if (subSettings["#" + folder.folderURL])
> +              folder.setFlag(Ci.nsMsgFolderFlags.Offline);
> +            else
> +              folder.clearFlag(Ci.nsMsgFolderFlags.Offline);
> +          }
> +          else {
> +            if (newSync)
> +              folder.setFlag(Ci.nsMsgFolderFlags.Offline);
> +            else
> +              folder.clearFlag(Ci.nsMsgFolderFlags.Offline);
> +          }

Wouldn't this restore any changes made by the user in the Advanced panel of the Account Settings with the originally stored values or am I reading this wrong?
(In reply to comment #38)
> This would obsolete the patch in bug 569161, right?

Yeah, but I think this is basically including the code in that patch.  :)

> I didn't test it, just had a brief look at it but noticed the following:
> > +          if (aSyncStatus == "some") {
> > +            // Restore the state of the folders that we saved from before.
> > +            if (subSettings["#" + folder.folderURL])
> > +              folder.setFlag(Ci.nsMsgFolderFlags.Offline);
> > +            else
> > +              folder.clearFlag(Ci.nsMsgFolderFlags.Offline);
> > +          }
> > +          else {
> > +            if (newSync)
> > +              folder.setFlag(Ci.nsMsgFolderFlags.Offline);
> > +            else
> > +              folder.clearFlag(Ci.nsMsgFolderFlags.Offline);
> > +          }
> 
> Wouldn't this restore any changes made by the user in the Advanced panel of
> the Account Settings with the originally stored values or am I reading this
> wrong?

No, because we save the settings into the subSettings object when the user changes away from the "some" radio button, so this would return them to the values the user last entered.

Thanks,
Blake.
Comment on attachment 449299 [details] [diff] [review]
A first cut at the patch.

(We'll work on this in bug 569161 instead of here.)
Attachment #449299 - Attachment is obsolete: true
Attachment #449299 - Flags: ui-review?(clarkbw)
Attachment #449299 - Flags: review?(bugmail)
I think this was fixed with the patch for bug 569161.

Charles, could you please give it a try with one of tonight's nightlies (or TB 3.1 rc2), and re-open this bug if it's still giving you problems?

Thanks,
Blake.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
I will - in fact, I've been meaning to play with one of the builds since b2 came out, but I've been slammed at work.

Hopefully by Wednesday I can give this a thumbs up.

And thanks to everyone who worked on this...
Whiteboard: [fixed as part of bug 569161]
Just tried it, fixed in 3.1 RC2 per bug 569161 comment #18. The proposed default is now "Do not synchronize" even if no folder customization was performed, which is fine with me but seems to be departing from the idea to propose "Synchronize" in that case (it certainly addresses comment #34 of a user too quickly hitting "Next" and thus screwing up the synchronization settings). For a profile which has customized folder settings, "Per account" was selected as the default and the settings retained after the Migration Assistant finished. Thus, works as intended on my side!
I can also confirm this seems to be working satisfactorily for me as well. All of my offline settings were retained, although I only created 4 accounts, instead of all 16 of them, so hopefully there's no bug if there are more than X number of accounts...

Thanks guys! This would have been a biggie had 3.1 shipped without this being fixed...
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: