433801 - Make the account wizard extensible

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Joshua Cranmer [:jcranmer]]

The account wizard currently recognizes three types of accounts:

* Mail (IMAP/POP)
* ISP data (RSS, movemail, gmail, etc.)
* News

If you try to add another account that's not ISP data, the account wizard assumes that it's really a news server.

Since this inhibits the ability to create radically different accounts without going through insane hackery, this bug is being nominated for blocking-TB3 and wanted-a2, as well as a major severity.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Seems like solid reasoning to me, and since improving our extensibility story is a goal of Tb3, approving both nominations.

Comment 2

[Karsten Düsterloh]

As a sidenote:
It's probably worth to keep in mind that the account manager should "go toolkit" some time, i.e. use the new prefwindow instead of the old widget state manager and such. (We've already derived a tree-using version of that for SM, which might come handy then.)

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: Extensible account wizard

Surprisingly simple. I'm holding off on requesting review on this patch until I finish the account-provider portion of my extension to see if there is anything else that could be done to aid extensions in this regard.

Possible future steps (albeit not-so-easy ones that I'm hesitant to implement):
* Move RSS and movemail to this format instead of relying on ispdata.
* Extending ispdata to allow for extension-provided account types. Very risky, and not at all well-developed.

For any extension developers interested: to add a new account type:
* overlay an xul:radio onto the radiogroup id="acctyperadio" (AccountWizard.xul)
* overlay an xul:wizardpage onto the wizardpages id="AccountWizard"
* set the radio button's id to a value
* set the radio button's value to a comma-separated list of the wizard pages' ids. You probably want to have accnamepage (the one that prompts for the account name) to be last.

For further information, see mailnews/base/prefs/resouces/content/AccountWizard.xul.

Comment 4

[Magnus Melin [:mkmelin]]

For the ISP data to be able to be for both pop/imap etc (on the same domain), the patch in bug 400931 fixes it.

Comment 5

[Joshua Cranmer [:jcranmer]]

Attachment: Updated version

Some more work on handling identity-less servers. I can probably also remove some special casing here now.

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: Patch with more additions

Same patch, a little bit more:
* Allowed whitespace in the magic radio value button
* Changed a [noscript] method in nsIRDFResource to allow JS RDF implementations (and by proxy, folder implementations)
* Changed a [noscript] method in nsIMsgFolder to allow JS folder implementations. Two other files are affected by the inversion of arguments. IMHO, this method shouldn't even exist in nsIMsgFolder at all.

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: Most recent patch

I've decided to drop the RDF change into my localfixes patch, since it's only really necessary until RDF is dropped.

I also decided to just leave out the subscribe UI changes since I'm not sure that what I did is the best way to go about doing so; for now it also goes into localfixes until I can get a better idea of what should be done.

Comment 8

[Joshua Cranmer [:jcranmer]]

dmose, are you going to be looking at this soon?

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 325407 [details] [diff] [review]
Most recent patch

Sorry for the reviewing delay.

>diff --git a/mailnews/base/prefs/resources/content/AccountWizard.js b/mailnews/base/prefs/resources/content/AccountWizard.js
>--- a/mailnews/base/prefs/resources/content/AccountWizard.js
>+++ b/mailnews/base/prefs/resources/content/AccountWizard.js

>@@ -417,17 +417,19 @@ function createAccount(accountData)
>     
>     var server = am.createIncomingServer(username,
>                                          server.hostName,
>                                          server.type);
> 
>     dump("am.createAccount()\n");
>     var account = am.createAccount();
>     
>-    if (accountData.identity.email) // only create an identity for this account if we really have one (use the email address as a check)
>+    if (accountData.identity && accountData.identity.email)
>+    // only create an identity for this account if we really have one
>+    // (use the email address as a check)

The comment belongs above the |if| rather than below, I'd say.

>+    if (server.type == "rss" || account.identities.Count() == 0)

It'd be nice to have a comment above this explaining why it makes sense to use .Count here (because copies and folders live on an identity object).

>@@ -952,16 +954,18 @@ function FixupAccountDataForIsp(accountD
>
> [...]
>
>     var email = accountData.identity.email;
>+    if (!email)
>+      return;

Is it possible for an identity not to have an email -- or is the real error here likely to be that the identity itself is null?
        
>diff --git a/mailnews/base/prefs/resources/content/aw-accounttype.js b/mailnews/base/prefs/resources/content/aw-accounttype.js
>--- a/mailnews/base/prefs/resources/content/aw-accounttype.js
>+++ b/mailnews/base/prefs/resources/content/aw-accounttype.js
>@@ -84,22 +84,27 @@ function setupWizardPanels() {
>       if (gCurrentAccountData.wizardSkipPanels)
>         skipPanels = gCurrentAccountData.wizardSkipPanels.toString().toLowerCase();
>     } catch(ex) {}
> 
>       // "done" is the only required panel for all accounts. We used to require an identity panel but not anymore.
>       // initialize wizardPanels with the optional mail/news panels
>       var wizardPanels, i;
>       var isMailAccount = pageData.accounttype.mailaccount;
>-    if (skipPanels == "true") // Support old syntax of true/false for wizardSkipPanels
>-      wizardPanels = new Array("identitypage"); 
>-    else if (isMailAccount && isMailAccount.value)
>-        wizardPanels = new Array("identitypage", "serverpage", "loginpage", "accnamepage");
>-      else
>-        wizardPanels = new Array("identitypage", "newsserver", "accnamepage");
>+      var isNewsAccount = pageData.accounttype.newsaccount;
>+  if (skipPanels == "true") // Support old syntax of true/false for wizardSkipPanels
>+    wizardPanels = new Array("identitypage"); 
>+  else if (isMailAccount && isMailAccount.value)
>+    wizardPanels = new Array("identitypage", "serverpage", "loginpage", "accnamepage");
>+  else if (isNewsAccount && isNewsAccount.value)
>+    wizardPanels = new Array("identitypage", "newsserver", "accnamepage");
>+  else { // An account created by an extension and XUL overlays
>+    var button = document.getElementById("acctyperadio").selectedItem;
>+    wizardPanels = button.value.split(/ *, */);
>+  }

Please fix the whitespace in the changed code to match the lines preceding it.

> 
>       // Create a hash table of the panels to skip
>       var skipArray = skipPanels.split(",");
>       var skipHash = new Array();
>       for (i = 0; i < skipArray.length; i++)
>         skipHash[skipArray[i]] = skipArray[i];
> 
>       // Remove skipped panels
>diff --git a/mailnews/base/public/nsIMsgFolder.idl b/mailnews/base/public/nsIMsgFolder.idl
>--- a/mailnews/base/public/nsIMsgFolder.idl
>+++ b/mailnews/base/public/nsIMsgFolder.idl
>@@ -431,17 +431,17 @@ interface nsIMsgFolder : nsISupports {
>   boolean matchOrChangeFilterDestination(in nsIMsgFolder folder,
>                                          in boolean caseInsensitive);
>   boolean confirmFolderDeletionForFilter(in nsIMsgWindow msgWindow);
>   void alertFilterChanged(in nsIMsgWindow msgWindow);
>   void throwAlertMsg(in string msgName, in nsIMsgWindow msgWindow);
>   AString getStringWithFolderNameFromBundle(in string msgName);
>   void notifyCompactCompleted();
>   long compareSortKeys(in nsIMsgFolder msgFolder);
>-  [noscript] void getSortKey(out octet_ptr key, out unsigned long length);
>+  void getSortKey(out unsigned long length, [array, size_is(length), retval] out octet key);

Please add doxygen commentary for this method and bump the UUID on the interface.

This is a great fix; thanks!

r=dmose with the various comments addressed as necessary.

Maybe slap your comments about adding a new account type up on a devmo page, even in the raw form that they are now?  Adding dev-doc-needed keyword.

Comment 10

[David :Bienvenu]

since we're rewriting the account wizard for autoconfig, I'm not sure how useful some of this is going to be.

Comment 11

[David :Bienvenu]

Comment on attachment 325407 [details] [diff] [review]
Most recent patch

sr=me, but you'll need to fit into the new account wizard stuff when it lands...

Comment 12

[Joshua Cranmer [:jcranmer]]

Checked in as changeset 7cd9b3d26cef, with dmose's nits addressed.

Comment 13

[Magnus Melin [:mkmelin]]

Would you mind posting the link to where it's documented when removing dev-doc-needed?

Comment 14

[Joshua Cranmer [:jcranmer]]

This was discussed over IRC, but jenzed had thought that this was deprecated by the accountconfig stuff. The utility of this bug is limited to being able to create new account types, so documentation now is not terribly imperative.

The account wizard may or may not be going away in Thunderbird 3.next--no decision yet, but the appropriate actions, included adding dev-doc-needed, will be taken at that time.