109162 - unify advanced server settings into a tabbed dialog

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[Phil Miller]

In the account manager, when a news account is selected, clicking the
"Advanced..." button opens a single-option window for setting "When sending
messages from this identity, always use the following server (SMTP)". 
For news accounts, no SMTP server is used, and so this dialog should be
eliminated. For mail accounts, this should be placed under the "Server"
sub-item, along with settings for the POP3/IMAP inbound server.

Comment 1

[Ninoschka Baca]

I agree that the news panels shouldn't have a reference to an SMTP Server.

Add jglick to the Cc: list for her input for the mail accounts. 

Comment 2

[Phil Miller]

Not only shouldn't there be reference to SMTP server in a news account, but the "Advanced Settings" dialog is unnecessary, because it only contains a single option which can be placed directly in the server page for the mail accounts.

Also, is this Windows or PC specific? I doubt it, since the interface is supposed to contain the same things on all platforms, and this ispurely an interface issue.

Comment 3

[jglick]

>For news accounts, no SMTP server is used, and so this dialog should be
>eliminated.

If that is true, I agree it shouldn't be available for news accounts.

Comment 4

[racham]

*** Bug 110871 has been marked as a duplicate of this bug. ***

Comment 5

[Phil Miller]

Changed Platform/OS to All/All because the interface is supposed to be uniform on all systems, so 
this problem would exist on all systems.

Comment 6

[Ninoschka Baca]

Bhuvan, is it ok that both of these requests are in this bug?
1. Move the contents of the Advanced button from the account level to the Server
panel.
2. Remove the Advanced button from the News account level since news doesn't use
SMTP.

Comment 7

[racham]

Yeah. It's fine. The code to fix both these bugs is in the same place.

Comment 8

[Ninoschka Baca]

*** Bug 114224 has been marked as a duplicate of this bug. ***

Comment 9

[Felix Miata]

Isn't SMTP used when an email To/CC/BCC is included in an NNTP post?

Comment 10

[jglick]

1. Remove "Advanced" button from top level Account Settings pane for all accounts.

2. Add "Advanced" button to Server Settings pane for POP
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerPop1P.gif
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerPopAdv1.gif

3. IMAP Server Settings pane already has an Advanced button. Change the dialog
that opens to include both IMAP and SMTP advanced settings (tab dialog).
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerImap1P.gif
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerImapAdv1.gif

4. If smtp not needed for newsgroup accounts, remove advanced button from
Account Settings pane and don't add to Server Settings pane.

Comment 11

[Samir Gehani]

Mail triage team: nsbeta1+/adt2

Comment 12

[(not reading, please use [e-mail] instead)]

re-assigning nsbeta1+ bugs

Comment 13

[(not reading, please use [e-mail] instead)]

> Isn't SMTP used when an email To/CC/BCC is included in an NNTP post?

yes, [e-mail] is right, that's a news identity has a smtp key for that reason.

that means we need the advanced button for news.

I'll keep this bug open for the tab dialog approach (in
http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerImapAdv1.gif)

clearing nsbeta1+, as this isn't about removing the button for news accounts.

futuring, as I don't have the cycles to move around the advanced settings UI and
I don't think it's that high of a priority.

jen / ninoschka, please correct me if I'm wrong.

Comment 14

[(not reading, please use [e-mail] instead)]

removing adt and nsbeta1+, since this bug has morphed and I don't think it is as
important anymore

Comment 15

[Ian Neal]

Taking

Comment 16

[Ian Neal]

Accepting

Comment 17

[Ian Neal]

Attachment: Initial Patch v0.1

Moves advanced button from main/identity pane to server settings pane and
combines it with the IMAP advanced button for IMAP servers as a tabbed dialog
window.
am-identity-advanced.xul/js/dtd and am-imap-advanced.xul/js/dtd are replaced by
the new am-server-advanced.xul/js/dtd
Not 100% sure how to generate a cvs diff to add new files so this patch may
have to be applied manually in parts.

Comment 18

[Ian Neal]

Attachment: Patch v0.1a taken against cleaner tree

As v0.1 except without fixes for another bug in

Comment 19

[Ian Neal]

Targetting

Comment 20

[[e-mail]]

Comment on attachment 129197 [details] [diff] [review]
Patch v0.1a taken against cleaner tree

>+    <button hidable="true" hidefor="movemail" label="&advancedButton.label;"
Why hide for movemail? You still need SMTP to send, right?
The button is in the wrong place anyway.

>+  var serverSettings = new Array;
The old code is wrong, use = {}; instead.

>Index: content/am-server-advanced.xul
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server-advanced.xul,v
This should say
RCS file: content/am-server-advanced.xul

>+<?xul-overlay href="chrome://global/content/dialogOverlay.xul"?>
>+
>+<!DOCTYPE page SYSTEM "chrome://messenger/locale/am-server-advanced.dtd">
>+
>+<page
Please convert this to a <dialog> (plus other neccessary changes). Obviously at
one time it was a page of the wizard, but nobody bothered to convert it before
:-(

>+        <menulist datasources="rdf:smtp"
>+                  containment="http://home.netscape.com/NC-rdf#child"
>+                  ref="NC:smtpservers"
>+                  oncommand="onSelected(event)" id="smtpServerList">
>+          <template>
>+            <rule>
>+              <menupopup>
>+                <menuitem uri="..." key="rdf:http://home.netscape.com/NC-rdf#Key"
Sigh. Please change this to value, menulists know about that attribute (see
below for an example).

>Index: content/am-server-advanced.js
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server-advanced.js,v
This should say
RCS file: content/am-server-advanced.js

>+  var selectedServerKey = server.smtpServerKey;
>+  if (selectedServerKey)
>+  {
>+    var preselectedItems = serverList.getElementsByAttribute("key", selectedServerKey);
>+
>+    if (preselectedItems && preselectedItems.length > 0)
>+      serverList.selectedItem = preselectedItems[0];
>+  }
>+  else
>+    // no key, so preselect the "useDefault" item.
>+    serverList.selectedItem = document.getElementById("useDefaultItem");
This should simplify to serverList.value = server.smtpServerKey; etc. In fact,
it should be possible to tweak getControls to return either just the smtp or
both sets of controls as appropriate.

>Index: locale/en-US/am-server-advanced.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/locale/en-US/am-server-advanced.dtd,v
This should say:
RCS file: locale/en-US/am-server-advanced.dtd

You don't seem to have edited jar.mn, which actually lists the files to go in
messenger.jar, or shown which files are no longer needed.

Comment 21

[[e-mail]]

OK, I take it back about the button.
BTW diffs of removed files look like added files except with --- for the file
and +++ for /dev/null.

Comment 22

[Ian Neal]

Attachment: Patch v0.2

Updated patch taking onboard Neil's comments.

Comment 23

[Ian Neal]

Attachment: Patch v0.4

Taking on board Neil's comments via IRC - prefered his "if (slot in server)" to
my method in patch v0.2 of creating IMAP entries in array no matter what the
server type was.

Comment 24

[Ian Neal]

Comment on attachment 129234 [details] [diff] [review]
Patch v0.2

Cancelling old review request

Comment 25

[[e-mail]]

Comment on attachment 129251 [details] [diff] [review]
Patch v0.4

I can't get this patch to apply to am-main.xul or am-main.dtd; can you try
again without -w please?

Comment 26

[Ian Neal]

Attachment: Patch v0.4a

Still with -w but should apply better.

Comment 27

[[e-mail]]

Comment on attachment 129251 [details] [diff] [review]
Patch v0.4

Turns out this conflicts with attachment 129132 [details] [diff] [review] :-)

Comment 28

[[e-mail]]

Comment on attachment 129289 [details] [diff] [review]
Patch v0.4a

>+    if (slot in server) {
Nit: { on its own line in this file
>+      var val = server[slot];
Not sure this is worth it as it's only used once.

Comment 29

[Ian Neal]

Attachment: Patch v0.4b

Nits addressed

Comment 30

[Ian Neal]

Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

Carrying forward r=

Comment 31

[David :Bienvenu]

Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

sr=bienvenu

Comment 32

[Ian Neal]

Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

Requesting driver approval for checkin to 1.5b

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

Approved for 1.5b

Comment 34

[David :Bienvenu]

I can handle checking this in for Ian.

Comment 35

[David :Bienvenu]

fix checked in.