The default bug view has changed. See this FAQ.

Make the account wizard extensible

RESOLVED FIXED in Thunderbird 3.0a3

Status

Thunderbird
Account Manager
--
major
RESOLVED FIXED
9 years ago
8 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

9 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

9 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

9 years ago
Created attachment 321183 [details] [diff] [review]
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.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED

Comment 4

9 years ago
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

9 years ago
Created attachment 322542 [details] [diff] [review]
Updated version

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

9 years ago
Created attachment 323222 [details] [diff] [review]
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.
Attachment #322542 - Attachment is obsolete: true
(Assignee)

Comment 7

9 years ago
Created attachment 325407 [details] [diff] [review]
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.
Attachment #323222 - Attachment is obsolete: true
Attachment #325407 - Flags: superreview?(bienvenu)
Attachment #325407 - Flags: review?
(Assignee)

Updated

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

Updated

9 years ago
Blocks: 449226
(Assignee)

Comment 8

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

Updated

9 years ago
Keywords: dev-doc-needed

Updated

9 years ago
Attachment #325407 - Flags: review+
(Assignee)

Updated

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

Comment 10

9 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

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

Comment 11

9 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

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

Updated

8 years ago
Keywords: dev-doc-needed

Comment 13

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

Comment 14

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