Closed Bug 109162 Opened 19 years ago Closed 17 years ago

unify advanced server settings into a tabbed dialog

Categories

(SeaMonkey :: MailNews: Account Configuration, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.5beta

People

(Reporter: unmobile+mozbugs, Assigned: iann_bugzilla)

References

()

Details

(Whiteboard: [ue])

Attachments

(1 file, 5 obsolete files)

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.
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. 
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
>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.
*** Bug 110871 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1
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.
OS: Windows 98 → All
Hardware: PC → All
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.
Yeah. It's fine. The code to fix both these bugs is in the same place.
*** Bug 114224 has been marked as a duplicate of this bug. ***
Summary: Remove "Advanced Account Settings" In Account Manager → Remove "Advanced Account Settings" In Account Manager for news
Isn't SMTP used when an email To/CC/BCC is included in an NNTP post?
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.
Keywords: nsbeta1
Whiteboard: [ue]
Mail triage team: nsbeta1+/adt2
Keywords: nsbeta1nsbeta1+
Whiteboard: [ue] → [adt2][ue]
re-assigning nsbeta1+ bugs
Assignee: racham → sspitzer
Status: ASSIGNED → NEW
> Isn't SMTP used when an email To/CC/BCC is included in an NNTP post?

yes, mrmazda@ij.net 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.
Keywords: nsbeta1+nsbeta1
Summary: Remove "Advanced Account Settings" In Account Manager for news → unify advanced server settings into a tabbed dialog
Target Milestone: mozilla1.0.1 → Future
removing adt and nsbeta1+, since this bug has morphed and I don't think it is as
important anymore
Keywords: nsbeta1
Whiteboard: [adt2][ue] → [ue]
Blocks: 62429
Taking
Assignee: sspitzer → bugzilla
Accepting
Status: NEW → ASSIGNED
Attached patch Initial Patch v0.1 (obsolete) — Splinter Review
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.
As v0.1 except without fixes for another bug in
Attachment #129196 - Attachment is obsolete: true
Attachment #129197 - Flags: review?(neil.parkwaycc.co.uk)
Targetting
Target Milestone: Future → mozilla1.5beta
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.
Attachment #129197 - Flags: review?(neil.parkwaycc.co.uk) → review-
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.
Attached patch Patch v0.2 (obsolete) — Splinter Review
Updated patch taking onboard Neil's comments.
Attachment #129197 - Attachment is obsolete: true
Attachment #129234 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v0.4 (obsolete) — Splinter Review
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.
Attachment #129234 - Attachment is obsolete: true
Comment on attachment 129234 [details] [diff] [review]
Patch v0.2

Cancelling old review request
Attachment #129234 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #129251 - Flags: review?(neil.parkwaycc.co.uk)
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?
Attachment #129251 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v0.4a (obsolete) — Splinter Review
Still with -w but should apply better.
Attachment #129251 - Attachment is obsolete: true
Attachment #129289 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 129251 [details] [diff] [review]
Patch v0.4

Turns out this conflicts with attachment 129132 [details] [diff] [review] :-)
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.
Attachment #129289 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attached patch Patch v0.4bSplinter Review
Nits addressed
Attachment #129289 - Attachment is obsolete: true
Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

Carrying forward r=
Attachment #129387 - Flags: superreview?(bienvenu)
Attachment #129387 - Flags: review+
Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

sr=bienvenu
Attachment #129387 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

Requesting driver approval for checkin to 1.5b
Attachment #129387 - Flags: approval1.5b?
Comment on attachment 129387 [details] [diff] [review]
Patch v0.4b

Approved for 1.5b
Attachment #129387 - Flags: approval1.5b? → approval1.5b+
I can handle checking this in for Ian.
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 216606
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.