When new accounts are created, there needs to be a way to select one-message-per-file storage (maildir).

Here's the first issue. I believe that we should have the specification of the store type in the advanced configuration, as we don't want to clutter up the simple account creation with this issue. But that requires that we open the full account manager, and to do that we create the account along with its msg database. The location of the message database is set by a call to store->GetSummaryFile(), so the store type has to be determined BEFORE we get to the advanced configuration dialog! But currently GetSummaryFile is identical for both store types, and is actually set from the folder file path. So this could just as easily be a property of the msgFolder (which I believe it used to be). The abstraction of letting the msgStore set the location was made in hopes of some future functionality where the store might want to move the db. I don't think the hope of the future is worth the cost today, so I will probably move the msgSummaryFile location back into the folder.

This is the account settings for the local folder, showing that maildir is not enabled for the account. It is disabled because you can only change this on initial account (but see further comments)

This is server settings for an IMAP account using maildir. Again disabled.

This is in the options screen, showing the ability to enable the default store type for future accounts added.

Some thoughts: - the type should be a dropdown and not a checkbox. That will probably end up making it clearer what it's about too, but more importantly, having a selectable storage backend was/is supposed to allow the creation of 3rd party backends too. Having the UI be a binary choice is not good for that. - although techy, the name of the message store ("maildir") should be included. Trying to simplify technically hard things with dumbed down language just makes it harder for the people actually looking at that option to form an opinion about whether it's a good idea or not.

I've got the code for all of this, but we need to discuss some of the issues. First, some background. Maildir can only be enabled on a per-account basis, and it must be set that way when the account is first created. There is also a default setting for all accounts that will affect new accounts. For most people, the correct way to work with maildir would be to not create accounts for a new profile immediately, instead change the default in the options. Then you create your accounts, and all accounts would use maildir. A big issue is that currently when you use the TB account wizard, it has no option for maildir, instead you have to get into the advanced options, which opens the full Account Settings. When it does that though, it has already created the account, so for example the new account shows in the folder pane. Unfortunately the act of creating the account so that it can be setup in Account Settings goes through steps to create a few minimal folders, so that it has already accessed the mailstore by the time you can configure it, so you should not be changing the mailstore at this point. So you cannot actually configure it in the advanced settings, it is already disabled even for new accounts. I was able to fix this for IMAP (whose offline stores can use maildir, and should represent the vast majority of our users), but not yet for POP3 and Local Folders. It is likely that fixes for POP3 and Local are also possible, it is mainly an issue of delaying portions of the account initialization until the Account Settings screen is exited rather than before the screen shows. I could get around this by offering maildir earlier, in the Mail Account Setup screen (Manual Config, but not Advanced Config) but I am reluctant to do that, as the UI is designed to be progressively more advanced, and at this point maildir more properly is an advanced setting. The proposed UI though accomplishes the following: 1) There is a path through the UI, short of using Config Editor, that allows use of maildir (that is to set the global option before creating accounts). 2) For new IMAP accounts, you can set maildir during account creation in the Advanced portion of the new account wizard (but not for POP3 and Local yet). 3) For all accounts, you can now see whether a particular account is using maildir or mbox. For point 3), a disabled checkbox is probably not the ideal UI to show an account status that the user has no current way of changing. Yet that is a backend issue at the moment, and may be fixed prior to TB 38 (yet the UI strings here must land in comm-central at TB 38). As we improve maildir support, I think that this UI is what we will want in the long run. Eventually setting here may be possible after the account has been created, that would then do some sort of account conversion. So I'm currently leaning toward landing the correct long-term UI and then improving the circumstances when it can be enabled.

Well, the dropdown can be disabled too (until we offer migration). My point was that a checkbox talking about files is not extensible. (What would an extension providing sql show?)

Maybe you could somehow determine whether the account is still "new", e.g. the mbox files of all folders are of zero size. And in that case still allow to choose maildir in the account manager. Maybe you could have an internal method to switch storage type (e.g. wipe the mbox directory and create the maildir directories).

Here's with a dropdown. I like this better as well. The disabled state, which is what you will mostly see, looks much better here.

Oops, the Advanced button got misplaced. I'll fix that.

This allows us to create the summary db file without needing the messageStore. This way we can create a minimal account without calling into the message store. (BTW the Advanced menu in the previous screenshot was not misplaced)

This new preference value can be set and tested to see if it is still possible to change the storeType

This uses a dropdown list in the options page.

This shows the dropdown box (usually disabled) in account settings.

I think "Message store type for new accounts: " would be more readable in the preferences dialog. I also vote for the dropdown as Magnus requested (both prefs and account manager). Should I file a followup bug for comment 8? It could be implemented later to allow accounts created as mbox to convert to maildir (or the other way round) later even after some usage.

Comment on attachment 8553377 [details] [diff] [review] Part 1, move getSummaryFile to folder Review of attachment 8553377 [details] [diff] [review]: ----------------------------------------------------------------- Isn't the location of .msf file dependent on store type? Will a generic nsMsgDBFolder::GetSummaryFile know what to do? Maybe today it works but if we create a third store type, it could have different needs.

(In reply to :aceman from comment #17) > > Isn't the location of .msf file dependent on store type? Will a generic > nsMsgDBFolder::GetSummaryFile know what to do? Maybe today it works but if > we create a third store type, it could have different needs. It's a reasonable question. Currently, there is a file path defined for the folder. What exactly is supposed to be at that location? Only two things are there, the summaryFile, and the message store. The message store is currently pluggable, meaning that in theory it does not have to be at that location. So the only thing left that nsIMsgFolder.filePath defines is the location of the message summary file. I will maintain that the summary file is a completely different thing than the store, and the two should be defined independently. There is no reason that the store should determine the summary file location. Plus that comes at a cost, it means that the summary file cannot be defined independently of the store. So what this changes does is this: it effectively confirms that the nsIFile returned by nsIMsgFolder.filePath defined the location of the summary database. Defacto that is true now. At some future time (perhaps in the next TB cycle) we would like to implement pluggable databases. That will be the point in time to re-ask this question. It is not hard to change, but personally I doubt if we will come to the decision that the right thing is for the store to define the database. If anything, it should be the other way around. Making this change allows selection of the message store type to be done during account setup for IMAP accounts in the advanced account dialog. Eventually I would also like to fix local accounts, so that you could define the store during account setup with them as well.

I only did the Account Manager integration into SeaMonkey. It should really have the global options added as well, but that can be a separate SM-specific patch.

Hmm, lots of reviewers for a fairly simple patch.

Comment on attachment 8557301 [details] [diff] [review] Part 3 version 2, UI for storeType in options and account manager Review of attachment 8557301 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok to me, with a few nits. ::: mail/components/preferences/advanced.js @@ +26,5 @@ > #ifdef MOZ_UPDATER > this.updateReadPrefs(); > #endif > > + // Default store type initialization nit: period @@ +140,5 @@ > displayed.value = 2; > }, > > + /** > + * set the default store contract ID Capitalize and end with period. ::: mail/components/preferences/advanced.xul @@ +232,5 @@ > + value="@mozilla.org/msgstore/berkeleystore;1" > + label="&mboxStore.label;"/> > + <menuitem id="maildirStore" > + value="@mozilla.org/msgstore/maildirstore;1" > + label="&maildirStore.label;"/> nit: I think it's more common to have label before value ::: mail/locales/en-US/chrome/messenger/preferences/advanced.dtd @@ +16,5 @@ > <!ENTITY enableGlodaSearch.label "Enable Global Search and Indexer"> > <!ENTITY enableGlodaSearch.accesskey "E"> > <!ENTITY allowHWAccel.label "Use hardware acceleration when available"> > <!ENTITY allowHWAccel.accesskey "h"> > +<!ENTITY storeType.label "New Account Message Store Type:"> Maybe Default Message Store Type: ::: mailnews/base/prefs/content/am-server.js @@ +33,5 @@ > // allow users to choose it anymore. Hide the option unless the user already > // has it set. > hideUnlessSelected(document.getElementById("connectionSecurityType-1")); > + > + // UI for account store type . ::: mailnews/base/prefs/content/am-server.xul @@ +38,1 @@ > please put id first

(In reply to Magnus Melin from comment #24) > Comment on attachment 8557301 [details] [diff] [review] > Part 3 version 2, UI for storeType in options and account manager > > Review of attachment 8557301 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: mail/locales/en-US/chrome/messenger/preferences/advanced.dtd > @@ +16,5 @@ > > <!ENTITY enableGlodaSearch.label "Enable Global Search and Indexer"> > > <!ENTITY enableGlodaSearch.accesskey "E"> > > <!ENTITY allowHWAccel.label "Use hardware acceleration when available"> > > <!ENTITY allowHWAccel.accesskey "h"> > > +<!ENTITY storeType.label "New Account Message Store Type:"> > > Maybe > Default Message Store Type: This is not a trivial point, perhaps we should discuss. This setting will only affect the value for new accounts, not existing accounts. I don't think that "Default" really communicates that well. "New Account" is an accurate description of what it does. Is that OK, or did you have another suggestion?

Yes I understood that. Isn't that what a default value is. Once you set up an account, it obviously gets a value set. I'm certainly open to suggestions. "Message Store Type for new accounts" would also be better, though personally I'd still favor default.

"default" can mean many things. For example, with typical server properties, you set the default value for all servers, and that gets propagated to all servers that don't have an explicit override value. So I can change mail.server.default.autosync_offline_stores and it gets propagated to existing servers. That is not true of the store type, it is explicitly set when the account is created, and cannot be changed after that. Nobody knows these subtle differences, which is shy I think we should be clearer here. I can live with "Message Store Type for new accounts" though that is a little longer than my original.

Joshua, I really need for you to look at this and make comments. I need the strings in TB 38.

Does there need to be an equivalent SeaMonkey patch for the preferences part of this?

Comment on attachment 8557299 [details] [diff] [review] Part 2 version 2, Define a preference to determine if storetype can be changed Review of attachment 8557299 [details] [diff] [review]: ----------------------------------------------------------------- I feel like we need documentation of "magic" server pref values somewhere, but I don't know where.

(In reply to Ian Neal from comment #29) > Does there need to be an equivalent SeaMonkey patch for the preferences part > of this? Yes. The existing patches should be enough that SeaMonkey works OK, but it really should have the UI to change the preference from part 3 if you want to give users a visible way of setting maildir as a default (which I think you should add). I don't recall how account setup is done in SeaMonkey, but the situation in Thunderbird makes it quite difficult to setup maildir for local folders. You really need to abort the initial account setup screen, change the default preference, then go back to create the account. You might choose to expose the maildir option in the initial account setup screen, which we have chosen not to do in Thunderbird. Maybe next year we will though. With the changes in this bug, you can select maildir successfully during advanced account setup for imap accounts, but not for local accounts. That is a backend issue of the account creation proceeding too far before we reach the setup screen. I'd like to fix this for local accounts, but that may or may not be possible as a tb-38 bugfix. I've been running maildir myself on my main profile for a little over a month (albeit with some patches that we are still landing) so I am fairly confident that it is usable. But it is still undertested, so some caution is still in order.

(In reply to Kent James (:rkent) from comment #31) I think at this stage it is fine if we enable to select maildir for only some account types (e.g. IMAP) and do the rest gradually in next versions. I trust you that maildir is basically usable with all the pending patches, but it surely is not polished/tested as much as mbox right now. So there are still possible uncovered edge cases (the "maildir problems" meta bug has many linked bugs not yet tested). So we should not push it to users with full force and then be overwhelmed with dataloss reports.

I've not followed this, but if you are really going to expose this in version 38 UI here are some thoughts: 1. relevant information should be exposed in Help | Troubleshooting 2. be aware that even with it buried in advanced settings and even if we don't announce it, once word gets out that this is available it will spread (including blogging) and we will see many users try it not knowing the risks 3. we should actively request users to test it hard during earlybird and beta, have them file bugs, and be sure we triage those bugs quickly. I think we did this once a long time ago (2-3 years ago?) and, too late, but we probably should have done it already again during the 31-38 development period. 4. if we find too many more bad bugs during aurora and beta, we should be prepared to remove the UI 5. should we have some telemetry data on this? 6. we will want make sure our support and QA folks know what is going to happen

(In reply to :aceman from comment #32) > (In reply to Kent James (:rkent) from comment #31) > I think at this stage it is fine if we enable to select maildir for only > some account types (e.g. IMAP) and do the rest gradually in next versions. I am glad we are thinking cautiously. I support this point, after looking at the list of local folder issues blocking the meta bug - https://bugzilla.mozilla.org/buglist.cgi?bug_id=890742%2C856087%2C1124105%2C1017028%2C791966%2C1044456%2C1032360%2C852080%2C857003%2C1078367%2C856396%2C856286%2C906469%2C555539%2C1124015%2C856532%2C797710%2C859011%2C1124948%2C816304%2C1093217%2C856519%2C753147%2C58308%2C1011399%2C855954%2C856407%2C765926%2C813459&list_id=12011828 Question - is there a way, and would we want, to provide strings to enable other non-imap account types but not expose it in 38.0 UI? In other words, that additional UI choices could made available via some code change, in 38.x?

Currently the only way to enable maildir in a local folder is to set the default for all accounts. There is UI in the account manager to set and observe the account-specific store type, but for local folders this does not allow you to set maildir for new accounts. So the request to only allow this for imap offline folders would be accomplished by removing the UI for the setting of the global default. I am committed to investigating any critical bugs that show up during testing from maildir. I'll look at bug 855954. Overall I agree with the caution, but that will be a decision we can make later.

Iann, I will be landing the part 2 and part 3 soon because they have strings which are needed before the next Thunderbird uplift. That will break SeaMonkey since it also needs the common strings in mailnews for the account manager.

Parts 2 - 4 pushed. https://hg.mozilla.org/comm-central/rev/e11c5540e233 https://hg.mozilla.org/comm-central/rev/b77afc91af46 https://hg.mozilla.org/comm-central/rev/ad83df67025a

Essential UI patches landed, followup bug created to deal with the issue from part 1 of the patches: Bug 1134944 - Move getSummaryFile to folder so that maildir can be selected in imap mail setup

Comment on attachment 8557295 [details] [diff] [review] Part 1 version 2, move getSummaryFile to folder obsoleting patch as it is moved to bug 1134944

Comment on attachment 8556803 [details] MaildirOptions.png Removing obsolete ui-review request, if this is still needed please consider moving to an open bug.