621042 - Advanced Account Settings should mention account name

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Karsten Düsterloh]

(Spinoff of bug 46688.)

Open account settings and go to "Server Settings" panel of a pop/imap account. Click "Advanced...".

The resulting dialog doesn't mention which account's settings you touch, but it should, probably in the window title or a group box frame.

Comment 1

[Edmund Wong (:ewong)]

Attachment: Added account Name to the title of the Advanced Account Settings dialog. (v1)

Comment 2

[Ian Neal]

Comment on attachment 540694 [details] [diff] [review]
Added account Name to the title of the Advanced Account Settings dialog. (v1)

>+++ b/mailnews/base/prefs/content/am-server-advanced.js

>+    var stringBundle = document.getElementById("bundle_prefs");

>+    var enumer = stringBundle.stringBundle.getSimpleEnumeration();
>+    while (enumer.hasMoreElements())
>+    {
>+      var stringName = enumer.getNext();
>+    }
Not sure why you are doing this enumer section.

>+    var msg = stringBundle.getFormattedString("forAccount",
>+                                              [prettyName]);
Without the enumer section you should be able to inline the stringBundle bit with this.

>+
>+    window.document.title = msg;
As it stands there is not enough space in the title bar to have the full prettyName if the prettyName happens to be longer than 12 characters (if it is the email address for example). Either need to put it somewhere other than the title or truncate it in the title.
If it is in the title then you can just use document.title no need for the window bit.

>+++ b/mailnews/base/prefs/content/am-server.js
>+  for (var accountid in parent.accountArray)
>+  {
>+    var account = parent.accountArray[accountid];
>+  }
Again not sure what this section is about, what is wrong with using gServer at this point?

>+  serverSettings.serverPrettyName = account.server.prettyName;
Why not just have serverSettings.prettyName ?

r-

This is outside scope of this bug (so probably needs a new one) but am-server-advanced.js is a bit of a strange file anyway. There are variables in there that:
* appear to be no longer used.
* have global names (of the form "gFoo") but are declared and used within a single function.
* are declared globally but only appear to be used in one function, so should not have a global form name either.

Comment 3

[Edmund Wong (:ewong)]

Attachment: Added account name to Advanced Account Settings dialog. (v2)

Comment 4

[Edmund Wong (:ewong)]

Attachment: Added account name to Advanced Account Settings dialog. (v3)

Comment 5

[Ian Neal]

Comment on attachment 540756 [details] [diff] [review]
Added account name to Advanced Account Settings dialog. (v3)

>+++ b/mailnews/base/prefs/content/am-server-advanced.js
>+  var prettyName = gServerSettings.serverPrettyName;
>+
>+  if (prettyName)
>+  {
Nit: Don't need braces here.
>+    document.getElementById("serverPrettyName").value = 
>+      document.getElementById("bundle_prefs")
>+              .getFormattedString("forAccount",
>+                                  [prettyName]);
Not worth line wrapping the [prettyName]);
>+  }

>+++ b/mailnews/base/prefs/content/am-server-advanced.xul

>   <stringbundle id="bundle_prefs" src="chrome://messenger/locale/prefs.properties"/>
> 
>+  <label id="serverPrettyName"/>
Nit: indentation wrong, needs to be indented by 2 more spaces.
>     <!-- IMAP Panel -->
>     <vbox id="imapPanel">
r=me with those fixed.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Added account name to Advanced Account Settings dialog. (v4) [Checked in: Comment 7]

Comment 7

[Ian Neal]

Comment on attachment 542012 [details] [diff] [review]
Added account name to Advanced Account Settings dialog. (v4) [Checked in: Comment 7]

http://hg.mozilla.org/comm-central/rev/d826f0f7f749

Comment 8

[Mark Banner (:standard8)]

The patch landed from this bug affects mailnews/ and caused a regression in Thunderbird. Although it was originally filed in the SeaMonkey product, it should really have been moved to Mailnews Core to track that better, and flag the dual product intentions.

Comment 9

[David :Bienvenu]

we should either fix this or back it out - it's broken in TB 8 beta.

Comment 10

[Mark Banner (:standard8)]

(In reply to David :Bienvenu from comment #9)
> we should either fix this or back it out - it's broken in TB 8 beta.

We're dealing with it in bug 678255.

Comment 11

[Mark Banner (:standard8)]

Attachment: Backout non string parts

This is the proposed backout which will be applied to aurora and beta. The fix will then make it into gecko 10 based versions onwards.

Comment 12

[David :Bienvenu]

who did you mean to ask for review?

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 567844 [details] [diff] [review]
Backout non string parts

Oops, sorry, I'm doing this on the wrong bug.

Comment 14

[Mark Banner (:standard8)]

Due to the backouts in bug 678255, this will now be in Thunderbird 10 and SeaMonkey 2.7a1