If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Advanced Account Settings should mention account name

RESOLVED FIXED in Thunderbird 10.0

Status

MailNews Core
Account Manager
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Karsten Düsterloh, Assigned: ewong)

Tracking

Trunk
Thunderbird 10.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Assignee: nobody → ewong
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 540694 [details] [diff] [review]
Added account Name to the title of the Advanced Account Settings dialog. (v1)
Attachment #540694 - Flags: review?(iann_bugzilla)

Updated

6 years ago
OS: Other → All
Hardware: Other → All

Comment 2

6 years ago
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.
Attachment #540694 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 540753 [details] [diff] [review]
Added account name to Advanced Account Settings dialog. (v2)
Attachment #540694 - Attachment is obsolete: true
Attachment #540753 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

6 years ago
Attachment #540753 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 4

6 years ago
Created attachment 540756 [details] [diff] [review]
Added account name to Advanced Account Settings dialog. (v3)
Attachment #540753 - Attachment is obsolete: true
Attachment #540756 - Flags: review?(iann_bugzilla)

Comment 5

6 years ago
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.
Attachment #540756 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 542012 [details] [diff] [review]
Added account name to Advanced Account Settings dialog. (v4) [Checked in: Comment 7]
Attachment #540756 - Attachment is obsolete: true
Attachment #542012 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Comment 7

6 years ago
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
Attachment #542012 - Attachment description: Added account name to Advanced Account Settings dialog. (v4) → Added account name to Advanced Account Settings dialog. (v4) [Checked in: Comment 7]

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.5
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.
Blocks: 678255
Component: MailNews: Account Configuration → Account Manager
Product: SeaMonkey → MailNews Core
QA Contact: mailnews-account → account-manager
Target Milestone: seamonkey2.5 → ---
Target Milestone: --- → Thunderbird 9.0
No longer blocks: 678255
Depends on: 678255

Comment 9

6 years ago
we should either fix this or back it out - it's broken in TB 8 beta.
(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.
Target Milestone: Thunderbird 9.0 → Thunderbird 8.0
Created attachment 567844 [details] [diff] [review]
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.
Attachment #567844 - Flags: review?

Comment 12

6 years ago
who did you mean to ask for review?
Attachment #567844 - Flags: review? → review?(dbienvenu)
Comment on attachment 567844 [details] [diff] [review]
Backout non string parts

Oops, sorry, I'm doing this on the wrong bug.
Attachment #567844 - Attachment is obsolete: true
Attachment #567844 - Flags: review?(dbienvenu)
Due to the backouts in bug 678255, this will now be in Thunderbird 10 and SeaMonkey 2.7a1
Target Milestone: Thunderbird 8.0 → Thunderbird 10.0
You need to log in before you can comment on or make changes to this bug.