1075913 - Select proper (currently used) account in account manager when possible.

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, enhancement)

Description

[:aceman]

There are some places where we allow opening the Account manager (e.g. in compose window -> Tools -> Account settings) but it opens the default account instead of the currently used one. Which one is the used account depends on the caller. So I propose adding a way to specify which account to open. This seems useful in the compose window as an example. From there, the user probably wants to edit the settings of the account that he is composing a message from (e.g. to select proper SMTP server or edit the identity).

Comment 1

[:aceman]

Attachment: patch

I've tested the TB changes. Neil please check the Seamonkey and mailnews parts.

Comment 2

[[e-mail]]

Comment on attachment 8498388 [details] [diff] [review]
patch

>+        if (!aServer) {
>+          try {
>+            aServer = GetSelectedMsgFolders()[0] || GetDefaultAccountRootFolder();
>+            aServer = aServer.server;
>+          } catch (ex) { /* functions might not be defined */ }
>+        }
Do we actually need this?

>-      case "cmd_account"            : MsgAccountManager(null); break;
>+      case "cmd_account"            :
>+        let currentAccountKey = getCurrentAccountKey();
>+        let account = gAccountManager.getAccount(currentAccountKey);
>+        MsgAccountManager(null, account.incomingServer);
>+        break;
This isn't going to cause us problems down the line because of let scoping rules, I hope?

Comment 3

[:aceman]

(In reply to [e-mail] from comment #2)
> >+          try {
> >+            aServer = GetSelectedMsgFolders()[0] || GetDefaultAccountRootFolder();
> >+            aServer = aServer.server;
> >+          } catch (ex) { /* functions might not be defined */ }
> Do we actually need this?
I'd leave it in. We may call this function from any place and those functions really may not be defined. E.g. in the compose window.

> >-      case "cmd_account"            : MsgAccountManager(null); break;
> >+      case "cmd_account"            :
> >+        let currentAccountKey = getCurrentAccountKey();
> >+        let account = gAccountManager.getAccount(currentAccountKey);
> >+        MsgAccountManager(null, account.incomingServer);
> >+        break;
> This isn't going to cause us problems down the line because of let scoping
> rules, I hope?
Not sure what you mean here. There are no further usage of variables with same names in this function. So there should be no potential collisions. Also I think the variables should exist until the call to MsgAccountManager finishes. Of course I can change it to 'var' if that is the style in SM.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 8498388 [details] [diff] [review]
patch

Review of attachment 8498388 [details] [diff] [review]:
-----------------------------------------------------------------

r=mkmelin with the below fixed

::: mailnews/base/prefs/content/accountUtils.js
@@ +242,5 @@
> +        if (!aServer) {
> +          try {
> +            aServer = GetSelectedMsgFolders()[0] || GetDefaultAccountRootFolder();
> +            aServer = aServer.server;
> +          } catch (ex) { /* functions might not be defined */ }

instead of try/catch, just check if the functions are defined or not. try-catch for reference errors is just bad

Comment 5

[:aceman]

Attachment: patch v2

OK, made the function detection more robust.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8499096 [details] [diff] [review]
patch v2

Review of attachment 8499096 [details] [diff] [review]:
-----------------------------------------------------------------

thx!

Comment 7

[:aceman]

Thanks.

Comment 8

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/39eef55f4d64 -> FIXED