Make the account wizard extensible

RESOLVED FIXED in Thunderbird 3.0a3

Status

--
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 3.0a3
Bug Flags:
blocking-thunderbird3 +
wanted-thunderbird3.0a2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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.
Flags: wanted-thunderbird3.0a2?
Flags: blocking-thunderbird3?
Seems like solid reasoning to me, and since improving our extensibility story is a goal of Tb3, approving both nominations.
Flags: wanted-thunderbird3.0a2?
Flags: wanted-thunderbird3.0a2+
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3+

Comment 2

11 years ago
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.)
(Assignee)

Comment 3

11 years ago
Posted patch Extensible account wizard (obsolete) — Splinter Review
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.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 5

11 years ago
Posted patch Updated version (obsolete) — Splinter Review
Some more work on handling identity-less servers. I can probably also remove some special casing here now.
Attachment #321183 - Attachment is obsolete: true
(Assignee)

Comment 6

11 years ago
Posted patch Patch with more additions (obsolete) — Splinter Review
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.
Attachment #322542 - Attachment is obsolete: true
(Assignee)

Comment 7

11 years ago
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.
Attachment #323222 - Attachment is obsolete: true
Attachment #325407 - Flags: superreview?(bienvenu)
Attachment #325407 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #325407 - Flags: review? → review?(dmose)
(Assignee)

Updated

11 years ago
Blocks: 449226
(Assignee)

Comment 8

11 years ago
dmose, are you going to be looking at this soon?
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.
Keywords: dev-doc-needed
(Assignee)

Updated

11 years ago
Attachment #325407 - Flags: review?(dmose)

Comment 10

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

Updated

11 years ago
Attachment #325407 - Flags: superreview?(bienvenu) → superreview+

Comment 11

11 years ago
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...
(Assignee)

Comment 12

11 years ago
Checked in as changeset 7cd9b3d26cef, with dmose's nits addressed.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b1

Updated

10 years ago
Keywords: dev-doc-needed
Would you mind posting the link to where it's documented when removing dev-doc-needed?
(Assignee)

Comment 14

10 years ago
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.
You need to log in before you can comment on or make changes to this bug.