1124015 - Add UI to select maildir for storage when creating accounts

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Kent James (:rkent)]

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

Comment 1

[Kent James (:rkent)]

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.

Comment 2

[Kent James (:rkent)]

Attachment: AccountSettingsLocalMaildir.png

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)

Comment 3

[Kent James (:rkent)]

Attachment: ServerSettingsMaildir.png

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

Comment 4

[Kent James (:rkent)]

Attachment: MaildirOptions.png

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

Comment 5

[Magnus Melin [:mkmelin]]

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.

Comment 6

[Kent James (:rkent)]

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.

Comment 7

[Magnus Melin [:mkmelin]]

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?)

Comment 8

[:aceman]

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).

Comment 9

[Kent James (:rkent)]

Attachment: IMAP store type with menulist

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.

Comment 10

[Kent James (:rkent)]

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

Comment 11

[Kent James (:rkent)]

Attachment: Part 1, move getSummaryFile to folder

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)

Comment 12

[Kent James (:rkent)]

Attachment: Part 2, Define a preference to determine if storetype can be changed

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

Comment 13

[Kent James (:rkent)]

Attachment: Part 3, UI for storeType in options and account manager

Comment 14

[Kent James (:rkent)]

Attachment: MaildirOptions.png

This uses a dropdown list in the options page.

Comment 15

[Kent James (:rkent)]

Attachment: MaildirAccountSettings.png

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

Comment 16

[:aceman]

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 17

[:aceman]

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.

Comment 18

[Kent James (:rkent)]

(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.

Comment 19

[Kent James (:rkent)]

Attachment: Part 1 version 2, move getSummaryFile to folder

Comment 20

[Kent James (:rkent)]

Attachment: Part 2 version 2, Define a preference to determine if storetype can be changed

Comment 21

[Kent James (:rkent)]

Attachment: Part 3 version 2, UI for storeType in options and account manager

Comment 22

[Kent James (:rkent)]

Attachment: Part 4 (consistent with version 2), SeaMonkey strings for Account Manager

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.

Comment 23

[Kent James (:rkent)]

Hmm, lots of reviewers for a fairly simple patch.

Comment 24

[Magnus Melin [:mkmelin]]

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

Comment 25

[Kent James (:rkent)]

(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?

Comment 26

[Magnus Melin [:mkmelin]]

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.

Comment 27

[Kent James (:rkent)]

"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.

Comment 28

[Kent James (:rkent)]

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

Comment 29

[Ian Neal]

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

Comment 30

[Joshua Cranmer [:jcranmer]]

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.

Comment 31

[Kent James (:rkent)]

(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.

Comment 32

[:aceman]

(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.

Comment 33

[Wayne Mery (:wsmwk)]

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

Comment 34

[Wayne Mery (:wsmwk)]

(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?

Comment 35

[Kent James (:rkent)]

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.

Comment 36

[Kent James (:rkent)]

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.

Comment 37

[Kent James (:rkent)]

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

Comment 38

[Kent James (:rkent)]

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 39

[Kent James (:rkent)]

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 40

[Mark Banner (:standard8)]

Comment on attachment 8556803 [details]
MaildirOptions.png

Removing obsolete ui-review request, if this is still needed please consider moving to an open bug.